[Intel-wired-lan] [PATCH iwl-next 12/13] ice: move prefetch enable to ice_setup_rx_ctx

[Keller, Jacob E]

> -----Original Message-----
> From: Kitszel, Przemyslaw <przemyslaw.kitszel at intel.com>
> Sent: Tuesday, August 27, 2024 11:39 PM
> To: Keller, Jacob E <jacob.e.keller at intel.com>
> Cc: Intel Wired LAN <intel-wired-lan at lists.osuosl.org>; Vladimir Oltean
> <olteanv at gmail.com>; Nguyen, Anthony L <anthony.l.nguyen at intel.com>
> Subject: Re: [PATCH iwl-next 12/13] ice: move prefetch enable to ice_setup_rx_ctx
> 
> On 8/27/24 23:52, Jacob Keller wrote:
> > The ice_write_rxq_ctx() function is responsible for programming the Rx
> > Queue context into hardware. It receives the configuration in unpacked form
> > via the ice_rlan_ctx structure.
> >
> > This function unconditionally modifies the context to set the prefetch
> > enable bit. This was done by commit c31a5c25bb19 ("ice: Always set prefena
> > when configuring an Rx queue"). Setting this bit makes sense, since
> > prefetching descriptors is almost always the preferred behavior.
> >
> > However, the ice_write_rxq_ctx() function is not the place that actually
> > defines the queue context. We initialize the Rx Queue context in
> > ice_setup_rx_ctx(). It is surprising to have the Rx queue context changed
> > by a function who's responsibility is to program the given context to
> > hardware.
> >
> > Following the principle of least surprise, move the setting of the prefetch
> > enable bit out of ice_write_rxq_ctx() and into the ice_setup_rx_ctx().
> >
> > Fixes: c31a5c25bb19 ("ice: Always set prefena when configuring an Rx queue")
> 
> The code is fine, but I would drop fixes tag.
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel at intel.com>
> 

I considered if a fixes because I think its extremely non-intuitive that the context is overwritten here. I understand the original motivation (this was in shared code, so this was the way to make sure every driver did it right...) but it is surprising.

That said, this doesn't fix any user-visible issues, so I'll drop the tag.