| Message ID | 20211016065734.28802-4-manivannan.sadhasivam@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | MHI patches for v5.16 | expand |
On Sat, Oct 16, 2021 at 12:27:34PM +0530, Manivannan Sadhasivam wrote: > From: Qing Wang <wangqing@vivo.com> > > 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 <wangqing@vivo.com> > Reviewed-by: Hemant Kumar <hemantk@codeaurora.org> > Reviewed-by: Manivannan Sadhasivam <mani@kernel.org> > Link: https://lore.kernel.org/r/1634095550-3978-1-git-send-email-wangqing@vivo.com > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > 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 <wangqing@vivo.com> > > 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 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 <wangqing@vivo.com> > > > 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 <wangqing@vivo.com> > > > > 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 <wangqing@vivo.com> > > > > 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 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 <wangqing@vivo.com> > > > > 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 <wangqing@vivo.com> > > Reviewed-by: Hemant Kumar <hemantk@codeaurora.org> > > Reviewed-by: Manivannan Sadhasivam <mani@kernel.org> > > Link: https://lore.kernel.org/r/1634095550-3978-1-git-send-email-wangqing@vivo.com > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > 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 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 <wangqing@vivo.com> >>>>> 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
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);