Re: [PATCH] scsi: ipr: fix build on 32-bit architectures

[Brian King]

On 06/08/2018 11:10 AM, Sinan Kaya wrote:
> On 6/8/2018 11:47 AM, Arnd Bergmann wrote:
>> On Fri, Jun 8, 2018 at 5:27 PM, Sinan Kaya <okaya@xxxxxxxxxxxxxx> wrote:
>>> +Will,
>>>
> 
> [snip]
> 
>>> So, it is difficult to judge how this barrier has been used without an
>>> expert opinion.
>>>
>>> Changing
>>>
>>> wmb() + writel()
>>>
>>> to
>>>
>>> wmb() + writel_relaxed()
>>>
>>> is safer than dropping the wmb() altogether.
>>
>> If the wmb() was not just about the writeq() then I would argue your patch
>> description was misleading. We certainly shouldn't replace random writeq()
>> calls with writeq_relaxed() just because we can show that the driver has
>> a barrier in front of it.
>>
>> In particular, the ipr_mask_and_clear_interrupts() function has multiple
>> writeq() or writel() calls, and even a readl() and your patch only changes
>> one of them, which seems like a rather pointless exercise as the function
>> still fully synchronizes the I/O multiple times.
> 
> You are right, I only searched wmb() + writel() combinations. Changed only
> the places where I found issues. 
> 
> We were given a direction to go to eliminating barriers direction as you already
> noted. 
> 
> I just wanted to highlight the difficulty of wholesale dropping of wmb() without
> careful inspection. [1] [2]
> 
> We certainly need a better patch that covers all use cases. Your patch is
> a step in the good direction. We just need some attention from the maintainer
> that we are not actually breaking something.

To be clear here, we are talking about two code paths that are not in the performance
path, so attempting to performance optimize them to use lighter weight mmio
accessors isn't exactly critical. 

I'm fine with the replacement patch from Arnd. Thanks Arnd!

Acked-by: Brian King <brking@xxxxxxxxxxxxxxxxxx>

> 
>>
>>> Will Deacon should probably look at why writeq_relaxed is missing on some ARM
>>> arches too.
>>>
>>> Drivers shouldn't worry about write derivatives.
>>
>> This driver defines writeq() itself for architectures that don't have it, but
>> it doesn't define writeq_relaxed() and doesn't include
>> linux/io-64-nonatomic-lo-hi.h
>> or linux/io-64-nonatomic-hi-lo.h. It seems that it needs a different behavior
>> from all other drivers here, storing the upper 32 bits into the lower
>> address and
>> the lower 32 bits into the upper address.
> 
> I don't think there is a consensus about using these includes in the community.
> I bumped into this issue before and came up with an include you pointed.
> I didn't get too much enthusiasm from the maintainer.
> 
> Why are we pushing the responsibility into the drivers? I'd think that architecture
> should take care of this. Is there a portability issue that I'm missing from some
> architecture I never heart of? (I work on Little-Endian machines most of the time)

The attributes of the adapter hardware can have an impact here. The ipr hardware, for
example, depends on the upper 4 bytes to be written first, then the lower 4 bytes
to be written second, and its the act of writing the lower 4 bytes that triggers
the adapter hardware to read the value and take action on it.

Thanks,

Brian

-- 
Brian King
Power Linux I/O
IBM Linux Technology Center