Re: [PATCH 0/3] PCI: designware: Fixing MSI handling flow

[Trent Piepho <tpiepho@impinj.com>]

On Thu, 2018-11-22 at 12:03 +0000, Gustavo Pimentel wrote:
> 
> > I just realised (at 1am!) that the first patch is awfully buggy:
> > - ENABLE and MASK have opposite polarities
> > - There is nothing initialising the ENABLE and MASK registers
> > 
> > I've stashed the following fix-up on top of the existing series:
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
> > b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index f06e67c60593..0fa9e8fdce66 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -166,7 +166,7 @@ static void dw_pci_bottom_mask(struct irq_data
> > *data)
> 
> (snip)
> 
> I used your patch and made it more perceptible in my opinion, (basically I
> transformed the variable irq_mask (previous irq_status) into a mirror of MASK
> registers, which removed the need for the *NOT* operation and forced the mask
> operation swap in the callbacks)

This would be the patch that enables all MSI interrupts on driver
initialization?

I don't think that's a good idea.  I was under the impression Marc
thought that as well.  It would be better to enable them when they are
enabled, via enable and disable methods.

I've looked into the interplay with enable and mask some more. 
Previous versions of the driver used the mask registers to both
en/disable and un/mask MSIs.  Both enable and mask methods were
provided, but they were the same mask function in the driver.

Functions like irq_percpu_enable() will call the irq_enable method, but
fall-back to irq_unmask if the chip does not provide irq_enable.  So a
driver can be sloppy about the distinction between masking and disable
an irq and it isn't necessarily apparent.

Which is how keystone has ks_pcie_msi_set_irq() that is part of the
unmask method (now), but certainly looks like it should be part of
enable.

I think the right way to fix this driver would be to:
Ack the irq in the irq_ack method.
Write enable and disable methods that use the enable register
Write mask/unmask methods that use the mask register
Rename the msi_(set|clr)_irq methods to msi_(en|dis)able_irq
Call ops->msi_enable_irq() from dw_pci_bottom_enable()
irq_status should get renamed to irq_mask, since it's just a driver 
cache of the mask register.  Could also cache the enable register, but
that reg is used less often.  Could also use regcache to cache these
registers.