On 30/08/16 17:18, Gregor Boirie wrote:

On 08/29/2016 09:01 PM, Jonathan Cameron wrote:

quoted

On 25/08/16 15:45, Gregor Boirie wrote:

quoted

Answers inline

On 08/25/2016 08:34 AM, Peter Meerwald-Stadler wrote:

quoted

quoted

+
+    zpa_init_runtime(slave);
+
+    err = devm_iio_device_register(slave, indio_dev);

can't use devm_ register if there is a non-empty remove()

Removal hooks are registered onto bus specific devices not onto
IIO ones. Enabling devm debug outputs a removal flow trace that
seems correct. Is this really wrong ?

Yes.  The unwind of devm_iio_device_register will occur after the
whatever is in the remove function.  As devm_iio_device_unregister
is responsible for removal of the userspace and other interfaces
until that happens they are still present.

Hence whilst a remove is going on the device can be powered
down but the interfaces still exposed.  A read at that point
is going to cause some an error to occur.

Hence, using the devm form is fine if everything else is
handled automatically, i.e. there is nothing in remove.
Otherwise, you almost always have a race condition.

It's a minor one given how often people remove modules, but
worth cleaning up anyway!

Got it.

But something still makes me feel uncomfortable. Shouldn't we ensure the
bus specific driver (i.e., zpa2326_i2c or zpa2326_spi in this case) be refcounted
upon entry into the IIO layer ? Using try_module_get()/module_put() would :
* prevent any attempt to remove module while being used
* prevent any attempt to use module while it is going away.

I'm not quite sure here because of possible race conditions however (feels like
mixing devm_ and module layers).

Anyway, it seems quite strange module removal succeeds despite being under use
by userspace. This leads to inconsistent system state where userspace process
gets stuck in poll syscall, waiting for data that will never come since modprobing
module again will in the end allocate an new IIO device, etc...

In theory it should all unwind fine.  Lars spent a while some time
ago hammering this stuff with some test scripts and getting all
the fall overs to happen. It should wake up the poll in the
device_unregister and subsequent read should give an error to
say the device has gone away.

Do you have a particular case where it gets stuck?

It's effectively impossible to prevent a device going away when
userspace is using it.  The module_get, module_put makes it
slightly less likely, but unfortunately some sensors can simply
be pulled from a usb port or similar or a remove forced from
userspace.

Jonathan

confused,
Grégor.

quoted

Jonathan

quoted

Many thanks for the review. Regards,
Greg.
-- 
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html