netdev - Re: [PATCH net-next 08/13] ice: use rd32_poll_timeout_atomic in ice_read_phy_tstamp_ll

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: <20250110172536.7165c528@kernel.org>
Date: Fri, 10 Jan 2025 17:25:36 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Jacob Keller <jacob.e.keller@...el.com>
Cc: Tony Nguyen <anthony.l.nguyen@...el.com>, <davem@...emloft.net>,
 <pabeni@...hat.com>, <edumazet@...gle.com>, <andrew+netdev@...n.ch>,
 <netdev@...r.kernel.org>, <anton.nadezhdin@...el.com>,
 <przemyslaw.kitszel@...el.com>, <milena.olech@...el.com>,
 <arkadiusz.kubalewski@...el.com>, <richardcochran@...il.com>, Karol
 Kolacinski <karol.kolacinski@...el.com>, Rinitha S <sx.rinitha@...el.com>
Subject: Re: [PATCH net-next 08/13] ice: use rd32_poll_timeout_atomic in
 ice_read_phy_tstamp_ll_e810

On Fri, 10 Jan 2025 16:50:44 -0800 Jacob Keller wrote:
> On 1/9/2025 6:21 PM, Jakub Kicinski wrote:
> > On Wed,  8 Jan 2025 14:17:45 -0800 Tony Nguyen wrote:  
> >> --- a/drivers/net/ethernet/intel/ice/ice_osdep.h
> >> +++ b/drivers/net/ethernet/intel/ice/ice_osdep.h
> >> @@ -26,6 +26,9 @@
> >>  
> >>  #define rd32_poll_timeout(a, addr, val, cond, delay_us, timeout_us) \
> >>  	read_poll_timeout(rd32, val, cond, delay_us, timeout_us, false, a, addr)
> >> +#define rd32_poll_timeout_atomic(a, addr, val, cond, delay_us, timeout_us) \
> >> +	read_poll_timeout_atomic(rd32, val, cond, delay_us, timeout_us, false, \
> >> +				 a, addr)  
> > 
> > Could you deprecate the use of the osdep header? At the very least don't
> > add new stuff here. Back in the day "no OS abstraction layers" was 
> > a pretty hard and fast rule. I don't hear it as much these days, but 
> > I think it's still valid since this just obfuscates the code for all
> > readers outside your team.  
> 
> I assume you are referring to the abstractions in general (rd32,
> rd32_poll_timeout, etc) and not simply the name of the header (osdep.h)?

I presume the two are causally interlinked.

> I do agree that the layering with the intent to create an OS abstraction
> is not preferred and that its been pushed back against for years. We
> have been working to move away from OS abstractions, including several
> refactors to the ice driver. Use of "rd32_poll_timeout" is in fact one
> of these refactors: there's no reason to re-implement read polling when
> its provided by the kernel.
> 
> However, I also think there is some value in shorthands for commonly
> used idioms like "readl(hw->hw_addr + reg_offset)" which make the intent
> more legible at least to me.
> 
> These rd32_* implementations are built in line with the readl* variants
> in <linux/iopoll.h>
> 
> I suppose it is more frustrating for someone on the opposite side who
> must content with each drivers variation of a register access macro. We
> could rip the rd32-etc out entirely and replace them with readl and
> friends directly... But that again feels like a lot of churn.

Right, too late for that.

> My goal with these macros was to make it easier for ice developers to
> use the read_poll_timeout bits within the existing framework, with an
> attempt to minimize the thrash to existing code.
> 
> Glancing through driver/net/ethernet, it appears many drivers to use a
> straight readl, while others use a rapper like sbus_readl, gem_readl,
> Intel's rd32, etc.

Ack, and short hands make sense. But both rd32_poll_timeout_atomic and
the exiting rd32_poll_timeout have a single user.