[PATCH net-next v2 0/2] net/ncsi: Use str_up_down to simplify the code

[PATCH net-next v2 0/2] net/ncsi: Use str_up_down to simplify the code

[Hongbo Li]

In commit a98ae7f045b2, str_up_down() helper is introduced to
return "up" or "down" string literal, so we can use it to
simplify the code and fix the coccinelle warning.

v2:
 - change subject into net-next

v1: https://lore.kernel.org/netdev/20240823162144.GW2164@kernel.org/T/

Hongbo Li (2):
  net/ncsi: Use str_up_down to simplify the code
  net/ncsi: Use str_up_down to simplify the code

 net/ncsi/ncsi-aen.c    | 2 +-
 net/ncsi/ncsi-manage.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH net-next v2 1/2] net/ncsi: Use str_up_down to simplify the code

[Hongbo Li]

As str_up_down is the common helper to reback "up/down"
string, we can replace the target with it to simplify
the code and fix the coccinelle warning.

Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
---
 net/ncsi/ncsi-manage.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 5ecf611c8820..13b4d393fb2d 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -1281,7 +1281,7 @@ static int ncsi_choose_active_channel(struct ncsi_dev_priv *ndp)
 				netdev_dbg(ndp->ndev.dev,
 					   "NCSI: Channel %u added to queue (link %s)\n",
 					   nc->id,
-					   ncm->data[2] & 0x1 ? "up" : "down");
+					   str_up_down(ncm->data[2] & 0x1));
 			}
 
 			spin_unlock_irqrestore(&nc->lock, cflags);
-- 
2.34.1

Re: [PATCH net-next v2 1/2] net/ncsi: Use str_up_down to simplify the code

[Michal Swiatkowski]

On Tue, Aug 27, 2024 at 10:52:45AM +0800, Hongbo Li wrote:
> As str_up_down is the common helper to reback "up/down"
> string, we can replace the target with it to simplify
> the code and fix the coccinelle warning.
> 
> Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
> ---
>  net/ncsi/ncsi-manage.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index 5ecf611c8820..13b4d393fb2d 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c
> @@ -1281,7 +1281,7 @@ static int ncsi_choose_active_channel(struct ncsi_dev_priv *ndp)
>  				netdev_dbg(ndp->ndev.dev,
>  					   "NCSI: Channel %u added to queue (link %s)\n",
>  					   nc->id,
> -					   ncm->data[2] & 0x1 ? "up" : "down");
> +					   str_up_down(ncm->data[2] & 0x1));
Are you basing the commit on sth else than net-next? I can't see such
helper in the repo.

>  			}
>  
>  			spin_unlock_irqrestore(&nc->lock, cflags);
> -- 
> 2.34.1
> 

You have the same commit message in both patches, they can be merged
into one I think.

Thanks,
Michal

Re: [PATCH net-next v2 1/2] net/ncsi: Use str_up_down to simplify the code

[kernel test robot]

Hi Hongbo,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Hongbo-Li/net-ncsi-Use-str_up_down-to-simplify-the-code/20240827-104622
base:   net-next/main
patch link:    https://lore.kernel.org/r/20240827025246.963115-2-lihongbo22%40huawei.com
patch subject: [PATCH net-next v2 1/2] net/ncsi: Use str_up_down to simplify the code
config: arc-randconfig-001-20240827 (https://download.01.org/0day-ci/archive/20240828/202408280039.0XTdLUnn-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240828/202408280039.0XTdLUnn-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408280039.0XTdLUnn-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/skbuff.h:38,
                    from include/net/net_namespace.h:43,
                    from include/linux/netdevice.h:38,
                    from net/ncsi/ncsi-manage.c:9:
   net/ncsi/ncsi-manage.c: In function 'ncsi_choose_active_channel':
>> net/ncsi/ncsi-manage.c:1284:44: error: implicit declaration of function 'str_up_down' [-Werror=implicit-function-declaration]
    1284 |                                            str_up_down(ncm->data[2] & 0x1));
         |                                            ^~~~~~~~~~~
   include/net/net_debug.h:66:60: note: in definition of macro 'netdev_dbg'
      66 |                 netdev_printk(KERN_DEBUG, __dev, format, ##args); \
         |                                                            ^~~~
>> net/ncsi/ncsi-manage.c:1282:44: warning: format '%s' expects argument of type 'char *', but argument 5 has type 'int' [-Wformat=]
    1282 |                                            "NCSI: Channel %u added to queue (link %s)\n",
         |                                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    1283 |                                            nc->id,
    1284 |                                            str_up_down(ncm->data[2] & 0x1));
         |                                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         |                                            |
         |                                            int
   include/net/net_debug.h:66:50: note: in definition of macro 'netdev_dbg'
      66 |                 netdev_printk(KERN_DEBUG, __dev, format, ##args); \
         |                                                  ^~~~~~
   net/ncsi/ncsi-manage.c:1282:84: note: format string is defined here
    1282 |                                            "NCSI: Channel %u added to queue (link %s)\n",
         |                                                                                   ~^
         |                                                                                    |
         |                                                                                    char *
         |                                                                                   %d
   cc1: some warnings being treated as errors


vim +/str_up_down +1284 net/ncsi/ncsi-manage.c

  1224	
  1225	static int ncsi_choose_active_channel(struct ncsi_dev_priv *ndp)
  1226	{
  1227		struct ncsi_channel *nc, *found, *hot_nc;
  1228		struct ncsi_channel_mode *ncm;
  1229		unsigned long flags, cflags;
  1230		struct ncsi_package *np;
  1231		bool with_link;
  1232	
  1233		spin_lock_irqsave(&ndp->lock, flags);
  1234		hot_nc = ndp->hot_channel;
  1235		spin_unlock_irqrestore(&ndp->lock, flags);
  1236	
  1237		/* By default the search is done once an inactive channel with up
  1238		 * link is found, unless a preferred channel is set.
  1239		 * If multi_package or multi_channel are configured all channels in the
  1240		 * whitelist are added to the channel queue.
  1241		 */
  1242		found = NULL;
  1243		with_link = false;
  1244		NCSI_FOR_EACH_PACKAGE(ndp, np) {
  1245			if (!(ndp->package_whitelist & (0x1 << np->id)))
  1246				continue;
  1247			NCSI_FOR_EACH_CHANNEL(np, nc) {
  1248				if (!(np->channel_whitelist & (0x1 << nc->id)))
  1249					continue;
  1250	
  1251				spin_lock_irqsave(&nc->lock, cflags);
  1252	
  1253				if (!list_empty(&nc->link) ||
  1254				    nc->state != NCSI_CHANNEL_INACTIVE) {
  1255					spin_unlock_irqrestore(&nc->lock, cflags);
  1256					continue;
  1257				}
  1258	
  1259				if (!found)
  1260					found = nc;
  1261	
  1262				if (nc == hot_nc)
  1263					found = nc;
  1264	
  1265				ncm = &nc->modes[NCSI_MODE_LINK];
  1266				if (ncm->data[2] & 0x1) {
  1267					found = nc;
  1268					with_link = true;
  1269				}
  1270	
  1271				/* If multi_channel is enabled configure all valid
  1272				 * channels whether or not they currently have link
  1273				 * so they will have AENs enabled.
  1274				 */
  1275				if (with_link || np->multi_channel) {
  1276					spin_lock_irqsave(&ndp->lock, flags);
  1277					list_add_tail_rcu(&nc->link,
  1278							  &ndp->channel_queue);
  1279					spin_unlock_irqrestore(&ndp->lock, flags);
  1280	
  1281					netdev_dbg(ndp->ndev.dev,
> 1282						   "NCSI: Channel %u added to queue (link %s)\n",
  1283						   nc->id,
> 1284						   str_up_down(ncm->data[2] & 0x1));
  1285				}
  1286	
  1287				spin_unlock_irqrestore(&nc->lock, cflags);
  1288	
  1289				if (with_link && !np->multi_channel)
  1290					break;
  1291			}
  1292			if (with_link && !ndp->multi_package)
  1293				break;
  1294		}
  1295	
  1296		if (list_empty(&ndp->channel_queue) && found) {
  1297			netdev_info(ndp->ndev.dev,
  1298				    "NCSI: No channel with link found, configuring channel %u\n",
  1299				    found->id);
  1300			spin_lock_irqsave(&ndp->lock, flags);
  1301			list_add_tail_rcu(&found->link, &ndp->channel_queue);
  1302			spin_unlock_irqrestore(&ndp->lock, flags);
  1303		} else if (!found) {
  1304			netdev_warn(ndp->ndev.dev,
  1305				    "NCSI: No channel found to configure!\n");
  1306			ncsi_report_link(ndp, true);
  1307			return -ENODEV;
  1308		}
  1309	
  1310		return ncsi_process_next_channel(ndp);
  1311	}
  1312	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next v2 1/2] net/ncsi: Use str_up_down to simplify the code

[kernel test robot]

Hi Hongbo,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Hongbo-Li/net-ncsi-Use-str_up_down-to-simplify-the-code/20240827-104622
base:   net-next/main
patch link:    https://lore.kernel.org/r/20240827025246.963115-2-lihongbo22%40huawei.com
patch subject: [PATCH net-next v2 1/2] net/ncsi: Use str_up_down to simplify the code
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20240828/202408280146.axPqeLlk-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 08e5a1de8227512d4774a534b91cb2353cef6284)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240828/202408280146.axPqeLlk-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408280146.axPqeLlk-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   In file included from net/ncsi/ncsi-manage.c:6:
   In file included from include/linux/module.h:19:
   In file included from include/linux/elf.h:6:
   In file included from arch/s390/include/asm/elf.h:181:
   In file included from arch/s390/include/asm/mmu_context.h:11:
   In file included from arch/s390/include/asm/pgalloc.h:18:
   In file included from include/linux/mm.h:2228:
   include/linux/vmstat.h:503:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     503 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     504 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:510:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     510 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     511 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:517:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     517 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
   include/linux/vmstat.h:523:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     523 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     524 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   In file included from net/ncsi/ncsi-manage.c:9:
   In file included from include/linux/netdevice.h:38:
   In file included from include/net/net_namespace.h:43:
   In file included from include/linux/skbuff.h:28:
   In file included from include/linux/dma-mapping.h:11:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:93:
   include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     548 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     561 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
         |                                                           ^
   include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
     102 | #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
         |                                                      ^
   In file included from net/ncsi/ncsi-manage.c:9:
   In file included from include/linux/netdevice.h:38:
   In file included from include/net/net_namespace.h:43:
   In file included from include/linux/skbuff.h:28:
   In file included from include/linux/dma-mapping.h:11:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:93:
   include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     574 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
         |                                                           ^
   include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
     115 | #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
         |                                                      ^
   In file included from net/ncsi/ncsi-manage.c:9:
   In file included from include/linux/netdevice.h:38:
   In file included from include/net/net_namespace.h:43:
   In file included from include/linux/skbuff.h:28:
   In file included from include/linux/dma-mapping.h:11:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:93:
   include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     585 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     595 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     605 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:693:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     693 |         readsb(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:701:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     701 |         readsw(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:709:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     709 |         readsl(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:718:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     718 |         writesb(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:727:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     727 |         writesw(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:736:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     736 |         writesl(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
>> net/ncsi/ncsi-manage.c:1284:9: error: call to undeclared function 'str_up_down'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    1284 |                                            str_up_down(ncm->data[2] & 0x1));
         |                                            ^
>> net/ncsi/ncsi-manage.c:1284:9: warning: format specifies type 'char *' but the argument has type 'int' [-Wformat]
    1282 |                                            "NCSI: Channel %u added to queue (link %s)\n",
         |                                                                                   ~~
         |                                                                                   %d
    1283 |                                            nc->id,
    1284 |                                            str_up_down(ncm->data[2] & 0x1));
         |                                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/net/net_debug.h:57:38: note: expanded from macro 'netdev_dbg'
      57 |         dynamic_netdev_dbg(__dev, format, ##args);              \
         |                                   ~~~~~~    ^~~~
   include/linux/dynamic_debug.h:278:19: note: expanded from macro 'dynamic_netdev_dbg'
     278 |                            dev, fmt, ##__VA_ARGS__)
         |                                 ~~~    ^~~~~~~~~~~
   include/linux/dynamic_debug.h:250:59: note: expanded from macro '_dynamic_func_call'
     250 |         _dynamic_func_call_cls(_DPRINTK_CLASS_DFLT, fmt, func, ##__VA_ARGS__)
         |                                                                  ^~~~~~~~~~~
   include/linux/dynamic_debug.h:248:65: note: expanded from macro '_dynamic_func_call_cls'
     248 |         __dynamic_func_call_cls(__UNIQUE_ID(ddebug), cls, fmt, func, ##__VA_ARGS__)
         |                                                                        ^~~~~~~~~~~
   include/linux/dynamic_debug.h:224:15: note: expanded from macro '__dynamic_func_call_cls'
     224 |                 func(&id, ##__VA_ARGS__);                       \
         |                             ^~~~~~~~~~~
   17 warnings and 1 error generated.


vim +/str_up_down +1284 net/ncsi/ncsi-manage.c

  1224	
  1225	static int ncsi_choose_active_channel(struct ncsi_dev_priv *ndp)
  1226	{
  1227		struct ncsi_channel *nc, *found, *hot_nc;
  1228		struct ncsi_channel_mode *ncm;
  1229		unsigned long flags, cflags;
  1230		struct ncsi_package *np;
  1231		bool with_link;
  1232	
  1233		spin_lock_irqsave(&ndp->lock, flags);
  1234		hot_nc = ndp->hot_channel;
  1235		spin_unlock_irqrestore(&ndp->lock, flags);
  1236	
  1237		/* By default the search is done once an inactive channel with up
  1238		 * link is found, unless a preferred channel is set.
  1239		 * If multi_package or multi_channel are configured all channels in the
  1240		 * whitelist are added to the channel queue.
  1241		 */
  1242		found = NULL;
  1243		with_link = false;
  1244		NCSI_FOR_EACH_PACKAGE(ndp, np) {
  1245			if (!(ndp->package_whitelist & (0x1 << np->id)))
  1246				continue;
  1247			NCSI_FOR_EACH_CHANNEL(np, nc) {
  1248				if (!(np->channel_whitelist & (0x1 << nc->id)))
  1249					continue;
  1250	
  1251				spin_lock_irqsave(&nc->lock, cflags);
  1252	
  1253				if (!list_empty(&nc->link) ||
  1254				    nc->state != NCSI_CHANNEL_INACTIVE) {
  1255					spin_unlock_irqrestore(&nc->lock, cflags);
  1256					continue;
  1257				}
  1258	
  1259				if (!found)
  1260					found = nc;
  1261	
  1262				if (nc == hot_nc)
  1263					found = nc;
  1264	
  1265				ncm = &nc->modes[NCSI_MODE_LINK];
  1266				if (ncm->data[2] & 0x1) {
  1267					found = nc;
  1268					with_link = true;
  1269				}
  1270	
  1271				/* If multi_channel is enabled configure all valid
  1272				 * channels whether or not they currently have link
  1273				 * so they will have AENs enabled.
  1274				 */
  1275				if (with_link || np->multi_channel) {
  1276					spin_lock_irqsave(&ndp->lock, flags);
  1277					list_add_tail_rcu(&nc->link,
  1278							  &ndp->channel_queue);
  1279					spin_unlock_irqrestore(&ndp->lock, flags);
  1280	
  1281					netdev_dbg(ndp->ndev.dev,
  1282						   "NCSI: Channel %u added to queue (link %s)\n",
  1283						   nc->id,
> 1284						   str_up_down(ncm->data[2] & 0x1));
  1285				}
  1286	
  1287				spin_unlock_irqrestore(&nc->lock, cflags);
  1288	
  1289				if (with_link && !np->multi_channel)
  1290					break;
  1291			}
  1292			if (with_link && !ndp->multi_package)
  1293				break;
  1294		}
  1295	
  1296		if (list_empty(&ndp->channel_queue) && found) {
  1297			netdev_info(ndp->ndev.dev,
  1298				    "NCSI: No channel with link found, configuring channel %u\n",
  1299				    found->id);
  1300			spin_lock_irqsave(&ndp->lock, flags);
  1301			list_add_tail_rcu(&found->link, &ndp->channel_queue);
  1302			spin_unlock_irqrestore(&ndp->lock, flags);
  1303		} else if (!found) {
  1304			netdev_warn(ndp->ndev.dev,
  1305				    "NCSI: No channel found to configure!\n");
  1306			ncsi_report_link(ndp, true);
  1307			return -ENODEV;
  1308		}
  1309	
  1310		return ncsi_process_next_channel(ndp);
  1311	}
  1312	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH net-next v2 2/2] net/ncsi: Use str_up_down to simplify the code

[Hongbo Li]

As str_up_down is the common helper to reback "up/down"
string, we can replace the target with it to simplify
the code and fix the coccinelle warning.

Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
---
 net/ncsi/ncsi-aen.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ncsi/ncsi-aen.c b/net/ncsi/ncsi-aen.c
index 62fb1031763d..25814f06d7da 100644
--- a/net/ncsi/ncsi-aen.c
+++ b/net/ncsi/ncsi-aen.c
@@ -75,7 +75,7 @@ static int ncsi_aen_handler_lsc(struct ncsi_dev_priv *ndp,
 	has_link = !!(data & 0x1);
 
 	netdev_dbg(ndp->ndev.dev, "NCSI: LSC AEN - channel %u state %s\n",
-		   nc->id, data & 0x1 ? "up" : "down");
+		   nc->id, str_up_down(data & 0x1));
 
 	chained = !list_empty(&nc->link);
 	state = nc->state;
-- 
2.34.1

Re: [PATCH net-next v2 2/2] net/ncsi: Use str_up_down to simplify the code

[kernel test robot]

Hi Hongbo,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Hongbo-Li/net-ncsi-Use-str_up_down-to-simplify-the-code/20240827-104622
base:   net-next/main
patch link:    https://lore.kernel.org/r/20240827025246.963115-3-lihongbo22%40huawei.com
patch subject: [PATCH net-next v2 2/2] net/ncsi: Use str_up_down to simplify the code
config: arc-randconfig-001-20240827 (https://download.01.org/0day-ci/archive/20240828/202408280152.l51y5bcU-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240828/202408280152.l51y5bcU-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408280152.l51y5bcU-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/skbuff.h:38,
                    from include/net/net_namespace.h:43,
                    from include/linux/netdevice.h:38,
                    from net/ncsi/ncsi-aen.c:9:
   net/ncsi/ncsi-aen.c: In function 'ncsi_aen_handler_lsc':
>> net/ncsi/ncsi-aen.c:78:28: error: implicit declaration of function 'str_up_down' [-Werror=implicit-function-declaration]
      78 |                    nc->id, str_up_down(data & 0x1));
         |                            ^~~~~~~~~~~
   include/net/net_debug.h:66:60: note: in definition of macro 'netdev_dbg'
      66 |                 netdev_printk(KERN_DEBUG, __dev, format, ##args); \
         |                                                            ^~~~
>> net/ncsi/ncsi-aen.c:77:35: warning: format '%s' expects argument of type 'char *', but argument 5 has type 'int' [-Wformat=]
      77 |         netdev_dbg(ndp->ndev.dev, "NCSI: LSC AEN - channel %u state %s\n",
         |                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      78 |                    nc->id, str_up_down(data & 0x1));
         |                            ~~~~~~~~~~~~~~~~~~~~~~~
         |                            |
         |                            int
   include/net/net_debug.h:66:50: note: in definition of macro 'netdev_dbg'
      66 |                 netdev_printk(KERN_DEBUG, __dev, format, ##args); \
         |                                                  ^~~~~~
   net/ncsi/ncsi-aen.c:77:70: note: format string is defined here
      77 |         netdev_dbg(ndp->ndev.dev, "NCSI: LSC AEN - channel %u state %s\n",
         |                                                                     ~^
         |                                                                      |
         |                                                                      char *
         |                                                                     %d
   cc1: some warnings being treated as errors


vim +/str_up_down +78 net/ncsi/ncsi-aen.c

   > 9	#include <linux/netdevice.h>
    10	#include <linux/skbuff.h>
    11	
    12	#include <net/ncsi.h>
    13	#include <net/net_namespace.h>
    14	#include <net/sock.h>
    15	
    16	#include "internal.h"
    17	#include "ncsi-pkt.h"
    18	
    19	static int ncsi_validate_aen_pkt(struct ncsi_aen_pkt_hdr *h,
    20					 const unsigned short payload)
    21	{
    22		u32 checksum;
    23		__be32 *pchecksum;
    24	
    25		if (h->common.revision != NCSI_PKT_REVISION)
    26			return -EINVAL;
    27		if (ntohs(h->common.length) != payload)
    28			return -EINVAL;
    29	
    30		/* Validate checksum, which might be zeroes if the
    31		 * sender doesn't support checksum according to NCSI
    32		 * specification.
    33		 */
    34		pchecksum = (__be32 *)((void *)(h + 1) + payload - 4);
    35		if (ntohl(*pchecksum) == 0)
    36			return 0;
    37	
    38		checksum = ncsi_calculate_checksum((unsigned char *)h,
    39						   sizeof(*h) + payload - 4);
    40		if (*pchecksum != htonl(checksum))
    41			return -EINVAL;
    42	
    43		return 0;
    44	}
    45	
    46	static int ncsi_aen_handler_lsc(struct ncsi_dev_priv *ndp,
    47					struct ncsi_aen_pkt_hdr *h)
    48	{
    49		struct ncsi_channel *nc, *tmp;
    50		struct ncsi_channel_mode *ncm;
    51		unsigned long old_data, data;
    52		struct ncsi_aen_lsc_pkt *lsc;
    53		struct ncsi_package *np;
    54		bool had_link, has_link;
    55		unsigned long flags;
    56		bool chained;
    57		int state;
    58	
    59		/* Find the NCSI channel */
    60		ncsi_find_package_and_channel(ndp, h->common.channel, NULL, &nc);
    61		if (!nc)
    62			return -ENODEV;
    63	
    64		/* Update the link status */
    65		lsc = (struct ncsi_aen_lsc_pkt *)h;
    66	
    67		spin_lock_irqsave(&nc->lock, flags);
    68		ncm = &nc->modes[NCSI_MODE_LINK];
    69		old_data = ncm->data[2];
    70		data = ntohl(lsc->status);
    71		ncm->data[2] = data;
    72		ncm->data[4] = ntohl(lsc->oem_status);
    73	
    74		had_link = !!(old_data & 0x1);
    75		has_link = !!(data & 0x1);
    76	
  > 77		netdev_dbg(ndp->ndev.dev, "NCSI: LSC AEN - channel %u state %s\n",
  > 78			   nc->id, str_up_down(data & 0x1));
    79	
    80		chained = !list_empty(&nc->link);
    81		state = nc->state;
    82		spin_unlock_irqrestore(&nc->lock, flags);
    83	
    84		if (state == NCSI_CHANNEL_INACTIVE)
    85			netdev_warn(ndp->ndev.dev,
    86				    "NCSI: Inactive channel %u received AEN!\n",
    87				    nc->id);
    88	
    89		if ((had_link == has_link) || chained)
    90			return 0;
    91	
    92		if (!ndp->multi_package && !nc->package->multi_channel) {
    93			if (had_link) {
    94				ndp->flags |= NCSI_DEV_RESHUFFLE;
    95				ncsi_stop_channel_monitor(nc);
    96				spin_lock_irqsave(&ndp->lock, flags);
    97				list_add_tail_rcu(&nc->link, &ndp->channel_queue);
    98				spin_unlock_irqrestore(&ndp->lock, flags);
    99				return ncsi_process_next_channel(ndp);
   100			}
   101			/* Configured channel came up */
   102			return 0;
   103		}
   104	
   105		if (had_link) {
   106			ncm = &nc->modes[NCSI_MODE_TX_ENABLE];
   107			if (ncsi_channel_is_last(ndp, nc)) {
   108				/* No channels left, reconfigure */
   109				return ncsi_reset_dev(&ndp->ndev);
   110			} else if (ncm->enable) {
   111				/* Need to failover Tx channel */
   112				ncsi_update_tx_channel(ndp, nc->package, nc, NULL);
   113			}
   114		} else if (has_link && nc->package->preferred_channel == nc) {
   115			/* Return Tx to preferred channel */
   116			ncsi_update_tx_channel(ndp, nc->package, NULL, nc);
   117		} else if (has_link) {
   118			NCSI_FOR_EACH_PACKAGE(ndp, np) {
   119				NCSI_FOR_EACH_CHANNEL(np, tmp) {
   120					/* Enable Tx on this channel if the current Tx
   121					 * channel is down.
   122					 */
   123					ncm = &tmp->modes[NCSI_MODE_TX_ENABLE];
   124					if (ncm->enable &&
   125					    !ncsi_channel_has_link(tmp)) {
   126						ncsi_update_tx_channel(ndp, nc->package,
   127								       tmp, nc);
   128						break;
   129					}
   130				}
   131			}
   132		}
   133	
   134		/* Leave configured channels active in a multi-channel scenario so
   135		 * AEN events are still received.
   136		 */
   137		return 0;
   138	}
   139	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next v2 0/2] net/ncsi: Use str_up_down to simplify the code

[Simon Horman]

On Tue, Aug 27, 2024 at 10:52:44AM +0800, Hongbo Li wrote:
> In commit a98ae7f045b2, str_up_down() helper is introduced to
> return "up" or "down" string literal, so we can use it to
> simplify the code and fix the coccinelle warning.
> 
> v2:
>  - change subject into net-next
> 
> v1: https://lore.kernel.org/netdev/20240823162144.GW2164@kernel.org/T/

Thanks, but another problem I raised wrt to v1 still stands.
There is a dependency of this patch that is not present in net-next:
commit a98ae7f045b2 ("lib/string_choices: Add str_up_down() helper").

So I think you will need to repost once that commit has
made it into net-next.

-- 
pw-bot: cr

Re: [PATCH net-next v2 0/2] net/ncsi: Use str_up_down to simplify the code

[Hongbo Li]

On 2024/8/27 16:52, Simon Horman wrote:
> On Tue, Aug 27, 2024 at 10:52:44AM +0800, Hongbo Li wrote:
>> In commit a98ae7f045b2, str_up_down() helper is introduced to
>> return "up" or "down" string literal, so we can use it to
>> simplify the code and fix the coccinelle warning.
>>
>> v2:
>>   - change subject into net-next
>>
>> v1: https://lore.kernel.org/netdev/20240823162144.GW2164@kernel.org/T/
> 
> Thanks, but another problem I raised wrt to v1 still stands.
> There is a dependency of this patch that is not present in net-next:
> commit a98ae7f045b2 ("lib/string_choices: Add str_up_down() helper").
> 
> So I think you will need to repost once that commit has
> made it into net-next.
> 
ok, I'll follow this. This also answer Michal's question.

Thanks,
hongbo