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: <a0b535bd-3569-4d36-9752-ec8dcdc23aaf@intel.com>
Date: Mon, 13 Jan 2025 10:18:05 -0800
From: Jacob Keller <jacob.e.keller@...el.com>
To: Jakub Kicinski <kuba@...nel.org>
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 1/10/2025 5:25 PM, Jakub Kicinski wrote:
> 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.
> 

Quite. But I also don't want to simply rename the file, or start putting
the same problematic code under a different name.

>> 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.

The intention with introducing these is to help make it easier for other
developers to use poll_timeout and friends throughout the driver.
There's only one user now, but my intention had been that we'd see more
as it becomes more known and is easier to use.