[PATCH] drivers: tty: replace snprintf in show functions with sysfs_emit

[PATCH] drivers: tty: replace snprintf in show functions with sysfs_emit

[cgel.zte]

From: Jing Yao <yao.jing2@zte.com.cn>

Reported-by: Zeal Robot <zealci@zte.com.cn>
Signed-off-by: Jing Yao <yao.jing2@zte.com.cn>
---
 drivers/tty/serial/8250/8250_port.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 5775cbff8f6e..557e8b13b5c1 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -3099,7 +3099,7 @@ static ssize_t rx_trig_bytes_show(struct device *dev,
 	if (rxtrig_bytes < 0)
 		return rxtrig_bytes;
 
-	return snprintf(buf, PAGE_SIZE, "%d\n", rxtrig_bytes);
+	return sysfs_emit(buf, PAGE_SIZE, "%d\n", rxtrig_bytes);
 }
 
 static int do_set_rxtrig(struct tty_port *port, unsigned char bytes)
-- 
2.25.1

Re: [PATCH] drivers: tty: replace snprintf in show functions with sysfs_emit

[Johan Hovold]

On Thu, Oct 28, 2021 at 10:13:50AM +0000, cgel.zte@gmail.com wrote:
> From: Jing Yao <yao.jing2@zte.com.cn>

Where's the commit message?

Also, look at the log for the driver you're changing for the Subject
prefix you should use. Including "drivers:" is never right.

> Reported-by: Zeal Robot <zealci@zte.com.cn>
> Signed-off-by: Jing Yao <yao.jing2@zte.com.cn>
> ---
>  drivers/tty/serial/8250/8250_port.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 5775cbff8f6e..557e8b13b5c1 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -3099,7 +3099,7 @@ static ssize_t rx_trig_bytes_show(struct device *dev,
>  	if (rxtrig_bytes < 0)
>  		return rxtrig_bytes;
>  
> -	return snprintf(buf, PAGE_SIZE, "%d\n", rxtrig_bytes);
> +	return sysfs_emit(buf, PAGE_SIZE, "%d\n", rxtrig_bytes);

FFS... This would not even compile, at least not without a warning, as
it's completely broken.

You do know that you have to at least compile-test your patches, right?

>  }
>  
>  static int do_set_rxtrig(struct tty_port *port, unsigned char bytes)

Johan

Re: [PATCH] drivers: tty: replace snprintf in show functions with sysfs_emit

[Maciej W. Rozycki]

On Thu, 28 Oct 2021, Johan Hovold wrote:

> Where's the commit message?

 Or to put it explicitly: please always include a full description of any 
change submitted, where you explain what your change is needed for and why 
you have made it like this.

 Please refer to Documentation/process/submitting-patches.rst in the 
source tree for a full recipe for submitting patches, which says, among 
others:

"[...] The text should be written in such detail so that when read
weeks, months or even years later, it can give the reader the needed
details to grasp the reasoning for **why** the patch was created."

 Thanks,

  Maciej

Re: [PATCH] drivers: tty: replace snprintf in show functions with sysfs_emit

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3641 bytes --]

Hi,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on tty/tty-testing]
[also build test WARNING on staging/staging-testing v5.15-rc7 next-20211028]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/cgel-zte-gmail-com/drivers-tty-replace-snprintf-in-show-functions-with-sysfs_emit/20211028-181624
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: ia64-defconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/03b103ac2507a7958ac9e45fe6a53110e1140625
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review cgel-zte-gmail-com/drivers-tty-replace-snprintf-in-show-functions-with-sysfs_emit/20211028-181624
        git checkout 03b103ac2507a7958ac9e45fe6a53110e1140625
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=ia64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from arch/ia64/include/asm/ptrace.h:46,
                    from arch/ia64/include/asm/processor.h:20,
                    from arch/ia64/include/asm/timex.h:15,
                    from include/linux/timex.h:65,
                    from include/linux/time32.h:13,
                    from include/linux/time.h:60,
                    from include/linux/stat.h:19,
                    from include/linux/module.h:13,
                    from drivers/tty/serial/8250/8250_port.c:14:
   drivers/tty/serial/8250/8250_port.c: In function 'rx_trig_bytes_show':
>> arch/ia64/include/asm/page.h:42:33: warning: passing argument 2 of 'sysfs_emit' makes pointer from integer without a cast [-Wint-conversion]
      42 | #define PAGE_SIZE               (__IA64_UL_CONST(1) << PAGE_SHIFT)
         |                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         |                                 |
         |                                 long unsigned int
   drivers/tty/serial/8250/8250_port.c:3102:32: note: in expansion of macro 'PAGE_SIZE'
    3102 |         return sysfs_emit(buf, PAGE_SIZE, "%d\n", rxtrig_bytes);
         |                                ^~~~~~~~~
   In file included from include/linux/kobject.h:20,
                    from include/linux/module.h:21,
                    from drivers/tty/serial/8250/8250_port.c:14:
   include/linux/sysfs.h:341:39: note: expected 'const char *' but argument is of type 'long unsigned int'
     341 | int sysfs_emit(char *buf, const char *fmt, ...);
         |                           ~~~~~~~~~~~~^~~


vim +/sysfs_emit +42 arch/ia64/include/asm/page.h

^1da177e4c3f415 include/asm-ia64/page.h Linus Torvalds 2005-04-16  41  
^1da177e4c3f415 include/asm-ia64/page.h Linus Torvalds 2005-04-16 @42  #define PAGE_SIZE		(__IA64_UL_CONST(1) << PAGE_SHIFT)
^1da177e4c3f415 include/asm-ia64/page.h Linus Torvalds 2005-04-16  43  #define PAGE_MASK		(~(PAGE_SIZE - 1))
^1da177e4c3f415 include/asm-ia64/page.h Linus Torvalds 2005-04-16  44  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 19957 bytes --]

Re: [PATCH] drivers: tty: replace snprintf in show functions with sysfs_emit

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3535 bytes --]

Hi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tty/tty-testing]
[also build test ERROR on staging/staging-testing v5.15-rc7 next-20211028]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/cgel-zte-gmail-com/drivers-tty-replace-snprintf-in-show-functions-with-sysfs_emit/20211028-181624
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: microblaze-buildonly-randconfig-r002-20211028 (attached as .config)
compiler: microblaze-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/03b103ac2507a7958ac9e45fe6a53110e1140625
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review cgel-zte-gmail-com/drivers-tty-replace-snprintf-in-show-functions-with-sysfs_emit/20211028-181624
        git checkout 03b103ac2507a7958ac9e45fe6a53110e1140625
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=microblaze 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/mm_types_task.h:16,
                    from include/linux/mm_types.h:5,
                    from include/linux/buildid.h:5,
                    from include/linux/module.h:14,
                    from drivers/tty/serial/8250/8250_port.c:14:
   drivers/tty/serial/8250/8250_port.c: In function 'rx_trig_bytes_show':
>> arch/microblaze/include/asm/page.h:24:25: error: passing argument 2 of 'sysfs_emit' makes pointer from integer without a cast [-Werror=int-conversion]
      24 | #define PAGE_SIZE       (ASM_CONST(1) << PAGE_SHIFT)
         |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
         |                         |
         |                         long unsigned int
   drivers/tty/serial/8250/8250_port.c:3102:32: note: in expansion of macro 'PAGE_SIZE'
    3102 |         return sysfs_emit(buf, PAGE_SIZE, "%d\n", rxtrig_bytes);
         |                                ^~~~~~~~~
   In file included from include/linux/kobject.h:20,
                    from include/linux/module.h:21,
                    from drivers/tty/serial/8250/8250_port.c:14:
   include/linux/sysfs.h:341:39: note: expected 'const char *' but argument is of type 'long unsigned int'
     341 | int sysfs_emit(char *buf, const char *fmt, ...);
         |                           ~~~~~~~~~~~~^~~
   cc1: all warnings being treated as errors


vim +/sysfs_emit +24 arch/microblaze/include/asm/page.h

4b87d7a4f91d31 Michal Simek      2009-03-27  21  
4b87d7a4f91d31 Michal Simek      2009-03-27  22  /* PAGE_SHIFT determines the page size */
ba9c4f88d74783 Steven J. Magnani 2010-05-13  23  #define PAGE_SHIFT		12
ba9c4f88d74783 Steven J. Magnani 2010-05-13 @24  #define PAGE_SIZE	(ASM_CONST(1) << PAGE_SHIFT)
4b87d7a4f91d31 Michal Simek      2009-03-27  25  #define PAGE_MASK	(~(PAGE_SIZE-1))
4b87d7a4f91d31 Michal Simek      2009-03-27  26  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34791 bytes --]

Re: [PATCH] drivers: tty: replace snprintf in show functions with sysfs_emit

[Greg KH]

On Thu, Oct 28, 2021 at 10:13:50AM +0000, cgel.zte@gmail.com wrote:
> From: Jing Yao <yao.jing2@zte.com.cn>
> 
> Reported-by: Zeal Robot <zealci@zte.com.cn>

Please fix your broken "robot" before submitting any more patches to the
Linux kernel project.

good luck!

greg k-h

Re: [PATCH] drivers: tty: replace snprintf in show functions with sysfs_emit

[Joe Perches]

On Fri, 2021-10-29 at 07:44 +0200, Greg KH wrote:
> On Thu, Oct 28, 2021 at 10:13:50AM +0000, cgel.zte@gmail.com wrote:
> > From: Jing Yao <yao.jing2@zte.com.cn>
> > 
> > Reported-by: Zeal Robot <zealci@zte.com.cn>
> 
> Please fix your broken "robot" before submitting any more patches to the
> Linux kernel project.

There's no indication the robot and the patch author/submitter are
the same.

I think the "robot" _reporting_ the issue is reasonable, but not the
patch author/submitter.

Though whatever the "Zeal Robot" is should likely be public otherwise
the Reported-by: reference isn't useful.

[PATCH v2] drivers: tty: replace snprintf in show functions with sysfs_emit

[cgel.zte]

From: Jing Yao <yao.jing2@zte.com.cn>

coccicheck complains about the use of snprintf() in sysfs show
functions:
WARNING use scnprintf or sprintf

Use sysfs_emit instead of scnprintf or sprintf makes more sense.

Reported-by: Zeal Robot <zealci@zte.com.cn>
Signed-off-by: Jing Yao <yao.jing2@zte.com.cn>
---
 drivers/tty/serial/8250/8250_port.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 5775cbff8f6e..3d58f383152e 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -3099,7 +3099,7 @@ static ssize_t rx_trig_bytes_show(struct device *dev,
 	if (rxtrig_bytes < 0)
 		return rxtrig_bytes;
 
-	return snprintf(buf, PAGE_SIZE, "%d\n", rxtrig_bytes);
+	return sysfs_emit(buf, "%d\n", rxtrig_bytes);
 }
 
 static int do_set_rxtrig(struct tty_port *port, unsigned char bytes)
-- 
2.25.1

Re: [PATCH v2] drivers: tty: replace snprintf in show functions with sysfs_emit

[Johan Hovold]

On Tue, Nov 02, 2021 at 06:52:06AM +0000, cgel.zte@gmail.com wrote:
> From: Jing Yao <yao.jing2@zte.com.cn>
> 
> coccicheck complains about the use of snprintf() in sysfs show
> functions:
> WARNING use scnprintf or sprintf
> 
> Use sysfs_emit instead of scnprintf or sprintf makes more sense.
> 
> Reported-by: Zeal Robot <zealci@zte.com.cn>
> Signed-off-by: Jing Yao <yao.jing2@zte.com.cn>
> ---

What changed since v1? Always include a changelog here.

Also you ignored my comment on the patch Subject. Including "drivers: "
is never correct. You should also mention which driver you're changing
since it's not done subtree-wide for tty. 

Again, look at the git log for the driver you're changing. In this case
it should have been something like:

	serial: 8250: use sysfs_emit...

>  drivers/tty/serial/8250/8250_port.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 5775cbff8f6e..3d58f383152e 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -3099,7 +3099,7 @@ static ssize_t rx_trig_bytes_show(struct device *dev,
>  	if (rxtrig_bytes < 0)
>  		return rxtrig_bytes;
>  
> -	return snprintf(buf, PAGE_SIZE, "%d\n", rxtrig_bytes);
> +	return sysfs_emit(buf, "%d\n", rxtrig_bytes);
>  }
>  
>  static int do_set_rxtrig(struct tty_port *port, unsigned char bytes)

Johan

[PATCH v3] serial: 8250: replace snprintf in show functions with sysfs_emit

[cgel.zte]

From: Jing Yao <yao.jing2@zte.com.cn>

coccicheck complains about the use of snprintf() in sysfs show
functions:
WARNING use scnprintf or sprintf

Use sysfs_emit instead of scnprintf or sprintf makes more sense.

Reported-by: Zeal Robot <zealci@zte.com.cn>
Signed-off-by: Jing Yao <yao.jing2@zte.com.cn>
---

Changes since v1 & v2:
 - Remove excess and wrong parameter 'PAGE_SIZE' in sysfs_emit function.
 - Revise the wrong patch Subject.

 drivers/tty/serial/8250/8250_port.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 5775cbff8f6e..3d58f383152e 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -3099,7 +3099,7 @@ static ssize_t rx_trig_bytes_show(struct device *dev,
 	if (rxtrig_bytes < 0)
 		return rxtrig_bytes;
 
-	return snprintf(buf, PAGE_SIZE, "%d\n", rxtrig_bytes);
+	return sysfs_emit(buf, "%d\n", rxtrig_bytes);
 }
 
 static int do_set_rxtrig(struct tty_port *port, unsigned char bytes)
-- 
2.25.1

Re: [PATCH v3] serial: 8250: replace snprintf in show functions with sysfs_emit

[Johan Hovold]

On Thu, Nov 04, 2021 at 11:47:54AM +0000, cgel.zte@gmail.com wrote:
> From: Jing Yao <yao.jing2@zte.com.cn>
> 
> coccicheck complains about the use of snprintf() in sysfs show
> functions:
> WARNING use scnprintf or sprintf
> 
> Use sysfs_emit instead of scnprintf or sprintf makes more sense.
> 
> Reported-by: Zeal Robot <zealci@zte.com.cn>
> Signed-off-by: Jing Yao <yao.jing2@zte.com.cn>
> ---
> 
> Changes since v1 & v2:
>  - Remove excess and wrong parameter 'PAGE_SIZE' in sysfs_emit function.
>  - Revise the wrong patch Subject.

Looks like there are a few more cases in 

	drivers/tty/serial/8250/8250_aspeed_vuart.c

which you could convert in a follow up patch.

>  drivers/tty/serial/8250/8250_port.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 5775cbff8f6e..3d58f383152e 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -3099,7 +3099,7 @@ static ssize_t rx_trig_bytes_show(struct device *dev,
>  	if (rxtrig_bytes < 0)
>  		return rxtrig_bytes;
>  
> -	return snprintf(buf, PAGE_SIZE, "%d\n", rxtrig_bytes);
> +	return sysfs_emit(buf, "%d\n", rxtrig_bytes);
>  }
>  
>  static int do_set_rxtrig(struct tty_port *port, unsigned char bytes)

This one looks good as is now:

Reviewed-by: Johan Hovold <johan@kernel.org>

Johan