Re: [PATCH net-next 2/2] net: broadcom: share header defining UniMAC registers

["Rafał Miłecki" <zajec5@gmail.com>]

On 06.01.2021 20:26, Florian Fainelli wrote:
> On 1/5/21 11:32 PM, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> UniMAC is integrated into multiple Broadcom's Ethernet controllers so
>> use a shared header file for it and avoid some code duplication.
>>
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>>   MAINTAINERS                                   |  2 +
> 
> Don't you need to update the BGMAC section to also list unimac.h since
> it is a shared header now? This looks good to me, the conversion does
> produce the following warnings on x86-64 (and probably arm64, too):
> 
> drivers/net/ethernet/broadcom/bgmac.c: In function 'bgmac_set_rx_mode':
> drivers/net/ethernet/broadcom/bgmac.c:788:33: warning: conversion from
> 'long unsigned int' to 'u32' {aka 'unsigned int'} changes value from
> '18446744073709551599' to '4294967279' [-Woverflow]
>    788 |   bgmac_umac_cmd_maskset(bgmac, ~CMD_PROMISC, 0, true);
> drivers/net/ethernet/broadcom/bgmac.c: In function 'bgmac_mac_speed':
> drivers/net/ethernet/broadcom/bgmac.c:828:13: warning: conversion from
> 'long unsigned int' to 'u32' {aka 'unsigned int'} changes value from
> '18446744073709550579' to '4294966259' [-Woverflow]
>    828 |  u32 mask = ~(CMD_SPEED_MASK << CMD_SPEED_SHIFT | CMD_HD_EN);
>        |             ^
> drivers/net/ethernet/broadcom/bgmac.c: In function 'bgmac_chip_reset':
> drivers/net/ethernet/broadcom/bgmac.c:999:11: warning: conversion from
> 'long unsigned int' to 'u32' {aka 'unsigned int'} changes value from
> '18446744073197811804' to '3783227484' [-Woverflow]
>    999 |           ~(CMD_TX_EN |
>        |           ^~~~~~~~~~~~~
>   1000 |      CMD_RX_EN |
>        |      ~~~~~~~~~~~
>   1001 |      CMD_RX_PAUSE_IGNORE |
>        |      ~~~~~~~~~~~~~~~~~~~~~
>   1002 |      CMD_TX_ADDR_INS |
>        |      ~~~~~~~~~~~~~~~~~
>   1003 |      CMD_HD_EN |
>        |      ~~~~~~~~~~~
>   1004 |      CMD_LCL_LOOP_EN |
>        |      ~~~~~~~~~~~~~~~~~
>   1005 |      CMD_CNTL_FRM_EN |
>        |      ~~~~~~~~~~~~~~~~~
>   1006 |      CMD_RMT_LOOP_EN |
>        |      ~~~~~~~~~~~~~~~~~
>   1007 |      CMD_RX_ERR_DISC |
>        |      ~~~~~~~~~~~~~~~~~
>   1008 |      CMD_PRBL_EN |
>        |      ~~~~~~~~~~~~~
>   1009 |      CMD_TX_PAUSE_IGNORE |
>        |      ~~~~~~~~~~~~~~~~~~~~~
>   1010 |      CMD_PAD_EN |
>        |      ~~~~~~~~~~~~
>   1011 |      CMD_PAUSE_FWD),
>        |      ~~~~~~~~~~~~~~
> drivers/net/ethernet/broadcom/bgmac.c: In function 'bgmac_enable':
> drivers/net/ethernet/broadcom/bgmac.c:1057:32: warning: conversion from
> 'long unsigned int' to 'u32' {aka 'unsigned int'} changes value from
> '18446744073709551612' to '4294967292' [-Woverflow]
>   1057 |  bgmac_umac_cmd_maskset(bgmac, ~(CMD_TX_EN | CMD_RX_EN),
>        |                                ^~~~~~~~~~~~~~~~~~~~~~~~
> drivers/net/ethernet/broadcom/bgmac.c: In function 'bgmac_chip_init':
> drivers/net/ethernet/broadcom/bgmac.c:1108:32: warning: conversion from
> 'long unsigned int' to 'u32' {aka 'unsigned int'} changes value from
> '18446744073709551359' to '4294967039' [-Woverflow]
>   1108 |  bgmac_umac_cmd_maskset(bgmac, ~CMD_RX_PAUSE_IGNORE, 0, true);
> drivers/net/ethernet/broadcom/bgmac.c:1117:33: warning: conversion from
> 'long unsigned int' to 'u32' {aka 'unsigned int'} changes value from
> '18446744073709518847' to '4294934527' [-Woverflow]
>   1117 |   bgmac_umac_cmd_maskset(bgmac, ~CMD_LCL_LOOP_EN, 0, false);

I can reproduce that after switching from mips to arm64. Before this
change bgmac.h was not using BIT() macro. Now it does and that macro
forces UL (unsigned long).

Is there any cleaner solution than below one?


diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index 075f6e146b29..1cb0ec3d9b3a 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -785,7 +785,7 @@ static void bgmac_set_rx_mode(struct net_device *net_dev)
  	if (net_dev->flags & IFF_PROMISC)
  		bgmac_umac_cmd_maskset(bgmac, ~0, CMD_PROMISC, true);
  	else
-		bgmac_umac_cmd_maskset(bgmac, ~CMD_PROMISC, 0, true);
+		bgmac_umac_cmd_maskset(bgmac, (u32)~CMD_PROMISC, 0, true);
  }

  #if 0 /* We don't use that regs yet */
@@ -825,7 +825,7 @@ static void bgmac_clear_mib(struct bgmac *bgmac)
  /* http://bcm-v4.sipsolutions.net/mac-gbit/gmac/gmac_speed */
  static void bgmac_mac_speed(struct bgmac *bgmac)
  {
-	u32 mask = ~(CMD_SPEED_MASK << CMD_SPEED_SHIFT | CMD_HD_EN);
+	u32 mask = (u32)~(CMD_SPEED_MASK << CMD_SPEED_SHIFT | CMD_HD_EN);
  	u32 set = 0;

  	switch (bgmac->mac_speed) {
@@ -996,7 +996,7 @@ static void bgmac_chip_reset(struct bgmac *bgmac)
  		cmdcfg_sr = CMD_SW_RESET_OLD;

  	bgmac_umac_cmd_maskset(bgmac,
-			       ~(CMD_TX_EN |
+			       (u32)~(CMD_TX_EN |
  				 CMD_RX_EN |
  				 CMD_RX_PAUSE_IGNORE |
  				 CMD_TX_ADDR_INS |
@@ -1054,7 +1054,7 @@ static void bgmac_enable(struct bgmac *bgmac)
  		cmdcfg_sr = CMD_SW_RESET_OLD;

  	cmdcfg = bgmac_umac_read(bgmac, UMAC_CMD);
-	bgmac_umac_cmd_maskset(bgmac, ~(CMD_TX_EN | CMD_RX_EN),
+	bgmac_umac_cmd_maskset(bgmac, (u32)~(CMD_TX_EN | CMD_RX_EN),
  			       cmdcfg_sr, true);
  	udelay(2);
  	cmdcfg |= CMD_TX_EN | CMD_RX_EN;
@@ -1105,7 +1105,7 @@ static void bgmac_chip_init(struct bgmac *bgmac)
  	bgmac_write(bgmac, BGMAC_INT_RECV_LAZY, 1 << BGMAC_IRL_FC_SHIFT);

  	/* Enable 802.3x tx flow control (honor received PAUSE frames) */
-	bgmac_umac_cmd_maskset(bgmac, ~CMD_RX_PAUSE_IGNORE, 0, true);
+	bgmac_umac_cmd_maskset(bgmac, (u32)~CMD_RX_PAUSE_IGNORE, 0, true);

  	bgmac_set_rx_mode(bgmac->net_dev);

@@ -1114,7 +1114,7 @@ static void bgmac_chip_init(struct bgmac *bgmac)
  	if (bgmac->loopback)
  		bgmac_umac_cmd_maskset(bgmac, ~0, CMD_LCL_LOOP_EN, false);
  	else
-		bgmac_umac_cmd_maskset(bgmac, ~CMD_LCL_LOOP_EN, 0, false);
+		bgmac_umac_cmd_maskset(bgmac, (u32)~CMD_LCL_LOOP_EN, 0, false);

  	bgmac_umac_write(bgmac, UMAC_MAX_FRAME_LEN, 32 + ETHER_MAX_LEN);



> I did verify that the md5sum of the objects does not change before and
> after changes (except bgmac.o, which is expected due to the warning
> above0, so that gives me good confidence that the changes are correct :)
> 
> Acked-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks!