Re: [net-next Patch v10 4/8] octeontx2-pf: Refactor schedular queue alloc/free calls

[Simon Horman <simon.horman@corigine.com>]

On Tue, May 16, 2023 at 10:05:54AM +0200, Simon Horman wrote:
> Hi Hariprasad,
> 
> On Sat, May 13, 2023 at 02:21:39PM +0530, Hariprasad Kelam wrote:
> > 1. Upon txschq free request, the transmit schedular config in hardware
> > is not getting reset. This patch adds necessary changes to do the same.
> > 
> 
> nit: s/schedular/scheduler/
> 
> > 2. Current implementation calls txschq alloc during interface
> > initialization and in response handler updates the default txschq array.
> > This creates a problem for htb offload where txsch alloc will be called
> > for every tc class. This patch addresses the issue by reading txschq
> > response in mbox caller function instead in the response handler.
> > 
> > 3. Current otx2_txschq_stop routine tries to free all txschq nodes
> > allocated to the interface. This creates a problem for htb offload.
> > This patch introduces the otx2_txschq_free_one to free txschq in a
> > given level.
> 
> This patch seems to be doing three things.
> Could it be split into three patches?

I see that I was a bit late with my review as the
series was applied yesterday.

> ...
> 
> > -int otx2_txschq_stop(struct otx2_nic *pfvf)
> > +void otx2_txschq_free_one(struct otx2_nic *pfvf, u16 lvl, u16 schq)
> >  {
> >  	struct nix_txsch_free_req *free_req;
> > -	int lvl, schq, err;
> > +	int err;
> >  
> >  	mutex_lock(&pfvf->mbox.lock);
> > -	/* Free the transmit schedulers */
> > +
> >  	free_req = otx2_mbox_alloc_msg_nix_txsch_free(&pfvf->mbox);
> 
> Mainly for my own edification:
> 
> 	- otx2_mbox_alloc_msg_nix_txsch_free is created via the
> 	  M(_name, _id, _fn_name, _req_type, _rsp_type) macro
> 	  around line 844 of  otx2_common.h
> 	- It calls otx2_mbox_alloc_msg_rsp
> 	- Which does not call any allocation functions such as kmalloc
> 
> >  	if (!free_req) {
> >  		mutex_unlock(&pfvf->mbox.lock);
> > -		return -ENOMEM;
> > +		netdev_err(pfvf->netdev,
> > +			   "Failed alloc txschq free req\n");
> 
> I think that given the above it's ok to log an error here.
> As the allocation core won't have (because it's not used here.
> But I wonder if it would be more consistent with how
> allocation errors are usually handled to move the logging into
> otx2_mbox_alloc_msg_rsp().
> 
> > +		return;
> >  	}
> >  
> > -	free_req->flags = TXSCHQ_FREE_ALL;
> > +	free_req->schq_lvl = lvl;
> > +	free_req->schq = schq;
> > +
> >  	err = otx2_sync_mbox_msg(&pfvf->mbox);
> > +	if (err) {
> > +		netdev_err(pfvf->netdev,
> > +			   "Failed stop txschq %d at level %d\n", schq, lvl);
> > +	}
> > +
> >  	mutex_unlock(&pfvf->mbox.lock);
> > +}
> > +
> > +void otx2_txschq_stop(struct otx2_nic *pfvf)
> > +{
> > +	int lvl, schq;
> > +
> > +	/* free non QOS TLx nodes */
> > +	for (lvl = 0; lvl < NIX_TXSCH_LVL_CNT; lvl++)
> > +		otx2_txschq_free_one(pfvf, lvl,
> > +				     pfvf->hw.txschq_list[lvl][0]);
> >  
> >  	/* Clear the txschq list */
> >  	for (lvl = 0; lvl < NIX_TXSCH_LVL_CNT; lvl++) {
> >  		for (schq = 0; schq < MAX_TXSCHQ_PER_FUNC; schq++)
> >  			pfvf->hw.txschq_list[lvl][schq] = 0;
> >  	}
> > -	return err;
> > +
> 
> nit: no blank line here.
> 
> >  }
> >  
> >  void otx2_sqb_flush(struct otx2_nic *pfvf)
> 
> ...