Messages in this thread | | | Subject | Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver | From | "Shenhar, Talel" <> | Date | Tue, 10 Sep 2019 09:21:01 +0300 |
| |
On 9/9/2019 6:16 PM, Arnd Bergmann wrote: > On Mon, Sep 9, 2019 at 4:11 PM Shenhar, Talel <talel@amazon.com> wrote: >> On 9/9/2019 4:41 PM, Arnd Bergmann wrote: >> >> In current implementation of v1, I am not doing any read barrier, Hence, >> using the non-relaxed will add unneeded memory barrier. >> >> I have no strong objection moving to the non-relaxed version and have an >> unneeded memory barrier, as this path is not "hot" one. > Ok, then please add it. ok, shall be part of v2 > >> Beside of avoiding the unneeded memory barrier, I would be happy to keep >> common behavior for our drivers: >> >> e.g. >> >> https://github.com/torvalds/linux/blob/master/drivers/irqchip/irq-al-fic.c#L49 >> >> >> So what do you think we should go with? relaxed or non-relaxed? > The al_fic_set_trigger() function is clearly a slow-path and should use the > non-relaxed functions. In case of al_fic_irq_handler(), the extra barrier > might introduce a measurable overhead, but at the same time I'm > not sure if that one is correct without the barrier: > > If you have an MSI-type interrupt for notifying a device driver of > a DMA completion, there might not be any other barrier between > the arrival of the MSI message and the CPU accessing the data. > Depending on how strict the hardware implements MSI and how > the IRQ is chained, this could lead to data corruption. > > If the interrupt is only used for level or edge triggered interrupts, > this is ok since you already need another register read in > the driver before it can safely access a DMA buffer. > > In either case, if you can prove that it's safe to use the relaxed > version here and you think that it may help, it would be good to > add a comment explaining the reasoning. Decided to go with the non-relaxed version as this is not hot path and likely be more clear to the common reader to have non relaxed version. > > Arnd
| |