| Message ID | 20240701090746.2171565-2-schalla@marvell.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fixes for CPT and RSS configuration | expand |
On Mon, Jul 01, 2024 at 02:37:41PM +0530, Srujana Challa wrote: > From: Nithin Dabilpuram <ndabilpuram@marvell.com> > > 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@marvell.com> > --- > 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. > 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? > > if (req->is_write) > rvu_write64(rvu, blkaddr, req->reg_offset, req->val); > -- > 2.25.1 > > Thanks, Michal
> > From: Nithin Dabilpuram <ndabilpuram@marvell.com> > > > > 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@marvell.com> > > --- > > 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
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; 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; if (req->is_write) rvu_write64(rvu, blkaddr, req->reg_offset, req->val);