[PATCH v5 0/5] Support managed interrupts for platform devices

[PATCH v5 0/5] Support managed interrupts for platform devices

[John Garry]

So far, managed interrupts are only used for PCI MSIs. This series adds
platform device support for managed interrupts. Initially this topic was
discussed at [0].

The method to enable managed interrupts is to allocate a group of IRQs for
the device, and then switch the interrupts to managed - this is done
through new function irq_update_affinity_desc().

Function devm_platform_get_irqs_affinity() is added as a helper to manage
this work, such that we don't need to export irq_update_affinity_desc() or
irq_create_affinity_masks().

In the devm_platform_get_irqs_affinity() release call we are required to
"put" or unmap the irqs which have been setup. The reason for this is that
per-irq mapping (and irq_desc) needs to be recreated anew for re-probing
the LLDD, such that we don't attempt to reconfigure the managed or any
other flag for an irq_desc.

For now, the HiSilicon SAS v2 hw driver is switched over. This is used
in the D05 dev board.

Performance gain observed for 6x SAS SSDs is ~357K -> 420K IOPs for fio read.

This series is tested based on Marc's "MSI: Track device proxying when
allocating MSIs" series, [1].

[0] https://lore.kernel.org/lkml/84a9411b-4ae3-1928-3d35-1666f2687ec8@huawei.com/
[1] https://lore.kernel.org/lkml/20201129135208.680293-1-maz@kernel.org/

Changes since v4:
- Add Rafael's tags (thanks)
- Expand commit message and code comment for genirq change

Changes since v3:
- Fix genirq change to re-activate interrupt if we have deactivated it
- Remove standalone platform_put_irq(), and combine code into
  devm_platform_get_irqs_affinity_release()
- Add new inline function in ioport.h rather than making
  acpi_dev_irqresource_disabled() public

Changes since v2:
- Update genirq change as follows:
 - Handle when the irq is started, active, or already managed
 - Reject update when CONFIG_GENERIC_IRQ_RESERVATION_MODE is set
- Revamp platform.c API as follows:
 - Make it devm type
 - Add platform_put_irq() and associated change in ACPI code to allow irq
   resource to be reset
 - Unmap irqs for driver removal
 - Change API to accept min and max vectors

John Garry (5):
  genirq/affinity: Add irq_update_affinity_desc()
  resource: Add irqresource_disabled()
  ACPI: Drop acpi_dev_irqresource_disabled()
  Driver core: platform: Add devm_platform_get_irqs_affinity()
  scsi: hisi_sas: Expose HW queues for v2 hw

 drivers/acpi/resource.c                |  17 +---
 drivers/base/platform.c                | 121 +++++++++++++++++++++++++
 drivers/scsi/hisi_sas/hisi_sas.h       |   4 +
 drivers/scsi/hisi_sas/hisi_sas_main.c  |  11 +++
 drivers/scsi/hisi_sas/hisi_sas_v2_hw.c |  66 +++++++++++---
 include/linux/interrupt.h              |   8 ++
 include/linux/ioport.h                 |   7 ++
 include/linux/platform_device.h        |   6 ++
 kernel/irq/manage.c                    |  70 ++++++++++++++
 9 files changed, 285 insertions(+), 25 deletions(-)

-- 
2.26.2

[PATCH v5 1/5] genirq/affinity: Add irq_update_affinity_desc()

[John Garry]

Add a function to allow the affinity of an interrupt be switched to
managed, such that interrupts allocated for platform devices may be
managed.

This new interface has certain limitations, and attempts to use it in the
following circumstances will fail:
- For when the kernel is configured for generic IRQ reservation mode (in
  config GENERIC_IRQ_RESERVATION_MODE). The reason being that it could
  conflict with managed vs. non-managed interrupt accounting.
- The interrupt is already started, which should not be the case during
  init
- The interrupt is already configured as managed, which means double init

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: John Garry <john.garry@huawei.com>
---
 include/linux/interrupt.h |  8 +++++
 kernel/irq/manage.c       | 70 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index ee8299eb1f52..870b3251e174 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -352,6 +352,8 @@ extern int irq_can_set_affinity(unsigned int irq);
 extern int irq_select_affinity(unsigned int irq);
 
 extern int irq_set_affinity_hint(unsigned int irq, const struct cpumask *m);
+extern int irq_update_affinity_desc(unsigned int irq,
+				    struct irq_affinity_desc *affinity);
 
 extern int
 irq_set_affinity_notifier(unsigned int irq, struct irq_affinity_notify *notify);
@@ -387,6 +389,12 @@ static inline int irq_set_affinity_hint(unsigned int irq,
 	return -EINVAL;
 }
 
+static inline int irq_update_affinity_desc(unsigned int irq,
+					   struct irq_affinity_desc *affinity)
+{
+	return -EINVAL;
+}
+
 static inline int
 irq_set_affinity_notifier(unsigned int irq, struct irq_affinity_notify *notify)
 {
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index c460e0496006..c826ba4141fe 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -371,6 +371,76 @@ int irq_set_affinity_locked(struct irq_data *data, const struct cpumask *mask,
 	return ret;
 }
 
+/**
+ * irq_update_affinity_desc - Update affinity management for an interrupt
+ * @irq:	The interrupt number to update
+ * @affinity:	Pointer to the affinity descriptor
+ *
+ * This interface can be used to configure the affinity management of
+ * interrupts which have been allocated already.
+ *
+ * There are certain limitations on when it may be used - attempts to use it
+ * for when the kernel is configured for generic IRQ reservation mode (in
+ * config GENERIC_IRQ_RESERVATION_MODE) will fail, as it may conflict with
+ * managed/non-managed interrupt accounting. In addition, attempts to use it on
+ * an interrupt which is already started or which has already been configured
+ * as managed will also fail, as these mean invalid init state or double init.
+ */
+int irq_update_affinity_desc(unsigned int irq,
+			     struct irq_affinity_desc *affinity)
+{
+	struct irq_desc *desc;
+	unsigned long flags;
+	bool activated;
+	int ret = 0;
+
+	/*
+	 * Supporting this with the reservation scheme used by x86 needs
+	 * some more thought. Fail it for now.
+	 */
+	if (IS_ENABLED(CONFIG_GENERIC_IRQ_RESERVATION_MODE))
+		return -EOPNOTSUPP;
+
+	desc = irq_get_desc_buslock(irq, &flags, 0);
+	if (!desc)
+		return -EINVAL;
+
+	/* Requires the interrupt to be shut down */
+	if (irqd_is_started(&desc->irq_data)) {
+		ret = -EBUSY;
+		goto out_unlock;
+	}
+
+	/* Interrupts which are already managed cannot be modified */
+	if (irqd_affinity_is_managed(&desc->irq_data)) {
+		ret = -EBUSY;
+		goto out_unlock;
+	}
+
+	/*
+	 * Deactivate the interrupt. That's required to undo
+	 * anything an earlier activation has established.
+	 */
+	activated = irqd_is_activated(&desc->irq_data);
+	if (activated)
+		irq_domain_deactivate_irq(&desc->irq_data);
+
+	if (affinity->is_managed) {
+		irqd_set(&desc->irq_data, IRQD_AFFINITY_MANAGED);
+		irqd_set(&desc->irq_data, IRQD_MANAGED_SHUTDOWN);
+	}
+
+	cpumask_copy(desc->irq_common_data.affinity, &affinity->mask);
+
+	/* Restore the activation state */
+	if (activated)
+		irq_domain_activate_irq(&desc->irq_data, false);
+
+out_unlock:
+	irq_put_desc_busunlock(desc, flags);
+	return ret;
+}
+
 int __irq_set_affinity(unsigned int irq, const struct cpumask *mask, bool force)
 {
 	struct irq_desc *desc = irq_to_desc(irq);
-- 
2.26.2

[PATCH v5 2/5] resource: Add irqresource_disabled()

[John Garry]

Add a common function to set the fields for a irq resource to disabled,
which mimics what is done in acpi_dev_irqresource_disabled(), with a view
to replace that function.

Signed-off-by: John Garry <john.garry@huawei.com>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 include/linux/ioport.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 5135d4b86cd6..f9bf374f9633 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -307,6 +307,13 @@ struct resource *devm_request_free_mem_region(struct device *dev,
 struct resource *request_free_mem_region(struct resource *base,
 		unsigned long size, const char *name);
 
+static inline void irqresource_disabled(struct resource *res, u32 irq)
+{
+	res->start = irq;
+	res->end = irq;
+	res->flags = IORESOURCE_IRQ | IORESOURCE_DISABLED | IORESOURCE_UNSET;
+}
+
 #ifdef CONFIG_IO_STRICT_DEVMEM
 void revoke_devmem(struct resource *res);
 #else
-- 
2.26.2

[PATCH v5 3/5] ACPI: Drop acpi_dev_irqresource_disabled()

[John Garry]

The functionality of acpi_dev_irqresource_disabled() is same as in common
irqresource_disabled(), so drop acpi_dev_irqresource_disabled() in favour
of that function.

Signed-off-by: John Garry <john.garry@huawei.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/resource.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
index ad04824ca3ba..58203193417e 100644
--- a/drivers/acpi/resource.c
+++ b/drivers/acpi/resource.c
@@ -380,13 +380,6 @@ unsigned int acpi_dev_get_irq_type(int triggering, int polarity)
 }
 EXPORT_SYMBOL_GPL(acpi_dev_get_irq_type);
 
-static void acpi_dev_irqresource_disabled(struct resource *res, u32 gsi)
-{
-	res->start = gsi;
-	res->end = gsi;
-	res->flags = IORESOURCE_IRQ | IORESOURCE_DISABLED | IORESOURCE_UNSET;
-}
-
 static void acpi_dev_get_irqresource(struct resource *res, u32 gsi,
 				     u8 triggering, u8 polarity, u8 shareable,
 				     bool legacy)
@@ -394,7 +387,7 @@ static void acpi_dev_get_irqresource(struct resource *res, u32 gsi,
 	int irq, p, t;
 
 	if (!valid_IRQ(gsi)) {
-		acpi_dev_irqresource_disabled(res, gsi);
+		irqresource_disabled(res, gsi);
 		return;
 	}
 
@@ -426,7 +419,7 @@ static void acpi_dev_get_irqresource(struct resource *res, u32 gsi,
 		res->start = irq;
 		res->end = irq;
 	} else {
-		acpi_dev_irqresource_disabled(res, gsi);
+		irqresource_disabled(res, gsi);
 	}
 }
 
@@ -463,7 +456,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
 		 */
 		irq = &ares->data.irq;
 		if (index >= irq->interrupt_count) {
-			acpi_dev_irqresource_disabled(res, 0);
+			irqresource_disabled(res, 0);
 			return false;
 		}
 		acpi_dev_get_irqresource(res, irq->interrupts[index],
@@ -473,7 +466,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
 	case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
 		ext_irq = &ares->data.extended_irq;
 		if (index >= ext_irq->interrupt_count) {
-			acpi_dev_irqresource_disabled(res, 0);
+			irqresource_disabled(res, 0);
 			return false;
 		}
 		if (is_gsi(ext_irq))
@@ -481,7 +474,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
 					 ext_irq->triggering, ext_irq->polarity,
 					 ext_irq->shareable, false);
 		else
-			acpi_dev_irqresource_disabled(res, 0);
+			irqresource_disabled(res, 0);
 		break;
 	default:
 		res->flags = 0;
-- 
2.26.2

[PATCH v5 4/5] Driver core: platform: Add devm_platform_get_irqs_affinity()

[John Garry]

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(+)

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 *);
-- 
2.26.2

Re: [PATCH v5 4/5] Driver core: platform: Add devm_platform_get_irqs_affinity()

[Greg KH]

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

Re: [PATCH v5 4/5] Driver core: platform: Add devm_platform_get_irqs_affinity()

[John Garry]

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

Re: [PATCH v5 4/5] Driver core: platform: Add devm_platform_get_irqs_affinity()

[Greg KH]

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

Re: [PATCH v5 4/5] Driver core: platform: Add devm_platform_get_irqs_affinity()

[John Garry]

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

Re: [PATCH v5 4/5] Driver core: platform: Add devm_platform_get_irqs_affinity()

[Marc Zyngier]

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.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v5 4/5] Driver core: platform: Add devm_platform_get_irqs_affinity()

[John Garry]

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

Re: [PATCH v5 4/5] Driver core: platform: Add devm_platform_get_irqs_affinity()

[Greg KH]

On Wed, Dec 09, 2020 at 07:39:03PM +0000, Marc Zyngier wrote:
> 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... ;-)

{sigh} why do hardware engineers ignore sane busses...

Anyway, if you all are going to maintain this, no objection from me, it
should go through the irq tree.

thanks,

greg k-h

Re: [PATCH v5 4/5] Driver core: platform: Add devm_platform_get_irqs_affinity()

[John Garry]

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

[PATCH v5 5/5] scsi: hisi_sas: Expose HW queues for v2 hw

[John Garry]

As a performance enhancement, make the completion queue interrupts managed.

In addition, in commit bf0beec0607d ("blk-mq: drain I/O when all CPUs in a
hctx are offline"), CPU hotplug for MQ devices using managed interrupts
is made safe. So expose HW queues to blk-mq to take advantage of this.

Flag Scsi_host.host_tagset is also set to ensure that the HBA is not sent
more commands than it can handle. However the driver still does not use
request tag for IPTT as there are many HW bugs means that special rules
apply for IPTT allocation.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/hisi_sas/hisi_sas.h       |  4 ++
 drivers/scsi/hisi_sas/hisi_sas_main.c  | 11 +++++
 drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 66 +++++++++++++++++++++-----
 3 files changed, 68 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas.h b/drivers/scsi/hisi_sas/hisi_sas.h
index a25cfc11c96d..aa67807c5693 100644
--- a/drivers/scsi/hisi_sas/hisi_sas.h
+++ b/drivers/scsi/hisi_sas/hisi_sas.h
@@ -14,6 +14,7 @@
 #include <linux/debugfs.h>
 #include <linux/dmapool.h>
 #include <linux/iopoll.h>
+#include <linux/irq.h>
 #include <linux/lcm.h>
 #include <linux/libata.h>
 #include <linux/mfd/syscon.h>
@@ -312,6 +313,7 @@ enum {
 
 struct hisi_sas_hw {
 	int (*hw_init)(struct hisi_hba *hisi_hba);
+	int (*interrupt_preinit)(struct hisi_hba *hisi_hba);
 	void (*setup_itct)(struct hisi_hba *hisi_hba,
 			   struct hisi_sas_device *device);
 	int (*slot_index_alloc)(struct hisi_hba *hisi_hba,
@@ -418,6 +420,8 @@ struct hisi_hba {
 	u32 refclk_frequency_mhz;
 	u8 sas_addr[SAS_ADDR_SIZE];
 
+	int *irq_map; /* v2 hw */
+
 	int n_phy;
 	spinlock_t lock;
 	struct semaphore sem;
diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index c8dd8588f800..624c5ec723fb 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -2614,6 +2614,13 @@ static struct Scsi_Host *hisi_sas_shost_alloc(struct platform_device *pdev,
 	return NULL;
 }
 
+static int hisi_sas_interrupt_preinit(struct hisi_hba *hisi_hba)
+{
+	if (hisi_hba->hw->interrupt_preinit)
+		return hisi_hba->hw->interrupt_preinit(hisi_hba);
+	return 0;
+}
+
 int hisi_sas_probe(struct platform_device *pdev,
 		   const struct hisi_sas_hw *hw)
 {
@@ -2671,6 +2678,10 @@ int hisi_sas_probe(struct platform_device *pdev,
 		sha->sas_port[i] = &hisi_hba->port[i].sas_port;
 	}
 
+	rc = hisi_sas_interrupt_preinit(hisi_hba);
+	if (rc)
+		goto err_out_ha;
+
 	rc = scsi_add_host(shost, &pdev->dev);
 	if (rc)
 		goto err_out_ha;
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
index b57177b52fac..9adfdefef9ca 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
@@ -3302,6 +3302,28 @@ static irq_handler_t fatal_interrupts[HISI_SAS_FATAL_INT_NR] = {
 	fatal_axi_int_v2_hw
 };
 
+#define CQ0_IRQ_INDEX (96)
+
+static int hisi_sas_v2_interrupt_preinit(struct hisi_hba *hisi_hba)
+{
+	struct platform_device *pdev = hisi_hba->platform_dev;
+	struct Scsi_Host *shost = hisi_hba->shost;
+	struct irq_affinity desc = {
+		.pre_vectors = CQ0_IRQ_INDEX,
+		.post_vectors = 16,
+	};
+	int resv = desc.pre_vectors + desc.post_vectors, minvec = resv + 1, nvec;
+
+	nvec = devm_platform_get_irqs_affinity(pdev, &desc, minvec, 128,
+					       &hisi_hba->irq_map);
+	if (nvec < 0)
+		return nvec;
+
+	shost->nr_hw_queues = hisi_hba->cq_nvecs = nvec - resv;
+
+	return 0;
+}
+
 /*
  * There is a limitation in the hip06 chipset that we need
  * to map in all mbigen interrupts, even if they are not used.
@@ -3310,14 +3332,11 @@ static int interrupt_init_v2_hw(struct hisi_hba *hisi_hba)
 {
 	struct platform_device *pdev = hisi_hba->platform_dev;
 	struct device *dev = &pdev->dev;
-	int irq, rc = 0, irq_map[128];
+	int irq, rc = 0;
 	int i, phy_no, fatal_no, queue_no;
 
-	for (i = 0; i < 128; i++)
-		irq_map[i] = platform_get_irq(pdev, i);
-
 	for (i = 0; i < HISI_SAS_PHY_INT_NR; i++) {
-		irq = irq_map[i + 1]; /* Phy up/down is irq1 */
+		irq = hisi_hba->irq_map[i + 1]; /* Phy up/down is irq1 */
 		rc = devm_request_irq(dev, irq, phy_interrupts[i], 0,
 				      DRV_NAME " phy", hisi_hba);
 		if (rc) {
@@ -3331,7 +3350,7 @@ static int interrupt_init_v2_hw(struct hisi_hba *hisi_hba)
 	for (phy_no = 0; phy_no < hisi_hba->n_phy; phy_no++) {
 		struct hisi_sas_phy *phy = &hisi_hba->phy[phy_no];
 
-		irq = irq_map[phy_no + 72];
+		irq = hisi_hba->irq_map[phy_no + 72];
 		rc = devm_request_irq(dev, irq, sata_int_v2_hw, 0,
 				      DRV_NAME " sata", phy);
 		if (rc) {
@@ -3343,7 +3362,7 @@ static int interrupt_init_v2_hw(struct hisi_hba *hisi_hba)
 	}
 
 	for (fatal_no = 0; fatal_no < HISI_SAS_FATAL_INT_NR; fatal_no++) {
-		irq = irq_map[fatal_no + 81];
+		irq = hisi_hba->irq_map[fatal_no + 81];
 		rc = devm_request_irq(dev, irq, fatal_interrupts[fatal_no], 0,
 				      DRV_NAME " fatal", hisi_hba);
 		if (rc) {
@@ -3354,24 +3373,22 @@ static int interrupt_init_v2_hw(struct hisi_hba *hisi_hba)
 		}
 	}
 
-	for (queue_no = 0; queue_no < hisi_hba->queue_count; queue_no++) {
+	for (queue_no = 0; queue_no < hisi_hba->cq_nvecs; queue_no++) {
 		struct hisi_sas_cq *cq = &hisi_hba->cq[queue_no];
 
-		cq->irq_no = irq_map[queue_no + 96];
+		cq->irq_no = hisi_hba->irq_map[queue_no + 96];
 		rc = devm_request_threaded_irq(dev, cq->irq_no,
 					       cq_interrupt_v2_hw,
 					       cq_thread_v2_hw, IRQF_ONESHOT,
 					       DRV_NAME " cq", cq);
 		if (rc) {
 			dev_err(dev, "irq init: could not request cq interrupt %d, rc=%d\n",
-				irq, rc);
+					cq->irq_no, rc);
 			rc = -ENOENT;
 			goto err_out;
 		}
+		cq->irq_mask = irq_get_affinity_mask(cq->irq_no);
 	}
-
-	hisi_hba->cq_nvecs = hisi_hba->queue_count;
-
 err_out:
 	return rc;
 }
@@ -3529,6 +3546,26 @@ static struct device_attribute *host_attrs_v2_hw[] = {
 	NULL
 };
 
+static int map_queues_v2_hw(struct Scsi_Host *shost)
+{
+	struct hisi_hba *hisi_hba = shost_priv(shost);
+	struct blk_mq_queue_map *qmap = &shost->tag_set.map[HCTX_TYPE_DEFAULT];
+	const struct cpumask *mask;
+	unsigned int queue, cpu;
+
+	for (queue = 0; queue < qmap->nr_queues; queue++) {
+		mask = irq_get_affinity_mask(hisi_hba->irq_map[96 + queue]);
+		if (!mask)
+			continue;
+
+		for_each_cpu(cpu, mask)
+			qmap->mq_map[cpu] = qmap->queue_offset + queue;
+	}
+
+	return 0;
+
+}
+
 static struct scsi_host_template sht_v2_hw = {
 	.name			= DRV_NAME,
 	.proc_name		= DRV_NAME,
@@ -3553,10 +3590,13 @@ static struct scsi_host_template sht_v2_hw = {
 #endif
 	.shost_attrs		= host_attrs_v2_hw,
 	.host_reset		= hisi_sas_host_reset,
+	.map_queues		= map_queues_v2_hw,
+	.host_tagset		= 1,
 };
 
 static const struct hisi_sas_hw hisi_sas_v2_hw = {
 	.hw_init = hisi_sas_v2_init,
+	.interrupt_preinit = hisi_sas_v2_interrupt_preinit,
 	.setup_itct = setup_itct_v2_hw,
 	.slot_index_alloc = slot_index_alloc_quirk_v2_hw,
 	.alloc_dev = alloc_dev_quirk_v2_hw,
-- 
2.26.2

Re: [PATCH v5 5/5] scsi: hisi_sas: Expose HW queues for v2 hw

[John Garry]

On 02/12/2020 10:36, John Garry wrote:
> As a performance enhancement, make the completion queue interrupts managed.
> 
> In addition, in commit bf0beec0607d ("blk-mq: drain I/O when all CPUs in a
> hctx are offline"), CPU hotplug for MQ devices using managed interrupts
> is made safe. So expose HW queues to blk-mq to take advantage of this.
> 
> Flag Scsi_host.host_tagset is also set to ensure that the HBA is not sent
> more commands than it can handle. However the driver still does not use
> request tag for IPTT as there are many HW bugs means that special rules
> apply for IPTT allocation.
> 

Hi Martin, James,

Is there some way in this patch can be merged for 5.11 via the SCSI 
tree? It has a dependency on the earlier patches in the series, now 
picked up via irqchip tree. I've seen multiple rounds of SCSI pull 
requests before, but not sure when we have that or if the tree is 
rebased for those.

Thanks,
John


> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>   drivers/scsi/hisi_sas/hisi_sas.h       |  4 ++
>   drivers/scsi/hisi_sas/hisi_sas_main.c  | 11 +++++
>   drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 66 +++++++++++++++++++++-----
>   3 files changed, 68 insertions(+), 13 deletions(-)
> 

Re: [PATCH v5 5/5] scsi: hisi_sas: Expose HW queues for v2 hw

[Martin K. Petersen]

John,

> Is there some way in this patch can be merged for 5.11 via the SCSI
> tree? It has a dependency on the earlier patches in the series, now 
> picked up via irqchip tree. I've seen multiple rounds of SCSI pull
> requests before, but not sure when we have that or if the tree is 
> rebased for those.

I'll do a postmerge branch for a few things, including your patch and
the megaraid patch.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v5 5/5] scsi: hisi_sas: Expose HW queues for v2 hw

[John Garry]

On 11/12/2020 17:20, Martin K. Petersen wrote:
>> Is there some way in this patch can be merged for 5.11 via the SCSI
>> tree? It has a dependency on the earlier patches in the series, now
>> picked up via irqchip tree. I've seen multiple rounds of SCSI pull
>> requests before, but not sure when we have that or if the tree is
>> rebased for those.
> I'll do a postmerge branch for a few things, including your patch and
> the megaraid patch.

Great, thanks.

By a strange coincidence both those patches are doing the same thing.

John

Re: [PATCH v5 0/5] Support managed interrupts for platform devices

[Marc Zyngier]

On Wed, 2 Dec 2020 18:36:52 +0800, John Garry wrote:
> So far, managed interrupts are only used for PCI MSIs. This series adds
> platform device support for managed interrupts. Initially this topic was
> discussed at [0].
> 
> The method to enable managed interrupts is to allocate a group of IRQs for
> the device, and then switch the interrupts to managed - this is done
> through new function irq_update_affinity_desc().
> 
> [...]

Applied to irq/irqchip-next, thanks!

[1/5] genirq/affinity: Add irq_update_affinity_desc()
      commit: 1d3aec89286254487df7641c30f1b14ad1d127a5
[2/5] resource: Add irqresource_disabled()
      commit: 9806731db684a475ade1e95d166089b9edbd9da3
[3/5] ACPI: Drop acpi_dev_irqresource_disabled()
      commit: 1c3f69b4543af0aad514c127298e5ea40392575d
[4/5] Driver core: platform: Add devm_platform_get_irqs_affinity()
      commit: e15f2fa959f2cce8a05e8e3a596e75d068cd42c5

Please route patch 5 via the SCSI tree when possible.

Cheers,

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