| Message ID | 20221103080847.162509-1-ramona.bolboaca@analog.com |
|---|---|
| Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org>
Delivered-To: ouuuleilei@gmail.com
Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp386158wru;
Thu, 3 Nov 2022 01:13:53 -0700 (PDT)
X-Google-Smtp-Source:
AMsMyM7shGGxcA7JAuHEwUKJ0REFN1Q0sRm7zd3dHxqHmz45cluxEDc6bbql+s6FhYkD36OVyQ9I
X-Received: by 2002:aa7:cfda:0:b0:461:9d22:3484 with SMTP id
r26-20020aa7cfda000000b004619d223484mr29640323edy.142.1667463233180;
Thu, 03 Nov 2022 01:13:53 -0700 (PDT)
ARC-Seal: i=1; a=rsa-sha256; t=1667463233; cv=none;
d=google.com; s=arc-20160816;
b=cCvGrJtZatOLMBsYKlhU1zGdsdRhAZih+QSiDrNhg6Is39wTlHajYYOc0VIW981oI8
dd0SdChS96x0+6IZIaSa8ny3b1nD+ofsfPqJaip4e2m/XKZUtyLIGdorbVxLHczMpDN8
0lysc2iYwKaaEBgBrq7GdIJxnyuV4Bn2/ck+4wWQ0O7fCk8hSrrbZsGupNOEIubo1l1F
9hb8wWX4vpqmf+ZBFeSJU0i0xQaq7yGcRorhhvH/P5sWI9Kzj5iYDrSEb6PmNGNWpliT
lndF6vxklmwwypxKx+uSwscIZYK9xvXU8AR5Y0vycIRUwlmyuYVJLHu50op/R+HuE688
QM8Q==
ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com;
s=arc-20160816;
h=list-id:precedence:content-transfer-encoding:mime-version
:message-id:date:subject:cc:to:from;
bh=KvcJuvcoJSlFi5v6aO7aibZI1dfqavwE6MuPqLhQf8U=;
b=fWP8xqBjtZy/0oCUnm2r31KQUTjJKZJ4kHBQ74YgbjyV1dhr7RGfSl1t/taR7Z245y
Zq0nqvFcf0+SNJC0q/BhfrrgBc456U4QYT6TyuwCVjn5Z/OE0V5j3tHrge/423pAyzb+
0IgN3OP7g5/7DRS8d8hWXcorksHQcc5y1kZeONc/12FCXBAluJw2JZ2ZDNMlwFo5BDTR
DV0Z+CYboQt9RIX3EOV0P7u8Zq1kCib1OclLCQendHE5IPSozlVgLGfz79TUCah2pwpW
5sJgWAkVSgPVYtxzqw6sAV/2cAR5mYAPcc5pZ9GMDUwPxzHEbucS328Oa97PV7LASg0I
UPeA==
ARC-Authentication-Results: i=1; mx.google.com;
spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org
designates 2620:137:e000::1:20 as permitted sender)
smtp.mailfrom=linux-kernel-owner@vger.kernel.org;
dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=analog.com
Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20])
by mx.google.com with ESMTP id
di21-20020a170906731500b0078da5f6ed9esi473413ejc.779.2022.11.03.01.13.27;
Thu, 03 Nov 2022 01:13:53 -0700 (PDT)
Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org
designates 2620:137:e000::1:20 as permitted sender)
client-ip=2620:137:e000::1:20;
Authentication-Results: mx.google.com;
spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org
designates 2620:137:e000::1:20 as permitted sender)
smtp.mailfrom=linux-kernel-owner@vger.kernel.org;
dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=analog.com
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
id S231280AbiKCIJO (ORCPT <rfc822;yves.mi.zy@gmail.com> + 99 others);
Thu, 3 Nov 2022 04:09:14 -0400
Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57302 "EHLO
lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
with ESMTP id S230171AbiKCIJJ (ORCPT
<rfc822;linux-kernel@vger.kernel.org>);
Thu, 3 Nov 2022 04:09:09 -0400
Received: from mx0a-00128a01.pphosted.com (mx0a-00128a01.pphosted.com
[148.163.135.77])
by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 744B0B1;
Thu, 3 Nov 2022 01:09:07 -0700 (PDT)
Received: from pps.filterd (m0167089.ppops.net [127.0.0.1])
by mx0a-00128a01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id
2A347R6s008154;
Thu, 3 Nov 2022 04:09:04 -0400
Received: from nwd2mta3.analog.com ([137.71.173.56])
by mx0a-00128a01.pphosted.com (PPS) with ESMTPS id 3kjn9u0rqe-1
(version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256
verify=NOT);
Thu, 03 Nov 2022 04:09:04 -0400
Received: from ASHBMBX9.ad.analog.com (ASHBMBX9.ad.analog.com [10.64.17.10])
by nwd2mta3.analog.com (8.14.7/8.14.7) with ESMTP id 2A3892hg004108
(version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256
verify=FAIL);
Thu, 3 Nov 2022 04:09:02 -0400
Received: from ASHBMBX9.ad.analog.com (10.64.17.10) by ASHBMBX9.ad.analog.com
(10.64.17.10) with Microsoft SMTP Server (version=TLS1_2,
cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.986.14; Thu, 3 Nov 2022
04:09:01 -0400
Received: from zeus.spd.analog.com (10.66.68.11) by ashbmbx9.ad.analog.com
(10.64.17.10) with Microsoft SMTP Server id 15.2.986.14 via Frontend
Transport; Thu, 3 Nov 2022 04:09:01 -0400
Received: from rbolboac.ad.analog.com ([10.48.65.139])
by zeus.spd.analog.com (8.15.1/8.15.1) with ESMTP id 2A388q0I009844;
Thu, 3 Nov 2022 04:08:54 -0400
From: Ramona Bolboaca <ramona.bolboaca@analog.com>
To: <jic23@kernel.org>, <linux-iio@vger.kernel.org>,
<linux-kernel@vger.kernel.org>
CC: Ramona Bolboaca <ramona.bolboaca@analog.com>
Subject: [PATCH v2 0/8] Remove adis_initial_startup usage
Date: Thu, 3 Nov 2022 10:08:39 +0200
Message-ID: <20221103080847.162509-1-ramona.bolboaca@analog.com>
X-Mailer: git-send-email 2.25.1
MIME-Version: 1.0
Content-Transfer-Encoding: 7BIT
Content-Type: text/plain; charset=US-ASCII
X-ADIRuleOP-NewSCL: Rule Triggered
X-Proofpoint-GUID: TacEA3Zes1yu2OZk2HRN9MZHYQwQbh7v
X-Proofpoint-ORIG-GUID: TacEA3Zes1yu2OZk2HRN9MZHYQwQbh7v
X-Proofpoint-Virus-Version: vendor=baseguard
engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1
definitions=2022-11-02_15,2022-11-02_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0
malwarescore=0 bulkscore=0
priorityscore=1501 lowpriorityscore=0 mlxscore=0 spamscore=0
impostorscore=0 clxscore=1015 suspectscore=0 mlxlogscore=999 phishscore=0
adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1
engine=8.12.0-2210170000 definitions=main-2211030057
X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_LOW,
RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_PASS autolearn=ham
autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?=
X-GMAIL-THRID: =?utf-8?q?1748461927095732626?=
X-GMAIL-MSGID: =?utf-8?q?1748461927095732626?=
|
| Series |
Remove adis_initial_startup usage
|
|
Message
Ramona Bolboaca
Nov. 3, 2022, 8:08 a.m. UTC
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(-)
Comments
> From: Ramona Bolboaca <ramona.bolboaca@analog.com> > Sent: Thursday, November 3, 2022 9:09 AM > To: jic23@kernel.org; linux-iio@vger.kernel.org; linux- > kernel@vger.kernel.org > Cc: Bolboaca, Ramona <Ramona.Bolboaca@analog.com> > 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@analog.com> - Nuno Sá
On Thu, 3 Nov 2022 12:35:31 +0000 "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > From: Ramona Bolboaca <ramona.bolboaca@analog.com> > > Sent: Thursday, November 3, 2022 9:09 AM > > To: jic23@kernel.org; linux-iio@vger.kernel.org; linux- > > kernel@vger.kernel.org > > Cc: Bolboaca, Ramona <Ramona.Bolboaca@analog.com> > > 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@analog.com> 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. 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? Jonathan > > - Nuno Sá
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@analog.com> wrote: > > > > From: Ramona Bolboaca <ramona.bolboaca@analog.com> > > > Sent: Thursday, November 3, 2022 9:09 AM > > > To: jic23@kernel.org; linux-iio@vger.kernel.org; linux- > > > kernel@vger.kernel.org > > > Cc: Bolboaca, Ramona <Ramona.Bolboaca@analog.com> > > > 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@analog.com> > > 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á