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. > >> 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) [1] https://patchwork.kernel.org/patch/10301861/ [2] https://www.mail-archive.com/netdev@xxxxxxxxxxxxxxx/msg227443.html > > Arnd > -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
