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" <wa...@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
