Re: [PATCH v8 5/7] xen/asm-generic: introduce generic device.h

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

> 
> On 14/02/2024 09:32, Oleksii wrote:
> > On Tue, 2024-02-13 at 18:09 +0000, Julien Grall wrote:
> > > > +#ifdef CONFIG_HAS_PASSTHROUGH
> > > > +    struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU
> > > > instance data */
> > > > +#endif
> > > > +};
> > > > +
> > > > +typedef struct device device_t;
> > > > +
> > > > +#ifdef CONFIG_HAS_DEVICE_TREE
> > > > +
> > > > +#include <xen/device_tree.h>
> > > > +
> > > > +#define dev_is_dt(dev)  ((dev)->type == DEV_DT)
> > > > +
> > > > +/**
> > > > + *  device_init - Initialize a device
> > > > + *  @dev: device to initialize
> > > > + *  @class: class of the device (serial, network...)
> > > > + *  @data: specific data for initializing the device
> > > > + *
> > > > + *  Return 0 on success.
> > > > + */
> > > > +int device_init(struct dt_device_node *dev, enum device_class
> > > > class,
> > > > +                const void *data);
> > > > +
> > > > +/**
> > > > + * device_get_type - Get the type of the device
> > > > + * @dev: device to match
> > > > + *
> > > > + * Return the device type on success or DEVICE_ANY on failure
> > > > + */
> > > > +enum device_class device_get_class(const struct dt_device_node
> > > > *dev);
> > > > +
> > > > +#define DT_DEVICE_START(name_, namestr_, class_)            \
> > > > +static const struct device_desc __dev_desc_##name_ __used   \
> > > > +__section(".dev.info") = {                                  \
> > > > +    .name = namestr_,                                       \
> > > > +    .class = class_,
> > > > +
> > > > +#define DT_DEVICE_END                                       \
> > > > +};
> > > > +
> > > > +#else /* !CONFIG_HAS_DEVICE_TREE */
> > > > +#define dev_is_dt(dev) ((void)(dev), false)
> > > > +#endif /* CONFIG_HAS_DEVICE_TREE */
> > > > +
> > > > +#define dev_is_pci(dev) ((dev)->type == DEV_PCI)
> > > > +
> > > > +struct device_desc {
> > > > +    /* Device name */
> > > > +    const char *name;
> > > > +    /* Device class */
> > > > +    enum device_class class;
> > > > +
> > > > +#ifdef CONFIG_HAS_DEVICE_TREE
> > > > +
> > > > +    /* List of devices supported by this driver */
> > > > +    const struct dt_device_match *dt_match;
> > > > +    /*
> > > > +     * Device initialization.
> > > > +     *
> > > > +     * -EAGAIN is used to indicate that device probing is
> > > > deferred.
> > > > +     */
> > > > +    int (*init)(struct dt_device_node *dev, const void *data);
> > > > +
> > > > +#endif
> > > > +};
> > > I am not sure I fully understand why "device_desc" is not
> > > protected
> > > by
> > > CONFIG_HAS_DEVICE_TREE. The structure doesn't mean much when the
> > > config
> > > is disabled. Can you clarify?
> > I thought that one day struct device_desc and acpi_device_desc will
> > be
> > "merged", and so decided just to #ifdef only DEVICE_TREE specific
> > fields.
> 
> It might be possible to merge the two if we were using an union for
> the 
> ACPI/DT specific part. However the majority of the parsing code needs
> to 
> differ. So I am not convinced there would be any value to merge the
> two 
> structures.
In this case, let's have two separate structures.

This is not the current situation, and I don't have a specific example.
It appears that all architectures will use Device Tree or ACPI.
However, does it make sense to keep 'struct device_desc' more generic
to accommodate non-DT or non-ACPI cases?

I am okay with making the following change, but I am just curious if
what I mentioned above makes sense at all:

#ifdef CONFIG_HAS_DEVICE_TREE
struct device_desc {
    /* Device name */
    const char *name;
    /* Device class */
    enum device_class class;

    /* List of devices supported by this driver */
    const struct dt_device_match *dt_match;
    /*
     * Device initialization.
     *
     * -EAGAIN is used to indicate that device probing is deferred.
     */
    int (*init)(struct dt_device_node *dev, const void *data);
};
#endif /* CONFIG_HAS_DEVICE_TREE */ 

> 
> > Another one reason it is if to protect fully struct device_desc
> > then it
> > would be needed more #ifdef in arm/device.c ( for example,
> > device_init() should be all protected then ) what will require to
> > ifdef
> > all calls of device_init(). As an option device_init can can be
> > defined
> > in case when !CONFIG_HAS_DEVICE_TREE as:
> >     int __init device_init(struct dt_device_node *dev, enum
> > device_class
> >     class,
> >                            const void *data)
> >     {
> >      return -EBADF;
> >     }
> >     
> > The similar thing will be needed for device_get_class() in Arm's
> > device.c.
> 
> I agree that in theory device_init() & co should be protected with 
> CONFIG_HAS_DEVICE_TREE. However, it is not possible to compile Xen on
> Arm without the Device-Tree part today. So I don't view adding the 
> #ifdef or any extra stub as necessary today.
> 
> This may be useful in the future though. Note this is not a request
> to 
> modify the patch more than...
> 
> > 
> > Would it be better to ifdef full struct device_desc ?
> .. moving structure within the #ifdef.
Well, I'll update the commit message of the next patch that it is not
possible to compile Xen without CONFIG_HAS_DEVICE_TREE, so
device_init() and Co won't be protected by CONFIG_HAS_DEVICE_TREE.

~ Oleksii