From patchwork Tue Jan 30 12:48:07 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bartosz Golaszewski X-Patchwork-Id: 768302 Received: from mail-wm1-f54.google.com (mail-wm1-f54.google.com [209.85.128.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 977746A326 for ; Tue, 30 Jan 2024 12:48:42 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.54 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1706618925; cv=none; b=SwA8FvAs9imbfLtLhDEPYEri9s2k6msV1SU06oHRKafRV9UxER+JpHXeWkJS5G6xwjCEM6SF28UiBHrcm0BChVFrSCH1ijd2PbHTHLjwKn5DLvmxYwTp2cFUjCaAMGm2kFak2ZavrorcEyhxf09PzYzCDqeLfEd3wPRWaP1wz20= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1706618925; c=relaxed/simple; bh=86Y+AQ3y8n9w/i3PI5/p7q/gCkmb53F9yvtd/1pyEI0=; h=From:To:Cc:Subject:Date:Message-Id:In-Reply-To:References: MIME-Version; b=im94lsujm5waF6Jzx0h+PO/+hYwLKX0UvYuBMQrDIFtkBCTsCWAUpdBMZmyAwR3pUW6LkR7xSvCUoXTlOSfT6Eo3Nrx7Kb5Qj6XRB+2ucARBPGRfMt60A6pt/DZFwMiArXNANP9suKs+fOpMtm0umnqpmfYehaYA8KRUgFgqARY= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=bgdev.pl; spf=none smtp.mailfrom=bgdev.pl; dkim=pass (2048-bit key) header.d=bgdev-pl.20230601.gappssmtp.com header.i=@bgdev-pl.20230601.gappssmtp.com header.b=y+AkX2c1; arc=none smtp.client-ip=209.85.128.54 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=bgdev.pl Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=bgdev.pl Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=bgdev-pl.20230601.gappssmtp.com header.i=@bgdev-pl.20230601.gappssmtp.com header.b="y+AkX2c1" Received: by mail-wm1-f54.google.com with SMTP id 5b1f17b1804b1-40e72a567eeso57155075e9.0 for ; Tue, 30 Jan 2024 04:48:42 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bgdev-pl.20230601.gappssmtp.com; s=20230601; t=1706618921; x=1707223721; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=3ISdQKhUHMbel1ayot3kDVw2CcPuKldTF7psGraEV5g=; b=y+AkX2c1TnVzv9UIr/kgjU8zLrK6dMRPTwAUAWfe29ypgt4O+fGBzpWgGGpChh9FOW NG8m1ndYm8YTHw7HALE/y2Mjm9wCadFVq4Q0qxIJcgTz3Oomm4J2b6toTtopqK790VL1 WOCovK0VFkM2dV7+9i49K80Y5+UwjMDuwNNBwfBmFrDWJUFthDN8Ygo/vKYuM82dj+90 N3gQly+Y928S4gkOUJwf/jMgVx+Sr6O1mnX2SuEsSIktryhrVkPs5ptrvxlBy9onrRTF LQFOixNC2nbS8tzVXEaCUjC8gS+77gis2NkpQDrh/3ruNg/cvDaSPLZK6/73H8YNLIIT 2gLw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1706618921; x=1707223721; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=3ISdQKhUHMbel1ayot3kDVw2CcPuKldTF7psGraEV5g=; b=uE26qnthL8hej585rC8dxqH0ONYH5LNeAjWXkMpvm5vmnzhmN3OZmbqa13Hn1Kbw7t 7zLf4ScZi4hVJollZNN/HYvXiZkUirvaZHyUs9rxrRZMnWNMScuOFonBWW8ir56eK406 Q6BUbtTYMEfFhoGhayBtCjk3jkSSrQmTYEkWz7ZbgDNLA75uRBKUyDXorzKHGRIhtRmZ byH+8ZioH6/nRDFpEuKs4XQ9wxht3BIvvPlbbs8C4zYUqK/5d90EkaMiWhn9BOamSoAw yVjQ4lcLe89pPe4uEfAok9UHWm3UVFRiBT89Nk9RCv9SgMVR6OqCU76D7Q8IP8qqlnKM ErfQ== X-Gm-Message-State: AOJu0YwkFa2y15X+nF0Krh1m2N949IJE+SfUz9fR/XNksUPbdYMbVQ8v zcBnI6s2H4pjBPieshGEF8Yi5OgeXYN0Kyplkq/NSwl7r7Zj9CsevPDuY34d8Mw= X-Google-Smtp-Source: AGHT+IGWn4BPiCCxI47pc0kQWYu/XZls9SyCDrIX1E8/QuR2urA7cmgjpCAIHRmwQbXkxheO43JsFg== X-Received: by 2002:a7b:c4c5:0:b0:40e:cf0f:5fc9 with SMTP id g5-20020a7bc4c5000000b0040ecf0f5fc9mr6490225wmk.31.1706618920639; Tue, 30 Jan 2024 04:48:40 -0800 (PST) Received: from brgl-uxlite.home ([2a01:cb1d:334:ac00:abc8:f77e:abeb:129c]) by smtp.gmail.com with ESMTPSA id en8-20020a056000420800b0033940016d6esm6625337wrb.93.2024.01.30.04.48.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 30 Jan 2024 04:48:40 -0800 (PST) From: Bartosz Golaszewski To: Linus Walleij , Kent Gibson , Alex Elder , Geert Uytterhoeven , "Paul E . McKenney" , Andy Shevchenko , Wolfram Sang Cc: linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org, Bartosz Golaszewski Subject: [PATCH 01/22] gpio: protect the list of GPIO devices with SRCU Date: Tue, 30 Jan 2024 13:48:07 +0100 Message-Id: <20240130124828.14678-2-brgl@bgdev.pl> X-Mailer: git-send-email 2.40.1 In-Reply-To: <20240130124828.14678-1-brgl@bgdev.pl> References: <20240130124828.14678-1-brgl@bgdev.pl> Precedence: bulk X-Mailing-List: linux-gpio@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 From: Bartosz Golaszewski We're working towards removing the "multi-function" GPIO spinlock that's implemented terribly wrong. We tried using an RW-semaphore to protect the list of GPIO devices but it turned out that we still have old code using legacy GPIO calls that need to translate the global GPIO number to the address of the associated descriptor and - to that end - traverse the list while holding the lock. If we change the spinlock to a sleeping lock then we'll end up with "scheduling while atomic" bugs. Let's allow lockless traversal of the list using SRCU and only use the mutex when modyfing the list. While at it: let's protect the period between when we start the lookup and when we finally request the descriptor (increasing the reference count of the GPIO device) with the SRCU read lock. Signed-off-by: Bartosz Golaszewski Reviewed-by: Linus Walleij --- drivers/gpio/gpiolib.c | 203 +++++++++++++++++++++-------------------- 1 file changed, 104 insertions(+), 99 deletions(-) diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index d50a786f8176..69979de76485 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -14,12 +15,14 @@ #include #include #include +#include #include #include #include #include #include #include +#include #include #include @@ -81,7 +84,12 @@ DEFINE_SPINLOCK(gpio_lock); static DEFINE_MUTEX(gpio_lookup_lock); static LIST_HEAD(gpio_lookup_list); + LIST_HEAD(gpio_devices); +/* Protects the GPIO device list against concurrent modifications. */ +static DEFINE_MUTEX(gpio_devices_lock); +/* Ensures coherence during read-only accesses to the list of GPIO devices. */ +DEFINE_STATIC_SRCU(gpio_devices_srcu); static DEFINE_MUTEX(gpio_machine_hogs_mutex); static LIST_HEAD(gpio_machine_hogs); @@ -113,20 +121,16 @@ static inline void desc_set_label(struct gpio_desc *d, const char *label) struct gpio_desc *gpio_to_desc(unsigned gpio) { struct gpio_device *gdev; - unsigned long flags; - spin_lock_irqsave(&gpio_lock, flags); - - list_for_each_entry(gdev, &gpio_devices, list) { - if (gdev->base <= gpio && - gdev->base + gdev->ngpio > gpio) { - spin_unlock_irqrestore(&gpio_lock, flags); - return &gdev->descs[gpio - gdev->base]; + scoped_guard(srcu, &gpio_devices_srcu) { + list_for_each_entry_srcu(gdev, &gpio_devices, list, + srcu_read_lock_held(&gpio_devices_srcu)) { + if (gdev->base <= gpio && + gdev->base + gdev->ngpio > gpio) + return &gdev->descs[gpio - gdev->base]; } } - spin_unlock_irqrestore(&gpio_lock, flags); - if (!gpio_is_valid(gpio)) pr_warn("invalid GPIO %d\n", gpio); @@ -282,7 +286,8 @@ static int gpiochip_find_base_unlocked(int ngpio) struct gpio_device *gdev; int base = GPIO_DYNAMIC_BASE; - list_for_each_entry(gdev, &gpio_devices, list) { + list_for_each_entry_srcu(gdev, &gpio_devices, list, + lockdep_is_held(&gpio_devices_lock)) { /* found a free space? */ if (gdev->base >= base + ngpio) break; @@ -354,23 +359,25 @@ static int gpiodev_add_to_list_unlocked(struct gpio_device *gdev) { struct gpio_device *prev, *next; + lockdep_assert_held(&gpio_devices_lock); + if (list_empty(&gpio_devices)) { /* initial entry in list */ - list_add_tail(&gdev->list, &gpio_devices); + list_add_tail_rcu(&gdev->list, &gpio_devices); return 0; } next = list_first_entry(&gpio_devices, struct gpio_device, list); if (gdev->base + gdev->ngpio <= next->base) { /* add before first entry */ - list_add(&gdev->list, &gpio_devices); + list_add_rcu(&gdev->list, &gpio_devices); return 0; } prev = list_last_entry(&gpio_devices, struct gpio_device, list); if (prev->base + prev->ngpio <= gdev->base) { /* add behind last entry */ - list_add_tail(&gdev->list, &gpio_devices); + list_add_tail_rcu(&gdev->list, &gpio_devices); return 0; } @@ -382,11 +389,13 @@ static int gpiodev_add_to_list_unlocked(struct gpio_device *gdev) /* add between prev and next */ if (prev->base + prev->ngpio <= gdev->base && gdev->base + gdev->ngpio <= next->base) { - list_add(&gdev->list, &prev->list); + list_add_rcu(&gdev->list, &prev->list); return 0; } } + synchronize_srcu(&gpio_devices_srcu); + return -EBUSY; } @@ -399,26 +408,21 @@ static int gpiodev_add_to_list_unlocked(struct gpio_device *gdev) static struct gpio_desc *gpio_name_to_desc(const char * const name) { struct gpio_device *gdev; - unsigned long flags; + struct gpio_desc *desc; if (!name) return NULL; - spin_lock_irqsave(&gpio_lock, flags); - - list_for_each_entry(gdev, &gpio_devices, list) { - struct gpio_desc *desc; + guard(srcu)(&gpio_devices_srcu); + list_for_each_entry_srcu(gdev, &gpio_devices, list, + srcu_read_lock_held(&gpio_devices_srcu)) { for_each_gpio_desc(gdev->chip, desc) { - if (desc->name && !strcmp(desc->name, name)) { - spin_unlock_irqrestore(&gpio_lock, flags); + if (desc->name && !strcmp(desc->name, name)) return desc; - } } } - spin_unlock_irqrestore(&gpio_lock, flags); - return NULL; } @@ -813,7 +817,6 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, struct lock_class_key *request_key) { struct gpio_device *gdev; - unsigned long flags; unsigned int i; int base = 0; int ret = 0; @@ -878,49 +881,47 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, gdev->ngpio = gc->ngpio; - spin_lock_irqsave(&gpio_lock, flags); - - /* - * TODO: this allocates a Linux GPIO number base in the global - * GPIO numberspace for this chip. In the long run we want to - * get *rid* of this numberspace and use only descriptors, but - * it may be a pipe dream. It will not happen before we get rid - * of the sysfs interface anyways. - */ - base = gc->base; - if (base < 0) { - base = gpiochip_find_base_unlocked(gc->ngpio); + scoped_guard(mutex, &gpio_devices_lock) { + /* + * TODO: this allocates a Linux GPIO number base in the global + * GPIO numberspace for this chip. In the long run we want to + * get *rid* of this numberspace and use only descriptors, but + * it may be a pipe dream. It will not happen before we get rid + * of the sysfs interface anyways. + */ + base = gc->base; if (base < 0) { - spin_unlock_irqrestore(&gpio_lock, flags); - ret = base; - base = 0; + base = gpiochip_find_base_unlocked(gc->ngpio); + if (base < 0) { + ret = base; + base = 0; + goto err_free_label; + } + + /* + * TODO: it should not be necessary to reflect the + * assigned base outside of the GPIO subsystem. Go over + * drivers and see if anyone makes use of this, else + * drop this and assign a poison instead. + */ + gc->base = base; + } else { + dev_warn(&gdev->dev, + "Static allocation of GPIO base is deprecated, use dynamic allocation.\n"); + } + + gdev->base = base; + + ret = gpiodev_add_to_list_unlocked(gdev); + if (ret) { + chip_err(gc, "GPIO integer space overlap, cannot add chip\n"); goto err_free_label; } - /* - * TODO: it should not be necessary to reflect the assigned - * base outside of the GPIO subsystem. Go over drivers and - * see if anyone makes use of this, else drop this and assign - * a poison instead. - */ - gc->base = base; - } else { - dev_warn(&gdev->dev, - "Static allocation of GPIO base is deprecated, use dynamic allocation.\n"); - } - gdev->base = base; - - ret = gpiodev_add_to_list_unlocked(gdev); - if (ret) { - spin_unlock_irqrestore(&gpio_lock, flags); - chip_err(gc, "GPIO integer space overlap, cannot add chip\n"); - goto err_free_label; } for (i = 0; i < gc->ngpio; i++) gdev->descs[i].gdev = gdev; - spin_unlock_irqrestore(&gpio_lock, flags); - BLOCKING_INIT_NOTIFIER_HEAD(&gdev->line_state_notifier); BLOCKING_INIT_NOTIFIER_HEAD(&gdev->device_notifier); init_rwsem(&gdev->sem); @@ -1011,9 +1012,9 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, goto err_print_message; } err_remove_from_list: - spin_lock_irqsave(&gpio_lock, flags); - list_del(&gdev->list); - spin_unlock_irqrestore(&gpio_lock, flags); + scoped_guard(mutex, &gpio_devices_lock) + list_del_rcu(&gdev->list); + synchronize_srcu(&gpio_devices_srcu); err_free_label: kfree_const(gdev->label); err_free_descs: @@ -1076,8 +1077,9 @@ void gpiochip_remove(struct gpio_chip *gc) dev_crit(&gdev->dev, "REMOVING GPIOCHIP WITH GPIOS STILL REQUESTED\n"); - scoped_guard(spinlock_irqsave, &gpio_lock) - list_del(&gdev->list); + scoped_guard(mutex, &gpio_devices_lock) + list_del_rcu(&gdev->list); + synchronize_srcu(&gpio_devices_srcu); /* * The gpiochip side puts its use of the device to rest here: @@ -1125,7 +1127,7 @@ struct gpio_device *gpio_device_find(void *data, */ might_sleep(); - guard(spinlock_irqsave)(&gpio_lock); + guard(srcu)(&gpio_devices_srcu); list_for_each_entry(gdev, &gpio_devices, list) { if (gdev->chip && match(gdev->chip, data)) @@ -4136,27 +4138,32 @@ static struct gpio_desc *gpiod_find_and_request(struct device *consumer, struct gpio_desc *desc; int ret; - desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx, &flags, &lookupflags); - if (gpiod_not_found(desc) && platform_lookup_allowed) { + scoped_guard(srcu, &gpio_devices_srcu) { + desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx, + &flags, &lookupflags); + if (gpiod_not_found(desc) && platform_lookup_allowed) { + /* + * Either we are not using DT or ACPI, or their lookup + * did not return a result. In that case, use platform + * lookup as a fallback. + */ + dev_dbg(consumer, + "using lookup tables for GPIO lookup\n"); + desc = gpiod_find(consumer, con_id, idx, &lookupflags); + } + + if (IS_ERR(desc)) { + dev_dbg(consumer, "No GPIO consumer %s found\n", + con_id); + return desc; + } + /* - * Either we are not using DT or ACPI, or their lookup did not - * return a result. In that case, use platform lookup as a - * fallback. + * If a connection label was passed use that, else attempt to use + * the device name as label */ - dev_dbg(consumer, "using lookup tables for GPIO lookup\n"); - desc = gpiod_find(consumer, con_id, idx, &lookupflags); + ret = gpiod_request(desc, label); } - - if (IS_ERR(desc)) { - dev_dbg(consumer, "No GPIO consumer %s found\n", con_id); - return desc; - } - - /* - * If a connection label was passed use that, else attempt to use - * the device name as label - */ - ret = gpiod_request(desc, label); if (ret) { if (!(ret == -EBUSY && flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE)) return ERR_PTR(ret); @@ -4727,35 +4734,33 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_device *gdev) static void *gpiolib_seq_start(struct seq_file *s, loff_t *pos) { - unsigned long flags; struct gpio_device *gdev = NULL; loff_t index = *pos; s->private = ""; - spin_lock_irqsave(&gpio_lock, flags); - list_for_each_entry(gdev, &gpio_devices, list) - if (index-- == 0) { - spin_unlock_irqrestore(&gpio_lock, flags); + guard(srcu)(&gpio_devices_srcu); + + list_for_each_entry(gdev, &gpio_devices, list) { + if (index-- == 0) return gdev; - } - spin_unlock_irqrestore(&gpio_lock, flags); + } return NULL; } static void *gpiolib_seq_next(struct seq_file *s, void *v, loff_t *pos) { - unsigned long flags; struct gpio_device *gdev = v; void *ret = NULL; - spin_lock_irqsave(&gpio_lock, flags); - if (list_is_last(&gdev->list, &gpio_devices)) - ret = NULL; - else - ret = list_first_entry(&gdev->list, struct gpio_device, list); - spin_unlock_irqrestore(&gpio_lock, flags); + scoped_guard(srcu, &gpio_devices_srcu) { + if (list_is_last(&gdev->list, &gpio_devices)) + ret = NULL; + else + ret = list_first_entry(&gdev->list, struct gpio_device, + list); + } s->private = "\n"; ++*pos;