Re: [PATCH net-next 1/3] net: libwx: add support for set_ringparam in wx_ethtool_ops_vf

[Jakub Kicinski <kuba@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.
---
net: libwx: add support for set_ringparam in wx_ethtool_ops_vf

This commit adds support for the set_ringparam callback in wx_ethtool_ops_vf,
allowing the ring sizes for ngbevf and txgbevf to be configured.

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
> index 5df971aca9e35..31034cd208703 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c

[ ... ]

> @@ -775,6 +776,64 @@ static int wx_get_link_ksettings_vf(struct net_device *netdev,
>  	return 0;
>  }
>  
> +static int wx_set_ringparam_vf(struct net_device *netdev,
> +			       struct ethtool_ringparam *ring,
> +			       struct kernel_ethtool_ringparam *kernel_ring,
> +			       struct netlink_ext_ack *extack)
> +{

[ ... ]

> +	wxvf_down(wx);
> +	wx_free_irq(wx);
> +
> +	wx_set_ring(wx, new_tx_count, new_rx_count, temp_ring);

wx_set_ring() returns void, so memory allocation failures inside it (like in
wx_setup_tx_resources()) are not passed back to the caller. Does this mean
wx_set_ringparam_vf() will falsely return success (0) to userspace even if
the ring sizes could not be changed due to memory exhaustion?

> +	kvfree(temp_ring);
> +
> +	wx_configure_vf(wx);
> +	wx_request_msix_irqs_vf(wx);

Is it safe to ignore the return value of wx_request_msix_irqs_vf() here?

If request_irq() fails, wx_request_msix_irqs_vf() cleans up and sets
wx->msix_q_entries to null. Because the error is not handled here, execution
continues to wxvf_up_complete(), which starts NAPI and the transmit queues
without hardware interrupts. Could this result in a silent network stall?

Additionally, if the interface is later brought up via wxvf_open(), it will
call wx_request_msix_irqs_vf() again. This loop calculates
entry = &wx->msix_q_entries[vector] and dereferences entry->vector. Could
this cause a null pointer dereference if wx->msix_q_entries was previously
freed and left as null?

> +	wxvf_up_complete(wx);
> +
> +clear_reset:
> +	clear_bit(WX_STATE_RESETTING, wx->state);
> +	mutex_unlock(&wx->reset_lock);
> +	return err;
> +}
-- 
pw-bot: cr