Re: [PATCH] sysfs: Remove page boundary align limitation on sysfs_emit and sysfs_emit_at

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Thu, Sep 09, 2021 at 05:59:01AM +0000, Yu, Lang wrote:
> [AMD Official Use Only]
> 
> 
> 
> >-----Original Message-----
> >From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >Sent: Thursday, September 9, 2021 1:35 PM
> >To: Yu, Lang <Lang.Yu@amd.com>
> >Cc: Joe Perches <joe@perches.com>; Rafael J . Wysocki <rafael@kernel.org>;
> >linux-kernel@vger.kernel.org
> >Subject: Re: [PATCH] sysfs: Remove page boundary align limitation on sysfs_emit
> >and sysfs_emit_at
> >
> >On Thu, Sep 09, 2021 at 05:27:49AM +0000, Yu, Lang wrote:
> >>
> >>
> >>
> >> >-----Original Message-----
> >> >From: Joe Perches <joe@perches.com>
> >> >Sent: Thursday, September 9, 2021 1:06 PM
> >> >To: Yu, Lang <Lang.Yu@amd.com>; Greg Kroah-Hartman
> >> ><gregkh@linuxfoundation.org>; Rafael J . Wysocki <rafael@kernel.org>;
> >> >linux- kernel@vger.kernel.org
> >> >Subject: Re: [PATCH] sysfs: Remove page boundary align limitation on
> >> >sysfs_emit and sysfs_emit_at
> >> >
> >> >On Wed, 2021-09-08 at 20:07 +0800, Lang Yu wrote:
> >> >> The key purpose of sysfs_emit and sysfs_emit_at is to ensure that
> >> >> no overrun is done. Make them more equivalent with scnprintf.
> >> >
> >> >I can't think of a single reason to do this.
> >> >sysfs_emit and sysfs_emit_at are specific to sysfs.
> >> >
> >> >Use of these functions outside of sysfs is not desired or supported.
> >> >
> >> >
> >> Thanks for your reply. But I'm still curious why you put such a limitation.
> >> As "Documentation/filesystems/sysfs.rst" described, we can just  use
> >> scnprintf(buf, PAGE_SIZE, "%s\n", dev->name) in show functions without
> >> such a limitation.
> >>
> >> But you said that " - show() should only use sysfs_emit() or
> >> sysfs_emit_at() when formatting the value to be returned to user space. " in
> >Documentation/filesystems/sysfs.rst.
> >>
> >> Some guys just try to replace scnprintf with sysfs_emit() or sysfs_emit_at() per
> >above documents.
> >
> >That is just fine in sysfs show() functions.
> >
> >> But sprintf and sysfs_emit/sysfs_emit_at are not totally equivalent(e.g., page
> >boundary align).
> >
> >Which is good, it checks for more error conditions.
> >
> >I fail to understand what problem you have had with this.  What sysfs file was
> >converted and had issues?
> >
> >> In my opinion, we add a new api and try to replace an old api. Does we
> >> need to make it more compatible with old api? Thanks.
> >
> >There is no "old api" here.  People used a wide variety of different things in sysfs
> >file show() functions, now you can just use a single one.
> 
> Yes, replace these things in sysfs show functions to avoid sprintf defects makes life better. 
> But assume users will put a page boundary align buffer address in its' first argument makes life so hard.

Not at all, that is what sysfs requires.

As you have pointed out, someone violated the sysfs rules, and these
functions caught that, which shows that the code is correct and that the
driver needs to be fixed.

thanks,

greg k-h