* [PATCH] irqchip/armada-370-xp: Enable MSI affinity configuration @ 2022-04-21 1:57 Nathan Rossi 2022-04-20 23:19 ` Marc Zyngier 0 siblings, 1 reply; 11+ messages in thread From: Nathan Rossi @ 2022-04-21 1:57 UTC (permalink / raw) To: linux-arm-kernel, linux-kernel Cc: Nathan Rossi, Nathan Rossi, Andrew Lunn, Gregory Clement, Sebastian Hesselbarth, Thomas Gleixner, Marc Zyngier From: Nathan Rossi <nathan.rossi@digi.com> With multiple devices attached via PCIe to an Armada 385 it is possible to overwhelm a single CPU with MSI interrupts. Under certain scenarios configuring the interrupts to be handled by more than one CPU would prevent the system from being overwhelmed. However the irqchip-aramada-370-xp driver is configured to only handle MSIs on the boot CPU, and provides no affinity configuration. This change adds support to the armada-370-xp driver to allow for configuring the affinity of specific MSI irqs and to generate the interrupts on secondary CPUs. This is done by enabling the private doorbell for all online CPUs and configures all CPUs to unmask MSI specific private doorbell bits. The CPU affinity selection of the interrupt is handled by the target list of the software triggered interrupt value, which is provided as the MSI message. The message has the associated CPU bit set for the target CPU. For private doorbell interrupts only one bit can be set otherwise all CPUs will receive the interrupt, so the lowest CPU in the affinity mask is used. This means that by default the first CPU will handle all the interrupts as was the case before. Signed-off-by: Nathan Rossi <nathan.rossi@digi.com> --- drivers/irqchip/irq-armada-370-xp.c | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c index 5b8d571c04..42c257f576 100644 --- a/drivers/irqchip/irq-armada-370-xp.c +++ b/drivers/irqchip/irq-armada-370-xp.c @@ -209,15 +209,37 @@ static struct msi_domain_info armada_370_xp_msi_domain_info = { static void armada_370_xp_compose_msi_msg(struct irq_data *data, struct msi_msg *msg) { +#ifdef CONFIG_SMP + unsigned int cpu = cpumask_first(irq_data_get_effective_affinity_mask(data)); + + msg->data = (1 << (cpu + 8)) | (data->hwirq + PCI_MSI_DOORBELL_START); +#else + msg->data = 0xf00 | (data->hwirq + PCI_MSI_DOORBELL_START); +#endif msg->address_lo = lower_32_bits(msi_doorbell_addr); msg->address_hi = upper_32_bits(msi_doorbell_addr); - msg->data = 0xf00 | (data->hwirq + PCI_MSI_DOORBELL_START); } static int armada_370_xp_msi_set_affinity(struct irq_data *irq_data, const struct cpumask *mask, bool force) { - return -EINVAL; +#ifdef CONFIG_SMP + unsigned int cpu; + + if (!force) + cpu = cpumask_any_and(mask, cpu_online_mask); + else + cpu = cpumask_first(mask); + + if (cpu >= nr_cpu_ids) + return -EINVAL; + + irq_data_update_effective_affinity(irq_data, cpumask_of(cpu)); + + return IRQ_SET_MASK_OK; +#else + return -EINVAL; +#endif } static struct irq_chip armada_370_xp_msi_bottom_irq_chip = { @@ -482,6 +504,7 @@ static void armada_xp_mpic_smp_cpu_init(void) static void armada_xp_mpic_reenable_percpu(void) { unsigned int irq; + u32 reg; /* Re-enable per-CPU interrupts that were enabled before suspend */ for (irq = 0; irq < ARMADA_370_XP_MAX_PER_CPU_IRQS; irq++) { @@ -501,6 +524,13 @@ static void armada_xp_mpic_reenable_percpu(void) } ipi_resume(); + + /* Enable MSI doorbell mask and combined cpu local interrupt */ + reg = readl(per_cpu_int_base + ARMADA_370_XP_IN_DRBEL_MSK_OFFS) + | PCI_MSI_DOORBELL_MASK; + writel(reg, per_cpu_int_base + ARMADA_370_XP_IN_DRBEL_MSK_OFFS); + /* Unmask local doorbell interrupt */ + writel(1, per_cpu_int_base + ARMADA_370_XP_INT_CLEAR_MASK_OFFS); } static int armada_xp_mpic_starting_cpu(unsigned int cpu) --- 2.35.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] irqchip/armada-370-xp: Enable MSI affinity configuration 2022-04-21 1:57 [PATCH] irqchip/armada-370-xp: Enable MSI affinity configuration Nathan Rossi @ 2022-04-20 23:19 ` Marc Zyngier 2022-04-21 8:32 ` Nathan Rossi 0 siblings, 1 reply; 11+ messages in thread From: Marc Zyngier @ 2022-04-20 23:19 UTC (permalink / raw) To: Nathan Rossi Cc: linux-arm-kernel, linux-kernel, Nathan Rossi, Andrew Lunn, Gregory Clement, Sebastian Hesselbarth, Thomas Gleixner Hi Nathan, On Thu, 21 Apr 2022 02:57:28 +0100, Nathan Rossi <nathan@nathanrossi.com> wrote: > > From: Nathan Rossi <nathan.rossi@digi.com> > > With multiple devices attached via PCIe to an Armada 385 it is possible > to overwhelm a single CPU with MSI interrupts. Under certain scenarios > configuring the interrupts to be handled by more than one CPU would > prevent the system from being overwhelmed. However the > irqchip-aramada-370-xp driver is configured to only handle MSIs on the > boot CPU, and provides no affinity configuration. > > This change adds support to the armada-370-xp driver to allow for > configuring the affinity of specific MSI irqs and to generate the > interrupts on secondary CPUs. This is done by enabling the private > doorbell for all online CPUs and configures all CPUs to unmask MSI > specific private doorbell bits. The CPU affinity selection of the > interrupt is handled by the target list of the software triggered > interrupt value, which is provided as the MSI message. The message has > the associated CPU bit set for the target CPU. For private doorbell > interrupts only one bit can be set otherwise all CPUs will receive the > interrupt, so the lowest CPU in the affinity mask is used. This means > that by default the first CPU will handle all the interrupts as was the > case before. > > Signed-off-by: Nathan Rossi <nathan.rossi@digi.com> > --- > drivers/irqchip/irq-armada-370-xp.c | 34 ++++++++++++++++++++++++++++++++-- > 1 file changed, 32 insertions(+), 2 deletions(-) > > diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c > index 5b8d571c04..42c257f576 100644 > --- a/drivers/irqchip/irq-armada-370-xp.c > +++ b/drivers/irqchip/irq-armada-370-xp.c > @@ -209,15 +209,37 @@ static struct msi_domain_info armada_370_xp_msi_domain_info = { > > static void armada_370_xp_compose_msi_msg(struct irq_data *data, struct msi_msg *msg) > { > +#ifdef CONFIG_SMP > + unsigned int cpu = cpumask_first(irq_data_get_effective_affinity_mask(data)); > + > + msg->data = (1 << (cpu + 8)) | (data->hwirq + PCI_MSI_DOORBELL_START); BIT(cpu + 8) | ... > +#else > + msg->data = 0xf00 | (data->hwirq + PCI_MSI_DOORBELL_START); This paints the existing code a bit differently. This seems to target all 4 CPUs. Why is that? I'd expect only bit 8 to be set, and the whole #ifdefery to go away. > +#endif > msg->address_lo = lower_32_bits(msi_doorbell_addr); > msg->address_hi = upper_32_bits(msi_doorbell_addr); > - msg->data = 0xf00 | (data->hwirq + PCI_MSI_DOORBELL_START); > } > > static int armada_370_xp_msi_set_affinity(struct irq_data *irq_data, > const struct cpumask *mask, bool force) > { > - return -EINVAL; > +#ifdef CONFIG_SMP > + unsigned int cpu; > + > + if (!force) > + cpu = cpumask_any_and(mask, cpu_online_mask); > + else > + cpu = cpumask_first(mask); > + > + if (cpu >= nr_cpu_ids) > + return -EINVAL; > + > + irq_data_update_effective_affinity(irq_data, cpumask_of(cpu)); > + > + return IRQ_SET_MASK_OK; > +#else > + return -EINVAL; > +#endif > } > > static struct irq_chip armada_370_xp_msi_bottom_irq_chip = { > @@ -482,6 +504,7 @@ static void armada_xp_mpic_smp_cpu_init(void) > static void armada_xp_mpic_reenable_percpu(void) > { > unsigned int irq; > + u32 reg; > > /* Re-enable per-CPU interrupts that were enabled before suspend */ > for (irq = 0; irq < ARMADA_370_XP_MAX_PER_CPU_IRQS; irq++) { > @@ -501,6 +524,13 @@ static void armada_xp_mpic_reenable_percpu(void) > } > > ipi_resume(); > + > + /* Enable MSI doorbell mask and combined cpu local interrupt */ > + reg = readl(per_cpu_int_base + ARMADA_370_XP_IN_DRBEL_MSK_OFFS) > + | PCI_MSI_DOORBELL_MASK; > + writel(reg, per_cpu_int_base + ARMADA_370_XP_IN_DRBEL_MSK_OFFS); > + /* Unmask local doorbell interrupt */ > + writel(1, per_cpu_int_base + ARMADA_370_XP_INT_CLEAR_MASK_OFFS); This is a duplicate of what is already in armada_370_xp_msi_init(). Please refactor it so that this doesn't happen twice on the first CPU. This otherwise seem like a valuable improvement on the current behaviour, M. -- Without deviation from the norm, progress is not possible. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] irqchip/armada-370-xp: Enable MSI affinity configuration 2022-04-20 23:19 ` Marc Zyngier @ 2022-04-21 8:32 ` Nathan Rossi 2022-04-21 9:51 ` Marc Zyngier 2022-04-21 12:00 ` Andrew Lunn 0 siblings, 2 replies; 11+ messages in thread From: Nathan Rossi @ 2022-04-21 8:32 UTC (permalink / raw) To: Marc Zyngier Cc: linux-arm-kernel, linux-kernel, Nathan Rossi, Andrew Lunn, Gregory Clement, Sebastian Hesselbarth, Thomas Gleixner On Thu, 21 Apr 2022 at 16:54, Marc Zyngier <maz@kernel.org> wrote: > > Hi Nathan, > > On Thu, 21 Apr 2022 02:57:28 +0100, > Nathan Rossi <nathan@nathanrossi.com> wrote: > > > > From: Nathan Rossi <nathan.rossi@digi.com> > > > > With multiple devices attached via PCIe to an Armada 385 it is possible > > to overwhelm a single CPU with MSI interrupts. Under certain scenarios > > configuring the interrupts to be handled by more than one CPU would > > prevent the system from being overwhelmed. However the > > irqchip-aramada-370-xp driver is configured to only handle MSIs on the > > boot CPU, and provides no affinity configuration. > > > > This change adds support to the armada-370-xp driver to allow for > > configuring the affinity of specific MSI irqs and to generate the > > interrupts on secondary CPUs. This is done by enabling the private > > doorbell for all online CPUs and configures all CPUs to unmask MSI > > specific private doorbell bits. The CPU affinity selection of the > > interrupt is handled by the target list of the software triggered > > interrupt value, which is provided as the MSI message. The message has > > the associated CPU bit set for the target CPU. For private doorbell > > interrupts only one bit can be set otherwise all CPUs will receive the > > interrupt, so the lowest CPU in the affinity mask is used. This means > > that by default the first CPU will handle all the interrupts as was the > > case before. > > > > Signed-off-by: Nathan Rossi <nathan.rossi@digi.com> > > --- > > drivers/irqchip/irq-armada-370-xp.c | 34 ++++++++++++++++++++++++++++++++-- > > 1 file changed, 32 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c > > index 5b8d571c04..42c257f576 100644 > > --- a/drivers/irqchip/irq-armada-370-xp.c > > +++ b/drivers/irqchip/irq-armada-370-xp.c > > @@ -209,15 +209,37 @@ static struct msi_domain_info armada_370_xp_msi_domain_info = { > > > > static void armada_370_xp_compose_msi_msg(struct irq_data *data, struct msi_msg *msg) > > { > > +#ifdef CONFIG_SMP > > + unsigned int cpu = cpumask_first(irq_data_get_effective_affinity_mask(data)); > > + > > + msg->data = (1 << (cpu + 8)) | (data->hwirq + PCI_MSI_DOORBELL_START); > > BIT(cpu + 8) | ... > > > +#else > > + msg->data = 0xf00 | (data->hwirq + PCI_MSI_DOORBELL_START); > > This paints the existing code a bit differently. This seems to target > all 4 CPUs. Why is that? I'd expect only bit 8 to be set, and the > whole #ifdefery to go away. I am not sure why this is targeting 4 CPUs, it will be masked by the percpu doorbell mask register and is effectively BIT(8). At least based on the documentation I have (only for armada 370/38x), which is why I left it as an #ifdef. I was also not able to find any specifics as to why it is targeting all 4 CPUs in git history. However this value was added with the initial driver implementation when only armada 370 was available in the kernel, so it is perhaps an inconsistent value that was never an issue due to the bits being reserved. I will remove the #ifdef in a v2 patch that addresses your other comments. > > > +#endif > > msg->address_lo = lower_32_bits(msi_doorbell_addr); > > msg->address_hi = upper_32_bits(msi_doorbell_addr); > > - msg->data = 0xf00 | (data->hwirq + PCI_MSI_DOORBELL_START); > > } > > > > static int armada_370_xp_msi_set_affinity(struct irq_data *irq_data, > > const struct cpumask *mask, bool force) > > { > > - return -EINVAL; > > +#ifdef CONFIG_SMP > > + unsigned int cpu; > > + > > + if (!force) > > + cpu = cpumask_any_and(mask, cpu_online_mask); > > + else > > + cpu = cpumask_first(mask); > > + > > + if (cpu >= nr_cpu_ids) > > + return -EINVAL; > > + > > + irq_data_update_effective_affinity(irq_data, cpumask_of(cpu)); > > + > > + return IRQ_SET_MASK_OK; > > +#else > > + return -EINVAL; > > +#endif > > } > > > > static struct irq_chip armada_370_xp_msi_bottom_irq_chip = { > > @@ -482,6 +504,7 @@ static void armada_xp_mpic_smp_cpu_init(void) > > static void armada_xp_mpic_reenable_percpu(void) > > { > > unsigned int irq; > > + u32 reg; > > > > /* Re-enable per-CPU interrupts that were enabled before suspend */ > > for (irq = 0; irq < ARMADA_370_XP_MAX_PER_CPU_IRQS; irq++) { > > @@ -501,6 +524,13 @@ static void armada_xp_mpic_reenable_percpu(void) > > } > > > > ipi_resume(); > > + > > + /* Enable MSI doorbell mask and combined cpu local interrupt */ > > + reg = readl(per_cpu_int_base + ARMADA_370_XP_IN_DRBEL_MSK_OFFS) > > + | PCI_MSI_DOORBELL_MASK; > > + writel(reg, per_cpu_int_base + ARMADA_370_XP_IN_DRBEL_MSK_OFFS); > > + /* Unmask local doorbell interrupt */ > > + writel(1, per_cpu_int_base + ARMADA_370_XP_INT_CLEAR_MASK_OFFS); > > This is a duplicate of what is already in armada_370_xp_msi_init(). > Please refactor it so that this doesn't happen twice on the first CPU. It is duplicated, however armada_xp_mpic_reenable_percpu is not called on the boot cpu as the setup is called with cpuhp_setup_state_nocalls. Thanks, Nathan > > This otherwise seem like a valuable improvement on the current > behaviour, > > M. > > -- > Without deviation from the norm, progress is not possible. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] irqchip/armada-370-xp: Enable MSI affinity configuration 2022-04-21 8:32 ` Nathan Rossi @ 2022-04-21 9:51 ` Marc Zyngier 2022-04-21 12:11 ` Andrew Lunn 2022-04-21 12:00 ` Andrew Lunn 1 sibling, 1 reply; 11+ messages in thread From: Marc Zyngier @ 2022-04-21 9:51 UTC (permalink / raw) To: Nathan Rossi Cc: linux-arm-kernel, linux-kernel, Nathan Rossi, Andrew Lunn, Gregory Clement, Sebastian Hesselbarth, Thomas Gleixner On Thu, 21 Apr 2022 09:32:23 +0100, Nathan Rossi <nathan@nathanrossi.com> wrote: > > On Thu, 21 Apr 2022 at 16:54, Marc Zyngier <maz@kernel.org> wrote: > > > > Hi Nathan, > > > > On Thu, 21 Apr 2022 02:57:28 +0100, > > Nathan Rossi <nathan@nathanrossi.com> wrote: > > > > > > From: Nathan Rossi <nathan.rossi@digi.com> > > > > > > With multiple devices attached via PCIe to an Armada 385 it is possible > > > to overwhelm a single CPU with MSI interrupts. Under certain scenarios > > > configuring the interrupts to be handled by more than one CPU would > > > prevent the system from being overwhelmed. However the > > > irqchip-aramada-370-xp driver is configured to only handle MSIs on the > > > boot CPU, and provides no affinity configuration. > > > > > > This change adds support to the armada-370-xp driver to allow for > > > configuring the affinity of specific MSI irqs and to generate the > > > interrupts on secondary CPUs. This is done by enabling the private > > > doorbell for all online CPUs and configures all CPUs to unmask MSI > > > specific private doorbell bits. The CPU affinity selection of the > > > interrupt is handled by the target list of the software triggered > > > interrupt value, which is provided as the MSI message. The message has > > > the associated CPU bit set for the target CPU. For private doorbell > > > interrupts only one bit can be set otherwise all CPUs will receive the > > > interrupt, so the lowest CPU in the affinity mask is used. This means > > > that by default the first CPU will handle all the interrupts as was the > > > case before. > > > > > > Signed-off-by: Nathan Rossi <nathan.rossi@digi.com> > > > --- > > > drivers/irqchip/irq-armada-370-xp.c | 34 ++++++++++++++++++++++++++++++++-- > > > 1 file changed, 32 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c > > > index 5b8d571c04..42c257f576 100644 > > > --- a/drivers/irqchip/irq-armada-370-xp.c > > > +++ b/drivers/irqchip/irq-armada-370-xp.c > > > @@ -209,15 +209,37 @@ static struct msi_domain_info armada_370_xp_msi_domain_info = { > > > > > > static void armada_370_xp_compose_msi_msg(struct irq_data *data, struct msi_msg *msg) > > > { > > > +#ifdef CONFIG_SMP > > > + unsigned int cpu = cpumask_first(irq_data_get_effective_affinity_mask(data)); > > > + > > > + msg->data = (1 << (cpu + 8)) | (data->hwirq + PCI_MSI_DOORBELL_START); > > > > BIT(cpu + 8) | ... > > > > > +#else > > > + msg->data = 0xf00 | (data->hwirq + PCI_MSI_DOORBELL_START); > > > > This paints the existing code a bit differently. This seems to target > > all 4 CPUs. Why is that? I'd expect only bit 8 to be set, and the > > whole #ifdefery to go away. > > I am not sure why this is targeting 4 CPUs, it will be masked by the > percpu doorbell mask register and is effectively BIT(8). At least > based on the documentation I have (only for armada 370/38x), which is > why I left it as an #ifdef. I was also not able to find any specifics > as to why it is targeting all 4 CPUs in git history. However this > value was added with the initial driver implementation when only > armada 370 was available in the kernel, so it is perhaps an > inconsistent value that was never an issue due to the bits being > reserved. I will remove the #ifdef in a v2 patch that addresses your > other comments. I guess we can get at least some testing from the platform maintainers to check that this doesn't regress the UP systems. > > > > > > +#endif > > > msg->address_lo = lower_32_bits(msi_doorbell_addr); > > > msg->address_hi = upper_32_bits(msi_doorbell_addr); > > > - msg->data = 0xf00 | (data->hwirq + PCI_MSI_DOORBELL_START); > > > } > > > > > > static int armada_370_xp_msi_set_affinity(struct irq_data *irq_data, > > > const struct cpumask *mask, bool force) > > > { > > > - return -EINVAL; > > > +#ifdef CONFIG_SMP > > > + unsigned int cpu; > > > + > > > + if (!force) > > > + cpu = cpumask_any_and(mask, cpu_online_mask); > > > + else > > > + cpu = cpumask_first(mask); > > > + > > > + if (cpu >= nr_cpu_ids) > > > + return -EINVAL; > > > + > > > + irq_data_update_effective_affinity(irq_data, cpumask_of(cpu)); > > > + > > > + return IRQ_SET_MASK_OK; > > > +#else > > > + return -EINVAL; > > > +#endif > > > } > > > > > > static struct irq_chip armada_370_xp_msi_bottom_irq_chip = { > > > @@ -482,6 +504,7 @@ static void armada_xp_mpic_smp_cpu_init(void) > > > static void armada_xp_mpic_reenable_percpu(void) > > > { > > > unsigned int irq; > > > + u32 reg; > > > > > > /* Re-enable per-CPU interrupts that were enabled before suspend */ > > > for (irq = 0; irq < ARMADA_370_XP_MAX_PER_CPU_IRQS; irq++) { > > > @@ -501,6 +524,13 @@ static void armada_xp_mpic_reenable_percpu(void) > > > } > > > > > > ipi_resume(); > > > + > > > + /* Enable MSI doorbell mask and combined cpu local interrupt */ > > > + reg = readl(per_cpu_int_base + ARMADA_370_XP_IN_DRBEL_MSK_OFFS) > > > + | PCI_MSI_DOORBELL_MASK; > > > + writel(reg, per_cpu_int_base + ARMADA_370_XP_IN_DRBEL_MSK_OFFS); > > > + /* Unmask local doorbell interrupt */ > > > + writel(1, per_cpu_int_base + ARMADA_370_XP_INT_CLEAR_MASK_OFFS); > > > > This is a duplicate of what is already in armada_370_xp_msi_init(). > > Please refactor it so that this doesn't happen twice on the first CPU. > > It is duplicated, however armada_xp_mpic_reenable_percpu is not called > on the boot cpu as the setup is called with cpuhp_setup_state_nocalls. Ah, right. Make sure we can get rid of the code duplication then. Thanks, M. -- Without deviation from the norm, progress is not possible. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] irqchip/armada-370-xp: Enable MSI affinity configuration 2022-04-21 9:51 ` Marc Zyngier @ 2022-04-21 12:11 ` Andrew Lunn 2022-04-22 4:49 ` Nathan Rossi 0 siblings, 1 reply; 11+ messages in thread From: Andrew Lunn @ 2022-04-21 12:11 UTC (permalink / raw) To: Marc Zyngier Cc: Nathan Rossi, linux-arm-kernel, linux-kernel, Nathan Rossi, Gregory Clement, Sebastian Hesselbarth, Thomas Gleixner > I guess we can get at least some testing from the platform maintainers > to check that this doesn't regress the UP systems. I have a 370rd which is single core. Let me know what you want testing. Andrew ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] irqchip/armada-370-xp: Enable MSI affinity configuration 2022-04-21 12:11 ` Andrew Lunn @ 2022-04-22 4:49 ` Nathan Rossi 2022-04-22 12:19 ` Andrew Lunn 0 siblings, 1 reply; 11+ messages in thread From: Nathan Rossi @ 2022-04-22 4:49 UTC (permalink / raw) To: Andrew Lunn Cc: Marc Zyngier, linux-arm-kernel, linux-kernel, Nathan Rossi, Gregory Clement, Sebastian Hesselbarth, Thomas Gleixner On Thu, 21 Apr 2022 at 22:11, Andrew Lunn <andrew@lunn.ch> wrote: > > > I guess we can get at least some testing from the platform maintainers > > to check that this doesn't regress the UP systems. > > I have a 370rd which is single core. Let me know what you want > testing. If you have a PCIe device that you can use with the 370 rd. Please test that the PCIe device probes and generates its interrupts in normal operation. I have sent out a v2 of this patch, So if testing please use that version https://lore.kernel.org/linux-arm-kernel/20220422043532.146946-1-nathan@nathanrossi.com/ Thanks, Nathan > > Andrew ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] irqchip/armada-370-xp: Enable MSI affinity configuration 2022-04-22 4:49 ` Nathan Rossi @ 2022-04-22 12:19 ` Andrew Lunn 2022-04-22 12:58 ` Nathan Rossi 0 siblings, 1 reply; 11+ messages in thread From: Andrew Lunn @ 2022-04-22 12:19 UTC (permalink / raw) To: Nathan Rossi Cc: Marc Zyngier, linux-arm-kernel, linux-kernel, Nathan Rossi, Gregory Clement, Sebastian Hesselbarth, Thomas Gleixner > If you have a PCIe device that you can use with the 370 rd. Sorry, nothing on its bus. The only device i have which might make sense testing is a WRT1900ac. That has an Armada XP dual core. I could try building a UP kernel for it, but i guess you have tried that already with your machine in order to test a UP setup? Andrew ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] irqchip/armada-370-xp: Enable MSI affinity configuration 2022-04-22 12:19 ` Andrew Lunn @ 2022-04-22 12:58 ` Nathan Rossi 2022-04-23 16:21 ` Andrew Lunn 0 siblings, 1 reply; 11+ messages in thread From: Nathan Rossi @ 2022-04-22 12:58 UTC (permalink / raw) To: Andrew Lunn Cc: Marc Zyngier, linux-arm-kernel, linux-kernel, Nathan Rossi, Gregory Clement, Sebastian Hesselbarth, Thomas Gleixner On Fri, 22 Apr 2022 at 22:19, Andrew Lunn <andrew@lunn.ch> wrote: > > > If you have a PCIe device that you can use with the 370 rd. > > Sorry, nothing on its bus. > > The only device i have which might make sense testing is a > WRT1900ac. That has an Armada XP dual core. I could try building a UP > kernel for it, but i guess you have tried that already with your > machine in order to test a UP setup? I have tested with SMP=n on a Armada 385 without any issues. So no need to do that on the Armada XP. There is still value in testing on the WRT1900ac device since so far I have only tested on a Armada 385. The Armada XP does have registers/etc. of the MPIC very similar to the 370, compared to the 385 having some significant differences. So if it is not too problematic to test on (it appears to have its wifi attached via PCIe?) please do. Thanks, Nathan > > Andrew ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] irqchip/armada-370-xp: Enable MSI affinity configuration 2022-04-22 12:58 ` Nathan Rossi @ 2022-04-23 16:21 ` Andrew Lunn 2022-05-04 0:37 ` Nathan Rossi 0 siblings, 1 reply; 11+ messages in thread From: Andrew Lunn @ 2022-04-23 16:21 UTC (permalink / raw) To: Nathan Rossi Cc: Marc Zyngier, linux-arm-kernel, linux-kernel, Nathan Rossi, Gregory Clement, Sebastian Hesselbarth, Thomas Gleixner On Fri, Apr 22, 2022 at 10:58:23PM +1000, Nathan Rossi wrote: > On Fri, 22 Apr 2022 at 22:19, Andrew Lunn <andrew@lunn.ch> wrote: > > > > > If you have a PCIe device that you can use with the 370 rd. > > > > Sorry, nothing on its bus. > > > > The only device i have which might make sense testing is a > > WRT1900ac. That has an Armada XP dual core. I could try building a UP > > kernel for it, but i guess you have tried that already with your > > machine in order to test a UP setup? > > I have tested with SMP=n on a Armada 385 without any issues. So no > need to do that on the Armada XP. > > There is still value in testing on the WRT1900ac device since so far I > have only tested on a Armada 385. The Armada XP does have > registers/etc. of the MPIC very similar to the 370, compared to the > 385 having some significant differences. So if it is not too > problematic to test on (it appears to have its wifi attached via > PCIe?) please do. Hi Nathan Turns out the WiFi drivers are in a sorry state. There is no mainline driver. There is an OpenWRT driver, but it has not been updated for a few years, tries to use get_fs() set_fs() pci_map_single() etc. I've mostly used this platform for Ethernet switch development, so i've never tried the WiFi before. I had no idea it was so broken... So sorry, i cannot test this. Andrew ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] irqchip/armada-370-xp: Enable MSI affinity configuration 2022-04-23 16:21 ` Andrew Lunn @ 2022-05-04 0:37 ` Nathan Rossi 0 siblings, 0 replies; 11+ messages in thread From: Nathan Rossi @ 2022-05-04 0:37 UTC (permalink / raw) To: Andrew Lunn Cc: Marc Zyngier, linux-arm-kernel, linux-kernel, Nathan Rossi, Gregory Clement, Sebastian Hesselbarth, Thomas Gleixner On Sun, 24 Apr 2022 at 02:21, Andrew Lunn <andrew@lunn.ch> wrote: > > On Fri, Apr 22, 2022 at 10:58:23PM +1000, Nathan Rossi wrote: > > On Fri, 22 Apr 2022 at 22:19, Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > > If you have a PCIe device that you can use with the 370 rd. > > > > > > Sorry, nothing on its bus. > > > > > > The only device i have which might make sense testing is a > > > WRT1900ac. That has an Armada XP dual core. I could try building a UP > > > kernel for it, but i guess you have tried that already with your > > > machine in order to test a UP setup? > > > > I have tested with SMP=n on a Armada 385 without any issues. So no > > need to do that on the Armada XP. > > > > There is still value in testing on the WRT1900ac device since so far I > > have only tested on a Armada 385. The Armada XP does have > > registers/etc. of the MPIC very similar to the 370, compared to the > > 385 having some significant differences. So if it is not too > > problematic to test on (it appears to have its wifi attached via > > PCIe?) please do. > > Hi Nathan > > Turns out the WiFi drivers are in a sorry state. There is no mainline > driver. There is an OpenWRT driver, but it has not been updated for a > few years, tries to use get_fs() set_fs() pci_map_single() etc. > > I've mostly used this platform for Ethernet switch development, so > i've never tried the WiFi before. I had no idea it was so broken... > > So sorry, i cannot test this. Thanks for giving it a try. I managed to source an existing one of our products that uses an Armada 370 with a PCIe device attached. Took me a bit to get a recent kernel booting on it, however I was able to confirm that this MSI affinity change works correctly on the 370 SoC (with and without CONFIG_SMP). Regards, Nathan ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] irqchip/armada-370-xp: Enable MSI affinity configuration 2022-04-21 8:32 ` Nathan Rossi 2022-04-21 9:51 ` Marc Zyngier @ 2022-04-21 12:00 ` Andrew Lunn 1 sibling, 0 replies; 11+ messages in thread From: Andrew Lunn @ 2022-04-21 12:00 UTC (permalink / raw) To: Nathan Rossi Cc: Marc Zyngier, linux-arm-kernel, linux-kernel, Nathan Rossi, Gregory Clement, Sebastian Hesselbarth, Thomas Gleixner > > > static void armada_370_xp_compose_msi_msg(struct irq_data *data, struct msi_msg *msg) > > > { > > > +#ifdef CONFIG_SMP > > > + unsigned int cpu = cpumask_first(irq_data_get_effective_affinity_mask(data)); > > > + > > > + msg->data = (1 << (cpu + 8)) | (data->hwirq + PCI_MSI_DOORBELL_START); > > > > BIT(cpu + 8) | ... > > > > > +#else > > > + msg->data = 0xf00 | (data->hwirq + PCI_MSI_DOORBELL_START); > > > > This paints the existing code a bit differently. This seems to target > > all 4 CPUs. Why is that? I'd expect only bit 8 to be set, and the > > whole #ifdefery to go away. > > I will remove the #ifdef in a v2 patch that addresses your > other comments. Please try to remove all the #ifdef'ery. > > > static int armada_370_xp_msi_set_affinity(struct irq_data *irq_data, > > > const struct cpumask *mask, bool force) > > > { > > > - return -EINVAL; > > > +#ifdef CONFIG_SMP > > > + unsigned int cpu; > > > + > > > + if (!force) > > > + cpu = cpumask_any_and(mask, cpu_online_mask); > > > + else > > > + cpu = cpumask_first(mask); > > > + > > > + if (cpu >= nr_cpu_ids) > > > + return -EINVAL; > > > + > > > + irq_data_update_effective_affinity(irq_data, cpumask_of(cpu)); > > > + > > > + return IRQ_SET_MASK_OK; > > > +#else > > > + return -EINVAL; > > > +#endif A quick look in cpumask.h suggests that if NR_CPUS == 1, there are stub functions which return constant values. So you might not need this #ifdef. However, i'm a network guy, not a scheduling guy, so don't trust what i say... Andrew ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-05-04 0:37 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-21 1:57 [PATCH] irqchip/armada-370-xp: Enable MSI affinity configuration Nathan Rossi 2022-04-20 23:19 ` Marc Zyngier 2022-04-21 8:32 ` Nathan Rossi 2022-04-21 9:51 ` Marc Zyngier 2022-04-21 12:11 ` Andrew Lunn 2022-04-22 4:49 ` Nathan Rossi 2022-04-22 12:19 ` Andrew Lunn 2022-04-22 12:58 ` Nathan Rossi 2022-04-23 16:21 ` Andrew Lunn 2022-05-04 0:37 ` Nathan Rossi 2022-04-21 12:00 ` Andrew Lunn
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).