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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Tue, 2024-02-13 at 18:09 +0000, Julien Grall wrote:
> Hi Oleksii,
> 
> On 09/02/2024 18:00, Oleksii Kurochko wrote:
> > diff --git a/xen/include/asm-generic/device.h b/xen/include/asm-
> > generic/device.h
> > new file mode 100644
> > index 0000000000..6e56658271
> > --- /dev/null
> > +++ b/xen/include/asm-generic/device.h
> > @@ -0,0 +1,149 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __ASM_GENERIC_DEVICE_H__
> > +#define __ASM_GENERIC_DEVICE_H__
> > +
> > +#include <xen/stdbool.h>
> > +
> > +enum device_type
> > +{
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> > +    DEV_DT,
> > +#endif
> > +    DEV_PCI
> > +};
> > +
> > +enum device_class
> > +{
> > +    DEVICE_SERIAL,
> > +    DEVICE_IOMMU,
> > +    DEVICE_INTERRUPT_CONTROLLER,
> > +    DEVICE_PCI_HOSTBRIDGE,
> > +    /* Use for error */
> > +    DEVICE_UNKNOWN,
> > +};
> > +
> > +struct dev_archdata {
> > +#ifdef CONFIG_HAS_PASSTHROUGH
> > +    void *iommu;    /* IOMMU private data */
> > +#endif > +};
> 
> It is a bit too late to change, but I thought I would point it if 
> someone wants to send a follow-up. It is a bit odd to have a
> structure 
> dev_archdata that, if I am not mistaken, is only used ...
> 
> > +
> > +/* struct device - The basic device structure */
> > +struct device
> > +{
> > +    enum device_type type;
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> > +    struct dt_device_node *of_node; /* Used by drivers imported
> > from Linux */
> > +#endif
> > +    struct dev_archdata archdata;
> 
> ... in struct device. Looking at the use, I believe this was only 
> introduced to try to keep that SMMU code close to Linux. I would 
> consider to fold the other structure and update dev_archdata() in 
> drivers/passthrough/arm/smmu.c.
I can do that in separate patch. struct dev_archdata was left because
of drivers/passthrough/arm/smmu.c.

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

Would it be better to ifdef full struct device_desc ?

~ Oleksii