From: Qing Wang <[email protected]>
coccicheck complains about the use of snprintf() in sysfs show functions.
Fix the following coccicheck warning:
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: Qing Wang <[email protected]>
Reviewed-by: Hemant Kumar <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Manivannan Sadhasivam <[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.25.1
On Sat, Oct 16, 2021 at 12:27:34PM +0530, Manivannan Sadhasivam wrote:
> From: Qing Wang <[email protected]>
>
> coccicheck complains about the use of snprintf() in sysfs show functions.
>
> Fix the following coccicheck warning:
> 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: Qing Wang <[email protected]>
> Reviewed-by: Hemant Kumar <[email protected]>
> Reviewed-by: Manivannan Sadhasivam <[email protected]>
> Link: https://lore.kernel.org/r/[email protected]
> Signed-off-by: Manivannan Sadhasivam <[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);
The text "Serial Number: " should not be in here, right? It's obvious
this is a serial number, that's what the documentation and file name
says. Userspace should not have to parse sysfs files.
And why is only one sysfs entry being changed in this file? Either they
all should be, or none, no need to do this one-patch-per-entry, right?
Note, I have rejected Qing's patches like this for other subsystems
already because they are not complete, this is something they are well
aware of by now...
thanks,
greg k-h
On Sat, 2021-10-16 at 09:37 +0200, Greg KH wrote:
> On Sat, Oct 16, 2021 at 12:27:34PM +0530, Manivannan Sadhasivam wrote:
> > From: Qing Wang <[email protected]>
> > coccicheck complains about the use of snprintf() in sysfs show functions.
[]
> > diff --git 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);
>
> The text "Serial Number: " should not be in here, right? It's obvious
> this is a serial number, that's what the documentation and file name
> says. Userspace should not have to parse sysfs files.
sysfs is ABI right? Parsing or not, it's what's already there.
On Sat, Oct 16, 2021 at 09:37:50AM +0200, Greg KH wrote:
> On Sat, Oct 16, 2021 at 12:27:34PM +0530, Manivannan Sadhasivam wrote:
> > From: Qing Wang <[email protected]>
> >
> > coccicheck complains about the use of snprintf() in sysfs show functions.
> >
> > Fix the following coccicheck warning:
> > 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: Qing Wang <[email protected]>
> > Reviewed-by: Hemant Kumar <[email protected]>
> > Reviewed-by: Manivannan Sadhasivam <[email protected]>
> > Link: https://lore.kernel.org/r/[email protected]
> > Signed-off-by: Manivannan Sadhasivam <[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);
>
> The text "Serial Number: " should not be in here, right? It's obvious
> this is a serial number, that's what the documentation and file name
> says. Userspace should not have to parse sysfs files.
>
Right, somehow missed it :/
> And why is only one sysfs entry being changed in this file? Either they
> all should be, or none, no need to do this one-patch-per-entry, right?
>
> Note, I have rejected Qing's patches like this for other subsystems
> already because they are not complete, this is something they are well
> aware of by now...
>
Oh, I'm not aware of this.
Qing: Please modify the other instance of snprintf also.
Thanks,
Mani
> thanks,
>
> greg k-h
On Sat, Oct 16, 2021 at 03:24:17AM -0700, Joe Perches wrote:
> On Sat, 2021-10-16 at 09:37 +0200, Greg KH wrote:
> > On Sat, Oct 16, 2021 at 12:27:34PM +0530, Manivannan Sadhasivam wrote:
> > > From: Qing Wang <[email protected]>
> > > coccicheck complains about the use of snprintf() in sysfs show functions.
> []
> > > diff --git 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);
> >
> > The text "Serial Number: " should not be in here, right? It's obvious
> > this is a serial number, that's what the documentation and file name
> > says. Userspace should not have to parse sysfs files.
>
> sysfs is ABI right? Parsing or not, it's what's already there.
If no tools rely on this, and we can change it, we should at least try.
We can not change ABI if something breaks. If nothing relies on it,
then it is fine to do so.
thanks,
greg k-h
On Sat, 2021-10-16 at 17:07 +0200, Greg KH wrote:
> On Sat, Oct 16, 2021 at 03:24:17AM -0700, Joe Perches wrote:
> > On Sat, 2021-10-16 at 09:37 +0200, Greg KH wrote:
> > > On Sat, Oct 16, 2021 at 12:27:34PM +0530, Manivannan Sadhasivam wrote:
> > > > From: Qing Wang <[email protected]>
> > > > coccicheck complains about the use of snprintf() in sysfs show functions.
> > []
> > > > diff --git 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);
> > >
> > > The text "Serial Number: " should not be in here, right? It's obvious
> > > this is a serial number, that's what the documentation and file name
> > > says. Userspace should not have to parse sysfs files.
> >
> > sysfs is ABI right? Parsing or not, it's what's already there.
>
> If no tools rely on this, and we can change it, we should at least try.
>
> We can not change ABI if something breaks. If nothing relies on it,
> then it is fine to do so.
That's a quite bad way to think of an ABI.
All that does is tempt fate as you don't know if something already
uses it until someone complains and by that time something else may
be written to depend on the new behavior.
On Sat, Oct 16, 2021 at 05:07:06PM +0200, Greg KH wrote:
> On Sat, Oct 16, 2021 at 03:24:17AM -0700, Joe Perches wrote:
> > On Sat, 2021-10-16 at 09:37 +0200, Greg KH wrote:
> > > On Sat, Oct 16, 2021 at 12:27:34PM +0530, Manivannan Sadhasivam wrote:
> > > > From: Qing Wang <[email protected]>
> > > > coccicheck complains about the use of snprintf() in sysfs show functions.
> > []
> > > > diff --git 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);
> > >
> > > The text "Serial Number: " should not be in here, right? It's obvious
> > > this is a serial number, that's what the documentation and file name
> > > says. Userspace should not have to parse sysfs files.
> >
> > sysfs is ABI right? Parsing or not, it's what's already there.
>
> If no tools rely on this, and we can change it, we should at least try.
>
> We can not change ABI if something breaks. If nothing relies on it,
> then it is fine to do so.
>
Hemant, Bhaumik, do you guys know if there are any possible users (scripts/apps)
of this ABI? I'm not 100% inclined to change it but if we are _sure_ that there
are no users yet, then I'm ok with it.
Thanks,
Mani
> thanks,
>
> greg k-h
On 10/16/2021 9:15 AM, Manivannan Sadhasivam wrote:
> On Sat, Oct 16, 2021 at 05:07:06PM +0200, Greg KH wrote:
>> On Sat, Oct 16, 2021 at 03:24:17AM -0700, Joe Perches wrote:
>>> On Sat, 2021-10-16 at 09:37 +0200, Greg KH wrote:
>>>> On Sat, Oct 16, 2021 at 12:27:34PM +0530, Manivannan Sadhasivam wrote:
>>>>> From: Qing Wang <[email protected]>
>>>>> coccicheck complains about the use of snprintf() in sysfs show functions.
>>> []
>>>>> diff --git 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);
>>>>
>>>> The text "Serial Number: " should not be in here, right? It's obvious
>>>> this is a serial number, that's what the documentation and file name
>>>> says. Userspace should not have to parse sysfs files.
>>>
>>> sysfs is ABI right? Parsing or not, it's what's already there.
>>
>> If no tools rely on this, and we can change it, we should at least try.
>>
>> We can not change ABI if something breaks. If nothing relies on it,
>> then it is fine to do so.
>>
>
> Hemant, Bhaumik, do you guys know if there are any possible users (scripts/apps)
> of this ABI? I'm not 100% inclined to change it but if we are _sure_ that there
> are no users yet, then I'm ok with it.
Mani, i dont know if any script being used by any user to parse "Serial
Number".
Thanks,
Hemant
>
> Thanks,
> Mani
>
>> thanks,
>>
>> greg k-h
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project