netdev - Re: [PATCH net-next 1/2] net: stmmac: add stmmac_mac_irq

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] [day] [month] [year] [list]

Message-ID: <fa194ce6-8fca-4974-8dc7-2eb22ec50bfb@bootlin.com>
Date: Sat, 25 Oct 2025 21:50:19 +0200
From: Maxime Chevallier <maxime.chevallier@...tlin.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>,
 Jakub Kicinski <kuba@...nel.org>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
 Alexandre Torgue <alexandre.torgue@...s.st.com>,
 Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 linux-arm-kernel@...ts.infradead.org,
 linux-stm32@...md-mailman.stormreply.com,
 Maxime Coquelin <mcoquelin.stm32@...il.com>, netdev@...r.kernel.org,
 Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next 1/2] net: stmmac: add stmmac_mac_irq_modify()

On 25/10/2025 10:02, Russell King (Oracle) wrote:
> On Fri, Oct 24, 2025 at 07:01:59PM -0700, Jakub Kicinski wrote:
>> On Thu, 23 Oct 2025 10:46:20 +0100 Russell King (Oracle) wrote:
>>> Add a function to allow interrupts to be enabled and disabled in a
>>> core independent manner.
>>
>> Sorry for a general question but I'm curious why stick to the callback
>> format this driver insists on. Looks like we could get away with
>> parameterizing the code with the register offset via the priv structure.
> 
> Not quite - some cores, it's a mask (bits need to be set to be disabled).
> Other cores, it's an enable (bits need to be set to enable). So one
> can't get away with just "this is where the register is", it would need
> three pieces of information - register offset, type of regster (mask or
> enable) and then a core specific bitmask.
> 
>> Mostly curious. Personally, I'm always annoyed having to dig thru the
>> indirections in this driver.
> 
> Me too, especially when it's not obvious what is an indirection and
> what is not.

Same here...

> There's the fun that a lot of the PTP-related indirection
> actually has no difference. For example, at the bottom of
> stmmac_hwtstamp.c, the struct stmmac_hwtimestamp initialisers have
> almost all of the methods pointing at the same implementation
> with the exeption of .get_ptptime, .timestamp_interrupt and
> .hwtstamp_correct_latency.

Well I introduced that last year. GMAC1000 and GMAC4 have what
appears to be different versions of the timestamping IP, registers
are either at a different address, or same address with a different
layout bitwise, or with just different behaviours.

There used to be a single instance of the stmmac_hwtimestamp ops,
which didn't even account for the differences between these IP
versions. TBH I don't even know why we had a stmmac_hwtimestamp
struct with a single instance back then, but I figured that using
that was a good way to at least split the gmac1000/gmac4 diffs
back then.

We coud now very much get rid of the common ops and avoid the
indirections for the TS ops that are the same between all IP blocks :)

As I'm doing quite a bit of timestamping in stmmac right now, I may
find a bit of time here and there to do that at some point :)

Maxime

>