RE: [EXTERNAL] Re: [PATCH net,1/6] octeontx2-af: replace cpt slot with lf id on reg write

[Srujana Challa]

> > From: Nithin Dabilpuram <ndabilpuram@xxxxxxxxxxx>
> >
> > Replace cpt slot id with lf id on reg read/write as CPTPF/VF driver
> > would send slot number instead of lf id in the reg offset.
> >
> > Fixes: ae454086e3c2 ("octeontx2-af: add mailbox interface for CPT")
> > Signed-off-by: Nithin Dabilpuram <ndabilpuram@xxxxxxxxxxx>
> > ---
> >  drivers/net/ethernet/marvell/octeontx2/af/rvu_cpt.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_cpt.c
> > b/drivers/net/ethernet/marvell/octeontx2/af/rvu_cpt.c
> > index f047185f38e0..98440a0241a2 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_cpt.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_cpt.c
> > @@ -663,6 +663,8 @@ static bool is_valid_offset(struct rvu *rvu, struct
> cpt_rd_wr_reg_msg *req)
> >  		if (lf < 0)
> >  			return false;
> >
> > +		req->reg_offset &= 0xFF000;
> > +		req->reg_offset += lf << 3;
> 
> I think it's not great to modify an input parameter from the function named
> like "is_valid_offset()". From the function like that I would rather expect doing
> just a simple check if the parameter is correct.
> It seems calling that function from a different context can be risky now.
I’ll make the necessary changes to the code to ensure that we avoid modifying
the input parameter within the is_valid_offset() function in next version.

> 
> >  		return true;
> >  	} else if (!(req->hdr.pcifunc & RVU_PFVF_FUNC_MASK)) {
> >  		/* Registers that can be accessed from PF */ @@ -707,12
> +709,13 @@
> > int rvu_mbox_handler_cpt_rd_wr_register(struct rvu *rvu,
> >  	    !is_cpt_vf(rvu, req->hdr.pcifunc))
> >  		return CPT_AF_ERR_ACCESS_DENIED;
> >
> > +	if (!is_valid_offset(rvu, req))
> > +		return CPT_AF_ERR_ACCESS_DENIED;
> > +
> >  	rsp->reg_offset = req->reg_offset;
> >  	rsp->ret_val = req->ret_val;
> >  	rsp->is_write = req->is_write;
> >
> > -	if (!is_valid_offset(rvu, req))
> > -		return CPT_AF_ERR_ACCESS_DENIED;
> 
> Is moving that call also a necessary part of the fix? Or is it just an extra
> improvement?
> Maybe it's worth mentioning in the commit message?
is_valid_offset() call is moved to ensure that rsp->reg_offset is correctly updated
with the modified req->reg_offset.

Thanks for the feedback.
> 
> >
> >  	if (req->is_write)
> >  		rvu_write64(rvu, blkaddr, req->reg_offset, req->val);
> > --
> > 2.25.1
> >
> >
> 
> 
> Thanks,
> Michal