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: <362ccb19-8f58-6bb6-a49b-c9eea93a5366@gmail.com>
Date:   Thu, 7 Jan 2021 09:14:17 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Rafał Miłecki <zajec5@...il.com>,
        Florian Fainelli <f.fainelli@...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 12:37 PM, Rafał Miłecki wrote:
> 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@...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 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?

Don't use BIT(), if the constants are 32-bit unsigned integer, maybe
open coding them as (1 << x) is acceptable for that purpose.
-- 
Florian