Re: [PATCH 3/5] ice: scope global configuration lock to the adapter

[Jacob Keller <jacob.e.keller@intel.com>]

On 10/22/2026 4:10 AM, Przemek Kitszel wrote:
> On 10/19/26 12:51 PM, Aaron Ma wrote:
>> ice_global_cfg_lock_sw is a module-global mutex, so a package download
>> on one E810 adapter blocks Admin Queue commands on unrelated adapters in
>> the same system. In addition, the mutex is acquired only after firmware
>> grants the Global Configuration Lock and dropped before firmware
>> releases it, while the opcode exemption list in ice_aq_send_cmd() does
>> not check whether the caller is the lock owner. Non-owning PFs on the
>> same adapter can therefore submit exempted Admin Queue commands or
>> sideband requests while another PF holds firmware ownership.
> 
> This is not a bug to allow exempted commands, does not matter from which
> PF.
> 

Its not strictly a "bug" but I do think it makes sense to only allow a
single PF to send the set of configuration commands.

>>
>> Move the software mutex to struct ice_adapter and record the owning task
>> across the entire Global Configuration Lock acquisition and release
>> sequence, including the core reset path in ice_cfg_tx_topo(). In
> 
> this is fine,
> would be better to not track mutex holder
> 

Yea. I'm struggling to understand what the reason we need to track the
owning task vs just having the lock.

>> ice_aq_send_cmd(), require non-owning tasks to wait on the adapter mutex
>> while allowing the owning task to issue the permitted configuration
>> commands. 
> 
> 
>> Route the Admin Queue fallback in ice_sbq_send_cmd() through
>> ice_aq_send_cmd() as well.
> 
> perhaps this should be separate commit?
> 

That might make it easier to review.. OTOH we also have been limited in
how many patches we can get merged, so I could see the pressure for
reducing the number where it reasonably makes sense...

>>
>> This patch depends on the previous patch in this series.
>>
>> Fixes: c76488109616 ("ice: Implement Dynamic Device Personalization
>> (DDP) download")
>> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
> 
> 
>> @@ -1895,17 +1894,24 @@ ice_aq_send_cmd(struct ice_hw *hw, struct
>> libie_aq_desc *desc, void *buf,
>>           u16 buf_size, struct ice_sq_cd *cd)
>>   {
>>       struct libie_aqc_req_res *cmd = libie_aq_raw(desc);
>> -    bool lock_acquired = false;
>> -    int status;
>> +    struct ice_pf *pf = hw->back;
>> +    struct ice_adapter *adapter = pf->adapter;
> 
> RCT violation
> 
>> +
>> +    if (READ_ONCE(adapter->global_cfg_owner) != current) {
>> +        guard(mutex)(&adapter->global_cfg_lock);
> 
> no guard()()
> especially here scoped_guard() fits nicely
> 

Yes. guard() is not well liked by the maintainers, and I believe
recently they have strengthened their preference against its use, but
its at least documented as discouraged:

https://lore.kernel.org/netdev/20240829152025.3203577-1-kuba@kernel.org/

I thought recently there were some high profile issues caused by __free
and/or guard() that strengthened this to a hard no but I can't find the
emails.

Please stick to scoped_guard or regular bare lock/unlock where
scoped_guard does not provide much benefit.