| 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
| ||
|
Message-ID: <ed92d6bd-0d07-afbb-6b53-23180a5abae9@gmail.com>
Date: Wed, 6 Jan 2021 21:37:39 +0100
From: Rafał Miłecki <zajec5@...il.com>
To: 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 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?
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@...il.com>
Thanks!
Powered by blists - more mailing lists