linux-kernel - Re: [PATCH 3/3] net: j2: Introduce J-Core EMAC

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: <02ce17e8f00955bab53194a366b9a542@artur-rojek.eu>
Date: Fri, 15 Aug 2025 23:14:08 +0200
From: Artur Rojek <contact@...ur-rojek.eu>
To: Andrew Lunn <andrew@...n.ch>
Cc: Rob Landley <rob@...dley.net>, Jeff Dionne <jeff@...esemi.io>, John Paul
 Adrian Glaubitz <glaubitz@...sik.fu-berlin.de>, Geert Uytterhoeven
 <geert+renesas@...der.be>, Andrew Lunn <andrew+netdev@...n.ch>, "David S .
 Miller" <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>, netdev@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] net: j2: Introduce J-Core EMAC

On 2025-08-15 22:52, Artur Rojek wrote:
> On 2025-08-15 22:16, Andrew Lunn wrote:
> 
> Hi Andrew,
> thanks for the review!
> 
>>> +static irqreturn_t jcore_emac_irq(int irq, void *data)
>>> +{
>>> +	struct jcore_emac *priv = data;
>>> +	struct net_device *ndev = priv->ndev;
>>> +	struct sk_buff *skb;
>>> +	struct {
>>> +		int packets;
>>> +		int bytes;
>>> +		int dropped;
>>> +		int crc_errors;
>>> +	} stats = {};
>>> +	unsigned int status, pkt_len, i;
>> 
>> netdev uses 'reverse christmas tree' for local variables. They should
>> be sorted longest to shortest. This sometimes means you need to move
>> assignments into the body of the function, in this case, ndev.

Should I move the struct stats members into stand alone variables as
well? Or is below sorting acceptable with regards to stats vs skb:

> 	unsigned int status, pkt_len, i;
> 	struct jcore_emac *priv = data;
> 	struct net_device *ndev;
> 	struct {
> 		int crc_errors;
> 		int dropped;
> 		int packets;
> 		int bytes;
> 	} stats = {};
> 	struct sk_buff *skb;

>> 
>>> +	jcore_emac_read_hw_addr(priv, mac);
>>> +	if (is_zero_ether_addr(mac)) {
>> 
>> It would be more normal to use !is_valid_ether_addr()
>> 
>> What support is there for MDIO? Normally the MAC driver would not be
>> setting the carrier status, phylink or phylib would do that.
> 
> From what I can tell, none. This is a very simple FPGA RTL
> implementation of a MAC, and looking at the VHDL, I don't see any MDIO
> registers.

> Moreover, the MDIO pin on the PHY IC on my dev board also
> appears unconnected.

I spoke too soon on that one. It appears to be connected through a trace
that goes under the IC. Nevertheless, I don't think MDIO support is in
the IP core design.

> Perhaps Rob L. or Jeff can shine more light on this
> design wise.
> 
> Cheers,
> Artur
> 
>> 
>>     Andrew
>> 
>> ---
>> pw-bot: cr