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

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

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.

> 
> 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

> 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?

> 
> 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

> +		return ice_sq_send_cmd_retry(hw, &hw->adminq, desc, buf,
> +					     buf_size, cd);
> +	}