Re: FAILED: patch "[PATCH] PCI: dwc: Move interrupt acking into the proper callback" failed to apply to 4.14-stable tree

[Niklas Cassel <niklas.cassel@linaro.org>]

On Fri, Jan 18, 2019 at 04:22:15PM +0000, Marc Zyngier wrote:
> On 18/01/2019 16:00, Niklas Cassel wrote:
> > On Thu, Jan 17, 2019 at 09:58:43AM +0000, Marc Zyngier wrote:
> >> On 16/01/2019 23:43, Niklas Cassel wrote:
> >>> On Tue, Jan 15, 2019 at 09:24:37AM +0100, gregkh@linuxfoundation.org wrote:
> >>>>
> >>>> The patch below does not apply to the 4.14-stable tree.
> >>>> If someone wants it applied there, or to any other stable or longterm
> >>>> tree, then please email the backport, including the original git commit
> >>>> id to <stable@vger.kernel.org>.
> >>>
> >>> Hello Marc,
> >>>
> >>> You said that you were going to backport this to 4.14, right?
> >>
> >> Me, or anyone else. Preferably someone who, like you, has the HW at hand (I don't).
> >>
> >>> IIRC, you said something about creating a simple dw_pci_bottom_ack()
> >>> (since 4.14 lacks a ack() function).
> >>
> >> Indeed. Something like this:
> >>
> >> diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c
> >> index bc3e2d8d0cce..f4f3eeee10af 100644
> >> --- a/drivers/pci/dwc/pcie-designware-host.c
> >> +++ b/drivers/pci/dwc/pcie-designware-host.c
> >> @@ -45,8 +45,19 @@ static int dw_pcie_wr_own_conf(struct pcie_port *pp, int where, int size,
> >>  	return dw_pcie_write(pci->dbi_base + where, size, val);
> >>  }
> >>  
> >> +static void dwc_irq_ack(struct irq_data *d)
> >> +{
> >> +	struct msi_desc *msi = irq_data_get_msi_desc(d);
> >> +	struct pcie_port *pp = msi_desc_to_pci_sysdata(msi);
> >> +	int pos = d->hwirq % 32;
> >> +	int i = d->hwirq / 32;
> >> +
> >> +	dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + i * 12, 4, BIT(pos));
> >> +}
> >> +
> > 
> > Thanks Marc.
> > 
> > This does not seem to work.
> > It appears that the ack function is never called.
> > I assume that this is because of:
> > 
> > irq_set_chip_and_handler(irq, &dw_msi_irq_chip, handle_simple_irq);
> 
> Ah, indeed, I forgot about that particular nugget.
> 
> > 
> > is it safe to simply change this to handle_edge_irq?
> > (Which seems to be the case after this after this driver's
> > MSI handling was heavily refactored in 4.17.)
> 
> Yes, this should be just a matter of using the right interrupt flow.
> Please let me know if that works for you.

Hello Marc,

I ran a simple stress test during the night with your patch plus:

-       irq_set_chip_and_handler(irq, &dw_msi_irq_chip, handle_simple_irq);
+       irq_set_chip_and_handler(irq, &dw_msi_irq_chip, handle_edge_irq);

without any issues. (Feel free to add my Tested-by).

So this looks good to me :) Thanks!


Kind regards,
Niklas

> 
> > 
> > I know that you were against reverting 8c934095fa2f ("PCI: dwc: Clear
> > MSI interrupt status after it is handled, not before") on mainline,
> > since that wouldn't hinder people from moving around stuff in the
> > future, but perhaps reverting this commit on 4.14 is safer than
> > changing the irq handler type?
> 
> I still don't think that it is a good idea. Using the correct flow is
> just as important in order to properly honors the interrupt masking
> which the "simple" flow completely ignores.
> 
> Thanks,
> 
> 	M.
> -- 
> Jazz is not dead. It just smells funny...