| Message ID | 20240116045151.3940401-13-lizhijian@fujitsu.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fix coccicheck warnings | expand |
On Tue, 16 Jan, 2024 12:51:24 +0800 Li Zhijian <lizhijian@fujitsu.com> wrote: > Per filesystems/sysfs.rst, show() should only use sysfs_emit() > or sysfs_emit_at() when formatting the value to be returned to user space. > > coccinelle complains that there are still a couple of functions that use > snprintf(). Convert them to sysfs_emit(). > >> ./drivers/ptp/ptp_sysfs.c:27:8-16: WARNING: please use sysfs_emit > > No functional change intended > > CC: Richard Cochran <richardcochran@gmail.com> > CC: netdev@vger.kernel.org > Signed-off-by: Li Zhijian <lizhijian@fujitsu.com> > --- > drivers/ptp/ptp_sysfs.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c > index f7a499a1bd39..49737ed6ef5f 100644 > --- a/drivers/ptp/ptp_sysfs.c > +++ b/drivers/ptp/ptp_sysfs.c > @@ -24,8 +24,7 @@ static ssize_t max_phase_adjustment_show(struct device *dev, > { > struct ptp_clock *ptp = dev_get_drvdata(dev); > > - return snprintf(page, PAGE_SIZE - 1, "%d\n", > - ptp->info->getmaxphase(ptp->info)); > + return sysfs_emit(page, "%d\n", ptp->info->getmaxphase(ptp->info)); > } > static DEVICE_ATTR_RO(max_phase_adjustment); I authored the lines that are being changed here, so figured I should provide my review. Doesn't PTP_SHOW_INT in the same file also use snprintf in the same manner and should be changed to sysfs_emit? -- Thanks, Rahul Rameshbabu
On 16/01/2024 13:01, Rahul Rameshbabu wrote: > On Tue, 16 Jan, 2024 12:51:24 +0800 Li Zhijian <lizhijian@fujitsu.com> wrote: >> Per filesystems/sysfs.rst, show() should only use sysfs_emit() >> or sysfs_emit_at() when formatting the value to be returned to user space. >> >> coccinelle complains that there are still a couple of functions that use >> snprintf(). Convert them to sysfs_emit(). >> >>> ./drivers/ptp/ptp_sysfs.c:27:8-16: WARNING: please use sysfs_emit >> >> No functional change intended >> >> CC: Richard Cochran <richardcochran@gmail.com> >> CC: netdev@vger.kernel.org >> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com> >> --- >> drivers/ptp/ptp_sysfs.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c >> index f7a499a1bd39..49737ed6ef5f 100644 >> --- a/drivers/ptp/ptp_sysfs.c >> +++ b/drivers/ptp/ptp_sysfs.c >> @@ -24,8 +24,7 @@ static ssize_t max_phase_adjustment_show(struct device *dev, >> { >> struct ptp_clock *ptp = dev_get_drvdata(dev); >> >> - return snprintf(page, PAGE_SIZE - 1, "%d\n", >> - ptp->info->getmaxphase(ptp->info)); >> + return sysfs_emit(page, "%d\n", ptp->info->getmaxphase(ptp->info)); >> } >> static DEVICE_ATTR_RO(max_phase_adjustment); > > I authored the lines that are being changed here, so figured I should > provide my review. Doesn't PTP_SHOW_INT in the same file also use > snprintf in the same manner and should be changed to sysfs_emit? Thanks for you review. Yes, i think so, beside PTP_SHOW_INT, there are 3 other places not detected by coccinelle should be fixed. I will update it in V2. $ grep snprintf drivers/ptp/ptp_sysfs.c return snprintf(page, PAGE_SIZE-1, "%d\n", ptp->info->var); \ cnt = snprintf(page, PAGE_SIZE, "%u %lld %u\n", size = snprintf(page, PAGE_SIZE - 1, "%u\n", ptp->n_vclocks); size = snprintf(page, PAGE_SIZE - 1, "%u\n", ptp->max_vclocks); Thanks Zhijian > > -- > Thanks, > > Rahul Rameshbabu
On Tue, 16 Jan 2024 12:51:24 +0800 Li Zhijian wrote: > Per filesystems/sysfs.rst, show() should only use sysfs_emit() > or sysfs_emit_at() when formatting the value to be returned to user space. > > coccinelle complains that there are still a couple of functions that use > snprintf(). Convert them to sysfs_emit(). > > > ./drivers/ptp/ptp_sysfs.c:27:8-16: WARNING: please use sysfs_emit If the patches don't depend on each other please post them separately. Series should only be used if there are dependencies and the same maintainer is expected to apply all patches. The ptp change should be reposted after the merge window, we don't take cleanups during the merge window.
Jakub, On 16/01/2024 23:33, Jakub Kicinski wrote: > On Tue, 16 Jan 2024 12:51:24 +0800 Li Zhijian wrote: >> Per filesystems/sysfs.rst, show() should only use sysfs_emit() >> or sysfs_emit_at() when formatting the value to be returned to user space. >> >> coccinelle complains that there are still a couple of functions that use >> snprintf(). Convert them to sysfs_emit(). >> >>> ./drivers/ptp/ptp_sysfs.c:27:8-16: WARNING: please use sysfs_emit > > If the patches don't depend on each other please post them separately. > Series should only be used if there are dependencies and the same > maintainer is expected to apply all patches> > The ptp change should be reposted after the merge window, we don't take > cleanups during the merge window. Understood. i will split them per subsystem/maintainer and repost them separately after the merge window. Thanks Zhijian (Grouping them into the same set helped us have an overview of this warning.)
diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c index f7a499a1bd39..49737ed6ef5f 100644 --- a/drivers/ptp/ptp_sysfs.c +++ b/drivers/ptp/ptp_sysfs.c @@ -24,8 +24,7 @@ static ssize_t max_phase_adjustment_show(struct device *dev, { struct ptp_clock *ptp = dev_get_drvdata(dev); - return snprintf(page, PAGE_SIZE - 1, "%d\n", - ptp->info->getmaxphase(ptp->info)); + return sysfs_emit(page, "%d\n", ptp->info->getmaxphase(ptp->info)); } static DEVICE_ATTR_RO(max_phase_adjustment);