netdev - Re: [net-next v4 4/5] net: mtip: The L2 switch driver for imx287

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: <20250411145430.3001f1db@wsk>
Date: Fri, 11 Apr 2025 14:54:30 +0200
From: Lukasz Majewski <lukma@...x.de>
To: Simon Horman <horms@...nel.org>
Cc: Andrew Lunn <andrew+netdev@...n.ch>, davem@...emloft.net, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
 <pabeni@...hat.com>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
 <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Shawn Guo
 <shawnguo@...nel.org>, Sascha Hauer <s.hauer@...gutronix.de>, Pengutronix
 Kernel Team <kernel@...gutronix.de>, Fabio Estevam <festevam@...il.com>,
 Richard Cochran <richardcochran@...il.com>, netdev@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org, Stefan Wahren
 <wahrenst@....net>
Subject: Re: [net-next v4 4/5] net: mtip: The L2 switch driver for imx287

Hi Simon,

> > It is unclear why hwentry, which is a pointer, is being cast to an
> > integer and then back to a pointer. I see pointer arithmetic, but
> > that can operate on pointers just as well as integers, without
> > making assumptions about how wide pointers are with respect to
> > longs.
> > 
> > And in any case, can't the types be used to directly access the
> > offsets needed like this?
> > 
> > 	atable = fep->hwentry.mtip_table64b_entry;
> > 
> > 	*read_lo = readl(&atable[index].lo);
> > 	*read_hi = readl(&atable[index].hi);
> >   
> 
> The code as is seems to be OK.
> 
> The (atable) memory structure is as follows:
> 
> 1. You can store 2048 MAC addresses (2x32 bit each).
> 
> 2. Memory from point 1 is addressed as follows:
> 	2.1 -> from MAC address the CRC8 is calculated (0x00 - 0xFF).
> 	This is the 'index' in the original code.
> 	2.2 -> as it may happen that for two different MAC address the
> 	same CRC8 is calculated (i.e. 'index' is the same), each
> 	'index' can store 8 entries for MAC addresses (and it is
> 	searched in a linear way if needed).
> 
> IMHO, the index above shall be multiplied by 8.

I've double check it and it turned out that you were right :-)

The following code:

struct addr_table64b_entry *atable_base =
fep->hwentry->mtip_table64b_entry;

*read_lo = readl(&atable_base[index].lo);
*read_hi = readl(&atable_base[index].hi);

Is more readable than the current code.

The same would be used for atable_write()

I will change it for v5.

> 
> > Also, and perhaps more importantly, readl expects to be passed
> > a pointer to __iomem. But the appropriate annotations seem
> > to be missing (forcing them with a cast is not advisable here IMHO).
> >   
> 
> I think that the code below:
> unsigned long atable_base = (unsigned long)fep->hwentry;
> 
> could be replaced with
> void __iomem *atable_base = fep->hwentry;
> 
> and the (index << 3) with (index * ATABLE_ENTRY_PER_SLOT)




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@...x.de

Content of type "application/pgp-signature" skipped