diff mbox series

[v7,5/7] xen/asm-generic: introduce generic device.h

Message ID 621ff5bd992ea8e6202ec03fa52c0e09aacd8f83.1706281994.git.oleksii.kurochko@gmail.com (mailing list archive)
State Superseded
Headers show
Series Introduce generic headers | expand

Commit Message

Oleksii Kurochko Jan. 26, 2024, 3:42 p.m. UTC 
Arm, PPC and RISC-V use the same device.h thereby device.h
was moved to asm-generic. Arm's device.h was taken as a base with
the following changes:
 - #ifdef PCI related things.
 - #ifdef ACPI related things.
 - Rename #ifdef guards.
 - Add SPDX tag.
 - #ifdef CONFIG_HAS_DEVICE_TREE related things.
 - #ifdef-ing iommu related things with CONFIG_HAS_PASSTHROUGH.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V7:
 - keeping DEVICE_PCI_HOSTBRIDGE available for every build based on the reply:
    https://lore.kernel.org/xen-devel/926a5c12-7f02-42ec-92a8-1c82d060c710@xen.org/
 - add comment above enum device_type.h with explanation about DEV_TYPE_MAX.        
 - separate patch  "[PATCH v6 9/9] xen/asm-generic: introduce generic device.h" into 3 patches.
---
Changes in V6:
 - Rebase only.
---
Changes in V5:
  - Removed generated file: xen/include/headers++.chk.new
  - Removed pointless #ifdef CONFIG_HAS_DEVICE_TREE ... #endif for PPC as
    CONFIG_HAS_DEVICE_TREE will be always used for PPC.
---
Changes in V4:
 - Updated the commit message
 - Switched Arm and PPC to asm-generic version of device.h
 - Replaced HAS_PCI with CONFIG_HAS_PCI
 - ifdef-ing iommu filed of dev_archdata struct with CONFIG_HAS_PASSTHROUGH
 - ifdef-ing iommu_fwspec of device struct with CONFIG_HAS_PASSTHROUGH
 - ifdef-ing DT related things with CONFIG_HAS_DEVICE_TREE
 - Updated the commit message ( remove a note with question about
   if device.h should be in asm-generic or not )
 - Replaced DEVICE_IC with DEVICE_INTERRUPT_CONTROLLER
 - Rationalized usage of CONFIG_HAS_* in device.h
 - Fixed indents for ACPI_DEVICE_START and ACPI_DEVICE_END
---
Changes in V3:
 - ifdef device tree related things.
 - update the commit message
---
Changes in V2:
	- take ( as common ) device.h from Arm as PPC and RISC-V use it as a base.
	- #ifdef PCI related things.
	- #ifdef ACPI related things.
	- rename DEVICE_GIC to DEVIC_IC.
	- rename #ifdef guards.
	- switch Arm and PPC to generic device.h
	- add SPDX tag
	- update the commit message
---
 xen/include/asm-generic/device.h | 162 +++++++++++++++++++++++++++++++
 1 file changed, 162 insertions(+)
 create mode 100644 xen/include/asm-generic/device.h

Comments

Jan Beulich Jan. 31, 2024, 2:54 p.m. UTC | #1 
On 26.01.2024 16:42, Oleksii Kurochko wrote:
> Arm, PPC and RISC-V use the same device.h thereby device.h
> was moved to asm-generic.

It's not "move" anymore with the splitting off of the Arm and PPC
parts. For reasons mentioned before, I'm not exactly happy with
it not being a move anymore, but I expect you were asked to split.

> Arm's device.h was taken as a base with
> the following changes:
>  - #ifdef PCI related things.

Well, not really, with ...

>  - #ifdef ACPI related things.
>  - Rename #ifdef guards.
>  - Add SPDX tag.
>  - #ifdef CONFIG_HAS_DEVICE_TREE related things.
>  - #ifdef-ing iommu related things with CONFIG_HAS_PASSTHROUGH.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> Changes in V7:
>  - keeping DEVICE_PCI_HOSTBRIDGE available for every build based on the reply:
>     https://lore.kernel.org/xen-devel/926a5c12-7f02-42ec-92a8-1c82d060c710@xen.org/

... this. Specifically ...

> --- /dev/null
> +++ b/xen/include/asm-generic/device.h
> @@ -0,0 +1,162 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __ASM_GENERIC_DEVICE_H__
> +#define __ASM_GENERIC_DEVICE_H__
> +
> +#include <xen/stdbool.h>
> +
> +/*
> + * DEV_TYPE_MAX is currently not in use, but it was added because the enum may
> + * be empty when !HAS_DEVICE_TREE and !HAS_PCI, which could lead to
> + * a compilation error.
> + */
> +enum device_type
> +{
> +#ifdef CONFIG_HAS_DEVICE_TREE
> +    DEV_DT,
> +#endif
> +
> +#ifdef CONFIG_HAS_PCI
> +    DEV_PCI,
> +#endif

... this is now inconsistent with ...

> +    DEV_TYPE_MAX,
> +};
> +
> +enum device_class
> +{
> +    DEVICE_SERIAL,
> +    DEVICE_IOMMU,
> +    DEVICE_INTERRUPT_CONTROLLER,
> +    DEVICE_PCI_HOSTBRIDGE,

... this. Either we want PCI-related #ifdef-ary, or we don't. There
shouldn't be a mix (unless there's a good reason).

Also the use of blank lines inside the earlier enum would better be
consistent.

> +    /* Use for error */
> +    DEVICE_UNKNOWN,
> +};
> +
> +struct dev_archdata {
> +#ifdef CONFIG_HAS_PASSTHROUGH
> +    void *iommu;    /* IOMMU private data */
> +#endif
> +};
> +
> +/* 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;
> +#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)                    \

Would be really nice if in the course of generalization these leading
underscores would also disappear. Yes, that'll require changing two
of the names more than just to drop the underscores, to account for ...

> +static const struct device_desc __dev_desc_##_name __used           \
> +__section(".dev.info") = {                                          \
> +    .name = _namestr,                                               \
> +    .class = _class,                                                \

... these field names.

Also there's a strack backslash on the last line above.

Both comments similarly apply to the ACPI stuff further down.

> +#define DT_DEVICE_END                                               \
> +};
> +
> +#else /* !CONFIG_HAS_DEVICE_TREE */
> +#define dev_is_dt(dev) ((void)(dev), false)
> +#endif /* CONFIG_HAS_DEVICE_TREE */
> +
> +#ifdef CONFIG_HAS_PCI
> +#define dev_is_pci(dev) ((dev)->type == DEV_PCI)
> +#else
> +#define dev_is_pci(dev) ((void)(dev), false)
> +#endif
> +
> +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
> +};
> +
> +#ifdef CONFIG_ACPI
> +
> +struct acpi_device_desc {
> +    /* Device name */
> +    const char *name;
> +    /* Device class */
> +    enum device_class class;

I understand it's this way on Arm right now, and I'm also not going
to insist that you do anything about it right here, but it's still
odd that struct device_desc doesn't simply have a union to cover for
both DT and ACPI.

Jan
Oleksii Kurochko Feb. 5, 2024, 12:59 p.m. UTC | #2 
On Wed, 2024-01-31 at 15:54 +0100, Jan Beulich wrote:
> On 26.01.2024 16:42, Oleksii Kurochko wrote:
> > Arm, PPC and RISC-V use the same device.h thereby device.h
> > was moved to asm-generic.
> 
> It's not "move" anymore with the splitting off of the Arm and PPC
> parts. For reasons mentioned before, I'm not exactly happy with
> it not being a move anymore, but I expect you were asked to split.
> 
> > Arm's device.h was taken as a base with
> > the following changes:
> >  - #ifdef PCI related things.
> 
> Well, not really, with ...
> 
> >  - #ifdef ACPI related things.
> >  - Rename #ifdef guards.
> >  - Add SPDX tag.
> >  - #ifdef CONFIG_HAS_DEVICE_TREE related things.
> >  - #ifdef-ing iommu related things with CONFIG_HAS_PASSTHROUGH.
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> > Changes in V7:
> >  - keeping DEVICE_PCI_HOSTBRIDGE available for every build based on
> > the reply:
> >    
> > https://lore.kernel.org/xen-devel/926a5c12-7f02-42ec-92a8-1c82d060c710@xen.org/
> 
> ... this. Specifically ...
> 
> > --- /dev/null
> > +++ b/xen/include/asm-generic/device.h
> > @@ -0,0 +1,162 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __ASM_GENERIC_DEVICE_H__
> > +#define __ASM_GENERIC_DEVICE_H__
> > +
> > +#include <xen/stdbool.h>
> > +
> > +/*
> > + * DEV_TYPE_MAX is currently not in use, but it was added because
> > the enum may
> > + * be empty when !HAS_DEVICE_TREE and !HAS_PCI, which could lead
> > to
> > + * a compilation error.
> > + */
> > +enum device_type
> > +{
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> > +    DEV_DT,
> > +#endif
> > +
> > +#ifdef CONFIG_HAS_PCI
> > +    DEV_PCI,
> > +#endif
> 
> ... this is now inconsistent with ...
> 
> > +    DEV_TYPE_MAX,
> > +};
> > +
> > +enum device_class
> > +{
> > +    DEVICE_SERIAL,
> > +    DEVICE_IOMMU,
> > +    DEVICE_INTERRUPT_CONTROLLER,
> > +    DEVICE_PCI_HOSTBRIDGE,
> 
> ... this. Either we want PCI-related #ifdef-ary, or we don't. There
> shouldn't be a mix (unless there's a good reason).
Agreed, I overlooked the fact that we now have inconsistency.

Then it is needed to update remove CONFIG_HAS_PCI in device_type enum
too.

> 
> Also the use of blank lines inside the earlier enum would better be
> consistent.
> 
> > +    /* Use for error */
> > +    DEVICE_UNKNOWN,
> > +};
> > +
> > +struct dev_archdata {
> > +#ifdef CONFIG_HAS_PASSTHROUGH
> > +    void *iommu;    /* IOMMU private data */
> > +#endif
> > +};
> > +
> > +/* 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;
> > +#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)                    \
> 
> Would be really nice if in the course of generalization these leading
> underscores would also disappear. Yes, that'll require changing two
> of the names more than just to drop the underscores, to account for
> ...
> 
> > +static const struct device_desc __dev_desc_##_name
> > __used           \
> > +__section(".dev.info") =
> > {                                          \
> > +    .name =
> > _namestr,                                               \
> > +    .class =
> > _class,                                                \
> 
> ... these field names.
> 
> Also there's a strack backslash on the last line above.
Sure, I'll drop the leading underscores. Thanks.

~ Oleksii
> 
> Both comments similarly apply to the ACPI stuff further down.
> 
> > +#define
> > DT_DEVICE_END                                               \
> > +};
> > +
> > +#else /* !CONFIG_HAS_DEVICE_TREE */
> > +#define dev_is_dt(dev) ((void)(dev), false)
> > +#endif /* CONFIG_HAS_DEVICE_TREE */
> > +
> > +#ifdef CONFIG_HAS_PCI
> > +#define dev_is_pci(dev) ((dev)->type == DEV_PCI)
> > +#else
> > +#define dev_is_pci(dev) ((void)(dev), false)
> > +#endif
> > +
> > +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
> > +};
> > +
> > +#ifdef CONFIG_ACPI
> > +
> > +struct acpi_device_desc {
> > +    /* Device name */
> > +    const char *name;
> > +    /* Device class */
> > +    enum device_class class;
> 
> I understand it's this way on Arm right now, and I'm also not going
> to insist that you do anything about it right here, but it's still
> odd that struct device_desc doesn't simply have a union to cover for
> both DT and ACPI.
> 
> Jan
diff mbox series

Patch

diff --git a/xen/include/asm-generic/device.h b/xen/include/asm-generic/device.h
new file mode 100644
index 0000000000..f12b30dc43
--- /dev/null
+++ b/xen/include/asm-generic/device.h
@@ -0,0 +1,162 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __ASM_GENERIC_DEVICE_H__
+#define __ASM_GENERIC_DEVICE_H__
+
+#include <xen/stdbool.h>
+
+/*
+ * DEV_TYPE_MAX is currently not in use, but it was added because the enum may
+ * be empty when !HAS_DEVICE_TREE and !HAS_PCI, which could lead to
+ * a compilation error.
+ */
+enum device_type
+{
+#ifdef CONFIG_HAS_DEVICE_TREE
+    DEV_DT,
+#endif
+
+#ifdef CONFIG_HAS_PCI
+    DEV_PCI,
+#endif
+    DEV_TYPE_MAX,
+};
+
+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
+};
+
+/* 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;
+#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 */
+
+#ifdef CONFIG_HAS_PCI
+#define dev_is_pci(dev) ((dev)->type == DEV_PCI)
+#else
+#define dev_is_pci(dev) ((void)(dev), false)
+#endif
+
+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
+};
+
+#ifdef CONFIG_ACPI
+
+struct acpi_device_desc {
+    /* Device name */
+    const char *name;
+    /* Device class */
+    enum device_class class;
+    /* type of device supported by the driver */
+    const int class_type;
+    /* Device initialization */
+    int (*init)(const void *data);
+};
+
+/**
+ *  acpi_device_init - Initialize a device
+ *  @class: class of the device (serial, network...)
+ *  @data: specific data for initializing the device
+ *
+ *  Return 0 on success.
+ */
+int acpi_device_init(enum device_class class,
+                     const void *data, int class_type);
+
+#define ACPI_DEVICE_START(_name, _namestr, _class)              \
+static const struct acpi_device_desc __dev_desc_##_name __used  \
+__section(".adev.info") = {                                     \
+    .name = _namestr,                                           \
+    .class = _class,                                            \
+
+#define ACPI_DEVICE_END                                         \
+};
+
+#endif /* CONFIG_ACPI */
+
+#endif /* __ASM_GENERIC_DEVICE_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */