netdev - 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: <7a4154eef1cd243e30953d3423e97ab1@artur-rojek.eu>
Date: Sat, 16 Aug 2025 01:25:36 +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-16 00:38, Andrew Lunn wrote:
> On Fri, Aug 15, 2025 at 11:14:08PM +0200, Artur Rojek wrote:
>> 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:
> 
> I would pull the structure definition out of the function. Then just
> create one instance of the structure on the stack.

Makes sense, thanks.

> 
>> > > 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.
> 
> MDIO is actually two pins. MDC and MDIO.
> 
> It might be there is a second IP core which implements MDIO. There is
> no reason it needs to be tightly integrated into the MAC. But it does
> make the MAC driver slightly more complex. You then need a Linux MDIO
> bus driver for it, and the DT for the MAC would include a phy-handle
> property pointing to the PHY on the MDIO bus.
> 
> Is there an Ethernet PHY on your board?

Yes, it's an IC+ IP101ALF 10/100 Ethernet PHY [1]. It does have both MDC
and MDIO pins connected, however I suspect that nothing really
configures it, and it simply runs on default register values (which
allow for valid operation in 100Mb/s mode, it seems). I doubt there is
another IP core to handle MDIO, as this SoC design is optimized for
minimal utilization of FPGA blocks. Does it make sense to you that a MAC
could run without any access to an MDIO bus?

If neither Rob L. or Jeff clarify on this topic, I'll hook up a logic
analyzer to the MDIO bus and see if anything (e.g. loader firmware)
touches it at any point.

Cheers,
Artur

[1] https://www.micros.com.pl/mediaserver/info-uiip101a.pdf

> 
> 	Andrew