All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] sysfs: Remove page boundary align limitation on sysfs_emit and sysfs_emit_at
@ 2021-09-08 12:07 Lang Yu
  2021-09-08 12:32 ` Greg Kroah-Hartman
  2021-09-09  5:05 ` Joe Perches
  0 siblings, 2 replies; 23+ messages in thread
From: Lang Yu @ 2021-09-08 12:07 UTC (permalink / raw)
  To: Joe Perches, Greg Kroah-Hartman, Rafael J . Wysocki, linux-kernel; +Cc: Lang Yu

The key purpose of sysfs_emit and sysfs_emit_at is to
ensure that no overrun is done. Make them more equivalent
with scnprintf.

Signed-off-by: Lang Yu <lang.yu@amd.com>
---
 fs/sysfs/file.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 9aefa7779b29..b754f2ef186f 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -732,14 +732,16 @@ EXPORT_SYMBOL_GPL(sysfs_change_owner);
 int sysfs_emit(char *buf, const char *fmt, ...)
 {
 	va_list args;
-	int len;
+	int len, offset;
 
-	if (WARN(!buf || offset_in_page(buf),
+	offset = offset_in_page(buf);
+
+	if (WARN(!buf,
 		 "invalid sysfs_emit: buf:%p\n", buf))
 		return 0;
 
 	va_start(args, fmt);
-	len = vscnprintf(buf, PAGE_SIZE, fmt, args);
+	len = vscnprintf(buf, PAGE_SIZE - offset, fmt, args);
 	va_end(args);
 
 	return len;
@@ -760,14 +762,16 @@ EXPORT_SYMBOL_GPL(sysfs_emit);
 int sysfs_emit_at(char *buf, int at, const char *fmt, ...)
 {
 	va_list args;
-	int len;
+	int len, offset;
+
+	offset = offset_in_page(buf);
 
-	if (WARN(!buf || offset_in_page(buf) || at < 0 || at >= PAGE_SIZE,
+	if (WARN(!buf || at < 0 || at + offset >= PAGE_SIZE,
 		 "invalid sysfs_emit_at: buf:%p at:%d\n", buf, at))
 		return 0;
 
 	va_start(args, fmt);
-	len = vscnprintf(buf + at, PAGE_SIZE - at, fmt, args);
+	len = vscnprintf(buf + at, PAGE_SIZE - at - offset, fmt, args);
 	va_end(args);
 
 	return len;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH] sysfs: Remove page boundary align limitation on sysfs_emit and sysfs_emit_at
  2021-09-08 12:07 [PATCH] sysfs: Remove page boundary align limitation on sysfs_emit and sysfs_emit_at Lang Yu
@ 2021-09-08 12:32 ` Greg Kroah-Hartman
  2021-09-08 12:52   ` Yu, Lang
  2021-09-09  5:05 ` Joe Perches
  1 sibling, 1 reply; 23+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-08 12:32 UTC (permalink / raw)
  To: Lang Yu; +Cc: Joe Perches, Rafael J . Wysocki, linux-kernel

On Wed, Sep 08, 2021 at 08:07:23PM +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.

That's not the only purpose.

So why are you changing this?

What in-kernel users are being tripped up by this, shouldn't we fix them
instead?

Remember, sysfs files are "one value per file", so why are the boundries
not properly set here?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 23+ messages in thread

* RE: [PATCH] sysfs: Remove page boundary align limitation on sysfs_emit and sysfs_emit_at
  2021-09-08 12:32 ` Greg Kroah-Hartman
@ 2021-09-08 12:52   ` Yu, Lang
  2021-09-08 13:04     ` Greg Kroah-Hartman
  0 siblings, 1 reply; 23+ messages in thread
From: Yu, Lang @ 2021-09-08 12:52 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Joe Perches, Rafael J . Wysocki, linux-kernel

[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?

Regards,
Lang  

>-----Original Message-----
>From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>Sent: Wednesday, September 8, 2021 8:32 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 Wed, Sep 08, 2021 at 08:07:23PM +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.
>
>That's not the only purpose.
>
>So why are you changing this?
>
>What in-kernel users are being tripped up by this, shouldn't we fix them instead?
>
>Remember, sysfs files are "one value per file", so why are the boundries not
>properly set here?
>
>thanks,
>
>greg k-h

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sysfs: Remove page boundary align limitation on sysfs_emit and sysfs_emit_at
  2021-09-08 12:52   ` Yu, Lang
@ 2021-09-08 13:04     ` Greg Kroah-Hartman
  2021-09-08 13:21       ` Yu, Lang
  0 siblings, 1 reply; 23+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-08 13:04 UTC (permalink / raw)
  To: Yu, Lang; +Cc: Joe Perches, Rafael J . Wysocki, linux-kernel

A: http://en.wikipedia.org/wiki/Top_post
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?

http://daringfireball.net/2007/07/on_top


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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* RE: [PATCH] sysfs: Remove page boundary align limitation on sysfs_emit and sysfs_emit_at
  2021-09-08 13:04     ` Greg Kroah-Hartman
@ 2021-09-08 13:21       ` Yu, Lang
  2021-09-08 13:49         ` Greg Kroah-Hartman
  0 siblings, 1 reply; 23+ messages in thread
From: Yu, Lang @ 2021-09-08 13:21 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Joe Perches, Rafael J . Wysocki, linux-kernel

[AMD Official Use Only]



>-----Original Message-----
>From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>Sent: Wednesday, September 8, 2021 9:04 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
>
>A:
>https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fen.wikipe
>dia.org%2Fwiki%2FTop_post&amp;data=04%7C01%7CLang.Yu%40amd.com%7C
>fed047de547541548fcc08d972c92627%7C3dd8961fe4884e608e11a82d994e183d
>%7C0%7C0%7C637667030534349355%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi
>MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&
>amp;sdata=LHujj041jxZjvoYxVYUKtNr7us%2FX4pl%2FdOkFSOP1W8U%3D&amp;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%2Fdaringfire
>ball.net%2F2007%2F07%2Fon_top&amp;data=04%7C01%7CLang.Yu%40amd.co
>m%7Cfed047de547541548fcc08d972c92627%7C3dd8961fe4884e608e11a82d99
>4e183d%7C0%7C0%7C637667030534349355%7CUnknown%7CTWFpbGZsb3d8ey
>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(),
and  recommend that use sysfs_emit() instead of scnprintf(),
and send many patches that replace  scnprintf() with sysfs_emit(),
and finally cause some invalid sysfs_emit_at: buf:00000000f19bdfde warnings.
 
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! 

Regards,
Lang

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sysfs: Remove page boundary align limitation on sysfs_emit and sysfs_emit_at
  2021-09-08 13:21       ` Yu, Lang
@ 2021-09-08 13:49         ` Greg Kroah-Hartman
  2021-09-08 15:33           ` Yu, Lang
  0 siblings, 1 reply; 23+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-08 13:49 UTC (permalink / raw)
  To: Yu, Lang; +Cc: Joe Perches, Rafael J . Wysocki, linux-kernel

On Wed, Sep 08, 2021 at 01:21:16PM +0000, Yu, Lang wrote:
> [AMD Official Use Only]
> 
> 
> 
> >-----Original Message-----
> >From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >Sent: Wednesday, September 8, 2021 9:04 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
> >
> >A:
> >https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fen.wikipe
> >dia.org%2Fwiki%2FTop_post&amp;data=04%7C01%7CLang.Yu%40amd.com%7C
> >fed047de547541548fcc08d972c92627%7C3dd8961fe4884e608e11a82d994e183d
> >%7C0%7C0%7C637667030534349355%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi
> >MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&
> >amp;sdata=LHujj041jxZjvoYxVYUKtNr7us%2FX4pl%2FdOkFSOP1W8U%3D&amp;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%2Fdaringfire
> >ball.net%2F2007%2F07%2Fon_top&amp;data=04%7C01%7CLang.Yu%40amd.co
> >m%7Cfed047de547541548fcc08d972c92627%7C3dd8961fe4884e608e11a82d99
> >4e183d%7C0%7C0%7C637667030534349355%7CUnknown%7CTWFpbGZsb3d8ey
> >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.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 23+ messages in thread

* RE: [PATCH] sysfs: Remove page boundary align limitation on sysfs_emit and sysfs_emit_at
  2021-09-08 13:49         ` Greg Kroah-Hartman
@ 2021-09-08 15:33           ` Yu, Lang
  2021-09-09  5:19             ` Greg Kroah-Hartman
  0 siblings, 1 reply; 23+ messages in thread
From: Yu, Lang @ 2021-09-08 15:33 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Joe Perches, Rafael J . Wysocki, linux-kernel

[AMD Official Use Only]



>-----Original Message-----
>From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>Sent: Wednesday, September 8, 2021 9:49 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 Wed, Sep 08, 2021 at 01:21:16PM +0000, Yu, Lang wrote:
>> [AMD Official Use Only]
>>
>>
>>
>> >-----Original Message-----
>> >From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> >Sent: Wednesday, September 8, 2021 9:04 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
>> >
>> >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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sysfs: Remove page boundary align limitation on sysfs_emit and sysfs_emit_at
  2021-09-08 12:07 [PATCH] sysfs: Remove page boundary align limitation on sysfs_emit and sysfs_emit_at Lang Yu
  2021-09-08 12:32 ` Greg Kroah-Hartman
@ 2021-09-09  5:05 ` Joe Perches
  2021-09-09  5:27   ` Yu, Lang
  1 sibling, 1 reply; 23+ messages in thread
From: Joe Perches @ 2021-09-09  5:05 UTC (permalink / raw)
  To: Lang Yu, Greg Kroah-Hartman, Rafael J . Wysocki, linux-kernel

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.




^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sysfs: Remove page boundary align limitation on sysfs_emit and sysfs_emit_at
  2021-09-08 15:33           ` Yu, Lang
@ 2021-09-09  5:19             ` Greg Kroah-Hartman
  2021-09-09  5:31               ` Yu, Lang
  0 siblings, 1 reply; 23+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-09  5:19 UTC (permalink / raw)
  To: Yu, Lang; +Cc: Joe Perches, Rafael J . Wysocki, linux-kernel

On Wed, Sep 08, 2021 at 03:33:51PM +0000, Yu, Lang wrote:
> >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.

Yes, and using that in sysfs functions is fine, there is nothing wrong
with this usage.

> But there are still some corner cases(e.g., a non page boundary aligned buf address : ).

I need a specific example of where this has gone wrong.  Please provide
a lore.kernel.org link as I fail to see the problem here.

Are you sure that you are not just abusing sysfs and having more than
one value per file?  Does this mean I need to go audit all of the gpu
sysfs file entries?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 23+ messages in thread

* RE: [PATCH] sysfs: Remove page boundary align limitation on sysfs_emit and sysfs_emit_at
  2021-09-09  5:05 ` Joe Perches
@ 2021-09-09  5:27   ` Yu, Lang
  2021-09-09  5:34     ` Greg Kroah-Hartman
  2021-09-09  5:44     ` Joe Perches
  0 siblings, 2 replies; 23+ messages in thread
From: Yu, Lang @ 2021-09-09  5:27 UTC (permalink / raw)
  To: Joe Perches, Greg Kroah-Hartman, Rafael J . Wysocki, linux-kernel

[AMD Official Use Only]



>-----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.
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? Thanks.

Regards,
Lang  






^ permalink raw reply	[flat|nested] 23+ messages in thread

* RE: [PATCH] sysfs: Remove page boundary align limitation on sysfs_emit and sysfs_emit_at
  2021-09-09  5:19             ` Greg Kroah-Hartman
@ 2021-09-09  5:31               ` Yu, Lang
  2021-09-09  6:05                 ` Greg Kroah-Hartman
  0 siblings, 1 reply; 23+ messages in thread
From: Yu, Lang @ 2021-09-09  5:31 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Joe Perches, Rafael J . Wysocki, linux-kernel

[AMD Official Use Only]



>-----Original Message-----
>From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>Sent: Thursday, September 9, 2021 1:19 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 Wed, Sep 08, 2021 at 03:33:51PM +0000, Yu, Lang wrote:
>> >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.
>
>Yes, and using that in sysfs functions is fine, there is nothing wrong with this
>usage.
>
>> But there are still some corner cases(e.g., a non page boundary aligned buf
>address : ).
>
>I need a specific example of where this has gone wrong.  Please provide a
>lore.kernel.org link as I fail to see the problem here.
>
>Are you sure that you are not just abusing sysfs and having more than one value
>per file?  Does this mean I need to go audit all of the gpu sysfs file entries?
>
Indeed, the one value per file rule was broken... Thanks.  

Regard,
Lang

>thanks,
>
>greg k-h

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sysfs: Remove page boundary align limitation on sysfs_emit and sysfs_emit_at
  2021-09-09  5:27   ` Yu, Lang
@ 2021-09-09  5:34     ` Greg Kroah-Hartman
  2021-09-09  5:59       ` Yu, Lang
  2021-09-09  5:44     ` Joe Perches
  1 sibling, 1 reply; 23+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-09  5:34 UTC (permalink / raw)
  To: Yu, Lang; +Cc: Joe Perches, Rafael J . Wysocki, linux-kernel

On Thu, Sep 09, 2021 at 05:27:49AM +0000, Yu, Lang wrote:
> [AMD Official Use Only]
> 
> 
> 
> >-----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.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sysfs: Remove page boundary align limitation on sysfs_emit and sysfs_emit_at
  2021-09-09  5:27   ` Yu, Lang
  2021-09-09  5:34     ` Greg Kroah-Hartman
@ 2021-09-09  5:44     ` Joe Perches
  2021-09-09  5:52       ` Yu, Lang
  1 sibling, 1 reply; 23+ messages in thread
From: Joe Perches @ 2021-09-09  5:44 UTC (permalink / raw)
  To: Yu, Lang, Greg Kroah-Hartman, Rafael J . Wysocki, linux-kernel

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.


> > -----Original Message-----
> > From: Joe Perches <joe@perches.com>
> > 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.



^ permalink raw reply	[flat|nested] 23+ messages in thread

* RE: [PATCH] sysfs: Remove page boundary align limitation on sysfs_emit and sysfs_emit_at
  2021-09-09  5:44     ` Joe Perches
@ 2021-09-09  5:52       ` Yu, Lang
  2021-09-09  6:07         ` Greg Kroah-Hartman
  0 siblings, 1 reply; 23+ messages in thread
From: Yu, Lang @ 2021-09-09  5:52 UTC (permalink / raw)
  To: Joe Perches, Greg Kroah-Hartman, Rafael J . Wysocki, linux-kernel

[Public]



>-----Original Message-----
>From: Joe Perches <joe@perches.com>
>Sent: Thursday, September 9, 2021 1:44 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 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@perches.com>
>> > 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. 

Regards,
Lang


^ permalink raw reply	[flat|nested] 23+ messages in thread

* RE: [PATCH] sysfs: Remove page boundary align limitation on sysfs_emit and sysfs_emit_at
  2021-09-09  5:34     ` Greg Kroah-Hartman
@ 2021-09-09  5:59       ` Yu, Lang
  2021-09-09  6:07         ` Greg Kroah-Hartman
  0 siblings, 1 reply; 23+ messages in thread
From: Yu, Lang @ 2021-09-09  5:59 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Joe Perches, Rafael J . Wysocki, linux-kernel

[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.

Regards,
Lang

>thanks,
>
>greg k-h

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sysfs: Remove page boundary align limitation on sysfs_emit and sysfs_emit_at
  2021-09-09  5:31               ` Yu, Lang
@ 2021-09-09  6:05                 ` Greg Kroah-Hartman
  0 siblings, 0 replies; 23+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-09  6:05 UTC (permalink / raw)
  To: Yu, Lang; +Cc: Joe Perches, Rafael J . Wysocki, linux-kernel

On Thu, Sep 09, 2021 at 05:31:40AM +0000, Yu, Lang wrote:
> [AMD Official Use Only]
> 
> 
> 
> >-----Original Message-----
> >From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >Sent: Thursday, September 9, 2021 1:19 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 Wed, Sep 08, 2021 at 03:33:51PM +0000, Yu, Lang wrote:
> >> >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.
> >
> >Yes, and using that in sysfs functions is fine, there is nothing wrong with this
> >usage.
> >
> >> But there are still some corner cases(e.g., a non page boundary aligned buf
> >address : ).
> >
> >I need a specific example of where this has gone wrong.  Please provide a
> >lore.kernel.org link as I fail to see the problem here.
> >
> >Are you sure that you are not just abusing sysfs and having more than one value
> >per file?  Does this mean I need to go audit all of the gpu sysfs file entries?
> >
> Indeed, the one value per file rule was broken... Thanks.  

Great, which file is that so we can fix it!

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sysfs: Remove page boundary align limitation on sysfs_emit and sysfs_emit_at
  2021-09-09  5:59       ` Yu, Lang
@ 2021-09-09  6:07         ` Greg Kroah-Hartman
  0 siblings, 0 replies; 23+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-09  6:07 UTC (permalink / raw)
  To: Yu, Lang; +Cc: Joe Perches, Rafael J . Wysocki, linux-kernel

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sysfs: Remove page boundary align limitation on sysfs_emit and sysfs_emit_at
  2021-09-09  5:52       ` Yu, Lang
@ 2021-09-09  6:07         ` Greg Kroah-Hartman
  2021-09-09  6:22           ` Yu, Lang
  0 siblings, 1 reply; 23+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-09  6:07 UTC (permalink / raw)
  To: Yu, Lang; +Cc: Joe Perches, Rafael J . Wysocki, linux-kernel

On Thu, Sep 09, 2021 at 05:52:23AM +0000, Yu, Lang wrote:
> [Public]
> 
> 
> 
> >-----Original Message-----
> >From: Joe Perches <joe@perches.com>
> >Sent: Thursday, September 9, 2021 1:44 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 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@perches.com>
> >> > 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?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 23+ messages in thread

* RE: [PATCH] sysfs: Remove page boundary align limitation on sysfs_emit and sysfs_emit_at
  2021-09-09  6:07         ` Greg Kroah-Hartman
@ 2021-09-09  6:22           ` Yu, Lang
  2021-09-09  6:35             ` Greg Kroah-Hartman
  0 siblings, 1 reply; 23+ messages in thread
From: Yu, Lang @ 2021-09-09  6:22 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Joe Perches, Rafael J . Wysocki, linux-kernel

[Public]



>-----Original Message-----
>From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>Sent: Thursday, September 9, 2021 2:08 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:52:23AM +0000, Yu, Lang wrote:
>> [Public]
>>
>>
>>
>> >-----Original Message-----
>> >From: Joe Perches <joe@perches.com>
>> >Sent: Thursday, September 9, 2021 1:44 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 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@perches.com> 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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sysfs: Remove page boundary align limitation on sysfs_emit and sysfs_emit_at
  2021-09-09  6:22           ` Yu, Lang
@ 2021-09-09  6:35             ` Greg Kroah-Hartman
  2021-09-09  7:48               ` Yu, Lang
  0 siblings, 1 reply; 23+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-09  6:35 UTC (permalink / raw)
  To: Yu, Lang; +Cc: Joe Perches, Rafael J . Wysocki, linux-kernel

On Thu, Sep 09, 2021 at 06:22:54AM +0000, Yu, Lang wrote:
> [Public]
> 
> 
> 
> >-----Original Message-----
> >From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >Sent: Thursday, September 9, 2021 2:08 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:52:23AM +0000, Yu, Lang wrote:
> >> [Public]
> >>
> >>
> >>
> >> >-----Original Message-----
> >> >From: Joe Perches <joe@perches.com>
> >> >Sent: Thursday, September 9, 2021 1:44 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 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@perches.com> 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.

Great, please do.

> Now that you introduced them, why not make them more flexible like scnprintf family functions.

Because that is not what they are for.

> The page boundary align rule makes life hard and I don't like it : ). Many thanks for your explanations!

Then fix your sysfs files to not violate the sysfs rules.

Again, which files are having problems and need to be fixed?  I will be
glad to do this for you.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 23+ messages in thread

* RE: [PATCH] sysfs: Remove page boundary align limitation on sysfs_emit and sysfs_emit_at
  2021-09-09  6:35             ` Greg Kroah-Hartman
@ 2021-09-09  7:48               ` Yu, Lang
  2021-09-09  7:59                 ` Greg Kroah-Hartman
  0 siblings, 1 reply; 23+ messages in thread
From: Yu, Lang @ 2021-09-09  7:48 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Joe Perches, Rafael J . Wysocki, linux-kernel

[Public]



>-----Original Message-----
>From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>Sent: Thursday, September 9, 2021 2:36 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 06:22:54AM +0000, Yu, Lang wrote:
>> [Public]
>>
>>
>>
>> >-----Original Message-----
>> >From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> >Sent: Thursday, September 9, 2021 2:08 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:52:23AM +0000, Yu, Lang wrote:
>> >> [Public]
>> >>
>> >>
>> >>
>> >> >-----Original Message-----
>> >> >From: Joe Perches <joe@perches.com>
>> >> >Sent: Thursday, September 9, 2021 1:44 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 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@perches.com> 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.
>
>Great, please do.
>
>> Now that you introduced them, why not make them more flexible like scnprintf
>family functions.
>
>Because that is not what they are for.
>
>> The page boundary align rule makes life hard and I don't like it : ). Many thanks
>for your explanations!
>
>Then fix your sysfs files to not violate the sysfs rules.
>
>Again, which files are having problems and need to be fixed?  I will be glad to do
>this for you.

Thanks. I can do it by myself instead of wasting your time... Many thanks!

Regards,
Lang

>thanks,
>
>greg k-h

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sysfs: Remove page boundary align limitation on sysfs_emit and sysfs_emit_at
  2021-09-09  7:48               ` Yu, Lang
@ 2021-09-09  7:59                 ` Greg Kroah-Hartman
  2021-09-09  8:48                   ` Yu, Lang
  0 siblings, 1 reply; 23+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-09  7:59 UTC (permalink / raw)
  To: Yu, Lang; +Cc: Joe Perches, Rafael J . Wysocki, linux-kernel

On Thu, Sep 09, 2021 at 07:48:38AM +0000, Yu, Lang wrote:
> [Public]
> 
> 
> 
> >-----Original Message-----
> >From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >Sent: Thursday, September 9, 2021 2:36 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 06:22:54AM +0000, Yu, Lang wrote:
> >> [Public]
> >>
> >>
> >>
> >> >-----Original Message-----
> >> >From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> >Sent: Thursday, September 9, 2021 2:08 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:52:23AM +0000, Yu, Lang wrote:
> >> >> [Public]
> >> >>
> >> >>
> >> >>
> >> >> >-----Original Message-----
> >> >> >From: Joe Perches <joe@perches.com>
> >> >> >Sent: Thursday, September 9, 2021 1:44 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 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@perches.com> 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.
> >
> >Great, please do.
> >
> >> Now that you introduced them, why not make them more flexible like scnprintf
> >family functions.
> >
> >Because that is not what they are for.
> >
> >> The page boundary align rule makes life hard and I don't like it : ). Many thanks
> >for your explanations!
> >
> >Then fix your sysfs files to not violate the sysfs rules.
> >
> >Again, which files are having problems and need to be fixed?  I will be glad to do
> >this for you.
> 
> Thanks. I can do it by myself instead of wasting your time... Many thanks!

When doing so, please switch to using DEVICE_ATTR_RO() instead of the
"open coded" DEVICE_ATTR() usage in the driver.  That way we all "know"
that these are read-only attributes.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 23+ messages in thread

* RE: [PATCH] sysfs: Remove page boundary align limitation on sysfs_emit and sysfs_emit_at
  2021-09-09  7:59                 ` Greg Kroah-Hartman
@ 2021-09-09  8:48                   ` Yu, Lang
  0 siblings, 0 replies; 23+ messages in thread
From: Yu, Lang @ 2021-09-09  8:48 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Joe Perches, Rafael J . Wysocki, linux-kernel

[Public]



>-----Original Message-----
>From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>Sent: Thursday, September 9, 2021 4:00 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 07:48:38AM +0000, Yu, Lang wrote:
>> [Public]
>>
>>
>>
>> >-----Original Message-----
>> >From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> >Sent: Thursday, September 9, 2021 2:36 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 06:22:54AM +0000, Yu, Lang wrote:
>> >> [Public]
>> >>
>> >>
>> >>
>> >> >-----Original Message-----
>> >> >From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> >> >Sent: Thursday, September 9, 2021 2:08 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:52:23AM +0000, Yu, Lang wrote:
>> >> >> [Public]
>> >> >>
>> >> >>
>> >> >>
>> >> >> >-----Original Message-----
>> >> >> >From: Joe Perches <joe@perches.com>
>> >> >> >Sent: Thursday, September 9, 2021 1:44 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 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@perches.com> 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.
>> >
>> >Great, please do.
>> >
>> >> Now that you introduced them, why not make them more flexible like
>> >> scnprintf
>> >family functions.
>> >
>> >Because that is not what they are for.
>> >
>> >> The page boundary align rule makes life hard and I don't like it :
>> >> ). Many thanks
>> >for your explanations!
>> >
>> >Then fix your sysfs files to not violate the sysfs rules.
>> >
>> >Again, which files are having problems and need to be fixed?  I will
>> >be glad to do this for you.
>>
>> Thanks. I can do it by myself instead of wasting your time... Many thanks!
>
>When doing so, please switch to using DEVICE_ATTR_RO() instead of the "open
>coded" DEVICE_ATTR() usage in the driver.  That way we all "know"
>that these are read-only attributes.

Many thanks for you reminders! Have a nice day!

Regards,
Lang
>
>thanks,
>
>greg k-h

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2021-09-09  8:48 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-08 12:07 [PATCH] sysfs: Remove page boundary align limitation on sysfs_emit and sysfs_emit_at Lang Yu
2021-09-08 12:32 ` Greg Kroah-Hartman
2021-09-08 12:52   ` Yu, Lang
2021-09-08 13:04     ` Greg Kroah-Hartman
2021-09-08 13:21       ` Yu, Lang
2021-09-08 13:49         ` Greg Kroah-Hartman
2021-09-08 15:33           ` Yu, Lang
2021-09-09  5:19             ` Greg Kroah-Hartman
2021-09-09  5:31               ` Yu, Lang
2021-09-09  6:05                 ` Greg Kroah-Hartman
2021-09-09  5:05 ` Joe Perches
2021-09-09  5:27   ` Yu, Lang
2021-09-09  5:34     ` Greg Kroah-Hartman
2021-09-09  5:59       ` Yu, Lang
2021-09-09  6:07         ` Greg Kroah-Hartman
2021-09-09  5:44     ` Joe Perches
2021-09-09  5:52       ` Yu, Lang
2021-09-09  6:07         ` Greg Kroah-Hartman
2021-09-09  6:22           ` Yu, Lang
2021-09-09  6:35             ` Greg Kroah-Hartman
2021-09-09  7:48               ` Yu, Lang
2021-09-09  7:59                 ` Greg Kroah-Hartman
2021-09-09  8:48                   ` Yu, Lang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.