On Mon, Sep 9, 2019 at 11:14 AM Talel Shenhar <[email protected]> wrote:
>
> The Amazon's Annapurna Labs SoCs includes Point Of Serialization error
> logging unit that reports an error in case write error (e.g. attempt to
> write to a read only register).
> This patch introduces the support for this unit.
>
> Signed-off-by: Talel Shenhar <[email protected]>
Looks ok overall, juts a few minor comments:
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Talel Shenhar");
> +MODULE_DESCRIPTION("Amazon's Annapurna Labs POS driver");
These usually go to the end of the file.
> + log1 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_1);
> + if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1))
> + return IRQ_NONE;
> +
> + log0 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_0);
> + writel_relaxed(0, pos->mmio_base + AL_POS_ERROR_LOG_1);
Why do you require _relaxed() accessors here? Please add a comment
explaining that, or use the regular readl()/writel().
> + resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + pos->mmio_base = devm_ioremap_resource(&pdev->dev, resource);
This can be simplified to devm_platform_ioremap_resource().
> + pos->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
And this is usually written as platform_get_irq()
Arnd
On 9/9/2019 12:44 PM, Arnd Bergmann wrote:
> On Mon, Sep 9, 2019 at 11:14 AM Talel Shenhar <[email protected]> wrote:
>> The Amazon's Annapurna Labs SoCs includes Point Of Serialization error
>> logging unit that reports an error in case write error (e.g. attempt to
>> write to a read only register).
>> This patch introduces the support for this unit.
>>
>> Signed-off-by: Talel Shenhar <[email protected]>
> Looks ok overall, juts a few minor comments:
Thanks.
>
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_AUTHOR("Talel Shenhar");
>> +MODULE_DESCRIPTION("Amazon's Annapurna Labs POS driver");
> These usually go to the end of the file.
Ack, Will move them as part of v2.
>
>> + log1 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_1);
>> + if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1))
>> + return IRQ_NONE;
>> +
>> + log0 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_0);
>> + writel_relaxed(0, pos->mmio_base + AL_POS_ERROR_LOG_1);
> Why do you require _relaxed() accessors here? Please add a comment
> explaining that, or use the regular readl()/writel().
I don't think commenting is needed here as there is nothing special in
this type of access.
I don't see this is common to comment the use of the _relaxed accessors.
This driver is for SoC using arm64 cpu.
If one uses the non-relaxed version of readl while running on arm64, he
shall cause read barrier, which is then doing dsm(ld).. This barrier is
not needed here, so we spare the use of the more heavy readl in favor of
the less "harmful" one.
Let me know what you think.
>
>> + resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + pos->mmio_base = devm_ioremap_resource(&pdev->dev, resource);
> This can be simplified to devm_platform_ioremap_resource().
Ack, Will simplify them in v2.
>
>> + pos->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> And this is usually written as platform_get_irq()
Ack, Will replace them with platform_get_irq() in v2.
>
> Arnd
On Mon, Sep 9, 2019 at 1:13 PM Shenhar, Talel <[email protected]> wrote:
> On 9/9/2019 12:44 PM, Arnd Bergmann wrote:
> > On Mon, Sep 9, 2019 at 11:14 AM Talel Shenhar <[email protected]> wrote:
> >> + writel_relaxed(0, pos->mmio_base + AL_POS_ERROR_LOG_1);
> > Why do you require _relaxed() accessors here? Please add a comment
> > explaining that, or use the regular readl()/writel().
>
> I don't think commenting is needed here as there is nothing special in
> this type of access.
>
> I don't see this is common to comment the use of the _relaxed accessors.
I usually mention it in driver reviews, but most authors revert back
to the normal accessors when there is no difference.
> This driver is for SoC using arm64 cpu.
>
> If one uses the non-relaxed version of readl while running on arm64, he
> shall cause read barrier, which is then doing dsm(ld).. This barrier is
> not needed here, so we spare the use of the more heavy readl in favor of
> the less "harmful" one.
>
> Let me know what you think.
If the barrier causes no harm, just leave it in to keep the code more
readable. Most developers don't need to know the difference between
the two, so using the less common interface just makes the reader
curious about why it was picked.
Avoiding the barrier can make a huge performance difference in a
hot code path, but the downside is that it can behave in unexpected
ways if the same code is run on a different CPU architecture that
does not have the exact same rules about what _relaxed() means.
In fact, replacing a 'readl()' with 'readl_relaxed() + rmb()' can lead
to slower rather than faster code when the explicit barrier is heavier
than the implied one (e.g. on x86), or readl_relaxed() does not skip
the barrier.
The general rule with kernel interfaces when you have two versions
that both do what you want is to pick the one with the shorter name.
See spin_lock()/spin_lock_irqsave(), ioremap()/ioremap_nocache(),
or ktime_get()/ktime_get_clocktai_ts64(). (yes, there are also
exceptions)
Arnd
On 9/9/2019 4:41 PM, Arnd Bergmann wrote:
> On Mon, Sep 9, 2019 at 1:13 PM Shenhar, Talel <[email protected]> wrote:
>> On 9/9/2019 12:44 PM, Arnd Bergmann wrote:
>>> On Mon, Sep 9, 2019 at 11:14 AM Talel Shenhar <[email protected]> wrote:
>>>> + writel_relaxed(0, pos->mmio_base + AL_POS_ERROR_LOG_1);
>>> Why do you require _relaxed() accessors here? Please add a comment
>>> explaining that, or use the regular readl()/writel().
>> I don't think commenting is needed here as there is nothing special in
>> this type of access.
>>
>> I don't see this is common to comment the use of the _relaxed accessors.
> I usually mention it in driver reviews, but most authors revert back
> to the normal accessors when there is no difference.
>
>> This driver is for SoC using arm64 cpu.
>>
>> If one uses the non-relaxed version of readl while running on arm64, he
>> shall cause read barrier, which is then doing dsm(ld).. This barrier is
>> not needed here, so we spare the use of the more heavy readl in favor of
>> the less "harmful" one.
>>
>> Let me know what you think.
> If the barrier causes no harm, just leave it in to keep the code more
> readable. Most developers don't need to know the difference between
> the two, so using the less common interface just makes the reader
> curious about why it was picked.
>
> Avoiding the barrier can make a huge performance difference in a
> hot code path, but the downside is that it can behave in unexpected
> ways if the same code is run on a different CPU architecture that
> does not have the exact same rules about what _relaxed() means.
>
> In fact, replacing a 'readl()' with 'readl_relaxed() + rmb()' can lead
> to slower rather than faster code when the explicit barrier is heavier
> than the implied one (e.g. on x86), or readl_relaxed() does not skip
> the barrier.
>
> The general rule with kernel interfaces when you have two versions
> that both do what you want is to pick the one with the shorter name.
> See spin_lock()/spin_lock_irqsave(), ioremap()/ioremap_nocache(),
> or ktime_get()/ktime_get_clocktai_ts64(). (yes, there are also
> exceptions)
>
> Arnd
Thanks for the detailed response.
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.
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?
On Mon, Sep 9, 2019 at 4:11 PM Shenhar, Talel <[email protected]> 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.
> 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.
Arnd