Re: [PATCH v3 05/22] common/idpf: avoid defensive programming

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 6/12/2024 5:52 AM, Soumyadeep Hore wrote:
> Based on the upstream feedback, driver should not use any
> defensive programming strategy by checking for NULL pointers
> and other conditional checks unnecessarily in the code flow
> to fall back, instead fail and fix the bug in a proper way.
> 
> Some of the checks are identified and removed/wrapped
> in this patch:
> - As the control queue is freed and deleted from the list after the
> idpf_ctlq_shutdown call, there is no need to have the ring_size
> check in idpf_ctlq_shutdown.
> - From the upstream perspective shared code is part of the Linux
> driver and it doesn't make sense to add zero 'len' and 'buf_size'
> check in idpf_ctlq_add as to start with, driver provides valid
> sizes, if not it is a bug.
> - Remove cq NULL and zero ring_size check wherever possible as
> the IDPF driver code flow does not pass any NULL cq pointer to
> the control queue callbacks. If it passes then it is a bug and
> should be fixed rather than checking for NULL pointer and falling
> back which is not the right way.

It seems that the commit log calls out changes that weren't made in this 
patch?

> 
> Signed-off-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
> ---
>   drivers/common/idpf/base/idpf_controlq.c | 4 ----
>   1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/common/idpf/base/idpf_controlq.c b/drivers/common/idpf/base/idpf_controlq.c
> index bada75abfc..b5ba9c3bd0 100644
> --- a/drivers/common/idpf/base/idpf_controlq.c
> +++ b/drivers/common/idpf/base/idpf_controlq.c
> @@ -98,9 +98,6 @@ static void idpf_ctlq_shutdown(struct idpf_hw *hw, struct idpf_ctlq_info *cq)
>   {
>   	idpf_acquire_lock(&cq->cq_lock);
>   
> -	if (!cq->ring_size)
> -		goto shutdown_sq_out;
> -
>   #ifdef SIMICS_BUILD
>   	wr32(hw, cq->reg.head, 0);
>   	wr32(hw, cq->reg.tail, 0);
> @@ -115,7 +112,6 @@ static void idpf_ctlq_shutdown(struct idpf_hw *hw, struct idpf_ctlq_info *cq)
>   	/* Set ring_size to 0 to indicate uninitialized queue */
>   	cq->ring_size = 0;
>   
> -shutdown_sq_out:
>   	idpf_release_lock(&cq->cq_lock);
>   	idpf_destroy_lock(&cq->cq_lock);
>   }

-- 
Thanks,
Anatoly