Re: About irq_create_affinity_masks() for a platform device driver

[Thomas Gleixner]

John!

John Garry <john.garry@xxxxxxxxxx> writes:
> So I'd figure that an API like this would be required:
>
> --- a/include/linux/platform_device.h
> +++ b/include/linux/platform_device.h
> @@ -11,6 +11,7 @@
>   #define _PLATFORM_DEVICE_H_
>
>   #include <linux/device.h>
> +#include <linux/interrupt.h>
>
>   #define PLATFORM_DEVID_NONE	(-1)
>   #define PLATFORM_DEVID_AUTO	(-2)
> @@ -27,6 +28,7 @@ struct platform_device {
>   	u64		dma_mask;
>   	u32		num_resources;
>   	struct resource	*resource;
> +	struct irq_affinity_desc *desc;
>
> and in platform.c, adding:
>
> /**
>   * platform_get_irqs_affinity - get all IRQs for a device with affinity
>   * @dev: platform device
>   * @affd: Affinity descriptor
>   * @count: pointer to count of IRQS
>   * @irqs: pointer holder for irqs numbers
>   *
>   * Gets a full set of IRQs for a platform device
>   *
>   * Return: 0 on success, negative error number on failure.
>   */
> int platform_get_irqs_affinity(struct platform_device *dev, struct 
> irq_affinity *affd, unsigned int *count, int **irqs)
> {
> 	int i;
> 	int *pirqs;
>
> 	if (ACPI_COMPANION(&dev->dev)) {
> 		*count = acpi_irq_get_count(ACPI_HANDLE(&dev->dev));
> 	} else {
> 		// TODO
> 	}
>
> 	pirqs = kzalloc(*count * sizeof(int), GFP_KERNEL);
> 	if (!pirqs)
> 		return -ENOMEM;
>
> 	dev->desc = irq_create_affinity_masks(*count, affd);
> 	if (!dev->desc) {
> 		kfree(irqs);

pirqs I assume and this also leaks the affinity masks and the pointer in
dev.

> 		return -ENOMEM;
> 	}
>
> 	for (i = 0; i < *count; i++) {
> 		pirqs[i] = platform_get_irq(dev, i);
> 		if (irqs[i] < 0) {
> 			kfree(dev->desc);
> 			kfree(irqs);
> 			return -ENOMEM;

That's obviously broken as well :)

> 		}
> 	}
>
> 	*irqs = pirqs;
>
> 	return 0;
> }
> EXPORT_SYMBOL_GPL(platform_get_irqs_affinity);
>
> Here we pass the affinity descriptor and allocate all IRQs for a device.
>
> So this is less than a half-baked solution. We only create the affinity 
> masks but do nothing with them, and the actual irq_desc 's generated 
> would not would have their affinity mask set and would not be managed. 
> Only the platform device driver itself would access the masks, to set 
> the irq affinity hint, etc.
>
> To achieve the proper result, we would somehow need to pass the
> per-IRQ affinity descriptor all the way down through
> platform_get_irq()->acpi_irq_get()->irq_create_fwspec_mapping()->irq_domain_alloc_irqs(),
> which could involve disruptive changes in different subsystems - not
> welcome, I'd say.
>
> I could take the alt approach to generate the interrupt affinity masks 
> in my LLDD instead. Considering I know some of the CPU and numa node 
> properties of the device host, I could generate the masks in the LLDD 
> itself simply, but I still would rather avoid this if possible and use 
> standard APIs.
>
> So if there are any better ideas on this, then it would be good to hear 
> them.

I wouldn't mind to expose a function which allows you to switch the
allocated interrupts to managed. The reason why we do it in one go in
the PCI code is that we get automatically the irq descriptors allocated
on the correct node. So if the node aware allocation is not a
showstopper for this then your function would do:

	...
	for (i = 0; i < count; i++) {
		pirqs[i] = platform_get_irq(dev, i);

                irq_update_affinity_desc(pirqs[i], affdescs + i);

        }

int irq_update_affinity_desc(unsigned int irq, irq_affinity_desc *affinity)
{
	unsigned long flags;
	struct irq_desc *desc = irq_get_desc_lock(irq, &flags, 0);

        if (!desc)
        	return -EINVAL;

        if (affinity->is_managed) {
        	irqd_set(&desc->irq_data, IRQD_IRQ_DISABLED);
	        irqd_set(&desc->irq_data, IRQD_IRQ_MASKED);
        }
        cpumask_copy(desc->irq_common_data.affinity, affinity);
        return 0;
}

That should just work.

Thanks,

        tglx