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 <[email protected]>
> > ---
> > Changes in V7:
> > - keeping DEVICE_PCI_HOSTBRIDGE available for every build based on
> > the reply:
> >
> > https://lore.kernel.org/xen-devel/[email protected]/
>
> ... 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
