[PATCH v3] gpiolib: match secondary fwnode too in gpio_device_find_by_fwnode()

[PATCH v3] gpiolib: match secondary fwnode too in gpio_device_find_by_fwnode()

[Bartosz Golaszewski]

In GPIOLIB, during fwnode lookup, after having resolved the consumer's
reference to a specific fwnode, we only match it against the primary
node of the controllers. Let's extend that to also the secondary node by
reworking gpio_chip_match_by_fwnode()

Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Reviewed-by: Danilo Krummrich <dakr@kernel.org>
Reviewed-by: Rafael J. Wysocki (Intel) <rafael@kernel.org>
Reviewed-by: Linus Walleij <linusw@kernel.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
Link: https://lore.kernel.org/all/aZUIFiOYt6GOlDQx@google.com/
---
Changes in v3:
- Remove the controversial patch 1/2 in favor of hand-coding its
  functionality in patch 2/2
- Link to v2: https://patch.msgid.link/20260223-device-match-secondary-fwnode-v2-0-966c00c9eeeb@oss.qualcomm.com

Changes in v2:
- Fold the code initially put into driver code into GPIOLIB as advised
  by Rafael
- Rework the logic as suggested by Andy
- To that end: make fwnode_is_primary() public
- Link to v1: https://patch.msgid.link/20260219-device-match-secondary-fwnode-v1-0-a64e8d4754bc@oss.qualcomm.com
---
 drivers/gpio/gpiolib.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index c52200eaaaff82b12f22dd1ee8459bdd8ec10d81..0182603de368f2125baf174fcf5f1e893917c154 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -11,6 +11,7 @@
 #include <linux/errno.h>
 #include <linux/file.h>
 #include <linux/fs.h>
+#include <linux/fwnode.h>
 #include <linux/idr.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
@@ -1395,7 +1396,16 @@ EXPORT_SYMBOL_GPL(gpio_device_find_by_label);
 
 static int gpio_chip_match_by_fwnode(struct gpio_chip *gc, const void *fwnode)
 {
-	return device_match_fwnode(&gc->gpiodev->dev, fwnode);
+	struct device *dev = &gc->gpiodev->dev;
+	struct fwnode_handle *node = dev_fwnode(dev);
+
+	if (IS_ERR(fwnode))
+		return 0;
+
+	if (device_match_fwnode(dev, fwnode))
+		return 1;
+
+	return node && !IS_ERR(node->secondary) && node->secondary == fwnode;
 }
 
 /**

---
base-commit: 50f68cc7be0a2cbf54d8f6aaf17df32fb01acc3f
change-id: 20260219-device-match-secondary-fwnode-4cc163ec47ee

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

Re: [PATCH v3] gpiolib: match secondary fwnode too in gpio_device_find_by_fwnode()

[Andy Shevchenko]

On Wed, Feb 25, 2026 at 11:12:25AM +0100, Bartosz Golaszewski wrote:
> In GPIOLIB, during fwnode lookup, after having resolved the consumer's
> reference to a specific fwnode, we only match it against the primary
> node of the controllers. Let's extend that to also the secondary node by
> reworking gpio_chip_match_by_fwnode()

...

> +	if (IS_ERR(fwnode))
> +		return 0;
> +
> +	if (device_match_fwnode(dev, fwnode))
> +		return 1;
> +
> +	return node && !IS_ERR(node->secondary) && node->secondary == fwnode;

I believe Rafael is right in suggesting just

	return node && node->secondary == fwnode;

At this point fwnode either NULL or valid. 'node' comes from device,
so it may be all three. Assuming it's actually can't be error pointer
the above check is sufficient. But maybe you wanted full check, then

	return !IS_ERR_OR_NULL(node) && node->secondary == fwnode;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] gpiolib: match secondary fwnode too in gpio_device_find_by_fwnode()

[Andy Shevchenko]

On Wed, Feb 25, 2026 at 12:52:35PM +0200, Andy Shevchenko wrote:
> On Wed, Feb 25, 2026 at 11:12:25AM +0100, Bartosz Golaszewski wrote:

...

> > +	if (IS_ERR(fwnode))
> > +		return 0;
> > +
> > +	if (device_match_fwnode(dev, fwnode))
> > +		return 1;
> > +
> > +	return node && !IS_ERR(node->secondary) && node->secondary == fwnode;
> 
> I believe Rafael is right in suggesting just
> 
> 	return node && node->secondary == fwnode;
> 
> At this point fwnode either NULL or valid. 'node' comes from device,
> so it may be all three. Assuming it's actually can't be error pointer
> the above check is sufficient. But maybe you wanted full check, then
> 
> 	return !IS_ERR_OR_NULL(node) && node->secondary == fwnode;

And probably NULL should be excluded from fwnode:

	return fwnode && !IS_ERR_OR_NULL(node) && node->secondary == fwnode;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] gpiolib: match secondary fwnode too in gpio_device_find_by_fwnode()

[Rafael J. Wysocki]

On Wed, Feb 25, 2026 at 11:12 AM Bartosz Golaszewski
<bartosz.golaszewski@oss.qualcomm.com> wrote:
>
> In GPIOLIB, during fwnode lookup, after having resolved the consumer's
> reference to a specific fwnode, we only match it against the primary
> node of the controllers. Let's extend that to also the secondary node by
> reworking gpio_chip_match_by_fwnode()
>
> Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Reviewed-by: Danilo Krummrich <dakr@kernel.org>
> Reviewed-by: Rafael J. Wysocki (Intel) <rafael@kernel.org>
> Reviewed-by: Linus Walleij <linusw@kernel.org>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> ---
> Link: https://lore.kernel.org/all/aZUIFiOYt6GOlDQx@google.com/
> ---
> Changes in v3:
> - Remove the controversial patch 1/2 in favor of hand-coding its
>   functionality in patch 2/2
> - Link to v2: https://patch.msgid.link/20260223-device-match-secondary-fwnode-v2-0-966c00c9eeeb@oss.qualcomm.com
>
> Changes in v2:
> - Fold the code initially put into driver code into GPIOLIB as advised
>   by Rafael
> - Rework the logic as suggested by Andy
> - To that end: make fwnode_is_primary() public
> - Link to v1: https://patch.msgid.link/20260219-device-match-secondary-fwnode-v1-0-a64e8d4754bc@oss.qualcomm.com
> ---
>  drivers/gpio/gpiolib.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index c52200eaaaff82b12f22dd1ee8459bdd8ec10d81..0182603de368f2125baf174fcf5f1e893917c154 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -11,6 +11,7 @@
>  #include <linux/errno.h>
>  #include <linux/file.h>
>  #include <linux/fs.h>
> +#include <linux/fwnode.h>
>  #include <linux/idr.h>
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
> @@ -1395,7 +1396,16 @@ EXPORT_SYMBOL_GPL(gpio_device_find_by_label);
>
>  static int gpio_chip_match_by_fwnode(struct gpio_chip *gc, const void *fwnode)
>  {
> -       return device_match_fwnode(&gc->gpiodev->dev, fwnode);
> +       struct device *dev = &gc->gpiodev->dev;
> +       struct fwnode_handle *node = dev_fwnode(dev);
> +
> +       if (IS_ERR(fwnode))
> +               return 0;
> +
> +       if (device_match_fwnode(dev, fwnode))
> +               return 1;
> +
> +       return node && !IS_ERR(node->secondary) && node->secondary == fwnode;

The second check is redundant because fwnode cannot be an error
pointer (it has been checked against that already above) and so if
node->secondary == fwnode, then node->secondary is not an error
pointer.

I'm not sure if fwnode can be NULL here, but if it can, it should be
checked against NULL.  Alternatively, node->secondary can be checked
against NULL and compared to fwnode.

So, if fwnode != NULL cannot be guaranteed,

        return fwnode && node && node->secondary == fwnode;

or

        return node && node->secondary && node->secondary == fwnode;

The overhead of the former may be a bit lower because it avoids
dereferencing node when fwnode is NULL, but the compiler should be
able to optimize this anyway.

>  }
>
>  /**
>
> ---

Re: [PATCH v3] gpiolib: match secondary fwnode too in gpio_device_find_by_fwnode()

[Rafael J. Wysocki]

On Wed, Feb 25, 2026 at 1:36 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Feb 25, 2026 at 11:12 AM Bartosz Golaszewski
> <bartosz.golaszewski@oss.qualcomm.com> wrote:
> >
> > In GPIOLIB, during fwnode lookup, after having resolved the consumer's
> > reference to a specific fwnode, we only match it against the primary
> > node of the controllers. Let's extend that to also the secondary node by
> > reworking gpio_chip_match_by_fwnode()
> >
> > Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Reviewed-by: Danilo Krummrich <dakr@kernel.org>
> > Reviewed-by: Rafael J. Wysocki (Intel) <rafael@kernel.org>
> > Reviewed-by: Linus Walleij <linusw@kernel.org>
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> > ---
> > Link: https://lore.kernel.org/all/aZUIFiOYt6GOlDQx@google.com/
> > ---
> > Changes in v3:
> > - Remove the controversial patch 1/2 in favor of hand-coding its
> >   functionality in patch 2/2
> > - Link to v2: https://patch.msgid.link/20260223-device-match-secondary-fwnode-v2-0-966c00c9eeeb@oss.qualcomm.com
> >
> > Changes in v2:
> > - Fold the code initially put into driver code into GPIOLIB as advised
> >   by Rafael
> > - Rework the logic as suggested by Andy
> > - To that end: make fwnode_is_primary() public
> > - Link to v1: https://patch.msgid.link/20260219-device-match-secondary-fwnode-v1-0-a64e8d4754bc@oss.qualcomm.com
> > ---
> >  drivers/gpio/gpiolib.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index c52200eaaaff82b12f22dd1ee8459bdd8ec10d81..0182603de368f2125baf174fcf5f1e893917c154 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/errno.h>
> >  #include <linux/file.h>
> >  #include <linux/fs.h>
> > +#include <linux/fwnode.h>
> >  #include <linux/idr.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/irq.h>
> > @@ -1395,7 +1396,16 @@ EXPORT_SYMBOL_GPL(gpio_device_find_by_label);
> >
> >  static int gpio_chip_match_by_fwnode(struct gpio_chip *gc, const void *fwnode)
> >  {
> > -       return device_match_fwnode(&gc->gpiodev->dev, fwnode);
> > +       struct device *dev = &gc->gpiodev->dev;
> > +       struct fwnode_handle *node = dev_fwnode(dev);
> > +
> > +       if (IS_ERR(fwnode))
> > +               return 0;
> > +
> > +       if (device_match_fwnode(dev, fwnode))
> > +               return 1;
> > +
> > +       return node && !IS_ERR(node->secondary) && node->secondary == fwnode;
>
> The second check is redundant because fwnode cannot be an error
> pointer (it has been checked against that already above) and so if
> node->secondary == fwnode, then node->secondary is not an error
> pointer.
>
> I'm not sure if fwnode can be NULL here, but if it can, it should be
> checked against NULL.  Alternatively, node->secondary can be checked
> against NULL and compared to fwnode.
>
> So, if fwnode != NULL cannot be guaranteed,
>
>         return fwnode && node && node->secondary == fwnode;
>
> or
>
>         return node && node->secondary && node->secondary == fwnode;
>
> The overhead of the former may be a bit lower because it avoids
> dereferencing node when fwnode is NULL, but the compiler should be
> able to optimize this anyway.

Or even the device_match_fwnode() check can be folded into the last line:

static int gpio_chip_match_by_fwnode(struct gpio_chip *gc, const void *fwnode)
 {
-       return device_match_fwnode(&gc->gpiodev->dev, fwnode);
+       struct device *dev = &gc->gpiodev->dev;
+       struct fwnode_handle *node = dev_fwnode(dev);
+
+       if (IS_ERR_OR_NULL(fwnode))
+               return 0;
+
+       return node == fwnode || (node && node->secondary == fwnode);
 }

Re: [PATCH v3] gpiolib: match secondary fwnode too in gpio_device_find_by_fwnode()

[Bartosz Golaszewski]

On Wed, Feb 25, 2026 at 1:44 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > The second check is redundant because fwnode cannot be an error
> > pointer (it has been checked against that already above) and so if
> > node->secondary == fwnode, then node->secondary is not an error
> > pointer.
> >
> > I'm not sure if fwnode can be NULL here, but if it can, it should be
> > checked against NULL.  Alternatively, node->secondary can be checked
> > against NULL and compared to fwnode.
> >
> > So, if fwnode != NULL cannot be guaranteed,
> >
> >         return fwnode && node && node->secondary == fwnode;
> >
> > or
> >
> >         return node && node->secondary && node->secondary == fwnode;
> >
> > The overhead of the former may be a bit lower because it avoids
> > dereferencing node when fwnode is NULL, but the compiler should be
> > able to optimize this anyway.
>
> Or even the device_match_fwnode() check can be folded into the last line:
>
> static int gpio_chip_match_by_fwnode(struct gpio_chip *gc, const void *fwnode)
>  {
> -       return device_match_fwnode(&gc->gpiodev->dev, fwnode);
> +       struct device *dev = &gc->gpiodev->dev;
> +       struct fwnode_handle *node = dev_fwnode(dev);
> +
> +       if (IS_ERR_OR_NULL(fwnode))
> +               return 0;
> +
> +       return node == fwnode || (node && node->secondary == fwnode);
>  }

device_match_fwnode() already contains the NULL check for fwnode. I'm
sending a v4 with the IS_ERR() check for secondary dropped I hope this
is the final one.

Bart

Re: [PATCH v3] gpiolib: match secondary fwnode too in gpio_device_find_by_fwnode()

[Rafael J. Wysocki]

On Thu, Feb 26, 2026 at 10:55 AM Bartosz Golaszewski <brgl@kernel.org> wrote:
>
> On Wed, Feb 25, 2026 at 1:44 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > The second check is redundant because fwnode cannot be an error
> > > pointer (it has been checked against that already above) and so if
> > > node->secondary == fwnode, then node->secondary is not an error
> > > pointer.
> > >
> > > I'm not sure if fwnode can be NULL here, but if it can, it should be
> > > checked against NULL.  Alternatively, node->secondary can be checked
> > > against NULL and compared to fwnode.
> > >
> > > So, if fwnode != NULL cannot be guaranteed,
> > >
> > >         return fwnode && node && node->secondary == fwnode;
> > >
> > > or
> > >
> > >         return node && node->secondary && node->secondary == fwnode;
> > >
> > > The overhead of the former may be a bit lower because it avoids
> > > dereferencing node when fwnode is NULL, but the compiler should be
> > > able to optimize this anyway.
> >
> > Or even the device_match_fwnode() check can be folded into the last line:
> >
> > static int gpio_chip_match_by_fwnode(struct gpio_chip *gc, const void *fwnode)
> >  {
> > -       return device_match_fwnode(&gc->gpiodev->dev, fwnode);
> > +       struct device *dev = &gc->gpiodev->dev;
> > +       struct fwnode_handle *node = dev_fwnode(dev);
> > +
> > +       if (IS_ERR_OR_NULL(fwnode))
> > +               return 0;
> > +
> > +       return node == fwnode || (node && node->secondary == fwnode);
> >  }
>
> device_match_fwnode() already contains the NULL check for fwnode.

Yes, it does, but if device_match_fwnode() returns false, you don't
know the exact reason: fwnode may be NULL or it may be non-NULL, but
different from the device's one.  You can't generally assume that
fwnode is not NULL in that case.

> I'm sending a v4 with the IS_ERR() check for secondary dropped I hope this
> is the final one.

This one is fine with me so long as NULL is never passed as fwnode to
this function.