Re: [PATCH net-next v3 2/3] net: libwx: add support for set_coalesce in wx_ethtool_ops_vf

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
net: libwx: add support for set_coalesce in wx_ethtool_ops_vf

Add support for set_coalesce in wx_ethtool_ops_vf, which is used to set
interrupt coalescing parameters.

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
> --- a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
[ ... ]
> @@ -488,7 +489,10 @@ int wx_set_coalesce(struct net_device *netdev,
>  		else
>  			/* rx only or mixed */
>  			q_vector->itr = rx_itr_param;
> -		wx_write_eitr(q_vector);
> +		if (wx->pdev->is_virtfn)
> +			wx_write_eitr_vf(q_vector);
> +		else
> +			wx_write_eitr(q_vector);
>  	}

[Severity: Medium]
Does this cause hardware register wrap-around for VFs when coalesce values
exceed 127 usecs?

The validation at the top of wx_set_coalesce() uses PF limits:

    max_eitr = WX_SP_MAX_EITR; // 0xFF8
    ...
    if (ec->rx_coalesce_usecs > (max_eitr >> 2))
        return -EINVAL;

For a VF, wx_write_eitr_vf() masks the interval against WX_VXITR_MASK
(0x1FF). Since the user-provided rx_coalesce_usecs is shifted left by 2
before writing:

    wx->rx_itr_setting = ec->rx_coalesce_usecs << 2;

Any value greater than 127 will pass the PF-specific validation limit but
silently wrap around when bitwise-ANDed with 0x1FF in wx_write_eitr_vf().

Could the validation logic be updated to check against the correct VF
limits before writing?

> [ ... ]
> @@ -844,6 +848,7 @@ static const struct ethtool_ops wx_ethtool_ops_vf = {
>  	.set_ringparam		= wx_set_ringparam_vf,
>  	.get_msglevel		= wx_get_msglevel,
>  	.get_coalesce		= wx_get_coalesce,
> +	.set_coalesce		= wx_set_coalesce,

[Severity: High]
This isn't a bug introduced by this patch, but does enabling this callback
for VFs expose a potential NULL pointer dereference?

wx_set_coalesce() unconditionally accesses wx->q_vector[0]:

    if (wx->q_vector[0]->tx.count && wx->q_vector[0]->rx.count) {
        ...

If wx->num_q_vectors is 0 (for example, if wx_init_interrupt_scheme() failed
during resume or setup, but the netdev is still registered), this will
dereference a NULL pointer. 

Should there be a check to ensure hardware queues are allocated
(wx->num_q_vectors > 0) before accessing the q_vector array?

>  	.get_ts_info		= ethtool_op_get_ts_info,
>  	.get_link_ksettings	= wx_get_link_ksettings_vf,
>  };