Re: [PATCH 00/12] net/xsc: Resolve issues from PVS and Coverity Scan

["Renyong Wan" <wanry@yunsilicon.com>]

Hello Stephen,

Thank you for your review.  I appreciate the detailed analysis and the 
suggestions for improvements.
Please find my feedback bellow.

On 2025/2/23 1:30, Stephen Hemminger wrote:
> On Sat, 22 Feb 2025 11:57:59 +0800
> "Renyong Wan" <wanry@yunsilicon.com> wrote:
>
>> This patch series resolves several issues reported by PVS and Coverity Scan,
>> which were earlier forwarded to us by Stephen Hemminger.
>>
>> ---
>> Renyong Wan (12):
>>    net/xsc: avoid integer overflow
>>    net/xsc: remove useless call
>>    net/xsc: address incorrect format warnings
>>    net/xsc: remove always-true if expressions
>>    net/xsc: avoid variable is assigned but not used
>>    net/xsc: check possible null pointer dereference
>>    net/xsc: avoid potential null pointer before used
>>    net/xsc: remove always-true part of if expression
>>    net/xsc: avoid assign the same value to a variable
>>    net/xsc: avoid initialize by same function
>>    net/xsc: optimize memcmp returns not 0 check
>>    net/xsc: avoid pointer cast to unrelated class
>>
>>   drivers/net/xsc/xsc_dev.c       |  2 +-
>>   drivers/net/xsc/xsc_ethdev.c    | 35 ++++++++----
>>   drivers/net/xsc/xsc_np.c        | 17 +++---
>>   drivers/net/xsc/xsc_rx.c        | 31 ++++++-----
>>   drivers/net/xsc/xsc_tx.c        |  7 +--
>>   drivers/net/xsc/xsc_vfio.c      | 97 ++++++++++++++++++++-------------
>>   drivers/net/xsc/xsc_vfio_mbox.c |  2 +-
>>   7 files changed, 111 insertions(+), 80 deletions(-)
>>
> Applied to next-net Great to see the Coverity issues fixed.
> Ran PVS Studio on it, and there are three potential things that could be fixed later.
>
> 1. xsc_ethdev.c (838)
> V1027	Pointer to an object of the 'rte_device' class is cast to unrelated 'rte_pci_device' class.
>
> This is a generic issue in bus_pci_driver.h that can be suppressed there.
Got it. Thank you.
>
> 2. xsc_rx.c (351)
> V522	There might be dereferencing of a potential null pointer 'rxq_data'.
>
> Looking at the code, there are two loops over the rxq's the first one just
> dereferences, and the second pass uses a helper function that could return NULL.
> Why not just use do direct index in second one:
The first one doesn't seem like a best solution. It should have used the 
helper function xsc_rxq_get() and checked if returned null. I will 
optimize it next time.
The second one seems to have been overlooked and not addressed. It 
should check the reutrn value of xsc_rxq_get().
>
> -- a/drivers/net/xsc/xsc_rx.c
> +++ b/drivers/net/xsc/xsc_rx.c
> @@ -347,7 +347,7 @@ xsc_rss_qp_create(struct xsc_ethdev_priv *priv, int port_id)
>          rqn_base = rte_be_to_cpu_32(out->qpn_base) & 0xffffff;
>
>          for (i = 0; i < priv->num_rq; i++) {
> -               rxq_data = xsc_rxq_get(priv, i);
> +               rxq_data = (*priv->rxqs)[i];
>                  rxq_data->wqes = rxq_data->rq_pas->addr;
>                  if (!xsc_dev_is_vf(xdev))
>                          rxq_data->rq_db = (uint32_t *)((uint8_t *)xdev->bar_addr +
>
>
> 3. xsc_vfio_mbox.c (575)
> V1048	The 'size' variable was assigned the same value.
>
> This one is harmless, since both commands have same size, the tool
> is just being annoying. Can suppress via comment
>
> --- a/drivers/net/xsc/xsc_vfio_mbox.c
> +++ b/drivers/net/xsc/xsc_vfio_mbox.c
> @@ -572,7 +572,7 @@ xsc_vfio_mbox_init(struct xsc_dev *xdev)
>          cmdq->req_lay = cmdq->req_mz->addr;
>
>          snprintf(name, RTE_MEMZONE_NAMESIZE, "%s_cmd_cq", xdev->pci_dev->device.name);
> -       size = (1 << XSC_CMDQ_DEPTH_LOG) * sizeof(struct xsc_cmdq_rsp_layout);
> +       size = (1 << XSC_CMDQ_DEPTH_LOG) * sizeof(struct xsc_cmdq_rsp_layout); // -V1048
OK, I understand.  Since the "// -V1048" comment style will trigger an 
error in checkpatch, I will use the "/*-V1048*/" instead.
>          cmdq->rsp_mz = rte_memzone_reserve_aligned(name,
>                                                     size, SOCKET_ID_ANY,
>                                                     RTE_MEMZONE_IOVA_CONTIG,

-- 
Best regards,
Renyong Wan