Re: [PATCH v3 2/6] PCI: iproc: Add INTx support with better modeling

[Andrew Murray]

On Wed, Dec 04, 2019 at 10:29:51AM +0200, Andy Shevchenko wrote:
> On Wed, Dec 4, 2019 at 12:09 AM Ray Jui <ray.jui@xxxxxxxxxxxx> wrote:
> > On 12/3/19 11:27 AM, Andy Shevchenko wrote:
> > > On Tue, Dec 3, 2019 at 5:55 PM Andrew Murray <andrew.murray@xxxxxxx> wrote:
> > >> On Tue, Dec 03, 2019 at 10:27:02AM +0530, Srinath Mannam wrote:
> > >
> > >>> +     /* go through INTx A, B, C, D until all interrupts are handled */
> > >>> +     do {
> > >>> +             status = iproc_pcie_read_reg(pcie, IPROC_PCIE_INTX_CSR);
> > >>
> > >> By performing this read once and outside of the do/while loop you may improve
> > >> performance. I wonder how probable it is to get another INTx whilst handling
> > >> one?
> > >
> > > May I ask how it can be improved?
> > > One read will be needed any way, and so does this code.
> > >
> >
> > I guess the current code will cause the IPROC_PCIE_INTX_CSR register to
> > be read TWICE, if it's ever set to start with.
> >
> > But then if we do it outside of the while loop, if we ever receive an
> > interrupt while servicing one, the interrupt will still need to be
> > serviced, and in this case, it will cause additional context switch
> > overhead by going out and back in the interrupt context.

Yes it's a trade off - if you dropped the do/while loop and thus had a single
read you'd reduce the overhead on interrupt handling in every case except
where another INTx is received whilst in this function. But as you point out
each time that does happen you'll pay the penalty of a context switch.

I don't have any knowledge of this platform so I have no idea if such a change
would be good/bad or material. However I thought I'd point it out. Looking at
the other controller drivers, some handle in a loop and some don't.


> >
> > My take is that it's probably more ideal to leave this portion of code
> > as it is.
> 
> Can't we simple drop a do-while completely and leave only
> for_each_set_bit() loop?
> 

I'm happy either way.

Thanks,

Andrew Murray

> >
> > >>> +             for_each_set_bit(bit, &status, PCI_NUM_INTX) {
> > >>> +                     virq = irq_find_mapping(pcie->irq_domain, bit);
> > >>> +                     if (virq)
> > >>> +                             generic_handle_irq(virq);
> > >>> +                     else
> > >>> +                             dev_err(dev, "unexpected INTx%u\n", bit);
> > >>> +             }
> > >>> +     } while ((status & SYS_RC_INTX_MASK) != 0);
> > >
> 
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko