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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxx>
> ---
> 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@xxxxxxx/

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