linux-gpio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] gpio: shared: handle the reset-gpios corner case
@ 2025-11-21 13:46 Bartosz Golaszewski
  2025-11-23  1:03 ` Val Packett
  0 siblings, 1 reply; 6+ messages in thread
From: Bartosz Golaszewski @ 2025-11-21 13:46 UTC (permalink / raw)
  To: Val Packett, Krzysztof Kozlowski, Philipp Zabel, Linus Walleij,
	Bartosz Golaszewski
  Cc: linux-gpio, linux-kernel, linux-arm-msm, Bartosz Golaszewski

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

There's an unexpected interaction between the reset-gpio driver and the
shared GPIO support. The reset-gpio device is an auxiliary device that's
created dynamically and fulfills a similar role to the gpio-shared-proxy
driver but is limited in scope to just supporting the "reset-gpios"
property.

The shared GPIO core code does not take into account that the machine
lookup entry we create when scanning the device-tree must connect the
reset-gpio device - that is the actual consumer of the GPIO and not the
consumer defined on the device tree, which in turn consumes the shared
reset control exposed by the reset-gpio device - to the GPIO controller.

We also must not skip the gpio-shared-proxy driver as it's possible that
a shared GPIO may be used by one consumer as a reset-gpios going through
the reset-gpio device and another that uses GPIOLIB.

We need to make it a special case handled in gpiolib-shared.c. Add a new
function - gpio_shared_dev_is_reset_gpio() - whose role it is to verify
if a non-matching consumer of a shared pin is a reset-gpio device and
make sure it's the right one for this pin. To that end make sure that
its parent is the GPIO controller in question and that the fwnode we
identified as sharing the pin references that controller via the
"reset-gpios" property.

Only include that code if the reset-gpio driver is enabled.

Fixes: a060b8c511ab ("gpiolib: implement low-level, shared GPIO support")
Reported-by: Val Packett <val@packett.cool>
Closes: https://lore.kernel.org/all/3b5d9df5-934d-4591-8827-6c9573a6f7ba@packett.cool/
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
This is targetting linux-next where the reset-gpio driver is now using
the auxiliary bus and software nodes rather than the platform bus and
GPIO machine lookup. The bug is the same in both cases but the fix would
be completely different.
---
 drivers/gpio/gpiolib-shared.c | 63 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 62 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib-shared.c b/drivers/gpio/gpiolib-shared.c
index 3803b5c938f9933dab01c6d777c349ed3b42ce9b..5d06ec66f55eb1478950caa01cd4b680ce606c52 100644
--- a/drivers/gpio/gpiolib-shared.c
+++ b/drivers/gpio/gpiolib-shared.c
@@ -253,6 +253,66 @@ static int gpio_shared_make_adev(struct gpio_device *gdev,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_RESET_GPIO)
+/*
+ * Special case: reset-gpio is an auxiliary device that's created dynamically
+ * and put in between the GPIO controller and consumers of shared GPIOs
+ * referred to by the "reset-gpios" property.
+ *
+ * If the supposed consumer of a shared GPIO didn't match any of the mappings
+ * we created when scanning the firmware nodes, it's still possible that it's
+ * the reset-gpio device which didn't exist at the time of the scan.
+ *
+ * This function verifies it an return true if it's the case.
+ */
+static bool gpio_shared_dev_is_reset_gpio(struct device *consumer,
+					  struct gpio_shared_entry *entry,
+					  struct gpio_shared_ref *ref)
+{
+	struct device *parent = consumer->parent;
+	struct fwnode_handle *remote;
+	bool ret;
+
+	if (!parent)
+		return false;
+
+	/*
+	 * FIXME: use device_is_compatible() once the reset-gpio drivers gains
+	 * a compatible string which it currently does not have.
+	 */
+	if (!strstarts(dev_name(consumer), "reset.gpio."))
+		return false;
+
+	/*
+	 * Parent of the reset-gpio auxiliary device is the GPIO chip whose
+	 * fwnode we stored in the entry structure.
+	 */
+	if (!device_match_fwnode(parent, entry->fwnode))
+		return false;
+
+	/*
+	 * The device associated with the shared reference's firmware node is
+	 * the consumer of the reset control exposed by the reset-gpio device.
+	 * It must have a "reset-gpios" property that's referencing the entry's
+	 * firmware node.
+	 */
+	remote = fwnode_find_reference(ref->fwnode, "reset-gpios", 0);
+	if (IS_ERR(remote))
+		return false;
+
+	ret = (remote == entry->fwnode);
+	fwnode_handle_put(remote);
+	return ret;
+}
+#else
+static bool gpio_shared_dev_is_reset_gpio(struct device *consumer,
+					  struct gpio_shared_entry *entry,
+					  struct gpio_shared_ref *ref)
+{
+	return false;
+}
+#endif /* CONFIG_RESET_GPIO */
+
 int gpio_shared_add_proxy_lookup(struct device *consumer, unsigned long lflags)
 {
 	const char *dev_id = dev_name(consumer);
@@ -268,7 +328,8 @@ int gpio_shared_add_proxy_lookup(struct device *consumer, unsigned long lflags)
 
 	list_for_each_entry(entry, &gpio_shared_list, list) {
 		list_for_each_entry(ref, &entry->refs, list) {
-			if (!device_match_fwnode(consumer, ref->fwnode))
+			if (!device_match_fwnode(consumer, ref->fwnode) &&
+			    !gpio_shared_dev_is_reset_gpio(consumer, entry, ref))
 				continue;
 
 			/* We've already done that on a previous request. */

---
base-commit: 6d12dd55830ab67dfd8569ff86322f949a1ac916
change-id: 20251121-gpiolib-shared-reset-gpio-fix-c8e161b9f6cb

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] gpio: shared: handle the reset-gpios corner case
  2025-11-21 13:46 [PATCH] gpio: shared: handle the reset-gpios corner case Bartosz Golaszewski
@ 2025-11-23  1:03 ` Val Packett
  2025-11-24  8:38   ` Bartosz Golaszewski
  0 siblings, 1 reply; 6+ messages in thread
From: Val Packett @ 2025-11-23  1:03 UTC (permalink / raw)
  To: Bartosz Golaszewski, Krzysztof Kozlowski, Philipp Zabel,
	Linus Walleij
  Cc: linux-gpio, linux-kernel, linux-arm-msm, Bartosz Golaszewski


On 11/21/25 10:46 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> There's an unexpected interaction between the reset-gpio driver and the
> shared GPIO support. The reset-gpio device is an auxiliary device that's
> created dynamically and fulfills a similar role to the gpio-shared-proxy
> driver but is limited in scope to just supporting the "reset-gpios"
> property.
>
> The shared GPIO core code does not take into account that the machine
> lookup entry we create when scanning the device-tree must connect the
> reset-gpio device - that is the actual consumer of the GPIO and not the
> consumer defined on the device tree, which in turn consumes the shared
> reset control exposed by the reset-gpio device - to the GPIO controller.
>
> We also must not skip the gpio-shared-proxy driver as it's possible that
> a shared GPIO may be used by one consumer as a reset-gpios going through
> the reset-gpio device and another that uses GPIOLIB.
>
> We need to make it a special case handled in gpiolib-shared.c. Add a new
> function - gpio_shared_dev_is_reset_gpio() - whose role it is to verify
> if a non-matching consumer of a shared pin is a reset-gpio device and
> make sure it's the right one for this pin. To that end make sure that
> its parent is the GPIO controller in question and that the fwnode we
> identified as sharing the pin references that controller via the
> "reset-gpios" property.
>
> Only include that code if the reset-gpio driver is enabled.
>
> Fixes: a060b8c511ab ("gpiolib: implement low-level, shared GPIO support")
> Reported-by: Val Packett <val@packett.cool>
> Closes: https://lore.kernel.org/all/3b5d9df5-934d-4591-8827-6c9573a6f7ba@packett.cool/
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
> This is targetting linux-next where the reset-gpio driver is now using
> the auxiliary bus and software nodes rather than the platform bus and
> GPIO machine lookup. The bug is the same in both cases but the fix would
> be completely different.
> ---
> [..]

Tried applying only this, as well as this + 
https://lore.kernel.org/all/20251120-reset-gpios-swnodes-v7-0-a100493a0f4b@linaro.org/ 
+ https://lore.kernel.org/all/20251121135739.66528-1-brgl@bgdev.pl/ (on 
top of next-20251120) and the issue is still present.. am I missing 
something?

~val


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] gpio: shared: handle the reset-gpios corner case
  2025-11-23  1:03 ` Val Packett
@ 2025-11-24  8:38   ` Bartosz Golaszewski
  2025-11-24 15:36     ` Bartosz Golaszewski
  0 siblings, 1 reply; 6+ messages in thread
From: Bartosz Golaszewski @ 2025-11-24  8:38 UTC (permalink / raw)
  To: Val Packett
  Cc: Krzysztof Kozlowski, Philipp Zabel, Linus Walleij, linux-gpio,
	linux-kernel, linux-arm-msm, Bartosz Golaszewski

On Sun, Nov 23, 2025 at 2:03 AM Val Packett <val@packett.cool> wrote:
>
> > ---
> > This is targetting linux-next where the reset-gpio driver is now using
> > the auxiliary bus and software nodes rather than the platform bus and
> > GPIO machine lookup. The bug is the same in both cases but the fix would
> > be completely different.
> > ---
> > [..]
>
> Tried applying only this, as well as this +
> https://lore.kernel.org/all/20251120-reset-gpios-swnodes-v7-0-a100493a0f4b@linaro.org/
> + https://lore.kernel.org/all/20251121135739.66528-1-brgl@bgdev.pl/ (on
> top of next-20251120) and the issue is still present.. am I missing
> something?
>

Can you try this branch?

  https://github.com/brgl/linux test/gpiolib-shared-reset-gpio-fix

I confirmed it works on my setup and fixes the problem with multiple
users of reset-gpio AND shared GPIOs enabled.

Bart

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] gpio: shared: handle the reset-gpios corner case
  2025-11-24  8:38   ` Bartosz Golaszewski
@ 2025-11-24 15:36     ` Bartosz Golaszewski
  2025-11-24 23:05       ` Val Packett
  0 siblings, 1 reply; 6+ messages in thread
From: Bartosz Golaszewski @ 2025-11-24 15:36 UTC (permalink / raw)
  To: Val Packett
  Cc: Krzysztof Kozlowski, Philipp Zabel, Linus Walleij, linux-gpio,
	linux-kernel, linux-arm-msm, Bartosz Golaszewski

On Mon, Nov 24, 2025 at 9:38 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Sun, Nov 23, 2025 at 2:03 AM Val Packett <val@packett.cool> wrote:
> >
> > > ---
> > > This is targetting linux-next where the reset-gpio driver is now using
> > > the auxiliary bus and software nodes rather than the platform bus and
> > > GPIO machine lookup. The bug is the same in both cases but the fix would
> > > be completely different.
> > > ---
> > > [..]
> >
> > Tried applying only this, as well as this +
> > https://lore.kernel.org/all/20251120-reset-gpios-swnodes-v7-0-a100493a0f4b@linaro.org/
> > + https://lore.kernel.org/all/20251121135739.66528-1-brgl@bgdev.pl/ (on
> > top of next-20251120) and the issue is still present.. am I missing
> > something?
> >
>
> Can you try this branch?
>
>   https://github.com/brgl/linux test/gpiolib-shared-reset-gpio-fix
>
> I confirmed it works on my setup and fixes the problem with multiple
> users of reset-gpio AND shared GPIOs enabled.
>

Actually linux-next got updated with all the prerequisites so you can
try this patch on top of next-20251124. I tested it and it works for
me. If it still doesn't for you, can you enable GPIO debug messages
and send me the entire kernel log?

Bartosz

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] gpio: shared: handle the reset-gpios corner case
  2025-11-24 15:36     ` Bartosz Golaszewski
@ 2025-11-24 23:05       ` Val Packett
  2025-11-25 10:20         ` Bartosz Golaszewski
  0 siblings, 1 reply; 6+ messages in thread
From: Val Packett @ 2025-11-24 23:05 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Krzysztof Kozlowski, Philipp Zabel, Linus Walleij, linux-gpio,
	linux-kernel, linux-arm-msm, Bartosz Golaszewski


On 11/24/25 12:36 PM, Bartosz Golaszewski wrote:
> On Mon, Nov 24, 2025 at 9:38 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>> On Sun, Nov 23, 2025 at 2:03 AM Val Packett <val@packett.cool> wrote:
>>>> ---
>>>> This is targetting linux-next where the reset-gpio driver is now using
>>>> the auxiliary bus and software nodes rather than the platform bus and
>>>> GPIO machine lookup. The bug is the same in both cases but the fix would
>>>> be completely different.
>>>> ---
>>>> [..]
>>> Tried applying only this, as well as this +
>>> https://lore.kernel.org/all/20251120-reset-gpios-swnodes-v7-0-a100493a0f4b@linaro.org/
>>> + https://lore.kernel.org/all/20251121135739.66528-1-brgl@bgdev.pl/ (on
>>> top of next-20251120) and the issue is still present.. am I missing
>>> something?
>> Can you try this branch?
>>
>>    https://github.com/brgl/linux test/gpiolib-shared-reset-gpio-fix
>>
>> I confirmed it works on my setup and fixes the problem with multiple
>> users of reset-gpio AND shared GPIOs enabled.
> Actually linux-next got updated with all the prerequisites so you can
> try this patch on top of next-20251124. I tested it and it works for
> me. If it still doesn't for you, can you enable GPIO debug messages
> and send me the entire kernel log?
>
> Bartosz

Rebased to next-20251124, still the same..

Here's a full dmesg: https://owo.packett.cool/lin/sound.gpio.dmesg

I even added a custom print to confirm the reason of the EBUSY:

[    9.233613] gpiolib: swnode: swnode_find_gpio: parsed 'reset-gpios' 
property of node 'node4[0]' - status (0)
[    9.233624] gpiolib_shared: Adding machine lookup entry for a shared 
GPIO for consumer reset.gpio.0, with key 'gpiolib_shared.proxy.8' and 
con_id 'reset'
[    9.233630] reset_gpio reset.gpio.0: using lookup tables for GPIO lookup
[    9.233640] gpio_shared_proxy gpiolib_shared.proxy.8: Shared GPIO 
requested, number of users: 1
[    9.233652] gpio_shared_proxy gpiolib_shared.proxy.8: Only one user 
of this shared GPIO, allowing to set direction to output with value 'low'
[    9.332317] reset_gpio reset.gpio.1: using swnode 'node5' for 'reset' 
GPIO lookup
[    9.332337] gpiolib: swnode: swnode_find_gpio: parsed 'reset-gpios' 
property of node 'node5[0]' - status (0)
[    9.332343] gpiolib_shared: Adding machine lookup entry for a shared 
GPIO for consumer reset.gpio.1, with key 'gpiolib_shared.proxy.8' and 
con_id 'reset'
[    9.332347] reset_gpio reset.gpio.1: using lookup tables for GPIO lookup
[    9.332353] gpio-856 (reset): gpiod_request_commit: flags 200043 
test_and_set_bit GPIOD_FLAG_REQUESTED -> EBUSY
[    9.332356] gpio-856 (reset): gpiod_request: status -16
[    9.332358] reset_gpio reset.gpio.1: error -EBUSY: Could not get 
reset gpios
[    9.332362] reset_gpio reset.gpio.1: probe with driver reset_gpio 
failed with error -16
[    9.441612] wcd938x_codec audio-codec: bound sdw:2:0:0217:010d:00:4 
(ops wcd_sdw_component_ops [snd_soc_wcd_common])
[    9.441644] wcd938x_codec audio-codec: bound sdw:3:0:0217:010d:00:3 
(ops wcd_sdw_component_ops [snd_soc_wcd_common])
[    9.445771] gpio_shared_proxy gpiolib_shared.proxy.8: Voted for value 
'high', effective value is 'high', number of votes for 'high': 1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] gpio: shared: handle the reset-gpios corner case
  2025-11-24 23:05       ` Val Packett
@ 2025-11-25 10:20         ` Bartosz Golaszewski
  0 siblings, 0 replies; 6+ messages in thread
From: Bartosz Golaszewski @ 2025-11-25 10:20 UTC (permalink / raw)
  To: Val Packett
  Cc: Krzysztof Kozlowski, Philipp Zabel, Linus Walleij, linux-gpio,
	linux-kernel, linux-arm-msm, Bartosz Golaszewski

On Tue, Nov 25, 2025 at 12:05 AM Val Packett <val@packett.cool> wrote:
>
>
> On 11/24/25 12:36 PM, Bartosz Golaszewski wrote:
> > On Mon, Nov 24, 2025 at 9:38 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >> On Sun, Nov 23, 2025 at 2:03 AM Val Packett <val@packett.cool> wrote:
> >>>> ---
> >>>> This is targetting linux-next where the reset-gpio driver is now using
> >>>> the auxiliary bus and software nodes rather than the platform bus and
> >>>> GPIO machine lookup. The bug is the same in both cases but the fix would
> >>>> be completely different.
> >>>> ---
> >>>> [..]
> >>> Tried applying only this, as well as this +
> >>> https://lore.kernel.org/all/20251120-reset-gpios-swnodes-v7-0-a100493a0f4b@linaro.org/
> >>> + https://lore.kernel.org/all/20251121135739.66528-1-brgl@bgdev.pl/ (on
> >>> top of next-20251120) and the issue is still present.. am I missing
> >>> something?
> >> Can you try this branch?
> >>
> >>    https://github.com/brgl/linux test/gpiolib-shared-reset-gpio-fix
> >>
> >> I confirmed it works on my setup and fixes the problem with multiple
> >> users of reset-gpio AND shared GPIOs enabled.
> > Actually linux-next got updated with all the prerequisites so you can
> > try this patch on top of next-20251124. I tested it and it works for
> > me. If it still doesn't for you, can you enable GPIO debug messages
> > and send me the entire kernel log?
> >
> > Bartosz
>
> Rebased to next-20251124, still the same..
>
> Here's a full dmesg: https://owo.packett.cool/lin/sound.gpio.dmesg
>
> I even added a custom print to confirm the reason of the EBUSY:
>
> [    9.233613] gpiolib: swnode: swnode_find_gpio: parsed 'reset-gpios'
> property of node 'node4[0]' - status (0)
> [    9.233624] gpiolib_shared: Adding machine lookup entry for a shared
> GPIO for consumer reset.gpio.0, with key 'gpiolib_shared.proxy.8' and
> con_id 'reset'
> [    9.233630] reset_gpio reset.gpio.0: using lookup tables for GPIO lookup
> [    9.233640] gpio_shared_proxy gpiolib_shared.proxy.8: Shared GPIO
> requested, number of users: 1
> [    9.233652] gpio_shared_proxy gpiolib_shared.proxy.8: Only one user
> of this shared GPIO, allowing to set direction to output with value 'low'
> [    9.332317] reset_gpio reset.gpio.1: using swnode 'node5' for 'reset'
> GPIO lookup
> [    9.332337] gpiolib: swnode: swnode_find_gpio: parsed 'reset-gpios'
> property of node 'node5[0]' - status (0)
> [    9.332343] gpiolib_shared: Adding machine lookup entry for a shared
> GPIO for consumer reset.gpio.1, with key 'gpiolib_shared.proxy.8' and
> con_id 'reset'
> [    9.332347] reset_gpio reset.gpio.1: using lookup tables for GPIO lookup
> [    9.332353] gpio-856 (reset): gpiod_request_commit: flags 200043
> test_and_set_bit GPIOD_FLAG_REQUESTED -> EBUSY
> [    9.332356] gpio-856 (reset): gpiod_request: status -16
> [    9.332358] reset_gpio reset.gpio.1: error -EBUSY: Could not get
> reset gpios
> [    9.332362] reset_gpio reset.gpio.1: probe with driver reset_gpio
> failed with error -16
> [    9.441612] wcd938x_codec audio-codec: bound sdw:2:0:0217:010d:00:4
> (ops wcd_sdw_component_ops [snd_soc_wcd_common])
> [    9.441644] wcd938x_codec audio-codec: bound sdw:3:0:0217:010d:00:3
> (ops wcd_sdw_component_ops [snd_soc_wcd_common])
> [    9.445771] gpio_shared_proxy gpiolib_shared.proxy.8: Voted for value
> 'high', effective value is 'high', number of votes for 'high': 1
>

Ok, so the checking in v1 was no strict enough to cover the use-case
of two reset-gpio devices consuming different pins from the same
controller. Please see v2 that I just sent. I reproduced the issue you
reported here and it fixed it.

Bart

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2025-11-25 10:20 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-21 13:46 [PATCH] gpio: shared: handle the reset-gpios corner case Bartosz Golaszewski
2025-11-23  1:03 ` Val Packett
2025-11-24  8:38   ` Bartosz Golaszewski
2025-11-24 15:36     ` Bartosz Golaszewski
2025-11-24 23:05       ` Val Packett
2025-11-25 10:20         ` Bartosz Golaszewski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).