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)
>
> ...
