Re: [RFC PATCH v3 4/7] bus/cdx: add cdx-MSI domain with gic-its domain as parent

From: Marc Zyngier
Date: Wed Sep 07 2022 - 09:18:45 EST

Next message: Radovanovic, Aleksandar: "RE: [RFC PATCH v3 4/7] bus/cdx: add cdx-MSI domain with gic-its domain as parent"
Previous message: Adrian Hunter: "Re: [PATCH 2/6] perf test: Use a test program in perf record tests"
In reply to: Radovanovic, Aleksandar: "RE: [RFC PATCH v3 4/7] bus/cdx: add cdx-MSI domain with gic-its domain as parent"
Next in thread: Nipun Gupta: "[RFC PATCH v3 5/7] bus/cdx: add bus and device attributes"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, 06 Sep 2022 14:47:58 +0100,
Nipun Gupta <nipun.gupta@xxxxxxx> wrote:
>
> Devices on cdx bus are dynamically detected and registered using
> platform_device_register API. As these devices are not linked to
> of node they need a separate MSI domain for handling device ID
> to be provided to the GIC ITS domain.
>
> This also introduces APIs to alloc and free IRQs for CDX domain.
>
> Signed-off-by: Nipun Gupta <nipun.gupta@xxxxxxx>
> Signed-off-by: Nikhil Agarwal <nikhil.agarwal@xxxxxxx>
> ---
> drivers/bus/cdx/cdx.c | 18 +++
> drivers/bus/cdx/cdx.h | 19 +++
> drivers/bus/cdx/cdx_msi.c | 236 +++++++++++++++++++++++++++++++++++
> drivers/bus/cdx/mcdi_stubs.c | 1 +
> include/linux/cdx/cdx_bus.h | 19 +++
> 5 files changed, 293 insertions(+)
> create mode 100644 drivers/bus/cdx/cdx_msi.c
>
> diff --git a/drivers/bus/cdx/cdx.c b/drivers/bus/cdx/cdx.c
> index fc417c32c59b..02ececce1c84 100644
> --- a/drivers/bus/cdx/cdx.c
> +++ b/drivers/bus/cdx/cdx.c
> @@ -17,6 +17,7 @@
> #include <linux/dma-map-ops.h>
> #include <linux/property.h>
> #include <linux/iommu.h>
> +#include <linux/msi.h>
> #include <linux/cdx/cdx_bus.h>
>
> #include "cdx.h"
> @@ -236,6 +237,7 @@ static int cdx_device_add(struct device *parent,
> struct cdx_dev_params_t *dev_params)
> {
> struct cdx_device *cdx_dev;
> + struct irq_domain *cdx_msi_domain;
> int ret;
>
> cdx_dev = kzalloc(sizeof(*cdx_dev), GFP_KERNEL);
> @@ -252,6 +254,7 @@ static int cdx_device_add(struct device *parent,
>
> /* Populate CDX dev params */
> cdx_dev->req_id = dev_params->req_id;
> + cdx_dev->num_msi = dev_params->num_msi;
> cdx_dev->vendor = dev_params->vendor;
> cdx_dev->device = dev_params->device;
> cdx_dev->bus_id = dev_params->bus_id;
> @@ -269,6 +272,21 @@ static int cdx_device_add(struct device *parent,
> dev_set_name(&cdx_dev->dev, "cdx-%02x:%02x", cdx_dev->bus_id,
> cdx_dev->func_id);
>
> + /* If CDX MSI domain is not created, create one. */
> + cdx_msi_domain = cdx_find_msi_domain(parent);

Why do we need such a wrapper around find_host_domain()?

> + if (!cdx_msi_domain) {
> + cdx_msi_domain = cdx_msi_domain_init(parent);

This is racy. If device are populated in parallel, bad things will
happen.

> + if (!cdx_msi_domain) {
> + dev_err(&cdx_dev->dev,
> + "cdx_msi_domain_init() failed: %d", ret);
> + kfree(cdx_dev);
> + return -1;

Use standard error codes.

> + }
> + }
> +
> + /* Set the MSI domain */
> + dev_set_msi_domain(&cdx_dev->dev, cdx_msi_domain);
> +
> ret = device_add(&cdx_dev->dev);
> if (ret != 0) {
> dev_err(&cdx_dev->dev,
> diff --git a/drivers/bus/cdx/cdx.h b/drivers/bus/cdx/cdx.h
> index db0569431c10..95df440ebd73 100644
> --- a/drivers/bus/cdx/cdx.h
> +++ b/drivers/bus/cdx/cdx.h
> @@ -20,6 +20,7 @@
> * @res: array of MMIO region entries
> * @res_count: number of valid MMIO regions
> * @req_id: Requestor ID associated with CDX device
> + * @num_msi: Number of MSI's supported by the device
> */
> struct cdx_dev_params_t {
> u16 vendor;
> @@ -29,6 +30,24 @@ struct cdx_dev_params_t {
> struct resource res[MAX_CDX_DEV_RESOURCES];
> u8 res_count;
> u32 req_id;
> + u32 num_msi;
> };
>
> +/**
> + * cdx_msi_domain_init - Init the CDX bus MSI domain.
> + * @dev: Device of the CDX bus controller
> + *
> + * Return CDX MSI domain, NULL on failure
> + */
> +struct irq_domain *cdx_msi_domain_init(struct device *dev);
> +
> +/**
> + * cdx_find_msi_domain - Get the CDX-MSI domain.
> + * @dev: CDX controller generic device
> + *
> + * Return CDX MSI domain, NULL on error or if CDX-MSI domain is
> + * not yet created.
> + */
> +struct irq_domain *cdx_find_msi_domain(struct device *parent);
> +
> #endif /* _CDX_H_ */
> diff --git a/drivers/bus/cdx/cdx_msi.c b/drivers/bus/cdx/cdx_msi.c
> new file mode 100644
> index 000000000000..2fb7bac18393
> --- /dev/null
> +++ b/drivers/bus/cdx/cdx_msi.c
> @@ -0,0 +1,236 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AMD CDX bus driver MSI support
> + *
> + * Copyright (C) 2022, Advanced Micro Devices, Inc.
> + *
> + */
> +
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/msi.h>
> +#include <linux/cdx/cdx_bus.h>
> +
> +#include "cdx.h"
> +
> +#ifdef GENERIC_MSI_DOMAIN_OPS
> +/*
> + * Generate a unique ID identifying the interrupt (only used within the MSI
> + * irqdomain. Combine the req_id with the interrupt index.
> + */
> +static irq_hw_number_t cdx_domain_calc_hwirq(struct cdx_device *dev,
> + struct msi_desc *desc)
> +{
> + /*
> + * Make the base hwirq value for req_id*10000 so it is readable
> + * as a decimal value in /proc/interrupts.
> + */
> + return (irq_hw_number_t)(desc->msi_index + (dev->req_id * 10000));

No, please. Use shifts, and use a script if decimal conversion fails
you. We're not playing these games. And the cast is pointless.

Yes, you have lifted it from the FSL code, bad move. /me makes a note
to go and clean-up this crap.

> +}
> +
> +static void cdx_msi_set_desc(msi_alloc_info_t *arg,
> + struct msi_desc *desc)
> +{
> + arg->desc = desc;
> + arg->hwirq = cdx_domain_calc_hwirq(to_cdx_device(desc->dev), desc);
> +}
> +#else
> +#define cdx_msi_set_desc NULL

Why the ifdefery? This should *only* be supported with
GENERIC_MSI_DOMAIN_OPS.

> +#endif
> +
> +static void cdx_msi_update_dom_ops(struct msi_domain_info *info)
> +{
> + struct msi_domain_ops *ops = info->ops;
> +
> + if (!ops)
> + return;
> +
> + /* set_desc should not be set by the caller */
> + if (!ops->set_desc)
> + ops->set_desc = cdx_msi_set_desc;

Then why are you allowing this to be overridden?

> +}
> +
> +static void cdx_msi_write_msg(struct irq_data *irq_data,
> + struct msi_msg *msg)
> +{
> + /*
> + * Do nothing as CDX devices have these pre-populated
> + * in the hardware itself.
> + */

We talked about this in a separate thread. This is a major problem.

> +}
> +
> +static void cdx_msi_update_chip_ops(struct msi_domain_info *info)
> +{
> + struct irq_chip *chip = info->chip;
> +
> + if (!chip)
> + return;
> +
> + /*
> + * irq_write_msi_msg should not be set by the caller
> + */
> + if (!chip->irq_write_msi_msg)
> + chip->irq_write_msi_msg = cdx_msi_write_msg;

Then why the check?

> +}
> +/**
> + * cdx_msi_create_irq_domain - Create a CDX MSI interrupt domain
> + * @fwnode: Optional firmware node of the interrupt controller
> + * @info: MSI domain info
> + * @parent: Parent irq domain
> + *
> + * Updates the domain and chip ops and creates a CDX MSI
> + * interrupt domain.
> + *
> + * Returns:
> + * A domain pointer or NULL in case of failure.
> + */
> +static struct irq_domain *cdx_msi_create_irq_domain(struct fwnode_handle *fwnode,
> + struct msi_domain_info *info,
> + struct irq_domain *parent)
> +{
> + if (WARN_ON((info->flags & MSI_FLAG_LEVEL_CAPABLE)))
> + info->flags &= ~MSI_FLAG_LEVEL_CAPABLE;

No. Just fail the domain creation. We shouldn't paper over these things.

> + if (info->flags & MSI_FLAG_USE_DEF_DOM_OPS)
> + cdx_msi_update_dom_ops(info);
> + if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS)
> + cdx_msi_update_chip_ops(info);

Under what circumstances would the default ops not be used? The only
caller is in this file and has pre-computed values.

This looks like a copy/paste from platform-msi.c.

> + info->flags |= MSI_FLAG_ALLOC_SIMPLE_MSI_DESCS | MSI_FLAG_FREE_MSI_DESCS;
> +
> + return msi_create_irq_domain(fwnode, info, parent);

This whole function makes no sense. You should move everything to the
relevant structures, and simply call msi_create_irq_domain() from the
sole caller of this function.

> +}
> +
> +int cdx_msi_domain_alloc_irqs(struct device *dev, unsigned int irq_count)
> +{
> + struct irq_domain *msi_domain;
> + int ret;
> +
> + msi_domain = dev_get_msi_domain(dev);
> + if (!msi_domain) {

How can that happen?

> + dev_err(dev, "msi domain get failed\n");
> + return -EINVAL;
> + }
> +
> + ret = msi_setup_device_data(dev);
> + if (ret) {
> + dev_err(dev, "msi setup device failed: %d\n", ret);
> + return ret;
> + }
> +
> + msi_lock_descs(dev);
> + if (msi_first_desc(dev, MSI_DESC_ALL))
> + ret = -EINVAL;
> + msi_unlock_descs(dev);
> + if (ret) {
> + dev_err(dev, "msi setup device failed: %d\n", ret);

Same message twice, not very useful. Consider grouping these things at
the end of the function and make use of a (oh Gawd) goto...

> + return ret;
> + }
> +
> + ret = msi_domain_alloc_irqs(msi_domain, dev, irq_count);
> + if (ret)
> + dev_err(dev, "Failed to allocate IRQs\n");
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(cdx_msi_domain_alloc_irqs);

EXPORT_SYMBOL_GPL(), please, for all the exports.

> +
> +void cdx_msi_domain_free_irqs(struct device *dev)
> +{
> + struct irq_domain *msi_domain;
> +
> + msi_domain = dev_get_msi_domain(dev);
> + if (!msi_domain)

Again, how can that happen?

> + return;
> +
> + msi_domain_free_irqs(msi_domain, dev);
> +}
> +EXPORT_SYMBOL(cdx_msi_domain_free_irqs);

This feels like a very pointless helper, and again a copy/paste from
the FSL code. I'd rather you change msi_domain_free_irqs() to only
take a device and use the implicit MSI domain.

> +
> +static struct irq_chip cdx_msi_irq_chip = {
> + .name = "CDX-MSI",
> + .irq_mask = irq_chip_mask_parent,
> + .irq_unmask = irq_chip_unmask_parent,
> + .irq_eoi = irq_chip_eoi_parent,
> + .irq_set_affinity = msi_domain_set_affinity

nit: please align things vertically.

> +};
> +
> +static int cdx_msi_prepare(struct irq_domain *msi_domain,
> + struct device *dev,
> + int nvec, msi_alloc_info_t *info)
> +{
> + struct cdx_device *cdx_dev = to_cdx_device(dev);
> + struct msi_domain_info *msi_info;
> + struct device *parent = dev->parent;
> + u32 dev_id;
> + int ret;
> +
> + /* Retrieve device ID from requestor ID using parent device */
> + ret = of_map_id(parent->of_node, cdx_dev->req_id, "msi-map",
> + "msi-map-mask", NULL, &dev_id);
> + if (ret) {
> + dev_err(dev, "of_map_id failed for MSI: %d\n", ret);
> + return ret;
> + }
> +
> + /* Set the device Id to be passed to the GIC-ITS */
> + info->scratchpad[0].ul = dev_id;
> +
> + msi_info = msi_get_domain_info(msi_domain->parent);
> +
> + /* Allocate at least 32 MSIs, and always as a power of 2 */

Where is this requirement coming from?

> + nvec = max_t(int, 32, roundup_pow_of_two(nvec));
> + return msi_info->ops->msi_prepare(msi_domain->parent, dev, nvec, info);
> +}
> +
> +static struct msi_domain_ops cdx_msi_ops __ro_after_init = {
> + .msi_prepare = cdx_msi_prepare,
> +};
> +
> +static struct msi_domain_info cdx_msi_domain_info = {
> + .flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS),
> + .ops = &cdx_msi_ops,
> + .chip = &cdx_msi_irq_chip,
> +};
> +
> +struct irq_domain *cdx_msi_domain_init(struct device *dev)
> +{
> + struct irq_domain *parent;
> + struct irq_domain *cdx_msi_domain;
> + struct fwnode_handle *fwnode_handle;
> + struct device_node *parent_node;
> + struct device_node *np = dev->of_node;
> +
> + fwnode_handle = of_node_to_fwnode(np);
> +
> + parent_node = of_parse_phandle(np, "msi-map", 1);

Huh. This only works because you are stuck with a single ITS per system.

> + if (!parent_node) {
> + dev_err(dev, "msi-map not present on cdx controller\n");
> + return NULL;
> + }
> +
> + parent = irq_find_matching_fwnode(of_node_to_fwnode(parent_node),
> + DOMAIN_BUS_NEXUS);
> + if (!parent || !msi_get_domain_info(parent)) {
> + dev_err(dev, "unable to locate ITS domain\n");
> + return NULL;
> + }
> +
> + cdx_msi_domain = cdx_msi_create_irq_domain(fwnode_handle,
> + &cdx_msi_domain_info, parent);
> + if (!cdx_msi_domain) {
> + dev_err(dev, "unable to create CDX-MSI domain\n");
> + return NULL;
> + }
> +
> + dev_dbg(dev, "CDX-MSI domain created\n");
> +
> + return cdx_msi_domain;
> +}
> +
> +struct irq_domain *cdx_find_msi_domain(struct device *parent)
> +{
> + return irq_find_host(parent->of_node);
> +}
> diff --git a/drivers/bus/cdx/mcdi_stubs.c b/drivers/bus/cdx/mcdi_stubs.c
> index cc9d30fa02f8..2c8db1f5a057 100644
> --- a/drivers/bus/cdx/mcdi_stubs.c
> +++ b/drivers/bus/cdx/mcdi_stubs.c
> @@ -45,6 +45,7 @@ int cdx_mcdi_get_func_config(struct cdx_mcdi_t *cdx_mcdi,
> dev_params->res_count = 2;
>
> dev_params->req_id = 0x250;
> + dev_params->num_msi = 4;

Why the hardcoded 4? Is that part of the firmware emulation stuff?

M.

--
Without deviation from the norm, progress is not possible.

Next message: Radovanovic, Aleksandar: "RE: [RFC PATCH v3 4/7] bus/cdx: add cdx-MSI domain with gic-its domain as parent"
Previous message: Adrian Hunter: "Re: [PATCH 2/6] perf test: Use a test program in perf record tests"
In reply to: Radovanovic, Aleksandar: "RE: [RFC PATCH v3 4/7] bus/cdx: add cdx-MSI domain with gic-its domain as parent"
Next in thread: Nipun Gupta: "[RFC PATCH v3 5/7] bus/cdx: add bus and device attributes"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]