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

Message-ID: <55655440-71b4-49e0-9fc8-d8b1b4f77ab4@intel.com>
Date: Fri, 10 Jan 2025 16:50:44 -0800
From: Jacob Keller <jacob.e.keller@...el.com>
To: Jakub Kicinski <kuba@...nel.org>, Tony Nguyen <anthony.l.nguyen@...el.com>
CC: <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 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 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.

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.