Re: [PATCH 19/21] gpio: swnode: replace gpiochip_find() with gpio_device_find_by_label()

Le mercredi 24 janvier 2024 =C3=A0 16:04 +0100, Bartosz Golaszewski a
=C3=A9crit=C2=A0:
> On Wed, Jan 24, 2024 at 3:59=E2=80=AFPM Paul Cercueil <[email protected]=
et>
> wrote:
> >=20
> > Hi Bartosz,
> >=20
> > Le mardi 05 septembre 2023 =C3=A0 20:53 +0200, Bartosz Golaszewski a
> > =C3=A9crit :
> > > From: Bartosz Golaszewski <[email protected]>
> > >=20
> > > We're porting all users of gpiochip_find() to using
> > > gpio_device_find().
> > > Update the swnode GPIO code.
> > >=20
> > > Signed-off-by: Bartosz Golaszewski
> > > <[email protected]>
> > > ---
> > > =C2=A0drivers/gpio/gpiolib-swnode.c | 29 ++++++++++++----------------=
-
> > > =C2=A01 file changed, 12 insertions(+), 17 deletions(-)
> > >=20
> > > diff --git a/drivers/gpio/gpiolib-swnode.c
> > > b/drivers/gpio/gpiolib-
> > > swnode.c
> > > index b5a6eaf3729b..56c8519be538 100644
> > > --- a/drivers/gpio/gpiolib-swnode.c
> > > +++ b/drivers/gpio/gpiolib-swnode.c
> > > @@ -31,31 +31,26 @@ static void swnode_format_propname(const char
> > > *con_id, char *propname,
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0 strscpy(propname, "gpios", max_size);
> > > =C2=A0}
> > >=20
> > > -static int swnode_gpiochip_match_name(struct gpio_chip *chip,
> > > void
> > > *data)
> > > +static struct gpio_device *swnode_get_gpio_device(struct
> > > fwnode_handle *fwnode)
> > > =C2=A0{
> > > -=C2=A0=C2=A0=C2=A0=C2=A0 return !strcmp(chip->label, data);
> > > -}
> > > +=C2=A0=C2=A0=C2=A0=C2=A0 const struct software_node *gdev_node;
> > > +=C2=A0=C2=A0=C2=A0=C2=A0 struct gpio_device *gdev;
> > >=20
> > > -static struct gpio_chip *swnode_get_chip(struct fwnode_handle
> > > *fwnode)
> > > -{
> > > -=C2=A0=C2=A0=C2=A0=C2=A0 const struct software_node *chip_node;
> > > -=C2=A0=C2=A0=C2=A0=C2=A0 struct gpio_chip *chip;
> > > -
> > > -=C2=A0=C2=A0=C2=A0=C2=A0 chip_node =3D to_software_node(fwnode);
> > > -=C2=A0=C2=A0=C2=A0=C2=A0 if (!chip_node || !chip_node->name)
> > > +=C2=A0=C2=A0=C2=A0=C2=A0 gdev_node =3D to_software_node(fwnode);
> > > +=C2=A0=C2=A0=C2=A0=C2=A0 if (!gdev_node || !gdev_node->name)
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0 return ERR_PTR(-EINVAL);
> > >=20
> > > -=C2=A0=C2=A0=C2=A0=C2=A0 chip =3D gpiochip_find((void *)chip_node->n=
ame,
> > > swnode_gpiochip_match_name);
> > > -=C2=A0=C2=A0=C2=A0=C2=A0 return chip ?: ERR_PTR(-EPROBE_DEFER);
> > > +=C2=A0=C2=A0=C2=A0=C2=A0 gdev =3D gpio_device_find_by_label((void *)=
gdev_node->name);
> > > +=C2=A0=C2=A0=C2=A0=C2=A0 return gdev ?: ERR_PTR(-EPROBE_DEFER);
> > > =C2=A0}
> > >=20
> > > =C2=A0struct gpio_desc *swnode_find_gpio(struct fwnode_handle *fwnode=
,
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 const char *con_id, unsign=
ed int
> > > idx,
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 unsigned long *flags)
> > > =C2=A0{
> > > +=C2=A0=C2=A0=C2=A0=C2=A0 struct gpio_device *gdev __free(gpio_device=
_put) =3D NULL;
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 const struct software_node *swnode;
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct fwnode_reference_args args;
> > > -=C2=A0=C2=A0=C2=A0=C2=A0 struct gpio_chip *chip;
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct gpio_desc *desc;
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 char propname[32]; /* 32 is max size o=
f property name */
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 int error;
> > > @@ -77,12 +72,12 @@ struct gpio_desc *swnode_find_gpio(struct
> > > fwnode_handle *fwnode,
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0 return ERR_PTR(error);
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }
> > >=20
> > > -=C2=A0=C2=A0=C2=A0=C2=A0 chip =3D swnode_get_chip(args.fwnode);
> > > +=C2=A0=C2=A0=C2=A0=C2=A0 gdev =3D swnode_get_gpio_device(args.fwnode=
);
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fwnode_handle_put(args.fwnode);
> > > -=C2=A0=C2=A0=C2=A0=C2=A0 if (IS_ERR(chip))
> > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 return ERR_CAST(chip);
> > > +=C2=A0=C2=A0=C2=A0=C2=A0 if (IS_ERR(gdev))
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 return ERR_CAST(gdev);
> >=20
> > I'm a bit late to the party, sorry.
> >=20
> > I'm looking at how __free() should be used to use it in my own
> > patchset, and I was wondering if this code actually works.
> >=20
> > What happens if swnode_get_gpio_device() returns an error pointer?
> > Won't that cause a call to gpio_device_put() with the invalid
> > pointer?
> >=20
> > Cheers,
> > -Paul
> >=20
>=20
> No. because the __free() callback is defined as:
>=20
> DEFINE_FREE(gpio_device_put, struct gpio_device *,
> =C2=A0=C2=A0=C2=A0 if (!IS_ERR_OR_NULL(_T)) gpio_device_put(_T))

Ok. I missed this.

I would argue that it's still not right though - it should probably use
IS_ERR() instead. gpio_device_put() only happens to accept NULL
pointers because the "dev" field is at the very beginning of the
"gpio_device" struct. I'm not sure this works with e.g.
CONFIG_RANDSTRUCT_FULL.

> Bart

Cheers,
-Paul

> > >=20
> > > -=C2=A0=C2=A0=C2=A0=C2=A0 desc =3D gpiochip_get_desc(chip, args.args[=
0]);
> > > +=C2=A0=C2=A0=C2=A0=C2=A0 desc =3D gpiochip_get_desc(gdev->chip, args=
.args[0]);
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 *flags =3D args.args[1]; /* We expect =
native GPIO flags */
> > >=20
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pr_debug("%s: parsed '%s' property of =
node '%pfwP[%d]' -
> > > status (%d)\n",
> >=20