Re: [PATCH] asm-generic/io.h: Implement read[bwlq]_relaxed()

[Daniel Thompson]

On 09/09/14 15:15, Will Deacon wrote:
> On Tue, Sep 09, 2014 at 02:14:54PM +0100, Daniel Thompson wrote:
>> On 09/09/14 14:03, Daniel Thompson wrote:
>>> On 09/09/14 13:28, Will Deacon wrote:
>>>> I have a larger series adding these (and the write equivalents) to all
>>>> architectures that I periodically post and then fail to get on top of.
>>>
>>> That's why you're on Cc:...
> 
> Ok, so why not just pick the asm-generic patch out of my series?

Only really that your patch introducing the writeX_relaxed() family at
the same time.

However it would be fine to subset your patch rather than use mine. You
did post it first...


>>>> The key part you're missing is defining some generic semantics for these
>>>> accessors. Without those, I don't think it makes sense to put them into
>>>> asm-generic, because drivers can't safely infer any meaning from the relaxed
>>>> definition.
>>>
>>> Currently the semantics are described as:
>>> --- cut here ---
>>> PCI ordering rules also guarantee that PIO read responses arrive after
>>> any outstanding DMA writes from that bus, since for some devices the
>>> result of a readb call may signal to the driver that a DMA transaction
>>> is complete. In many cases, however, the driver may want to indicate
>>> that the next readb call has no relation to any previous DMA writes
>>> performed by the device. The driver can use readb_relaxed for these
>>> cases, although only some platforms will honor the relaxed semantics.
>>> Using the relaxed read functions will provide significant performance
>>> benefits on platforms that support it. The qla2xxx driver provides
>>> examples of how to use readX_relaxed . In many cases, a majority of the
>>> driverâs readX calls can safely be converted to readX_relaxed calls,
>>> since only a few will indicate or depend on DMA completion.
>>> --- cut here ---
>>>
>>> The implementation provided in the patch trivially meets this definition
>>> (by not honouring the relaxedness).
> 
> I still think we need to mention ordering of relaxed reads against each
> other and also against spinlocks.

I don't disagree.

I just think the documentation being sub-optimal is not a good reason to
avoid implementing the read functions.


>>>> Ben and I agreed on something back in May:
>>>>
>>>>   https://lkml.org/lkml/2014/5/22/468
>>>
>>> ... and didn't you also conclude with hpa that the very relaxed x86
>>> implementation of readl_relaxed() already meets this definition (as do
>>> these changes to asm-generic/io.h).
>>
>> Sorry. "very relaxed" is always a very stupid thing to say about x86
>> (especially to an arm guy).
>>
>> More exactly I was referring to the absence of memory clobber in x86
>> readl_relaxed().
> 
> Yeah, my series just adds the relaxed write accessors for x86.
> 
>>> Thus allowing its use to perculate more widely really shouldn't do an harm.
>>>
>>>
>>>> but I need to send a new version including:
>>>>
>>>>   - ioreadX_relaxed and iowriteX_relaxed
>>>>   - Strengthening non-relaxed I/O accessors on architectures with non-empty
>>>>     mmiowb()
>>>>
>>>> I'll bump it up the list. In the meantime, you can have a look at my io
>>>> branch on kernel.org
>>>
>>> I'd really like to see your work included (which I spotted after I wrote
>>> the patch and when it occured to me to visit
>>> https://www.google.com/search?q=asm-generic+readl_relaxed to see if
>>> there was a well known reason not to make this change).
>>>
>>> However... I really can't see why we should delay introducing an already
>>> documented function to the remaining architectures.
> 
> I'd just rather fix the interface once instead of churning it about

Churn? 12 lines of code where two people independently produce the same
thing (apart from ordering within the file)?


> How about I dust off the series again?

Dusting off the series again would be great.

Would you consider putting readX_relaxed() and its documentation at the
front of the patchset? That way if the writel_relaxed() side log jams
again we can still get some of it delivered.


Daniel.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/