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.
