2021-12-08 08:08:40

by Yihao Han

Subject: [PATCH] bus: mhi: core: replace snprintf with sysfs_emit

coccinelle report:
./drivers/bus/mhi/core/init.c:97:8-16:
WARNING: use scnprintf or sprintf
Use sysfs_emit instead of scnprintf or sprintf makes more sense.

Signed-off-by: Yihao Han <[email protected]>
---
drivers/bus/mhi/core/init.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index 5aaca6d0f52b..a5a5c722731e 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -94,7 +94,7 @@ static ssize_t serial_number_show(struct device *dev,
struct mhi_device *mhi_dev = to_mhi_device(dev);
struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;

- return snprintf(buf, PAGE_SIZE, "Serial Number: %u\n",
+ return sysfs_emit(buf, "Serial Number: %u\n",
mhi_cntrl->serial_number);
}
static DEVICE_ATTR_RO(serial_number);
--
2.17.1

2021-12-08 08:14:50

by Greg Kroah-Hartman

Subject: Re: [PATCH] bus: mhi: core: replace snprintf with sysfs_emit

On Wed, Dec 08, 2021 at 12:07:53AM -0800, Yihao Han wrote:
> coccinelle report:
> ./drivers/bus/mhi/core/init.c:97:8-16:
> WARNING: use scnprintf or sprintf
> Use sysfs_emit instead of scnprintf or sprintf makes more sense.
>
> Signed-off-by: Yihao Han <[email protected]>
> ---
> drivers/bus/mhi/core/init.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index 5aaca6d0f52b..a5a5c722731e 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -94,7 +94,7 @@ static ssize_t serial_number_show(struct device *dev,
> struct mhi_device *mhi_dev = to_mhi_device(dev);
> struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
>
> - return snprintf(buf, PAGE_SIZE, "Serial Number: %u\n",
> + return sysfs_emit(buf, "Serial Number: %u\n",
> mhi_cntrl->serial_number);
> }
> static DEVICE_ATTR_RO(serial_number);
> --
> 2.17.1
>

Why are you only changing one function in this file? If you realyl want
to make a change like this, fix ALL sysfs show functions.

thanks,

greg k-h

2021-12-08 14:03:40

by Yihao Han

Subject: Re: [PATCH] bus: mhi: core: replace snprintf with sysfs_emit

On 2021/12/8 16:14, Greg Kroah-Hartman wrote:
> On Wed, Dec 08, 2021 at 12:07:53AM -0800, Yihao Han wrote:
>> coccinelle report:
>> ./drivers/bus/mhi/core/init.c:97:8-16:
>> WARNING: use scnprintf or sprintf
>> Use sysfs_emit instead of scnprintf or sprintf makes more sense.
>>
>> Signed-off-by: Yihao Han <[email protected]>
>> ---
>> drivers/bus/mhi/core/init.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
>> index 5aaca6d0f52b..a5a5c722731e 100644
>> --- a/drivers/bus/mhi/core/init.c
>> +++ b/drivers/bus/mhi/core/init.c
>> @@ -94,7 +94,7 @@ static ssize_t serial_number_show(struct device *dev,
>> struct mhi_device *mhi_dev = to_mhi_device(dev);
>> struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
>>
>> - return snprintf(buf, PAGE_SIZE, "Serial Number: %u\n",
>> + return sysfs_emit(buf, "Serial Number: %u\n",
>> mhi_cntrl->serial_number);
>> }
>> static DEVICE_ATTR_RO(serial_number);
>> --
>> 2.17.1
>>
>
> Why are you only changing one function in this file? If you realyl want
> to make a change like this, fix ALL sysfs show functions.
>
> thanks,
>
> greg k-h
Because "sysfs_emit()" is "scnprintf()" equivalent with "size" parameter
equals to PAGE_SIZE,So sysfs_emit can only be used here in this file.
And "scnprintf" is better than "snprintf" because the former returns
number of characters written to "buf". So I think we can use
"sysfs_emit()" instead of "snprintf()".

Thanks,
Yihao

2021-12-08 14:08:00

by Greg Kroah-Hartman

Subject: Re: [PATCH] bus: mhi: core: replace snprintf with sysfs_emit

On Wed, Dec 08, 2021 at 10:03:30PM +0800, Yihao Han wrote:
>
>
> On 2021/12/8 16:14, Greg Kroah-Hartman wrote:
> > On Wed, Dec 08, 2021 at 12:07:53AM -0800, Yihao Han wrote:
> > > coccinelle report:
> > > ./drivers/bus/mhi/core/init.c:97:8-16:
> > > WARNING: use scnprintf or sprintf
> > > Use sysfs_emit instead of scnprintf or sprintf makes more sense.
> > >
> > > Signed-off-by: Yihao Han <[email protected]>
> > > ---
> > > drivers/bus/mhi/core/init.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> > > index 5aaca6d0f52b..a5a5c722731e 100644
> > > --- a/drivers/bus/mhi/core/init.c
> > > +++ b/drivers/bus/mhi/core/init.c
> > > @@ -94,7 +94,7 @@ static ssize_t serial_number_show(struct device *dev,
> > > struct mhi_device *mhi_dev = to_mhi_device(dev);
> > > struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
> > > - return snprintf(buf, PAGE_SIZE, "Serial Number: %u\n",
> > > + return sysfs_emit(buf, "Serial Number: %u\n",
> > > mhi_cntrl->serial_number);
> > > }
> > > static DEVICE_ATTR_RO(serial_number);
> > > --
> > > 2.17.1
> > >
> >
> > Why are you only changing one function in this file? If you realyl want
> > to make a change like this, fix ALL sysfs show functions.
> >
> > thanks,
> >
> > greg k-h
> Because "sysfs_emit()" is "scnprintf()" equivalent with "size" parameter
> equals to PAGE_SIZE,So sysfs_emit can only be used here in this file. And
> "scnprintf" is better than "snprintf" because the former returns number of
> characters written to "buf". So I think we can use "sysfs_emit()" instead of
> "snprintf()".

I am not disagreeing with changing this at all that is is incorrect. I
am complaining that you are only changing ONE of the functions in this
file and not ALL of them. If this type of change is so important to
make, please do it for all places it is needed in the file, and in all
of the drivers/bus/mhi/ code.

thanks,

greg k-h