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

[Jakub Kicinski <kuba@kernel.org>]

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.