[PATCH 0/4] irqchip/sg2042-msi: Fix broken affinity setting

[PATCH 0/4] irqchip/sg2042-msi: Fix broken affinity setting

[Inochi Amaoto]

When using NVME on SG2044, the NVME always complains "I/O tag XXX
(XXX) QID XX timeout, completion polled", which is caused by the
broken handler of the sg2042-msi driver.

As PLIC driver can only setting affinity when enabling, the sg2042-msi
does not properly handled affinity setting previously and enable irq in
an unexpected executing path.

Add irq_startup/irq_shutdown support to the PCI template domain,
then set irq_chip_[startup/shutdown]_parent for irq_startup/
irq_shutdown of the sg2042-msi driver. So the irq can be started
properly.

Inochi Amaoto (4):
  genirq: Add irq_chip_(startup/shutdown)_parent
  PCI/MSI: Add startup/shutdown support for per device MSI[X] domains
  irqchip/sg2042-msi: Fix broken affinity setting
  irqchip/sg2042-msi: Set MSI_FLAG_MULTI_PCI_MSI flags for SG2044

 drivers/irqchip/irq-sg2042-msi.c | 13 ++++++--
 drivers/pci/msi/irqdomain.c      | 52 ++++++++++++++++++++++++++++++++
 include/linux/irq.h              |  2 ++
 include/linux/msi.h              |  2 ++
 kernel/irq/chip.c                | 37 +++++++++++++++++++++++
 5 files changed, 104 insertions(+), 2 deletions(-)

--
2.50.1

[PATCH 1/4] genirq: Add irq_chip_(startup/shutdown)_parent

[Inochi Amaoto]

Add helper irq_chip_startup_parent and irq_chip_shutdown_parent. The
helper implement the default behavior in case irq_startup or irq_shutdown
is not implemented for the parent interrupt chip, which will fallback
to irq_chip_enable_parent or irq_chip_disable_parent if not available.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
---
 include/linux/irq.h |  2 ++
 kernel/irq/chip.c   | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index 1d6b606a81ef..890e1371f5d4 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -669,6 +669,8 @@ extern int irq_chip_set_parent_state(struct irq_data *data,
 extern int irq_chip_get_parent_state(struct irq_data *data,
 				     enum irqchip_irq_state which,
 				     bool *state);
+extern void irq_chip_shutdown_parent(struct irq_data *data);
+extern unsigned int irq_chip_startup_parent(struct irq_data *data);
 extern void irq_chip_enable_parent(struct irq_data *data);
 extern void irq_chip_disable_parent(struct irq_data *data);
 extern void irq_chip_ack_parent(struct irq_data *data);
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 0d0276378c70..3ffa0d80ddd1 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -1259,6 +1259,43 @@ int irq_chip_get_parent_state(struct irq_data *data,
 }
 EXPORT_SYMBOL_GPL(irq_chip_get_parent_state);
 
+/**
+ * irq_chip_shutdown_parent - Shutdown the parent interrupt
+ * @data:	Pointer to interrupt specific data
+ *
+ * Invokes the irq_shutdown() callback of the parent if available or falls
+ * back to irq_chip_disable_parent().
+ */
+void irq_chip_shutdown_parent(struct irq_data *data)
+{
+	struct irq_data *parent = data->parent_data;
+
+	if (parent->chip->irq_shutdown)
+		parent->chip->irq_shutdown(parent);
+	else
+		irq_chip_disable_parent(data);
+}
+EXPORT_SYMBOL_GPL(irq_chip_shutdown_parent);
+
+/**
+ * irq_chip_startup_parent - Startup the parent interrupt
+ * @data:	Pointer to interrupt specific data
+ *
+ * Invokes the irq_startup() callback of the parent if available or falls
+ * back to irq_chip_enable_parent().
+ */
+unsigned int irq_chip_startup_parent(struct irq_data *data)
+{
+	struct irq_data *parent = data->parent_data;
+
+	if (parent->chip->irq_startup)
+		return parent->chip->irq_startup(parent);
+
+	irq_chip_enable_parent(data);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(irq_chip_startup_parent);
+
 /**
  * irq_chip_enable_parent - Enable the parent interrupt (defaults to unmask if
  * NULL)
-- 
2.50.1

[PATCH 2/4] PCI/MSI: Add startup/shutdown support for per device MSI[X] domains

[Inochi Amaoto]

As The RISC-V PLIC can not apply affinity setting without calling
irq_enable(), it will make the interrupt unavaible when using as
an underlying irq chip for MSI controller.

Introduce the irq_startup/irq_shutdown for PCI domain template with
new MSI domain flag. This allow the PLIC can be properly configurated
when calling irq_startup().

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
---
 drivers/pci/msi/irqdomain.c | 52 +++++++++++++++++++++++++++++++++++++
 include/linux/msi.h         |  2 ++
 2 files changed, 54 insertions(+)

diff --git a/drivers/pci/msi/irqdomain.c b/drivers/pci/msi/irqdomain.c
index 0938ef7ebabf..f0d18cadbe20 100644
--- a/drivers/pci/msi/irqdomain.c
+++ b/drivers/pci/msi/irqdomain.c
@@ -148,6 +148,23 @@ static void pci_device_domain_set_desc(msi_alloc_info_t *arg, struct msi_desc *d
 	arg->hwirq = desc->msi_index;
 }
 
+static __always_inline void cond_shutdown_parent(struct irq_data *data)
+{
+	struct msi_domain_info *info = data->domain->host_data;
+
+	if (unlikely(info->flags & MSI_FLAG_PCI_MSI_STARTUP_PARENT))
+		irq_chip_shutdown_parent(data);
+}
+
+static __always_inline unsigned int cond_startup_parent(struct irq_data *data)
+{
+	struct msi_domain_info *info = data->domain->host_data;
+
+	if (unlikely(info->flags & MSI_FLAG_PCI_MSI_STARTUP_PARENT))
+		return irq_chip_startup_parent(data);
+	return 0;
+}
+
 static __always_inline void cond_mask_parent(struct irq_data *data)
 {
 	struct msi_domain_info *info = data->domain->host_data;
@@ -164,6 +181,23 @@ static __always_inline void cond_unmask_parent(struct irq_data *data)
 		irq_chip_unmask_parent(data);
 }
 
+static void pci_irq_shutdown_msi(struct irq_data *data)
+{
+	struct msi_desc *desc = irq_data_get_msi_desc(data);
+
+	pci_msi_mask(desc, BIT(data->irq - desc->irq));
+	cond_shutdown_parent(data);
+}
+
+static unsigned int pci_irq_startup_msi(struct irq_data *data)
+{
+	struct msi_desc *desc = irq_data_get_msi_desc(data);
+	unsigned int ret = cond_startup_parent(data);
+
+	pci_msi_unmask(desc, BIT(data->irq - desc->irq));
+	return ret;
+}
+
 static void pci_irq_mask_msi(struct irq_data *data)
 {
 	struct msi_desc *desc = irq_data_get_msi_desc(data);
@@ -194,6 +228,8 @@ static void pci_irq_unmask_msi(struct irq_data *data)
 static const struct msi_domain_template pci_msi_template = {
 	.chip = {
 		.name			= "PCI-MSI",
+		.irq_startup		= pci_irq_startup_msi,
+		.irq_shutdown		= pci_irq_shutdown_msi,
 		.irq_mask		= pci_irq_mask_msi,
 		.irq_unmask		= pci_irq_unmask_msi,
 		.irq_write_msi_msg	= pci_msi_domain_write_msg,
@@ -210,6 +246,20 @@ static const struct msi_domain_template pci_msi_template = {
 	},
 };
 
+static void pci_irq_shutdown_msix(struct irq_data *data)
+{
+	pci_msix_mask(irq_data_get_msi_desc(data));
+	cond_shutdown_parent(data);
+}
+
+static unsigned int pci_irq_startup_msix(struct irq_data *data)
+{
+	unsigned int ret = cond_startup_parent(data);
+
+	pci_msix_unmask(irq_data_get_msi_desc(data));
+	return ret;
+}
+
 static void pci_irq_mask_msix(struct irq_data *data)
 {
 	pci_msix_mask(irq_data_get_msi_desc(data));
@@ -234,6 +284,8 @@ EXPORT_SYMBOL_GPL(pci_msix_prepare_desc);
 static const struct msi_domain_template pci_msix_template = {
 	.chip = {
 		.name			= "PCI-MSIX",
+		.irq_startup		= pci_irq_startup_msix,
+		.irq_shutdown		= pci_irq_shutdown_msix,
 		.irq_mask		= pci_irq_mask_msix,
 		.irq_unmask		= pci_irq_unmask_msix,
 		.irq_write_msi_msg	= pci_msi_domain_write_msg,
diff --git a/include/linux/msi.h b/include/linux/msi.h
index e5e86a8529fb..3111ba95fbde 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -568,6 +568,8 @@ enum {
 	MSI_FLAG_PARENT_PM_DEV		= (1 << 8),
 	/* Support for parent mask/unmask */
 	MSI_FLAG_PCI_MSI_MASK_PARENT	= (1 << 9),
+	/* Support for parent startup/shutdown */
+	MSI_FLAG_PCI_MSI_STARTUP_PARENT	= (1 << 10),
 
 	/* Mask for the generic functionality */
 	MSI_GENERIC_FLAGS_MASK		= GENMASK(15, 0),
-- 
2.50.1

[PATCH 3/4] irqchip/sg2042-msi: Fix broken affinity setting

[Inochi Amaoto]

When using NVME on SG2044, the NVME always complains "I/O tag XXX
(XXX) QID XX timeout, completion polled", which is caused by the
broken handler of the sg2042-msi driver.

As PLIC driver can only setting affinity when enabling, the sg2042-msi
does not properly handled affinity setting previously and enable irq in
an unexpected executing path.

Since the PCI template domain supports irq_startup/irq_shutdown, set
irq_chip_[startup/shutdown]_parent for irq_startup/irq_shutdown. So
the irq can be started properly.

Fixes: e96b93a97c90 ("irqchip/sg2042-msi: Add the Sophgo SG2044 MSI interrupt controller")
Reported-by: Han Gao <rabenda.cn@gmail.com>
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
---
 drivers/irqchip/irq-sg2042-msi.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-sg2042-msi.c b/drivers/irqchip/irq-sg2042-msi.c
index bcfddc51bc6a..2b7ee17232ab 100644
--- a/drivers/irqchip/irq-sg2042-msi.c
+++ b/drivers/irqchip/irq-sg2042-msi.c
@@ -85,6 +85,8 @@ static void sg2042_msi_irq_compose_msi_msg(struct irq_data *d, struct msi_msg *m
 
 static const struct irq_chip sg2042_msi_middle_irq_chip = {
 	.name			= "SG2042 MSI",
+	.irq_startup		= irq_chip_startup_parent,
+	.irq_shutdown		= irq_chip_shutdown_parent,
 	.irq_ack		= sg2042_msi_irq_ack,
 	.irq_mask		= irq_chip_mask_parent,
 	.irq_unmask		= irq_chip_unmask_parent,
@@ -114,6 +116,8 @@ static void sg2044_msi_irq_compose_msi_msg(struct irq_data *d, struct msi_msg *m
 
 static struct irq_chip sg2044_msi_middle_irq_chip = {
 	.name			= "SG2044 MSI",
+	.irq_startup		= irq_chip_startup_parent,
+	.irq_shutdown		= irq_chip_shutdown_parent,
 	.irq_ack		= sg2044_msi_irq_ack,
 	.irq_mask		= irq_chip_mask_parent,
 	.irq_unmask		= irq_chip_unmask_parent,
@@ -186,7 +190,9 @@ static const struct irq_domain_ops sg204x_msi_middle_domain_ops = {
 };
 
 #define SG2042_MSI_FLAGS_REQUIRED (MSI_FLAG_USE_DEF_DOM_OPS |	\
-				   MSI_FLAG_USE_DEF_CHIP_OPS)
+				   MSI_FLAG_USE_DEF_CHIP_OPS |	\
+				   MSI_FLAG_PCI_MSI_MASK_PARENT |\
+				   MSI_FLAG_PCI_MSI_STARTUP_PARENT)
 
 #define SG2042_MSI_FLAGS_SUPPORTED MSI_GENERIC_FLAGS_MASK
 
@@ -201,7 +207,9 @@ static const struct msi_parent_ops sg2042_msi_parent_ops = {
 };
 
 #define SG2044_MSI_FLAGS_REQUIRED (MSI_FLAG_USE_DEF_DOM_OPS |	\
-				   MSI_FLAG_USE_DEF_CHIP_OPS)
+				   MSI_FLAG_USE_DEF_CHIP_OPS |	\
+				   MSI_FLAG_PCI_MSI_MASK_PARENT |\
+				   MSI_FLAG_PCI_MSI_STARTUP_PARENT)
 
 #define SG2044_MSI_FLAGS_SUPPORTED (MSI_GENERIC_FLAGS_MASK |	\
 				    MSI_FLAG_PCI_MSIX)
-- 
2.50.1

[PATCH 4/4] irqchip/sg2042-msi: Set MSI_FLAG_MULTI_PCI_MSI flags for SG2044

[Inochi Amaoto]

The MSI controller on SG2044 has the ability to allocate
multiple PCI MSI interrupt if the controller supports it.
Add the missing flag so the controller can make full use
of it.

Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
---
 drivers/irqchip/irq-sg2042-msi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/irqchip/irq-sg2042-msi.c b/drivers/irqchip/irq-sg2042-msi.c
index 2b7ee17232ab..6efb34a91937 100644
--- a/drivers/irqchip/irq-sg2042-msi.c
+++ b/drivers/irqchip/irq-sg2042-msi.c
@@ -212,6 +212,7 @@ static const struct msi_parent_ops sg2042_msi_parent_ops = {
 				   MSI_FLAG_PCI_MSI_STARTUP_PARENT)
 
 #define SG2044_MSI_FLAGS_SUPPORTED (MSI_GENERIC_FLAGS_MASK |	\
+				    MSI_FLAG_MULTI_PCI_MSI |	\
 				    MSI_FLAG_PCI_MSIX)
 
 static const struct msi_parent_ops sg2044_msi_parent_ops = {
-- 
2.50.1

Re: [PATCH 2/4] PCI/MSI: Add startup/shutdown support for per device MSI[X] domains

[Bjorn Helgaas]

In subject, s/MSI[X]// and s/support for/for/

The "MSI[X]" notation really isn't used anywhere else, and we already
include "PCI/MSI" in the prefix, so I don't think we need it again.

On Thu, Aug 07, 2025 at 07:23:23PM +0800, Inochi Amaoto wrote:
> As The RISC-V PLIC can not apply affinity setting without calling
> irq_enable(), it will make the interrupt unavaible when using as
> an underlying irq chip for MSI controller.

s/As The/As the/
s/unavaible/unavailable/
s/irq chip/IRQ chip/

> Introduce the irq_startup/irq_shutdown for PCI domain template with
> new MSI domain flag. This allow the PLIC can be properly configurated
> when calling irq_startup().

Maybe something like:

  Implement .irq_startup() and .irq_shutdown() for the PCI MSI and
  MSI-X templates.  For chips that specify MSI_FLAG_PCI_MSI_STARTUP_PARENT, 
  these startup and shutdown the parent as well, which allows ...

s/This allow/This allows/
s/can be properly configurated/to be configured/

Evidently PLIC depends on this "parent" connection, but that isn't
explained at all in the commit log.

> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
> ---
>  drivers/pci/msi/irqdomain.c | 52 +++++++++++++++++++++++++++++++++++++
>  include/linux/msi.h         |  2 ++
>  2 files changed, 54 insertions(+)
> 
> diff --git a/drivers/pci/msi/irqdomain.c b/drivers/pci/msi/irqdomain.c
> index 0938ef7ebabf..f0d18cadbe20 100644
> --- a/drivers/pci/msi/irqdomain.c
> +++ b/drivers/pci/msi/irqdomain.c
> @@ -148,6 +148,23 @@ static void pci_device_domain_set_desc(msi_alloc_info_t *arg, struct msi_desc *d
>  	arg->hwirq = desc->msi_index;
>  }
>  
> +static __always_inline void cond_shutdown_parent(struct irq_data *data)

Is there a functional reason why we need __always_inline?

If not, it seems like this annotation is just clutter, and the compiler
will probably inline it all by itself.

> +{
> +	struct msi_domain_info *info = data->domain->host_data;
> +
> +	if (unlikely(info->flags & MSI_FLAG_PCI_MSI_STARTUP_PARENT))
> +		irq_chip_shutdown_parent(data);
> +}
> +
> +static __always_inline unsigned int cond_startup_parent(struct irq_data *data)
> +{
> +	struct msi_domain_info *info = data->domain->host_data;
> +
> +	if (unlikely(info->flags & MSI_FLAG_PCI_MSI_STARTUP_PARENT))
> +		return irq_chip_startup_parent(data);
> +	return 0;
> +}
> +
>  static __always_inline void cond_mask_parent(struct irq_data *data)
>  {
>  	struct msi_domain_info *info = data->domain->host_data;
> @@ -164,6 +181,23 @@ static __always_inline void cond_unmask_parent(struct irq_data *data)
>  		irq_chip_unmask_parent(data);
>  }
>  
> +static void pci_irq_shutdown_msi(struct irq_data *data)
> +{
> +	struct msi_desc *desc = irq_data_get_msi_desc(data);
> +
> +	pci_msi_mask(desc, BIT(data->irq - desc->irq));
> +	cond_shutdown_parent(data);
> +}
> +
> +static unsigned int pci_irq_startup_msi(struct irq_data *data)
> +{
> +	struct msi_desc *desc = irq_data_get_msi_desc(data);
> +	unsigned int ret = cond_startup_parent(data);
> +
> +	pci_msi_unmask(desc, BIT(data->irq - desc->irq));
> +	return ret;
> +}
> +
>  static void pci_irq_mask_msi(struct irq_data *data)
>  {
>  	struct msi_desc *desc = irq_data_get_msi_desc(data);
> @@ -194,6 +228,8 @@ static void pci_irq_unmask_msi(struct irq_data *data)
>  static const struct msi_domain_template pci_msi_template = {
>  	.chip = {
>  		.name			= "PCI-MSI",
> +		.irq_startup		= pci_irq_startup_msi,
> +		.irq_shutdown		= pci_irq_shutdown_msi,
>  		.irq_mask		= pci_irq_mask_msi,
>  		.irq_unmask		= pci_irq_unmask_msi,
>  		.irq_write_msi_msg	= pci_msi_domain_write_msg,
> @@ -210,6 +246,20 @@ static const struct msi_domain_template pci_msi_template = {
>  	},
>  };
>  
> +static void pci_irq_shutdown_msix(struct irq_data *data)
> +{
> +	pci_msix_mask(irq_data_get_msi_desc(data));
> +	cond_shutdown_parent(data);
> +}
> +
> +static unsigned int pci_irq_startup_msix(struct irq_data *data)
> +{
> +	unsigned int ret = cond_startup_parent(data);
> +
> +	pci_msix_unmask(irq_data_get_msi_desc(data));
> +	return ret;
> +}
> +
>  static void pci_irq_mask_msix(struct irq_data *data)
>  {
>  	pci_msix_mask(irq_data_get_msi_desc(data));
> @@ -234,6 +284,8 @@ EXPORT_SYMBOL_GPL(pci_msix_prepare_desc);
>  static const struct msi_domain_template pci_msix_template = {
>  	.chip = {
>  		.name			= "PCI-MSIX",
> +		.irq_startup		= pci_irq_startup_msix,
> +		.irq_shutdown		= pci_irq_shutdown_msix,
>  		.irq_mask		= pci_irq_mask_msix,
>  		.irq_unmask		= pci_irq_unmask_msix,
>  		.irq_write_msi_msg	= pci_msi_domain_write_msg,
> diff --git a/include/linux/msi.h b/include/linux/msi.h
> index e5e86a8529fb..3111ba95fbde 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -568,6 +568,8 @@ enum {
>  	MSI_FLAG_PARENT_PM_DEV		= (1 << 8),
>  	/* Support for parent mask/unmask */
>  	MSI_FLAG_PCI_MSI_MASK_PARENT	= (1 << 9),
> +	/* Support for parent startup/shutdown */
> +	MSI_FLAG_PCI_MSI_STARTUP_PARENT	= (1 << 10),
>  
>  	/* Mask for the generic functionality */
>  	MSI_GENERIC_FLAGS_MASK		= GENMASK(15, 0),
> -- 
> 2.50.1
> 

Re: [PATCH 2/4] PCI/MSI: Add startup/shutdown support for per device MSI[X] domains

[Inochi Amaoto]

On Thu, Aug 07, 2025 at 11:25:21AM -0500, Bjorn Helgaas wrote:
> In subject, s/MSI[X]// and s/support for/for/
> 
> The "MSI[X]" notation really isn't used anywhere else, and we already
> include "PCI/MSI" in the prefix, so I don't think we need it again.
> 
> On Thu, Aug 07, 2025 at 07:23:23PM +0800, Inochi Amaoto wrote:
> > As The RISC-V PLIC can not apply affinity setting without calling
> > irq_enable(), it will make the interrupt unavaible when using as
> > an underlying irq chip for MSI controller.
> 
> s/As The/As the/
> s/unavaible/unavailable/
> s/irq chip/IRQ chip/
> 

These are good for me. I will take it.

> > Introduce the irq_startup/irq_shutdown for PCI domain template with
> > new MSI domain flag. This allow the PLIC can be properly configurated
> > when calling irq_startup().
> 
> Maybe something like:
> 
>   Implement .irq_startup() and .irq_shutdown() for the PCI MSI and
>   MSI-X templates.  For chips that specify MSI_FLAG_PCI_MSI_STARTUP_PARENT, 
>   these startup and shutdown the parent as well, which allows ...
> 
> s/This allow/This allows/
> s/can be properly configurated/to be configured/
> 

Thanks, I will update my commit log.

> Evidently PLIC depends on this "parent" connection, but that isn't
> explained at all in the commit log.
> 

When call irq_startup, the PLIC is called irq_enable() instead of 
irq_unmask(), the the irq on PLIC can be enabled.

I will add this to the commit log.

> > Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> > Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
> > ---
> >  drivers/pci/msi/irqdomain.c | 52 +++++++++++++++++++++++++++++++++++++
> >  include/linux/msi.h         |  2 ++
> >  2 files changed, 54 insertions(+)
> > 
> > diff --git a/drivers/pci/msi/irqdomain.c b/drivers/pci/msi/irqdomain.c
> > index 0938ef7ebabf..f0d18cadbe20 100644
> > --- a/drivers/pci/msi/irqdomain.c
> > +++ b/drivers/pci/msi/irqdomain.c
> > @@ -148,6 +148,23 @@ static void pci_device_domain_set_desc(msi_alloc_info_t *arg, struct msi_desc *d
> >  	arg->hwirq = desc->msi_index;
> >  }
> >  
> > +static __always_inline void cond_shutdown_parent(struct irq_data *data)
> 
> Is there a functional reason why we need __always_inline?
> 

I am not sure for this. As I found other cond_[mask/unmask]_parent()
also have this attribute, I added this as well.

> If not, it seems like this annotation is just clutter, and the compiler
> will probably inline it all by itself.
> 

I will see if someone know the reason. If there is no other objection,
I will remove this in the next version

Regards,
Inochi

Re: [PATCH 2/4] PCI/MSI: Add startup/shutdown support for per device MSI[X] domains

[Thomas Gleixner]

On Fri, Aug 08 2025 at 07:18, Inochi Amaoto wrote:
> On Thu, Aug 07, 2025 at 11:25:21AM -0500, Bjorn Helgaas wrote:
>> >  
>> > +static __always_inline void cond_shutdown_parent(struct irq_data *data)
>> 
>> Is there a functional reason why we need __always_inline?
>>
> I am not sure for this. As I found other cond_[mask/unmask]_parent()
> also have this attribute, I added this as well.
>
>> If not, it seems like this annotation is just clutter, and the compiler
>> will probably inline it all by itself.

Most of the time, but yes in this case it's irrelevant.

Re: [PATCH 4/4] irqchip/sg2042-msi: Set MSI_FLAG_MULTI_PCI_MSI flags for SG2044

[Thomas Gleixner]

On Thu, Aug 07 2025 at 19:23, Inochi Amaoto wrote:

> The MSI controller on SG2044 has the ability to allocate
> multiple PCI MSI interrupt if the controller supports it.

interrupts ...

Which controller?

if the PCI device supports multi MSI.

> Add the missing flag so the controller can make full use
> of it.

Again, the controller does not make use of it. The controller supports
it and the device driver can use it if both the PCI device and the
underlying MSI controller support it.

Re: [PATCH 1/4] genirq: Add irq_chip_(startup/shutdown)_parent

[Thomas Gleixner]

On Thu, Aug 07 2025 at 19:23, Inochi Amaoto wrote:

Please use 'function()' notation for functions. See

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

I'm sure I pointed you to this documented at least three times in the
past. Do you think this was written for entertainment?

> Add helper irq_chip_startup_parent and irq_chip_shutdown_parent. The
> helper implement the default behavior in case irq_startup or irq_shutdown
> is not implemented for the parent interrupt chip, which will fallback
> to irq_chip_enable_parent or irq_chip_disable_parent if not available.

Also please use the documented structure for change logs. Starting with
'Add' is just wrong. See Documentation.

Re: [PATCH 1/4] genirq: Add irq_chip_(startup/shutdown)_parent

[Andy Shevchenko]

On Thu, Aug 07, 2025 at 07:23:22PM +0800, Inochi Amaoto wrote:
> Add helper irq_chip_startup_parent and irq_chip_shutdown_parent. The

irq_chip_startup_parent() and irq_chip_shutdown_parent()

> helper implement the default behavior in case irq_startup or irq_shutdown

In the same way...

> is not implemented for the parent interrupt chip, which will fallback
> to irq_chip_enable_parent or irq_chip_disable_parent if not available.

In the same way...


...

> +/**
> + * irq_chip_startup_parent - Startup the parent interrupt
> + * @data:	Pointer to interrupt specific data
> + *
> + * Invokes the irq_startup() callback of the parent if available or falls
> + * back to irq_chip_enable_parent().

kernel-doc validator is not happy: Missing Return section.

> + */

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] irqchip/sg2042-msi: Fix broken affinity setting

[Andy Shevchenko]

On Thu, Aug 07, 2025 at 07:23:24PM +0800, Inochi Amaoto wrote:
> When using NVME on SG2044, the NVME always complains "I/O tag XXX
> (XXX) QID XX timeout, completion polled", which is caused by the
> broken handler of the sg2042-msi driver.
> 
> As PLIC driver can only setting affinity when enabling, the sg2042-msi
> does not properly handled affinity setting previously and enable irq in
> an unexpected executing path.
> 
> Since the PCI template domain supports irq_startup/irq_shutdown, set
> irq_chip_[startup/shutdown]_parent for irq_startup/irq_shutdown. So
> the irq can be started properly.

> Fixes: e96b93a97c90 ("irqchip/sg2042-msi: Add the Sophgo SG2044 MSI interrupt controller")
> Reported-by: Han Gao <rabenda.cn@gmail.com>

Closes ?

> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Inochi Amaoto <inochiama@gmail.com>

...

>  #define SG2042_MSI_FLAGS_REQUIRED (MSI_FLAG_USE_DEF_DOM_OPS |	\
> -				   MSI_FLAG_USE_DEF_CHIP_OPS)
> +				   MSI_FLAG_USE_DEF_CHIP_OPS |	\
> +				   MSI_FLAG_PCI_MSI_MASK_PARENT |\

Can we indent \ to be on the same column (using TABs)?

> +				   MSI_FLAG_PCI_MSI_STARTUP_PARENT)

...

>  #define SG2044_MSI_FLAGS_REQUIRED (MSI_FLAG_USE_DEF_DOM_OPS |	\
> -				   MSI_FLAG_USE_DEF_CHIP_OPS)
> +				   MSI_FLAG_USE_DEF_CHIP_OPS |	\
> +				   MSI_FLAG_PCI_MSI_MASK_PARENT |\
> +				   MSI_FLAG_PCI_MSI_STARTUP_PARENT)

Ditto.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/4] genirq: Add irq_chip_(startup/shutdown)_parent

[Inochi Amaoto]

On Mon, Aug 11, 2025 at 04:37:30PM +0200, Thomas Gleixner wrote:
> On Thu, Aug 07 2025 at 19:23, Inochi Amaoto wrote:
> 
> Please use 'function()' notation for functions. See
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html
> 
> I'm sure I pointed you to this documented at least three times in the
> past. Do you think this was written for entertainment?
> 

Yeah, I have remembered to add this brace for the function. It seems
like I have forgot to fix this when I do the rebase. I apology for it.

> > Add helper irq_chip_startup_parent and irq_chip_shutdown_parent. The
> > helper implement the default behavior in case irq_startup or irq_shutdown
> > is not implemented for the parent interrupt chip, which will fallback
> > to irq_chip_enable_parent or irq_chip_disable_parent if not available.
> 
> Also please use the documented structure for change logs. Starting with
> 'Add' is just wrong. See Documentation.
> 

OK, I will structure the changelog.

Regards,
Inochi.

Re: [PATCH 4/4] irqchip/sg2042-msi: Set MSI_FLAG_MULTI_PCI_MSI flags for SG2044

[Inochi Amaoto]

On Mon, Aug 11, 2025 at 04:33:06PM +0200, Thomas Gleixner wrote:
> On Thu, Aug 07 2025 at 19:23, Inochi Amaoto wrote:
> 
> > The MSI controller on SG2044 has the ability to allocate
> > multiple PCI MSI interrupt if the controller supports it.
> 
> interrupts ...
> 
> Which controller?
> 
> if the PCI device supports multi MSI.
> 

The PCIe controller, in detail, the Synopsys DesignWare PCIe controller.
I will update the comment.

> > Add the missing flag so the controller can make full use
> > of it.
> 
> Again, the controller does not make use of it. The controller supports
> it and the device driver can use it if both the PCI device and the
> underlying MSI controller support it.
> 
> 

Re: [PATCH 3/4] irqchip/sg2042-msi: Fix broken affinity setting

[Inochi Amaoto]

On Mon, Aug 11, 2025 at 04:43:00PM +0200, Andy Shevchenko wrote:
> On Thu, Aug 07, 2025 at 07:23:24PM +0800, Inochi Amaoto wrote:
> > When using NVME on SG2044, the NVME always complains "I/O tag XXX
> > (XXX) QID XX timeout, completion polled", which is caused by the
> > broken handler of the sg2042-msi driver.
> > 
> > As PLIC driver can only setting affinity when enabling, the sg2042-msi
> > does not properly handled affinity setting previously and enable irq in
> > an unexpected executing path.
> > 
> > Since the PCI template domain supports irq_startup/irq_shutdown, set
> > irq_chip_[startup/shutdown]_parent for irq_startup/irq_shutdown. So
> > the irq can be started properly.
> 
> > Fixes: e96b93a97c90 ("irqchip/sg2042-msi: Add the Sophgo SG2044 MSI interrupt controller")
> > Reported-by: Han Gao <rabenda.cn@gmail.com>
> 
> Closes ?
> 

I got a direct private email from Han, so I think there is no pulic
Closes.

> > Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> > Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
> 
> ...
> 
> >  #define SG2042_MSI_FLAGS_REQUIRED (MSI_FLAG_USE_DEF_DOM_OPS |	\
> > -				   MSI_FLAG_USE_DEF_CHIP_OPS)
> > +				   MSI_FLAG_USE_DEF_CHIP_OPS |	\
> > +				   MSI_FLAG_PCI_MSI_MASK_PARENT |\
> 
> Can we indent \ to be on the same column (using TABs)?
> 

Yeah, of course

> > +				   MSI_FLAG_PCI_MSI_STARTUP_PARENT)
> 
> ...
> 
> >  #define SG2044_MSI_FLAGS_REQUIRED (MSI_FLAG_USE_DEF_DOM_OPS |	\
> > -				   MSI_FLAG_USE_DEF_CHIP_OPS)
> > +				   MSI_FLAG_USE_DEF_CHIP_OPS |	\
> > +				   MSI_FLAG_PCI_MSI_MASK_PARENT |\
> > +				   MSI_FLAG_PCI_MSI_STARTUP_PARENT)
> 
> Ditto.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH 4/4] irqchip/sg2042-msi: Set MSI_FLAG_MULTI_PCI_MSI flags for SG2044

[Inochi Amaoto]

On Tue, Aug 12, 2025 at 06:36:20AM +0800, Inochi Amaoto wrote:
> On Mon, Aug 11, 2025 at 04:33:06PM +0200, Thomas Gleixner wrote:
> > On Thu, Aug 07 2025 at 19:23, Inochi Amaoto wrote:
> > 
> > > The MSI controller on SG2044 has the ability to allocate
> > > multiple PCI MSI interrupt if the controller supports it.
> > 
> > interrupts ...
> > 
> > Which controller?
> > 
> > if the PCI device supports multi MSI.
> > 
> 
> The PCIe controller, in detail, the Synopsys DesignWare PCIe controller.
> I will update the comment.

This is still too short, I mean if the PCIe controller also supports 
this feature, so the MSI controller and the PCIe device driver can
negotiate and then use this feature.

> 
> > > Add the missing flag so the controller can make full use
> > > of it.
> > 
> > Again, the controller does not make use of it. The controller supports
> > it and the device driver can use it if both the PCI device and the
> > underlying MSI controller support it.
> > 

Yeah, this does thing I want to describe.

Regards,
Inochi

Re: [PATCH 3/4] irqchip/sg2042-msi: Fix broken affinity setting

[Andy Shevchenko]

On Tue, Aug 12, 2025 at 06:37:35AM +0800, Inochi Amaoto wrote:
> On Mon, Aug 11, 2025 at 04:43:00PM +0200, Andy Shevchenko wrote:
> > On Thu, Aug 07, 2025 at 07:23:24PM +0800, Inochi Amaoto wrote:
> > > When using NVME on SG2044, the NVME always complains "I/O tag XXX
> > > (XXX) QID XX timeout, completion polled", which is caused by the
> > > broken handler of the sg2042-msi driver.
> > > 
> > > As PLIC driver can only setting affinity when enabling, the sg2042-msi
> > > does not properly handled affinity setting previously and enable irq in
> > > an unexpected executing path.
> > > 
> > > Since the PCI template domain supports irq_startup/irq_shutdown, set
> > > irq_chip_[startup/shutdown]_parent for irq_startup/irq_shutdown. So
> > > the irq can be started properly.
> > 
> > > Fixes: e96b93a97c90 ("irqchip/sg2042-msi: Add the Sophgo SG2044 MSI interrupt controller")
> > > Reported-by: Han Gao <rabenda.cn@gmail.com>
> > 
> > Closes ?
> 
> I got a direct private email from Han, so I think there is no pulic
> Closes.

It's better to follow current practise (by at least some of the fix code
authors) to make this as a comment in the patch (text, that goes after '---'
but before actual diff).

No need to resent just for that, as you basically explained it here.
(but it seems we will have a new version anyway)

-- 
With Best Regards,
Andy Shevchenko