Message ID | 20230920085639.152441-1-brgl@bgdev.pl |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:612c:172:b0:3f2:4152:657d with SMTP id h50csp4127568vqi; Wed, 20 Sep 2023 06:09:33 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEzrP2fpO7VxsUpe+ssVgWjIuCWag59kGA/ogyQ+5RpXofccLvNF1eHZoZyzrMwEUGv3OpS X-Received: by 2002:a05:6a20:13cb:b0:159:e1c8:61c2 with SMTP id ho11-20020a056a2013cb00b00159e1c861c2mr2228069pzc.3.1695215373257; Wed, 20 Sep 2023 06:09:33 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1695215373; cv=none; d=google.com; s=arc-20160816; b=tmXd3azN5gR/N8o9Q9stPoV0H0xzOJo7tCdj3g9QYZZxnl3/J1iKgNrgePv/z05sx3 m3Ok7UPke+fSF/t+hLeKRiQ20VW9eYHs7h5wV/vYzjRBcWL/KmaR4HkMSXnMSoRn+HN5 y51UDXWBxBpBVZ0lJBjIbumk4iwCwjDrbRFsn9QRB5K2P02oGgm7pWAB5ZRqJam2vWRW rvg4YixivYAtlrIlnPuc++84rMbBSSpJkHtoIsbJqhrKfmz5FT6MtrA7VKxFEdWMwFKq 5a0CASPwO+vbruSt0rmmQxRPkA6lXuXhSyXR9JOPQIQGUUd2T+XgoAEYwG/POtNmV7t/ x9sA== 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:dkim-signature; bh=tWw1t4ALS79pB9kxjWM5PMdLLOt6nPp1AWWZtrHrPQ8=; fh=fw6biwzn34ngIUS3OuPB+D+TvAzoz1YwnvitHxA+Jyw=; b=SLhn+eCeLJxsfiPv15ihBIHvGYEHfg104p6JIRuPABKMNPM1QXtZzOf27l0RivRS10 9H+jNS4PKpWkuXiu3WUU5N+hyJSd0+hY3kCkCXNLg4pBxDw2dKTiuwSKYBn2n8zJ21Qb 1KEan/bR6bLYxdsi1STsQKmojVKXdDHDgW3ZwktRkxS4hbQM6q9ryx+Kkw2Z2+vzSOSt wnP0WjRCFG89JNH9M1YhEYoU8b2arKIAF5sG5e8b7WpPV20BjaiqL1O5N1VCcZTpVOvV pluFsXr+3RPAZb9Kj/2UVnpug9dboEJV1IepbOuZocHQtUvULBpSiRfStbXKnntymni+ mccg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@bgdev-pl.20230601.gappssmtp.com header.s=20230601 header.b=e7+mjuYJ; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from pete.vger.email (pete.vger.email. [2620:137:e000::3:6]) by mx.google.com with ESMTPS id q12-20020a056a00150c00b0068e405d9217si12401517pfu.302.2023.09.20.06.09.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 20 Sep 2023 06:09:33 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6; Authentication-Results: mx.google.com; dkim=pass header.i=@bgdev-pl.20230601.gappssmtp.com header.s=20230601 header.b=e7+mjuYJ; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by pete.vger.email (Postfix) with ESMTP id 397CC82C9D8C; Wed, 20 Sep 2023 01:57:21 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at pete.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233781AbjITI47 (ORCPT <rfc822;toshivichauhan@gmail.com> + 26 others); Wed, 20 Sep 2023 04:56:59 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45714 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233743AbjITI45 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 20 Sep 2023 04:56:57 -0400 Received: from mail-ed1-x530.google.com (mail-ed1-x530.google.com [IPv6:2a00:1450:4864:20::530]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 8128B93 for <linux-kernel@vger.kernel.org>; Wed, 20 Sep 2023 01:56:49 -0700 (PDT) Received: by mail-ed1-x530.google.com with SMTP id 4fb4d7f45d1cf-5310a63cf7bso4046126a12.1 for <linux-kernel@vger.kernel.org>; Wed, 20 Sep 2023 01:56:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bgdev-pl.20230601.gappssmtp.com; s=20230601; t=1695200208; x=1695805008; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=tWw1t4ALS79pB9kxjWM5PMdLLOt6nPp1AWWZtrHrPQ8=; b=e7+mjuYJcgP7VkEekbsmVnlyvFKX7E80u/bZBPvgDbEt9PQl2Wb1Q+3XZHZV3Y2XaU 6f0aPFeoeAb2Kcpz9uv8moXPSWybQKRjqAYCaSjJrSe/u8D/ltXgMCAvSnzmyXEbG5Jf FVBQgE5ga54gxGAkaL/H2dOWArSA2cmGkvrExzV2VjQrWoB5r1jGj9eFHmd4cBQKSDek 7dDK/xtVFf5rfH+gIJ2/182NmEBYNkGUWzGk0H96nh/7UBQtD/GxQZFK+T3XwFFydjxG uotEkm7cGeeZ+GHHNn/L+C7e9DADNwZtpJAaAGuNbfUz8FeKVkFSfXjc1/C//jYGdGmm A3IA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695200208; x=1695805008; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=tWw1t4ALS79pB9kxjWM5PMdLLOt6nPp1AWWZtrHrPQ8=; b=Ur0xolJrdlD/5mDBJnwXBsu/ggsPfcleqqd1QDyn4S98txfTD7JMDYy0jU4wtayzLY 3BnkNyEF5W+M5+3ybIjvFLOPqmkUO6lNzrkrHknUGPt2nnuWfFWpJbH5Do3mabLgZzpi InRxDwDNLvQl+vScYVjwndAiUCtby3JXql0YXwj7OnClS4KKfZMYi9OfRBkqm90sdOZE 2e654GGvEXyS7G9JnGJ7a1k0AG0iSTuKYzqhlmzJC/+uc1x+Eyem07RxZK+oEtPGnCO5 /wI6dApT1Vx7jvzeX2QS0Mx76dMPaXrvZ4tE1EeVkv+xk4+2MYDVULSUV/ZX05qBSkRF eYLQ== X-Gm-Message-State: AOJu0Yw4sRsO+UwAlUwGypybpCV432TM0lWFbYRuVb9fMglgRZZ/suoL EQCZ4VkPkCu1x/yZLhE6BVOAYOv3y3fwS4h16sVQfQ== X-Received: by 2002:a05:6402:1b1b:b0:52b:db31:3c5c with SMTP id by27-20020a0564021b1b00b0052bdb313c5cmr1667421edb.0.1695200207902; Wed, 20 Sep 2023 01:56:47 -0700 (PDT) Received: from brgl-uxlite.. (static-212-193-78-212.thenetworkfactory.nl. [212.78.193.212]) by smtp.gmail.com with ESMTPSA id t18-20020a056402021200b0052fdfd8870bsm8621789edv.89.2023.09.20.01.56.47 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 20 Sep 2023 01:56:47 -0700 (PDT) From: Bartosz Golaszewski <brgl@bgdev.pl> To: Linus Walleij <linus.walleij@linaro.org>, Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org, Bartosz Golaszewski <bartosz.golaszewski@linaro.org> Subject: [PATCH] gpiolib: extend the critical sections of lookup tables Date: Wed, 20 Sep 2023 10:56:39 +0200 Message-Id: <20230920085639.152441-1-brgl@bgdev.pl> X-Mailer: git-send-email 2.39.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Wed, 20 Sep 2023 01:57:21 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1777558862762243704 X-GMAIL-MSGID: 1777562154999430727 |
Series |
gpiolib: extend the critical sections of lookup tables
|
|
Commit Message
Bartosz Golaszewski
Sept. 20, 2023, 8:56 a.m. UTC
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> There are two places in the code where we retrieve a lookup table using gpiod_find_lookup_table() (which protects the table list with the lookup table lock) and then use it after the lock is released. We need to keep the lookup table mutex locked the entire time we're using the tables. Remove the locking from gpiod_find_lookup_table() and use guards to protect the code actually using the table objects. Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> --- drivers/gpio/gpiolib.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-)
Comments
On Wed, Sep 20, 2023 at 10:56 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > There are two places in the code where we retrieve a lookup table using > gpiod_find_lookup_table() (which protects the table list with the lookup > table lock) and then use it after the lock is released. > > We need to keep the lookup table mutex locked the entire time we're using > the tables. Remove the locking from gpiod_find_lookup_table() and use > guards to protect the code actually using the table objects. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> I convinced myself that this change is correct, good find! > @@ -3822,8 +3822,6 @@ static struct gpiod_lookup_table *gpiod_find_lookup_table(struct device *dev) Can we rename this function gpiod_find_lookup_table_locked() as per precedents in the kernel, to indicate that it needs to be called with a lock held? Either way: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Wed, 20 Sep 2023 11:12:58 +0200, Linus Walleij <linus.walleij@linaro.org> said: > On Wed, Sep 20, 2023 at 10:56 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > >> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> >> >> There are two places in the code where we retrieve a lookup table using >> gpiod_find_lookup_table() (which protects the table list with the lookup >> table lock) and then use it after the lock is released. >> >> We need to keep the lookup table mutex locked the entire time we're using >> the tables. Remove the locking from gpiod_find_lookup_table() and use >> guards to protect the code actually using the table objects. >> >> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > I convinced myself that this change is correct, good find! > >> @@ -3822,8 +3822,6 @@ static struct gpiod_lookup_table *gpiod_find_lookup_table(struct device *dev) > > Can we rename this function gpiod_find_lookup_table_locked() > as per precedents in the kernel, to indicate that it needs to be > called with a lock held? > I think you mean gpiod_find_lookup_table_unlocked() as with this change it will no longer take the lock? Bart > Either way: > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > Yours, > Linus Walleij >
On Wed, 20 Sep 2023 12:58:58 +0200, Linus Walleij <linus.walleij@linaro.org> said: > On Wed, Sep 20, 2023 at 11:33 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: >> On Wed, 20 Sep 2023 11:12:58 +0200, Linus Walleij >> <linus.walleij@linaro.org> said: >> > On Wed, Sep 20, 2023 at 10:56 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > >> > Can we rename this function gpiod_find_lookup_table_locked() >> > as per precedents in the kernel, to indicate that it needs to be >> > called with a lock held? >> > >> >> I think you mean gpiod_find_lookup_table_unlocked() as with this change it >> will no longer take the lock? > > I think the pattern is the one I indicated: *_locked() means the function > is to be called with the appropriate lock held, cf > arch/arm64/kvm/hyp/nvhe/mm.c > > pkvm_create_mappings() takes a lock and then calls > pkvm_create_mappings_locked() which even asserts that > the lock is held. > Ha! I always though the pattern is to call the functions that *DON'T* take the lock _unlocked(). This is what I used in gpiolib-cdev.c or gpio-sim.c. I guess both conventions make sense in some way. Bart
On Wed, Sep 20, 2023 at 2:51 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Wed, 20 Sep 2023 12:58:58 +0200, Linus Walleij <linus.walleij@linaro.org> said: > > On Wed, Sep 20, 2023 at 11:33 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > >> On Wed, 20 Sep 2023 11:12:58 +0200, Linus Walleij > >> <linus.walleij@linaro.org> said: > >> > On Wed, Sep 20, 2023 at 10:56 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > >> > Can we rename this function gpiod_find_lookup_table_locked() > >> > as per precedents in the kernel, to indicate that it needs to be > >> > called with a lock held? > >> > > >> > >> I think you mean gpiod_find_lookup_table_unlocked() as with this change it > >> will no longer take the lock? > > > > I think the pattern is the one I indicated: *_locked() means the function > > is to be called with the appropriate lock held, cf > > arch/arm64/kvm/hyp/nvhe/mm.c > > > > pkvm_create_mappings() takes a lock and then calls > > pkvm_create_mappings_locked() which even asserts that > > the lock is held. > > > > Ha! I always though the pattern is to call the functions that *DON'T* take > the lock _unlocked(). This is what I used in gpiolib-cdev.c or gpio-sim.c. > > I guess both conventions make sense in some way. > > Bart I don't think I will be doing it just now. We don't use this convention elsewhere in drivers/gpio/ and we'll have a lot of locking reworked soon anyway. We may get to it when that is done. Bart
On Mon, Sep 25, 2023 at 4:24 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Wed, Sep 20, 2023 at 2:51 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > On Wed, 20 Sep 2023 12:58:58 +0200, Linus Walleij <linus.walleij@linaro.org> said: > > > On Wed, Sep 20, 2023 at 11:33 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > >> On Wed, 20 Sep 2023 11:12:58 +0200, Linus Walleij > > >> <linus.walleij@linaro.org> said: > > >> > On Wed, Sep 20, 2023 at 10:56 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > > >> > Can we rename this function gpiod_find_lookup_table_locked() > > >> > as per precedents in the kernel, to indicate that it needs to be > > >> > called with a lock held? > > >> > > > >> > > >> I think you mean gpiod_find_lookup_table_unlocked() as with this change it > > >> will no longer take the lock? > > > > > > I think the pattern is the one I indicated: *_locked() means the function > > > is to be called with the appropriate lock held, cf > > > arch/arm64/kvm/hyp/nvhe/mm.c > > > > > > pkvm_create_mappings() takes a lock and then calls > > > pkvm_create_mappings_locked() which even asserts that > > > the lock is held. > > > > > > > Ha! I always though the pattern is to call the functions that *DON'T* take > > the lock _unlocked(). This is what I used in gpiolib-cdev.c or gpio-sim.c. > > > > I guess both conventions make sense in some way. > > > > Bart > > I don't think I will be doing it just now. We don't use this > convention elsewhere in drivers/gpio/ and we'll have a lot of locking > reworked soon anyway. We may get to it when that is done. > > Bart If there are no objections, I'd like to queue it this week. Bart
On Wed, Sep 27, 2023 at 9:01 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> If there are no objections, I'd like to queue it this week.
Go ahead.
Kernel looks better after this patch than before => queue it
Yours,
Linus Walleij
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index edffa0d2acaa..7c27a1efc1b0 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -3822,8 +3822,6 @@ static struct gpiod_lookup_table *gpiod_find_lookup_table(struct device *dev) const char *dev_id = dev ? dev_name(dev) : NULL; struct gpiod_lookup_table *table; - mutex_lock(&gpio_lookup_lock); - list_for_each_entry(table, &gpio_lookup_list, list) { if (table->dev_id && dev_id) { /* @@ -3831,21 +3829,18 @@ static struct gpiod_lookup_table *gpiod_find_lookup_table(struct device *dev) * a match */ if (!strcmp(table->dev_id, dev_id)) - goto found; + return table; } else { /* * One of the pointers is NULL, so both must be to have * a match */ if (dev_id == table->dev_id) - goto found; + return table; } } - table = NULL; -found: - mutex_unlock(&gpio_lookup_lock); - return table; + return NULL; } static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id, @@ -3855,6 +3850,8 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id, struct gpiod_lookup_table *table; struct gpiod_lookup *p; + guard(mutex)(&gpio_lookup_lock); + table = gpiod_find_lookup_table(dev); if (!table) return desc; @@ -3920,15 +3917,18 @@ static int platform_gpio_count(struct device *dev, const char *con_id) struct gpiod_lookup *p; unsigned int count = 0; - table = gpiod_find_lookup_table(dev); - if (!table) - return -ENOENT; + scoped_guard(mutex, &gpio_lookup_lock) { + table = gpiod_find_lookup_table(dev); + if (!table) + return -ENOENT; - for (p = &table->table[0]; p->key; p++) { - if ((con_id && p->con_id && !strcmp(con_id, p->con_id)) || - (!con_id && !p->con_id)) - count++; + for (p = &table->table[0]; p->key; p++) { + if ((con_id && p->con_id && !strcmp(con_id, p->con_id)) || + (!con_id && !p->con_id)) + count++; + } } + if (!count) return -ENOENT;