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 <[email protected]>
>
>
>> @@ -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/[email protected]/
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.
