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

[Yu, Lang]

[AMD Official Use Only]



>-----Original Message-----
>From: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>Sent: Wednesday, September 8, 2021 9:49 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 Wed, Sep 08, 2021 at 01:21:16PM +0000, Yu, Lang wrote:
>> [AMD Official Use Only]
>>
>>
>>
>> >-----Original Message-----
>> >From: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>> >Sent: Wednesday, September 8, 2021 9:04 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
>> >
>> >A:
>> >https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fen.w
>> >ikipe%2F&amp;data=04%7C01%7CLang.Yu%40amd.com%7C43d1354fdeda45
>713a340
>> >8d972cf6fcd%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C63766
>7057520
>> >373013%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2l
>uMzIiLC
>> >JBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=tfUI5HXg6YbMRtFXs7
>X0o7Z
>> >rRgKdwJfk%2FwIykAEkNCY%3D&amp;reserved=0
>> >dia.org%2Fwiki%2FTop_post&amp;data=04%7C01%7CLang.Yu%40amd.com%
>7C
>> >fed047de547541548fcc08d972c92627%7C3dd8961fe4884e608e11a82d994e1
>83d
>> >%7C0%7C0%7C637667030534349355%7CUnknown%7CTWFpbGZsb3d8eyJWIj
>oi
>> >MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C100
>0&
>> >amp;sdata=LHujj041jxZjvoYxVYUKtNr7us%2FX4pl%2FdOkFSOP1W8U%3D&am
>p;r
>> >eserved=0
>> >Q: Were do I find info about this thing called top-posting?
>> >A: Because it messes up the order in which people normally read text.
>> >Q: Why is top-posting such a bad thing?
>> >A: Top-posting.
>> >Q: What is the most annoying thing in e-mail?
>> >
>> >A: No.
>> >Q: Should I include quotations after my reply?
>> >
>> >https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdari
>> >ngfire
>> >ball.net%2F2007%2F07%2Fon_top&amp;data=04%7C01%7CLang.Yu%40amd.
>co
>> >m%7Cfed047de547541548fcc08d972c92627%7C3dd8961fe4884e608e11a82d
>99
>> >4e183d%7C0%7C0%7C637667030534349355%7CUnknown%7CTWFpbGZsb3d
>8ey
>> >JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%
>7C
>> >1000&amp;sdata=AOLGBdj01XiEjhmsBSGTNuqejgU%2B6jg416Paz5XdM1A%3D
>&a
>> >mp;reserved=0
>> >
>> >
>> >On Wed, Sep 08, 2021 at 12:52:43PM +0000, Yu, Lang wrote:
>> >> [AMD Official Use Only]
>> >>
>> >> Thanks for your reply.
>> >> Just curious if we don't put such a limitation, what are the consequences?
>> >> If we remove the limitation, sys_emit/sys_emit_at api will be more flexible.
>> >> Since the comments of  sysfs_emit/ sys_emit_at api are " sysfs_emit
>> >> - scnprintf equivalent, aware of PAGE_SIZE buffer. ", Why not make
>> >> them more equivalent with scnprintf?
>> >
>> >Because this is not a general replacement for scnprintf(), it is only
>> >to be used with sysfs files.
>> >
>> >Where else are you wanting to use these functions that this patch
>> >woulud be required that does not haver to deal with sysfs?
>> >
>> >thanks,
>> >
>> >greg k-h
>>
>> But some guys think it is a general replacement for scnprintf(),
>
>Who thinks that?  Where?  The name should give them a clue that this is not true.
>
>> and  recommend that use sysfs_emit() instead of scnprintf(),
>
>Please no.
>
>> and send many patches that replace  scnprintf() with sysfs_emit(), and
>> finally cause some invalid sysfs_emit_at: buf:00000000f19bdfde warnings.
>
>Where were those patches sent?  I will be glad to review those.
>
>> I think we better not put " scnprintf equivalent, aware of PAGE_SIZE buffer "
>words in comments.
>> It is obviously not. Some  guys are misled by that. Thanks!
>
>Please feel free to add better documentation for the functions if you feel people
>are getting confused, do not change the existing behavior of the code as it rightly
>caught it being misused.

You can find many patches named "convert sysfs scnprintf/snprintf to syfs_emit/sysfs_emit_at".
or "use sysfs_emit/sysfs_emit_at in show functions". They may think it's better to use syfs_emit/sysfs_emit_at
given its overrun avoidance. But there are still some corner cases(e.g., a non page boundary aligned buf address : ).
Thanks for your explanations and have a nice day!

Regards,
Lang

>
>thanks,
>
>greg k-h