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

[Trent Piepho <tpiepho@impinj.com>]

On Wed, 2018-11-14 at 22:44 +0000, Marc Zyngier wrote:
>  static struct irq_chip dw_pcie_msi_irq_chip = {
> >  	.name = "PCI-MSI",
> >  	.irq_ack = dw_msi_ack_irq,
> >  	.irq_mask = dw_msi_mask_irq,
> >  	.irq_unmask = dw_msi_unmask_irq,
> > +	.irq_enable = dw_msi_enable_irq,
> 
> If you're doing that, please implement both enable *and* disable.

Yes, certainly.  I'm not yet convinced I've should put this here or if
my enable method has not missed something.  Just putting this out there
as a way to test the code.

> 
> > +static void dw_pci_bottom_enable(struct irq_data *data)
> > +{
> > +	struct pcie_port *pp = irq_data_get_irq_chip_data(data);
> > +	unsigned int res, bit, ctrl;
> > +	unsigned long flags;
> > +	u32 enable;
> > +
> > +	ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL;
> > +	res = ctrl * MSI_REG_CTRL_BLOCK_SIZE;
> > +	bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL;
> > +
> > +	raw_spin_lock_irqsave(&pp->lock, flags);
> > +
> > +	dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4,
> > &enable);
> > +	enable |= BIT(bit);
> > +	dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4,
> > enable);

I wonder if there are ENABLE_SET and ENABLE_CLR registers to allow
setting/clearing a bit atomically with one write, as is possible with
the status register.

> > +
> > +	raw_spin_unlock_irqrestore(&pp->lock, flags);
> > +}
> 
> How does it work for drivers that use the callbacks stuff?

Do you mean the ops callbacks in the pcie_port?  There isn't a callback
in dw_pcie_host_ops for enable/disable.  But there is
msi_set/clear_irq, undocumented.  Looks like it's the maybe the same
thing using different terms.  Only user is keystone.  Only callsite is
from the dw_pci_bottom_un/**mask**() methods.

It seems someone else was unclear on the distinction between disabling
and masking an IRQ.

So my patch should play ok with the only affected user, keystone, in
the sense it causes no new problems.

But there might be another flaw here, fixed by:

1. Decide msi_set/clear_irq should enable/disable the irq, remove it
from the un/mask methods and move it into the en/disable methods.

2. Decide msi_set/clear_irq should un/mask the irq and keystone should
not be using it with the keystone "ENABLE" register.

3. Like 2, but decide the keystone enable register really is a mask and
everything is fine.

My bet is on 1.