| Message ID | 1606905417-183214-5-git-send-email-john.garry@huawei.com |
|---|---|
| State | Accepted |
| Commit | e15f2fa959f2cce8a05e8e3a596e75d068cd42c5 |
| Headers | show |
| Series | None | expand |
On Wed, Dec 02, 2020 at 06:36:56PM +0800, John Garry wrote: > Drivers for multi-queue platform devices may also want managed interrupts > for handling HW queue completion interrupts, so add support. Why would a platform device want all of this? Shouldn't such a device be on a "real" bus instead? What in-kernel driver needs this complexity? I can't take new apis without a real user in the tree, sorry. thanks, greg k-h
On 09/12/2020 18:32, Greg KH wrote: > On Wed, Dec 02, 2020 at 06:36:56PM +0800, John Garry wrote: >> Drivers for multi-queue platform devices may also want managed interrupts >> for handling HW queue completion interrupts, so add support. > Hi Greg, > Why would a platform device want all of this? Shouldn't such a device > be on a "real" bus instead? For this HW version, the device is on the system bus, directly addressable by the CPU. Motivation is that I wanted to switch the HW completion queues to use managed interrupts. > > What in-kernel driver needs this complexity? I can't take new apis > without a real user in the tree, sorry. It's in the final patch in the series https://lore.kernel.org/linux-scsi/1606905417-183214-1-git-send-email-john.garry@huawei.com/T/#m0df7e7cd6f0819b99aaeb6b7f8939ef1e17b8a83. I don't anticipate a huge number of users of this API in future, as most multi-queue devices are PCI devices; so we could do the work of this API in the driver itself, but the preference was not to export genirq functions like irq_update_affinity_desc() or irq_create_affinity_masks(), and rather have a common helper in the core platform code. Thanks, John
On Wed, Dec 09, 2020 at 07:04:02PM +0000, John Garry wrote: > On 09/12/2020 18:32, Greg KH wrote: > > On Wed, Dec 02, 2020 at 06:36:56PM +0800, John Garry wrote: > > > Drivers for multi-queue platform devices may also want managed interrupts > > > for handling HW queue completion interrupts, so add support. > > > > Hi Greg, > > > Why would a platform device want all of this? Shouldn't such a device > > be on a "real" bus instead? > > For this HW version, the device is on the system bus, directly addressable > by the CPU. What do you mean by "system bus"? > Motivation is that I wanted to switch the HW completion queues to use > managed interrupts. Fair enough, seems like overkill for a "platform" bus though :) > > What in-kernel driver needs this complexity? I can't take new apis > > without a real user in the tree, sorry. > > It's in the final patch in the series https://lore.kernel.org/linux-scsi/1606905417-183214-1-git-send-email-john.garry@huawei.com/T/#m0df7e7cd6f0819b99aaeb6b7f8939ef1e17b8a83. Ah, I missed that, I thought that was some high-speed scsi thing, not a tiny platform driver... > I don't anticipate a huge number of users of this API in future, as most > multi-queue devices are PCI devices; so we could do the work of this API in > the driver itself, but the preference was not to export genirq functions > like irq_update_affinity_desc() or irq_create_affinity_masks(), and rather > have a common helper in the core platform code. Ok, I'd like to have the irq maintainers/developers ack this before taking it in the driver core, as someone is going to have to maintain this crazy thing for forever if it gets merged. thanks, greg k-h
On 09/12/2020 19:13, Greg KH wrote: Hi Greg, >> For this HW version, the device is on the system bus, directly addressable >> by the CPU. > What do you mean by "system bus"? Maybe my terminology is wrong, the point is that we have a platform device driver. > >> Motivation is that I wanted to switch the HW completion queues to use >> managed interrupts. > Fair enough, seems like overkill for a "platform" bus though:) > >>> What in-kernel driver needs this complexity? I can't take new apis >>> without a real user in the tree, sorry. >> It's in the final patch in the serieshttps://lore.kernel.org/linux-scsi/1606905417-183214-1-git-send-email-john.garry@huawei.com/T/#m0df7e7cd6f0819b99aaeb6b7f8939ef1e17b8a83. > Ah, I missed that, I thought that was some high-speed scsi thing, not a > tiny platform driver... It is actually is a high-speed SCSI thing also, SAS 3.0 :) > >> I don't anticipate a huge number of users of this API in future, as most >> multi-queue devices are PCI devices; so we could do the work of this API in >> the driver itself, but the preference was not to export genirq functions >> like irq_update_affinity_desc() or irq_create_affinity_masks(), and rather >> have a common helper in the core platform code. > Ok, I'd like to have the irq maintainers/developers ack this before > taking it in the driver core, as someone is going to have to maintain > this crazy thing for forever if it gets merged. > irq experts are cc'ed and have been very helpful here So the API mushroomed a bit over time, as I realized that we need to support tearing down the irq mapping, make as devm method, use irq_calc_affinity_vectors(). Not sure how we could factor any of it out to become less of your problem. Thanks, John
On 2020-12-09 19:13, Greg KH wrote: > On Wed, Dec 09, 2020 at 07:04:02PM +0000, John Garry wrote: >> On 09/12/2020 18:32, Greg KH wrote: >> > On Wed, Dec 02, 2020 at 06:36:56PM +0800, John Garry wrote: >> > > Drivers for multi-queue platform devices may also want managed interrupts >> > > for handling HW queue completion interrupts, so add support. >> > >> >> Hi Greg, >> >> > Why would a platform device want all of this? Shouldn't such a device >> > be on a "real" bus instead? >> >> For this HW version, the device is on the system bus, directly >> addressable >> by the CPU. > > What do you mean by "system bus"? > >> Motivation is that I wanted to switch the HW completion queues to use >> managed interrupts. > > Fair enough, seems like overkill for a "platform" bus though :) You should see the box, really... ;-) > >> > What in-kernel driver needs this complexity? I can't take new apis >> > without a real user in the tree, sorry. >> >> It's in the final patch in the series >> https://lore.kernel.org/linux-scsi/1606905417-183214-1-git-send-email-john.garry@huawei.com/T/#m0df7e7cd6f0819b99aaeb6b7f8939ef1e17b8a83. > > Ah, I missed that, I thought that was some high-speed scsi thing, not a > tiny platform driver... > >> I don't anticipate a huge number of users of this API in future, as >> most >> multi-queue devices are PCI devices; so we could do the work of this >> API in >> the driver itself, but the preference was not to export genirq >> functions >> like irq_update_affinity_desc() or irq_create_affinity_masks(), and >> rather >> have a common helper in the core platform code. > > Ok, I'd like to have the irq maintainers/developers ack this before > taking it in the driver core, as someone is going to have to maintain > this crazy thing for forever if it gets merged. I'm actually quite happy with this, and as it turns out, the crazy system that has this SAS thing keeps my backside warm all year long. As long as this machine keeps ticking, I'm happy to help with this. So if that helps: Acked-by: Marc Zyngier <maz@kernel.org> We need to work out the merge strategy for the whole lot though, given that it crosses 3 subsystems over two series... M.
On 09/12/2020 19:39, Marc Zyngier wrote: >> >> Ok, I'd like to have the irq maintainers/developers ack this before >> taking it in the driver core, as someone is going to have to maintain >> this crazy thing for forever if it gets merged. > > I'm actually quite happy with this, and as it turns out, the crazy > system that has this SAS thing keeps my backside warm all year long. > As long as this machine keeps ticking, I'm happy to help with this. > > So if that helps: > > Acked-by: Marc Zyngier <maz@kernel.org> Cheers > > We need to work out the merge strategy for the whole lot though, given > that it crosses 3 subsystems over two series... Thomas originally suggested taking the genirq change himself and then providing a tag for others to merge: https://lore.kernel.org/linux-scsi/87h7qf1yp0.fsf@nanos.tec.linutronix.de/ Not sure if that still stands. The small ACPI change could go in a cycle after rest merged, but may be best not to split up. The worst that will happen without Marc's series is that is remove + re-probe the SCSI driver is broken, so I'm happy as long as that ends up in same kernel version somehow: https://lore.kernel.org/lkml/20201129135208.680293-1-maz@kernel.org/ Thanks, John
Hi Greg, > {sigh} why do hardware engineers ignore sane busses... The next HW version is an integrated PCI endpoint, so there is hope. > > Anyway, if you all are going to maintain this, no objection from me, it > should go through the irq tree. OK, thanks. So this is getting quite late for 5.11, and none of it has seen -next obviously. However, the changes are additive and should only affect a single driver now. I'm talking about this series now, not Marc's companion series. I just need to hear from Thomas on any merge preference. Thanks, John
diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 88aef93eb4dd..ea8add164b89 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -15,6 +15,8 @@ #include <linux/of_irq.h> #include <linux/module.h> #include <linux/init.h> +#include <linux/interrupt.h> +#include <linux/ioport.h> #include <linux/dma-mapping.h> #include <linux/memblock.h> #include <linux/err.h> @@ -289,6 +291,125 @@ int platform_irq_count(struct platform_device *dev) } EXPORT_SYMBOL_GPL(platform_irq_count); +struct irq_affinity_devres { + unsigned int count; + unsigned int irq[]; +}; + +static void platform_disable_acpi_irq(struct platform_device *pdev, int index) +{ + struct resource *r; + + r = platform_get_resource(pdev, IORESOURCE_IRQ, index); + if (r) + irqresource_disabled(r, 0); +} + +static void devm_platform_get_irqs_affinity_release(struct device *dev, + void *res) +{ + struct irq_affinity_devres *ptr = res; + int i; + + for (i = 0; i < ptr->count; i++) { + irq_dispose_mapping(ptr->irq[i]); + + if (has_acpi_companion(dev)) + platform_disable_acpi_irq(to_platform_device(dev), i); + } +} + +/** + * devm_platform_get_irqs_affinity - devm method to get a set of IRQs for a + * device using an interrupt affinity descriptor + * @dev: platform device pointer + * @affd: affinity descriptor + * @minvec: minimum count of interrupt vectors + * @maxvec: maximum count of interrupt vectors + * @irqs: pointer holder for IRQ numbers + * + * Gets a set of IRQs for a platform device, and updates IRQ afffinty according + * to the passed affinity descriptor + * + * Return: Number of vectors on success, negative error number on failure. + */ +int devm_platform_get_irqs_affinity(struct platform_device *dev, + struct irq_affinity *affd, + unsigned int minvec, + unsigned int maxvec, + int **irqs) +{ + struct irq_affinity_devres *ptr; + struct irq_affinity_desc *desc; + size_t size; + int i, ret, nvec; + + if (!affd) + return -EPERM; + + if (maxvec < minvec) + return -ERANGE; + + nvec = platform_irq_count(dev); + + if (nvec < minvec) + return -ENOSPC; + + nvec = irq_calc_affinity_vectors(minvec, nvec, affd); + if (nvec < minvec) + return -ENOSPC; + + if (nvec > maxvec) + nvec = maxvec; + + size = sizeof(*ptr) + sizeof(unsigned int) * nvec; + ptr = devres_alloc(devm_platform_get_irqs_affinity_release, size, + GFP_KERNEL); + if (!ptr) + return -ENOMEM; + + ptr->count = nvec; + + for (i = 0; i < nvec; i++) { + int irq = platform_get_irq(dev, i); + if (irq < 0) { + ret = irq; + goto err_free_devres; + } + ptr->irq[i] = irq; + } + + desc = irq_create_affinity_masks(nvec, affd); + if (!desc) { + ret = -ENOMEM; + goto err_free_devres; + } + + for (i = 0; i < nvec; i++) { + ret = irq_update_affinity_desc(ptr->irq[i], &desc[i]); + if (ret) { + dev_err(&dev->dev, "failed to update irq%d affinity descriptor (%d)\n", + ptr->irq[i], ret); + goto err_free_desc; + } + } + + devres_add(&dev->dev, ptr); + + kfree(desc); + + *irqs = ptr->irq; + + return nvec; + +err_free_desc: + kfree(desc); +err_free_devres: + devres_free(ptr); + return ret; +} +EXPORT_SYMBOL_GPL(devm_platform_get_irqs_affinity); + /** * platform_get_resource_byname - get a resource for a device by name * @dev: platform device diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h index 77a2aada106d..4d75633e6735 100644 --- a/include/linux/platform_device.h +++ b/include/linux/platform_device.h @@ -15,6 +15,7 @@ #define PLATFORM_DEVID_NONE (-1) #define PLATFORM_DEVID_AUTO (-2) +struct irq_affinity; struct mfd_cell; struct property_entry; struct platform_device_id; @@ -70,6 +71,11 @@ devm_platform_ioremap_resource_byname(struct platform_device *pdev, extern int platform_get_irq(struct platform_device *, unsigned int); extern int platform_get_irq_optional(struct platform_device *, unsigned int); extern int platform_irq_count(struct platform_device *); +extern int devm_platform_get_irqs_affinity(struct platform_device *dev, + struct irq_affinity *affd, + unsigned int minvec, + unsigned int maxvec, + int **irqs); extern struct resource *platform_get_resource_byname(struct platform_device *, unsigned int, const char *);
Drivers for multi-queue platform devices may also want managed interrupts for handling HW queue completion interrupts, so add support. The function accepts an affinity descriptor pointer, which covers all IRQs expected for the device. The function is devm class as the only current in-tree user will also use devm method for requesting the interrupts; as such, the function is made as devm as it can ensure ordering of freeing the irq and disposing of the mapping. Signed-off-by: John Garry <john.garry@huawei.com> --- drivers/base/platform.c | 121 ++++++++++++++++++++++++++++++++ include/linux/platform_device.h | 6 ++ 2 files changed, 127 insertions(+)