Re: [Intel-wired-lan] [PATCH iwl-next v2] ice: split ice_aq_wait_for_event() func into two

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 8/4/23 16:35, Simon Horman wrote:
> On Thu, Aug 03, 2023 at 11:13:47AM -0400, Przemek Kitszel wrote:
>> Mitigate race between registering on wait list and receiving
>> AQ Response from FW.
>>
>> ice_aq_prep_for_event() should be called before sending AQ command,
>> ice_aq_wait_for_event() should be called after sending AQ command,
>> to wait for AQ Response.
>>
>> struct ice_aq_task is exposed to callers, what takes burden of memory
>> ownership out from AQ-wait family of functions.
>>
>> Embed struct ice_rq_event_info event into struct ice_aq_task
>> (instead of it being a ptr), to remove some more code from the callers.

see [1] below

>>
>> Additional fix: one of the checks in ice_aq_check_events() was off by one.
> 
> Hi Przemek,
> 
> This patch seems to be doing three things:
> 
> 1. Refactoring code, in order to allow
> 2. Addressing a race condition

those two are hard to split, perhaps some shuffling of code prior to 
actual 2., eg [1] above.

> 3. Correcting an off-by-one error

That's literally one line-fix, which would be overwritten/touched by 
next patch then.

> 
> All good stuff. But all complex, and 1 somewhat buries 2 and 3.
> I'm wondering if the patch could be broken up into smaller patches
> to aid both review new and inspection later.

Overall, I've started with more patches locally when developing that, 
and with "avoid trashing" principle concluded to squash.
Still, I agree that next attempt at splitting would be beneficial, will 
post v3.

> 
> The above notwithstanding, the code does seems fine to me.
> 
>> Please note, that this was found by reading the code,
>> an actual race has not yet materialized.
> 
> Sure. But I do wonder if a fixes tag might be appropriate anyway.

For this off-by-one, (3. on your list) sure.

For the race (2.), I think it's not so good - ice_aq_wait_for_event() 
was introduced to handle FW update that is counted in seconds, so the 
race was theoretical in that scenario. Later we started adding new 
usages to (general, in principle) waiting "API", with more to come, so 
still worth "fixing".

> 
>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>

Anyway, let's see what v3 will bring :)
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan