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

[Julien Grall <julien@xxxxxxx>]

Hi Oleksii,

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.
> 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.

Cheers,

--
Julien Grall