Re: [PATCHv3 net-next] octeontx2-pf: Add RSS multi group support

[Jakub Kicinski]

On Wed, 09 Dec 2020 11:08:24 -0800 Saeed Mahameed wrote:
> > -/* Configure RSS table and hash key */
> > -static int otx2_set_rxfh(struct net_device *dev, const u32 *indir,
> > -			 const u8 *hkey, const u8 hfunc)
> > +static int otx2_get_rxfh_context(struct net_device *dev, u32 *indir,
> > +				 u8 *hkey, u8 *hfunc, u32 rss_context)
> >  {
> >  	struct otx2_nic *pfvf = netdev_priv(dev);
> > +	struct otx2_rss_ctx *rss_ctx;
> >  	struct otx2_rss_info *rss;
> >  	int idx;
> >  
> > -	if (hfunc != ETH_RSS_HASH_NO_CHANGE && hfunc !=
> > ETH_RSS_HASH_TOP)
> > -		return -EOPNOTSUPP;
> > -
> >  	rss = &pfvf->hw.rss_info;
> >  
> >  	if (!rss->enable) {
> > -		netdev_err(dev, "RSS is disabled, cannot change
> > settings\n");
> > +		netdev_err(dev, "RSS is disabled\n");
> >  		return -EIO;
> >  	}  
> 
> I see that you init/enable rss on open, is this is your way to block
> getting rss info if device is not open ? why do you need to report an
> error anyway, why not just report whatever default config you will be
> setting up on next open ? 
> 
> to me reporting errors to ethtool queries when device is down is a bad
> user experience.
> 
> > +	if (rss_context >= MAX_RSS_GROUPS)
> > +		return -EINVAL;
> > +  
> 
> -ENOENT
> > +	rss_ctx = rss->rss_ctx[rss_context];
> > +	if (!rss_ctx)
> > +		return -EINVAL;
> >   
> 
> -ENOENT

Plus looks like this version introduces a W=1 C=1 warning:

drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c:768:28: warning: Using plain integer as NULL pointer