[01/10] gpiolib: provide gpiochip_dup_line_label()

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

gpiochip_is_requested() not only has a misleading name but it returns
a pointer to a string that is freed when the descriptor is released.

Provide a new helper meant to replace it, which returns a copy of the
label string instead.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c      | 29 +++++++++++++++++++++++++++++
 include/linux/gpio/driver.h |  1 +
 2 files changed, 30 insertions(+)

On Wed, Nov 29, 2023 at 03:24:02PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> gpiochip_is_requested() not only has a misleading name but it returns
> a pointer to a string that is freed when the descriptor is released.
> 
> Provide a new helper meant to replace it, which returns a copy of the
> label string instead.

...

> +/**
> + * gpiochip_dup_line_label - Get a copy of the consumer label.
> + * @gc: GPIO chip controlling this line.
> + * @offset: Hardware offset of the line.
> + *
> + * Returns:
> + * Pointer to a copy of the consumer label if the line is requested or NULL
> + * if it's not. If a valid pointer was returned, it must be freed using
> + * kfree(). In case of a memory allocation error, the function returns %ENOMEM.

kfree_const() ? (see below)

> + * Must not be called from atomic context.
> + */
> +char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset)
> +{
> +	const char *label;
> +	char *cpy;

Why not "copy"?

> +
> +	label = gpiochip_is_requested(gc, offset);
> +	if (!label)
> +		return NULL;

> +	cpy = kstrdup(label, GFP_KERNEL);

You probably want to have kstrdup_const(). However, I haven't checked
if we have such use cases.

> +	if (!cpy)
> +		return ERR_PTR(-ENOMEM);
> +
> +	return cpy;
> +}

So, how does this differ from the previous one? You need to hold a reference
to the descriptor before copying and release it after.

On Wed, Nov 29, 2023 at 3:57 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Nov 29, 2023 at 03:24:02PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > gpiochip_is_requested() not only has a misleading name but it returns
> > a pointer to a string that is freed when the descriptor is released.
> >
> > Provide a new helper meant to replace it, which returns a copy of the
> > label string instead.
>
> ...
>
> > +/**
> > + * gpiochip_dup_line_label - Get a copy of the consumer label.
> > + * @gc: GPIO chip controlling this line.
> > + * @offset: Hardware offset of the line.
> > + *
> > + * Returns:
> > + * Pointer to a copy of the consumer label if the line is requested or NULL
> > + * if it's not. If a valid pointer was returned, it must be freed using
> > + * kfree(). In case of a memory allocation error, the function returns %ENOMEM.
>
> kfree_const() ? (see below)
>
> > + * Must not be called from atomic context.
> > + */
> > +char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset)
> > +{
> > +     const char *label;
> > +     char *cpy;
>
> Why not "copy"?
>
> > +
> > +     label = gpiochip_is_requested(gc, offset);
> > +     if (!label)
> > +             return NULL;
>
> > +     cpy = kstrdup(label, GFP_KERNEL);
>
> You probably want to have kstrdup_const(). However, I haven't checked
> if we have such use cases.

I thought about it but I'm thinking that it would be confusing to
users and lead to bugs. This is not used very much and only for
debugfs output. Let's keep it simple.

>
> > +     if (!cpy)
> > +             return ERR_PTR(-ENOMEM);
> > +
> > +     return cpy;
> > +}
>
> So, how does this differ from the previous one? You need to hold a reference
> to the descriptor before copying and release it after.
>

The last patch reworks it to hold the obsolete gpio_lock and the
upcoming changes will make this perform the copy under the descriptor
lock and return it once it's released.

Bart

> --
> With Best Regards,
> Andy Shevchenko
>
>