Re: [PATCH 1/3] iio: light: Add driver for ap3216c

[Sven Van Asbroeck <thesven73@gmail.com>]

Again thanks for the feedback, Jonathan !

On Wed, Feb 20, 2019 at 7:32 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> Looks to me like that happens (I haven't checked that thoroughly) via
> kernfs_fops_write which takes a mutex - so we have a barrier.
>

Yes, if there's a mutex in the path somewhere (which sysfs/kernfs appears to
provide in this case) then we're ok, as the mutex_unlock() should provide
the barrier which makes the flags visible to the threaded handler.

If that never changes, we are good. As with regmap_bulk_write(), changing the
current behaviour may break many drivers. So it's unlikely to change.

>
> That is a serious - "in theory" circumstance.  The moment we hit any path at
> all that results in a memory barrier it will see it.  Here its not critical
> so we can wait.  In this case this is triggered by a userspace write.

I wish this was an "in theory" circumstance ! I've personally been stung by
this very issue. Code worked great in all tests we could throw at it, but
failed at the customer, of course.

IMHO it's easy for developers to be lulled into a false sense of security.
In many/most cases, there'll be a barrier in the 'invisible' supporting code
somewhere. Or we have to use a lock anyway to protect against concurrent
inconsistent writes, which also implies a barrier. So when developers forget
that these visibility limitations even exist, usually nothing bad will happen.

Until it does, and then we get bitten :)