On Sat, 2022-11-05 at 15:06 +0000, Jonathan Cameron wrote: > On Thu, 3 Nov 2022 12:35:31 +0000 > "Sa, Nuno" <Nuno.Sa@xxxxxxxxxx> wrote: > > > > From: Ramona Bolboaca <ramona.bolboaca@xxxxxxxxxx> > > > Sent: Thursday, November 3, 2022 9:09 AM > > > To: jic23@xxxxxxxxxx; linux-iio@xxxxxxxxxxxxxxx; linux- > > > kernel@xxxxxxxxxxxxxxx > > > Cc: Bolboaca, Ramona <Ramona.Bolboaca@xxxxxxxxxx> > > > Subject: [PATCH v2 0/8] Remove adis_initial_startup usage > > > > > > > > > Remove 'adis_initial_startup()' usage due to the fact that it > > > leads to a > > > deadlock. > > > The same mutex is acquired twice, without releasing it, once > > > inside > > > 'adis_initial_startup()' and once inside 'adis_enable_irq()'. > > > Instead of 'adis_initial_startup()', use > > > '__adis_initial_startup()'. > > > > > > Ramona Bolboaca (8): > > > iio: accel: adis16201: Fix deadlock in probe > > > iio: accel: adis16209: Fix deadlock in probe > > > iio: gyro: adis16136: Fix deadlock in probe > > > iio: gyro: adis16260: Fix deadlock in probe > > > iio: imu: adis16400: Fix deadlock in probe > > > staging: iio: accel: adis16203: Fix deadlock in probe > > > staging: iio: accel: adis16240: Fix deadlock in probe > > > iio: imu: adis: Remove adis_initial_startup function > > > > > > drivers/iio/accel/adis16201.c | 2 +- > > > drivers/iio/accel/adis16209.c | 2 +- > > > drivers/iio/gyro/adis16136.c | 2 +- > > > drivers/iio/gyro/adis16260.c | 2 +- > > > drivers/iio/imu/adis16400.c | 2 +- > > > drivers/staging/iio/accel/adis16203.c | 2 +- > > > drivers/staging/iio/accel/adis16240.c | 2 +- > > > include/linux/iio/imu/adis.h | 12 ------------ > > > 8 files changed, 7 insertions(+), 19 deletions(-) > > > > > > > You could have placed your v2 changelog in the cover letter. > > Moreover it's the same for all patches... Anyways: > > > > Reviewed-by: Nuno Sá <nuno.sa@xxxxxxxxxx> > > This feels a little backwards. Normally we'd expect the > outer function to take the lock and the inner call to not > do so. Now it's fine to not take the lock here at all because > the outer function call is in probe anyway, before we reach > the point where there should be an concurrency. > > I wonder if we should instead do this by having > an unlocked __adis_enable_irq() that is always called > by __adis_initial_startup(). That would be the fix that > then needs backporting. > I did mentioned the same thing in the first version of the series but did not really pushed for it. Now that you mention, I agree it feels weird (and wrong from a design perspective) to have the lock, "silently", taken inside a function starting with double underscore (which should mean unlocked call). > Switching the calls from adis_initial_startup() to > __adis_initial_startup() would then just be a trivial > optimization to not take locks before they should ever matter. > > This all hinges on my assumption that the lock isn't useful. > Am I right on that? > I think so as all the calls happen during probe before registering the userspace interface. - Nuno Sá
