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

[Yu, Lang]

[Public]



>-----Original Message-----
>From: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>Sent: Thursday, September 9, 2021 2:08 PM
>To: Yu, Lang <Lang.Yu@xxxxxxx>
>Cc: Joe Perches <joe@xxxxxxxxxxx>; Rafael J . Wysocki <rafael@xxxxxxxxxx>;
>linux-kernel@xxxxxxxxxxxxxxx
>Subject: Re: [PATCH] sysfs: Remove page boundary align limitation on sysfs_emit
>and sysfs_emit_at
>
>On Thu, Sep 09, 2021 at 05:52:23AM +0000, Yu, Lang wrote:
>> [Public]
>>
>>
>>
>> >-----Original Message-----
>> >From: Joe Perches <joe@xxxxxxxxxxx>
>> >Sent: Thursday, September 9, 2021 1:44 PM
>> >To: Yu, Lang <Lang.Yu@xxxxxxx>; Greg Kroah-Hartman
>> ><gregkh@xxxxxxxxxxxxxxxxxxx>; Rafael J . Wysocki <rafael@xxxxxxxxxx>;
>> >linux- kernel@xxxxxxxxxxxxxxx
>> >Subject: Re: [PATCH] sysfs: Remove page boundary align limitation on
>> >sysfs_emit and sysfs_emit_at
>> >
>> >On Thu, 2021-09-09 at 05:27 +0000, Yu, Lang wrote:
>> >> [AMD Official Use Only]
>> >
>> >this is a public list and this marker is not appropriate.
>>
>> Sorry for that.
>> >
>> >> > -----Original Message-----
>> >> > From: Joe Perches <joe@xxxxxxxxxxx> 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.
>> >
>> >There's nothing particularly wrong with the use of scnprintf as above.
>> >
>> >The only real reason that sysfs_emit exists is to be able to reduce
>> >the kernel treewide quantity of uses of the sprintf family of
>> >functions that need to be analyzed for possible buffer overruns.
>> >
>> >The issue there is that buf is already known to be both a PAGE_SIZE
>> >buffer and PAGE_SIZE aligned for sysfs show functions so there's no
>> >real reason to use scnprintf.
>> >
>> >sysfs_emit is a shorter/smaller function and using it could avoid
>> >some sprintf defects.
>> >
>> >> Some guys just try to replace scnprintf with sysfs_emit() or
>> >> sysfs_emit_at() per
>> >above documents.
>> >
>> >So don't do that.
>> >
>> >> But sprintf and sysfs_emit/sysfs_emit_at are not totally
>> >> equivalent(e.g., page
>> >boundary align).
>> >>
>> >> 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?
>> >
>> >IMO: no.
>> >
>> But why you said " - 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 ?
>>
>> Obviously, sysfs_emit() and sysfs_emit_at()  can't cover all the cases in show
>functions.
>
>Why not, what usage model can it not cover?

Of course, we can modify driver code to obey sysfs_emit and sysfs_emit_at rules or just use scnprintf in show functions.
Now that you introduced them, why not make them more flexible like scnprintf family functions.
The page boundary align rule makes life hard and I don't like it : ). Many thanks for your explanations!

Regards,
Lang

>thanks,
>
>greg k-h