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

From: Florian Fainelli <f.fainelli@gmail.com>
To: "Rafał Miłecki" <zajec5@gmail.com>,
	"David S . Miller" <davem@davemloft.net>,
	"Jakub Kicinski" <kuba@kernel.org>
Cc: "Florian Fainelli" <f.fainelli@gmail.com>,
	"Doug Berger" <opendmb@gmail.com>,
	"Ray Jui" <ray.jui@broadcom.com>,
	"Arun Parameswaran" <arun.parameswaran@broadcom.com>,
	"Murali Krishna Policharla" <murali.policharla@broadcom.com>,
	"Timur Tabi" <timur@kernel.org>,
	"Heiner Kallweit" <hkallweit1@gmail.com>,
	"Vladimir Oltean" <vladimir.oltean@nxp.com>,
	netdev@vger.kernel.org, bcm-kernel-feedback-list@broadcom.com,
	"Rafał Miłecki" <rafal@milecki.pl>
Subject: Re: [PATCH net-next 2/2] net: broadcom: share header defining UniMAC registers
Date: Wed, 6 Jan 2021 11:26:23 -0800	[thread overview]
Message-ID: <284cc000-edf1-e943-2531-8c23e9470de1@gmail.com> (raw)
In-Reply-To: <20210106073245.32597-2-zajec5@gmail.com>

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 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 for doing this.
-- 
Florian