Re: [PATCH net-next v5 14/15] idpf: add ethtool callbacks

[Jakub Kicinski <kuba@kernel.org>]

On Mon, 21 Aug 2023 13:41:15 -0700 Linga, Pavan Kumar wrote:
> On 8/18/2023 11:58 AM, Jakub Kicinski wrote:
> > On Tue, 15 Aug 2023 17:43:04 -0700 Tony Nguyen wrote:  
> >> +static u32 idpf_get_rxfh_indir_size(struct net_device *netdev)
> >> +{
> >> +	struct idpf_vport *vport = idpf_netdev_to_vport(netdev);
> >> +	struct idpf_vport_user_config_data *user_config;
> >> +
> >> +	if (!vport)
> >> +		return -EINVAL;  
> > 
> > defensive programming? how do we have a netdev and no vport?
> 
> During a hardware reset, the control plane will reinitialize its vport 
> configuration along with the hardware resources which in turn requires 
> the driver to reallocate the vports as well. For this reason the vports 
> will be freed, but the netdev will be preserved.

HW reset path should take appropriate locks so that the normal control
path can't be exposed to transient errors.

User space will 100% not know what to do with a GET reporting EINVAL.

> >> +	dev = &vport->adapter->pdev->dev;
> >> +	if (!(ch->combined_count || (ch->rx_count && ch->tx_count))) {
> >> +		dev_err(dev, "Please specify at least 1 Rx and 1 Tx channel\n");  
> > 
> > The error msg doesn't seem to fit the second part of the condition.
> >
> 
> The negation part is to the complete check which means it takes 0 
> [tx|rx]_count into consideration.

Ah, missed the negation. In that case I think the check is not needed,
pretty sure core checks this.

> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	num_req_tx_q = ch->combined_count + ch->tx_count;
> >> +	num_req_rx_q = ch->combined_count + ch->rx_count;
> >> +
> >> +	dev = &vport->adapter->pdev->dev;
> >> +	/* It's possible to specify number of queues that exceeds max in a way
> >> +	 * that stack won't catch for us, this should catch that.
> >> +	 */  
> > 
> > How, tho?
> 
> If the user tries to pass the combined along with the txq or rxq values, 
> then it is possbile to cross the max supported values. So the following 
> checks are needed to protect those cases. Core checks the max values for 
> the individual arguments but not the combined + [tx|rx].

I see, please add something along those lines to the comment.