Re: [PATCH] vdpa/ifc/base: wait for queue disable before saving q-state

[Maxime Coquelin <maxime.coquelin@redhat.com>]

Hi Abhishek,

On 10/25/22 09:51, Abhishek Maheshwari wrote:
> Some ifc hardware require synchronization between disabling a queue and
> saving queue-state from LM registers. When queue is disabled from vDPA
> driver, ifc device stops executing new virtio-cmds and then updates LM
> registers with used/avail index. Before saving the queue-state, vDPA
> driver should wait until the queue is disabled from backend.


I think it should be considered a fix, and so requires a Fixes tag and
stable@dpdk.org to be cc'ed.

> Signed-off-by: Abhishek Maheshwari <abhishek.maheshwari@intel.com>
> ---
>   drivers/vdpa/ifc/base/ifcvf.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/vdpa/ifc/base/ifcvf.c b/drivers/vdpa/ifc/base/ifcvf.c
> index f1e1474447..76690c1f55 100644
> --- a/drivers/vdpa/ifc/base/ifcvf.c
> +++ b/drivers/vdpa/ifc/base/ifcvf.c
> @@ -257,6 +257,7 @@ ifcvf_hw_disable(struct ifcvf_hw *hw)
>   	u32 i;
>   	struct ifcvf_pci_common_cfg *cfg;
>   	u32 ring_state;
> +	int q_disable_tries;
>   
>   	cfg = hw->common_cfg;
>   	if (!cfg) {
> @@ -275,6 +276,20 @@ ifcvf_hw_disable(struct ifcvf_hw *hw)
>   			continue;
>   		}
>   
> +		/* Some ifc hardware require synchronization between disabling a queue
> +		 * and saving queue-state from LM registers. When queue is disabled from
> +		 * vDPA driver, ifc device stops executing new virtio-cmds and then
> +		 * updates LM registers with used/avail index. Before saving the
> +		 * queue-state, vDPA driver waits until the queue is disabled from
> +		 * backend.
> +		 */
> +		q_disable_tries = 10;
> +		while (q_disable_tries-- && IFCVF_READ_REG16(&cfg->queue_enable))
> +			msec_delay(10);
> +
> +		if (IFCVF_READ_REG16(&cfg->queue_enable))
> +			WARNINGOUT("Failed Q disable: %d. saved state is invalid\n", i);

Maybe checking q_disable_tries > 0 is enough here.
Also, should we really continue the disablement if synchronization
failed?

> +
>   		if (hw->device_type == IFCVF_BLK)
>   			ring_state = *(u32 *)(hw->lm_cfg +
>   					IFCVF_LM_RING_STATE_OFFSET +