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

[Marc Zyngier <marc.zyngier@arm.com>]

On Mon, 26 Nov 2018 15:52:42 +0000,
Trent Piepho <tpiepho@impinj.com> wrote:
> 
> 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.

What gain does this bring? Frankly, I see exactly zero advantage of
doing so. It may look cosmetically appealing in the sense that it
makes use of of a register that the IP offers, but for Linux the
advantage is basically null.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.