RE: [PATCH 3/4] PCI/portdrv: Check platform supported service IRQ's

[Bharat Kumar Gogada]

> Subject: Re: [PATCH 3/4] PCI/portdrv: Check platform supported service IRQ's
> 
> On Fri, Aug 10, 2018 at 09:09:39PM +0530, Bharat Kumar Gogada wrote:
> > Platforms may have dedicated IRQ lines for PCIe services like AER/PME
> > etc., check for such IRQ lines.
> > Check mask and fill legacy irq line for services other than
> 
> Capitalize "IRQ" consistently in English text like this.
> 
> Insert blank lines between paragraphs.
> 
> Add a reference to the relevant spec sections.  PCIe r4.0, sec 6.2.4.1.2, 6.2.6,
> 7.5.3.12 seem pertinent.
> 
Agreed, will do in next patch.
> > platform supported service IRQ number.
> >
> > Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xxxxxxxxxx>
> > ---
> >  drivers/pci/pcie/portdrv_core.c |   19 +++++++++++++++++--
> >  1 files changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/pcie/portdrv_core.c
> > b/drivers/pci/pcie/portdrv_core.c index e0261ad..a7d024c 100644
> > --- a/drivers/pci/pcie/portdrv_core.c
> > +++ b/drivers/pci/pcie/portdrv_core.c
> > @@ -166,6 +166,19 @@ static int pcie_init_service_irqs(struct pci_dev
> *dev, int *irqs, int mask)
> >  		irqs[i] = -1;
> >
> >  	/*
> > +	 * Some platforms have dedicated interrupt line from root complex
> to
> > +	 * interrupt controller for PCIe services like AER/PME etc., check
> > +	 * if platform registered with any such IRQ.
> > +	 */
> > +	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
> > +		int plat_mask;
> > +
> > +		plat_mask = pci_check_platform_service_irqs(dev, irqs,
> mask);
> > +		if (plat_mask > 0)
> 
> Masks should be unsigned and tested for zero or "mask & bit".  The rest of
> this file uses "int", which is sloppy because it does the wrong thing if we
> happen to use the high-order bit in the mask.
> 
> > +			mask = mask & plat_mask;
> > +	}
> 
> I think these platform IRQs are neither MSI-X/MSI nor PCI INTx wires.
> But as written, I think this patch executes pcie_port_enable_irq_vec(), which
> only does MSI-X/MSI stuff.  So this must rely on that failing?
> 
> And then we fall through to run pci_alloc_irq_vectors(), which is for PCI INTx
> interrupts, which doesn't seem appropriate either.
> 
> It seems like this platform IRQ case should be completely separated from the
> other MSI/INTx cases, i.e., set irqs[PCIE_PORT_SERVICE_AER_SHIFT] directly
> (I think you already do this inside pci_check_platform_service_irqs()) and
> immediately return.
> Then I think you wouldn't need the other hunk below.
Agreed if we check platform service irqs here we need to deal with different combination
of service IRQ's like AER MSI, pme platform .. and change code accordingly to fill irqs[i].
yes it is better to call platform IRQ separately to avoid code changes in different scenarios and 
chunk below.

Can we do the following code flow: 
int pcie_port_device_register(struct pci_dev *dev)
{
 ... 
status = pcie_init_service_irqs(dev, irqs, capabilities);
        if (status) {
...
}
pci_check_platform_service_irqs(dev, irqs, capabilities);

Thanks,
Bharat