Re: [PATCH v7] cdx: add MSI support for CDX bus

[Thomas Gleixner]

On Fri, Feb 02 2024 at 17:08, Nipun Gupta wrote:
> Add CDX-MSI domain per CDX controller with gic-its domain as
> a parent, to support MSI for CDX devices. CDX devices allocate
> MSIs from the CDX domain. Also, introduce APIs to alloc and free
> IRQs for CDX domain.
>
> In CDX subsystem firmware is a controller for all devices and
> their configuration. CDX bus controller sends all the write_msi_msg
> commands to firmware running on RPU and the firmware interfaces with
> actual devices to pass this information to devices
>
> Since, CDX controller is the only way to communicate with the Firmware
> for MSI write info, CDX domain per controller required in contrast to
> having a CDX domain per device.
>
> Changes v6->v7:
> - Rebased on Linux 6.8-rc2
>  ...
> Changes v1->v2:
> - fixed scenario where msi write was called asynchronously in
>   an atomic context, by using irq_chip_(un)lock, and using sync
>   MCDI API for write MSI message.
> - fixed broken Signed-off-by chain.

Please put the Changes documentation after the --- separator. That's not
part of the change log and just creates work for the maintainer to remove.

> +#ifdef CONFIG_GENERIC_MSI_IRQ

Why do you need this #ifdef? AFAICT it's completely pointless 

> +/**
> + * 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);
> +#endif
>  #endif /* _CDX_H_ */

> +	/*
> +	 * dev_configure is a controller callback which can interact with

s/dev_configure/dev_configure()/ which makes it clear that this is about
a function

> +	 * Firmware or other entities, and can sleep, so invoke this function
> +	 * outside of the mutex lock.

s/lock/held region/

> +	 */
> +	dev_config.type = CDX_DEV_MSI_CONF;
> +	if (cdx->ops->dev_configure)
> +		cdx->ops->dev_configure(cdx, cdx_dev->bus_num, cdx_dev->dev_num,
> +					&dev_config);

Please use either a single line, which is within the 100 character limit
or place brackets around the condition:

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#bracket-rules

All over the place.

> +int cdx_msi_domain_alloc_irqs(struct device *dev, unsigned int irq_count)
> +{
> +	int ret;
> +
> +	ret = msi_setup_device_data(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = msi_domain_alloc_irqs_range(dev, MSI_DEFAULT_DOMAIN,
> +					  0, irq_count - 1);
> +	if (ret)
> +		dev_err(dev, "Failed to allocate IRQs: %d\n", ret);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(cdx_msi_domain_alloc_irqs);

Why does this need a special allocation function instead of setting the
irq domain of the device and using the generic allocation function
directly?

> +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 device *parent = cdx_dev->cdx->dev;
> +	struct msi_domain_info *msi_info;
> +	u32 dev_id;
> +	int ret;
> +
> +	/* Retrieve device ID from requestor ID using parent device */
> +	ret = of_map_id(parent->of_node, cdx_dev->msi_dev_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;
> +	}
> +
> +#ifdef GENERIC_MSI_DOMAIN_OPS
> +	/* Set the device Id to be passed to the GIC-ITS */
> +	info->scratchpad[0].ul = dev_id;
> +#endif

Is this ever used on a platform which does not have
GENERIC_MSI_DOMAIN_OPS ?

> @@ -120,9 +135,13 @@ struct cdx_controller {
>   * @req_id: Requestor ID associated with CDX device
>   * @is_bus: Is this bus device
>   * @enabled: is this bus enabled
> + * @msi_dev_id: MSI Device ID associated with CDX device
> + * @num_msi: Number of MSI's supported by the device
>   * @driver_override: driver name to force a match; do not set directly,
>   *                   because core frees it; use driver_set_override() to
>   *                   set or clear it.
> + * @irqchip_lock: lock to synchronize irq/msi configuration
> + * @msi_write_pending: MSI write pending for this device
>   */
>  struct cdx_device {
>  	struct device dev;
> @@ -144,7 +163,11 @@ struct cdx_device {
>  	u32 req_id;
>  	bool is_bus;
>  	bool enabled;
> +	u32 msi_dev_id;
> +	u32 num_msi;
>  	const char *driver_override;
> +	struct mutex irqchip_lock; /* Serialize write msi configuration */

This tail comment is pointless. It's already documented above, no?

Other than those nitpicks this looks reasonable.

Thanks,

        tglx