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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b03b0817-8d22-0c95-651d-7d14300a4124@gmail.com>
Date:   Wed, 6 Jan 2021 11:45:32 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>,
        Rafał Miłecki <zajec5@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
Cc:     Doug Berger <opendmb@...il.com>, Ray Jui <ray.jui@...adcom.com>,
        Arun Parameswaran <arun.parameswaran@...adcom.com>,
        Murali Krishna Policharla <murali.policharla@...adcom.com>,
        Timur Tabi <timur@...nel.org>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Vladimir Oltean <vladimir.oltean@....com>,
        netdev@...r.kernel.org, bcm-kernel-feedback-list@...adcom.com,
        Rafał Miłecki <rafal@...ecki.pl>
Subject: Re: [PATCH net-next 2/2] net: broadcom: share header defining UniMAC
 registers

On 1/6/21 11:26 AM, Florian Fainelli wrote:
> On 1/5/21 11:32 PM, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@...ecki.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@...ecki.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@...il.com>
> 
> Thanks for doing this.

For GENET and SYSTEMPORT you should be able to share the MIB counters as
well, and in premise we could even get a step further and share the
ethtool stats array between drivers since they are the exact same. You
don't have to include that as part of your series though, we can address
it later. We have a third driver coming up which is also using an UniMAC
and could benefit from not replicating these headers.
-- 
Florian