[PATCH v1 1/1] iio: adc: qcom-spmi-adc5: Fix the channel name

[PATCH v1 1/1] iio: adc: qcom-spmi-adc5: Fix the channel name

[Andy Shevchenko]

The node name can contain an address part which is not used by
the driver. Cut it out before assigning the channel name.

Fixes: 4f47a236a23d ("iio: adc: qcom-spmi-adc5: convert to device properties")
Reported-by: Marijn Suijten <marijn.suijten@somainline.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/adc/qcom-spmi-adc5.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/qcom-spmi-adc5.c b/drivers/iio/adc/qcom-spmi-adc5.c
index e90c299c913a..c52bf6722a6a 100644
--- a/drivers/iio/adc/qcom-spmi-adc5.c
+++ b/drivers/iio/adc/qcom-spmi-adc5.c
@@ -628,12 +628,19 @@ static int adc5_get_fw_channel_data(struct adc5_chip *adc,
 				    struct fwnode_handle *fwnode,
 				    const struct adc5_data *data)
 {
-	const char *name = fwnode_get_name(fwnode), *channel_name;
+	const char *name, *channel_name;
 	u32 chan, value, varr[2];
 	u32 sid = 0;
 	int ret;
 	struct device *dev = adc->dev;
 
+	name = devm_kasprintf(dev, GFP_KERNEL, "%pfwP", fwnode);
+	if (!name)
+		return -ENOMEM;
+
+	/* Cut the address part */
+	name[strchrnul(name, '@') - name] = '\0';
+
 	ret = fwnode_property_read_u32(fwnode, "reg", &chan);
 	if (ret) {
 		dev_err(dev, "invalid channel number %s\n", name);
-- 
2.39.0

Re: [PATCH v1 1/1] iio: adc: qcom-spmi-adc5: Fix the channel name

[Marijn Suijten]

On 2023-01-17 11:39:44, Andy Shevchenko wrote:
> The node name can contain an address part which is not used by
> the driver. Cut it out before assigning the channel name.

This explanation doesn't cut it.  It's not that the driver "doesn't use"
the address part, it is that this string is propagated into the
userspace label, sysfs /filenames/ *and breaking ABI*.

> Fixes: 4f47a236a23d ("iio: adc: qcom-spmi-adc5: convert to device properties")
> Reported-by: Marijn Suijten <marijn.suijten@somainline.org>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/iio/adc/qcom-spmi-adc5.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/qcom-spmi-adc5.c b/drivers/iio/adc/qcom-spmi-adc5.c
> index e90c299c913a..c52bf6722a6a 100644
> --- a/drivers/iio/adc/qcom-spmi-adc5.c
> +++ b/drivers/iio/adc/qcom-spmi-adc5.c
> @@ -628,12 +628,19 @@ static int adc5_get_fw_channel_data(struct adc5_chip *adc,
>  				    struct fwnode_handle *fwnode,
>  				    const struct adc5_data *data)
>  {
> -	const char *name = fwnode_get_name(fwnode), *channel_name;
> +	const char *name, *channel_name;

I don't think this'll compile as name is still a pointer to const data,
while you're assigning (a '\0' char) to it below.

- Marijn

>  	u32 chan, value, varr[2];
>  	u32 sid = 0;
>  	int ret;
>  	struct device *dev = adc->dev;
>  
> +	name = devm_kasprintf(dev, GFP_KERNEL, "%pfwP", fwnode);
> +	if (!name)
> +		return -ENOMEM;
> +
> +	/* Cut the address part */
> +	name[strchrnul(name, '@') - name] = '\0';
> +
>  	ret = fwnode_property_read_u32(fwnode, "reg", &chan);
>  	if (ret) {
>  		dev_err(dev, "invalid channel number %s\n", name);
> -- 
> 2.39.0
> 

Re: [PATCH v1 1/1] iio: adc: qcom-spmi-adc5: Fix the channel name

[Andy Shevchenko]

On Wed, Jan 18, 2023 at 12:12:04AM +0100, Marijn Suijten wrote:
> On 2023-01-17 11:39:44, Andy Shevchenko wrote:
> > The node name can contain an address part which is not used by
> > the driver. Cut it out before assigning the channel name.
> 
> This explanation doesn't cut it.  It's not that the driver "doesn't use"

Driver doesn't use it still. There is no contradiction, but I agree that
below part is good to have in the commit message.

> the address part, it is that this string is propagated into the
> userspace label, sysfs /filenames/ *and breaking ABI*.

So I will add it into v2 in case the fix works (see below).

...

> > -	const char *name = fwnode_get_name(fwnode), *channel_name;
> > +	const char *name, *channel_name;
> 
> I don't think this'll compile as name is still a pointer to const data,
> while you're assigning (a '\0' char) to it below.

Right, it's always hard for me to compile things for ARM on x86 :-)
Thanks for catching this up!

But does this fix the issue after compilation fix?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] iio: adc: qcom-spmi-adc5: Fix the channel name

[Marijn Suijten]

On 2023-01-18 09:46:14, Andy Shevchenko wrote:
> On Wed, Jan 18, 2023 at 12:12:04AM +0100, Marijn Suijten wrote:
> > On 2023-01-17 11:39:44, Andy Shevchenko wrote:
> > > The node name can contain an address part which is not used by
> > > the driver. Cut it out before assigning the channel name.
> > 
> > This explanation doesn't cut it.  It's not that the driver "doesn't use"
> 
> Driver doesn't use it still. There is no contradiction, but I agree that
> below part is good to have in the commit message.

You can leave that in if you want but that's not the issue that I
reported/described.  Having both describes the situation in full.

> > the address part, it is that this string is propagated into the
> > userspace label, sysfs /filenames/ *and breaking ABI*.
> 
> So I will add it into v2 in case the fix works (see below).
> 
> ...
> 
> > > -	const char *name = fwnode_get_name(fwnode), *channel_name;
> > > +	const char *name, *channel_name;
> > 
> > I don't think this'll compile as name is still a pointer to const data,
> > while you're assigning (a '\0' char) to it below.
> 
> Right, it's always hard for me to compile things for ARM on x86 :-)
> Thanks for catching this up!

Thanks for sending this in regardless; as said before I rather break ABI
and clean the driver up properly (no more extend_name...) than sending a
fix like this :)

> But does this fix the issue after compilation fix?

It does, no more @xx in sysfs filenames nor label contents!

- Marijn

Re: [PATCH v1 1/1] iio: adc: qcom-spmi-adc5: Fix the channel name

[Andy Shevchenko]

On Wed, Jan 18, 2023 at 10:29:50AM +0100, Marijn Suijten wrote:
> On 2023-01-18 09:46:14, Andy Shevchenko wrote:
> > On Wed, Jan 18, 2023 at 12:12:04AM +0100, Marijn Suijten wrote:
> > > On 2023-01-17 11:39:44, Andy Shevchenko wrote:

...

> > > > -	const char *name = fwnode_get_name(fwnode), *channel_name;
> > > > +	const char *name, *channel_name;
> > > 
> > > I don't think this'll compile as name is still a pointer to const data,
> > > while you're assigning (a '\0' char) to it below.
> > 
> > Right, it's always hard for me to compile things for ARM on x86 :-)
> > Thanks for catching this up!
> 
> Thanks for sending this in regardless; as said before I rather break ABI
> and clean the driver up properly (no more extend_name...) than sending a
> fix like this :)

Yes, but we need to backport something and ABI breakage is definitely not
an option for that.

> > But does this fix the issue after compilation fix?
> 
> It does, no more @xx in sysfs filenames nor label contents!

Okay, I'm about to send v2 and hopefully you can give your Tested-by.

-- 
With Best Regards,
Andy Shevchenko