[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZoP4iG30sr9lZohg@localhost.localdomain>
Date: Tue, 2 Jul 2024 14:54:32 +0200
From: Michal Kubiak <michal.kubiak@...el.com>
To: Srujana Challa <schalla@...vell.com>
CC: <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<kuba@...nel.org>, <davem@...emloft.net>, <pabeni@...hat.com>,
<sgoutham@...vell.com>, <lcherian@...vell.com>, <gakula@...vell.com>,
<jerinj@...vell.com>, <hkelam@...vell.com>, <sbhatta@...vell.com>, "Nithin
Dabilpuram" <ndabilpuram@...vell.com>
Subject: Re: [PATCH net,1/6] octeontx2-af: replace cpt slot with lf id on reg
write
On Mon, Jul 01, 2024 at 02:37:41PM +0530, Srujana Challa wrote:
> From: Nithin Dabilpuram <ndabilpuram@...vell.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@...vell.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
Powered by blists - more mailing lists