| Message ID | 20230929071621.853570-1-saikrishnag@marvell.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] octeontx2-af: Enable hardware timestamping for VFs | expand |
On 9/29/2023 12:16 AM, Sai Krishna wrote: > Currently for VFs, mailbox returns error when hardware timestamping enable > is requested. This patch fixes this issue. > The subject title implies that this is implementing a new feature (and thus not really a good candidate for net), but the content implies this is a fix for an existing feature thats not working properly. It could use a slightly improved commit message. > Fixes: 421572175ba5 ("octeontx2-af: Support to enable/disable HW timestamping") > Signed-off-by: Subbaraya Sundeep <sbhatta@marvell.com> > Signed-off-by: Sunil Kovvuri Goutham <sgoutham@marvell.com> > Signed-off-by: Sai Krishna <saikrishnag@marvell.com> Typically the author signed-off-by should go first, and then other signed-off-by are for people in the chain of patch delivery. If the other two co-authored the patch they should have Co-developed-by tag or something. Otherwise I would expect that Subbaraya Sundeepd would be the patch author since that signed-off-by is first. > --- > drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > The fix itself seems straight forward, though the comment is confusing to me. You also changed the return from -ENODEV to -EPERM which makes sense enough I suppose, but thats not called out in the change. > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c > index f2b1edf1bb43..aba0c530160c 100644 > --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c > +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c > @@ -756,12 +756,11 @@ static int rvu_cgx_ptp_rx_cfg(struct rvu *rvu, u16 pcifunc, bool enable) > if (!is_mac_feature_supported(rvu, pf, RVU_LMAC_FEAT_PTP)) > return 0; > > - /* This msg is expected only from PFs that are mapped to CGX LMACs, > + /* This msg is expected only from PFs that are mapped to CGX/RPM LMACs, > * if received from other PF/VF simply ACK, nothing to do. > */ This comment implies to me that we wouldn't expect this message from a VF? > - if ((pcifunc & RVU_PFVF_FUNC_MASK) || > - !is_pf_cgxmapped(rvu, pf)) > - return -ENODEV; > + if (!is_pf_cgxmapped(rvu, rvu_get_pf(pcifunc))) > + return -EPERM; > > rvu_get_cgx_lmac_id(rvu->pf2cgxlmac_map[pf], &cgx_id, &lmac_id); > cgxd = rvu_cgx_pdata(cgx_id, rvu);
> -----Original Message----- > From: Jacob Keller <jacob.e.keller@intel.com> > Sent: Friday, September 29, 2023 11:24 PM > To: Sai Krishna Gajula <saikrishnag@marvell.com>; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Sunil Kovvuri > Goutham <sgoutham@marvell.com>; Geethasowjanya Akula > <gakula@marvell.com>; richardcochran@gmail.com; Linu Cherian > <lcherian@marvell.com>; Jerin Jacob Kollanukkaran <jerinj@marvell.com>; > Hariprasad Kelam <hkelam@marvell.com>; Subbaraya Sundeep Bhatta > <sbhatta@marvell.com> > Subject: Re: [net PATCH] octeontx2-af: Enable hardware timestamping > for VFs > > > On 9/29/2023 12:16 AM, Sai Krishna wrote: > > Currently for VFs, mailbox returns error when hardware timestamping > > enable is requested. This patch fixes this issue. > > > > The subject title implies that this is implementing a new feature (and thus > not really a good candidate for net), but the content implies this is a fix for an > existing feature thats not working properly. > > It could use a slightly improved commit message. Ack, I will modify the commit message in V2 > > > Fixes: 421572175ba5 ("octeontx2-af: Support to enable/disable HW > > timestamping") > > Signed-off-by: Subbaraya Sundeep <sbhatta@marvell.com> > > Signed-off-by: Sunil Kovvuri Goutham <sgoutham@marvell.com> > > Signed-off-by: Sai Krishna <saikrishnag@marvell.com> > > Typically the author signed-off-by should go first, and then other signed-off- > by are for people in the chain of patch delivery. If the other two co-authored > the patch they should have Co-developed-by tag or something. Otherwise I > would expect that Subbaraya Sundeepd would be the patch author since > that signed-off-by is first. Ack, I will change the author name in V2 > > > --- > > drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c | 7 +++---- > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > The fix itself seems straight forward, though the comment is confusing to > me. You also changed the return from -ENODEV to -EPERM which makes > sense enough I suppose, but thats not called out in the change. > Ack, I will modify the commit message in V2 > > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c > > b/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c > > index f2b1edf1bb43..aba0c530160c 100644 > > --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c > > +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c > > @@ -756,12 +756,11 @@ static int rvu_cgx_ptp_rx_cfg(struct rvu *rvu, u16 > pcifunc, bool enable) > > if (!is_mac_feature_supported(rvu, pf, RVU_LMAC_FEAT_PTP)) > > return 0; > > > > - /* This msg is expected only from PFs that are mapped to CGX > LMACs, > > + /* This msg is expected only from PFs that are mapped to CGX/RPM > > +LMACs, > > * if received from other PF/VF simply ACK, nothing to do. > > */ > > This comment implies to me that we wouldn't expect this message from a > VF? Ack, I will modify the comment in V2 > > > - if ((pcifunc & RVU_PFVF_FUNC_MASK) || > > - !is_pf_cgxmapped(rvu, pf)) > > - return -ENODEV; > > + if (!is_pf_cgxmapped(rvu, rvu_get_pf(pcifunc))) > > + return -EPERM; > > > > rvu_get_cgx_lmac_id(rvu->pf2cgxlmac_map[pf], &cgx_id, > &lmac_id); > > cgxd = rvu_cgx_pdata(cgx_id, rvu);
On Fri, 29 Sep 2023 12:46:21 +0530 Sai Krishna wrote: > Subject: [net PATCH] octeontx2-af: Enable hardware timestamping for VFs > Date: Fri, 29 Sep 2023 12:46:21 +0530 > X-Mailer: git-send-email 2.25.1 > > Currently for VFs, mailbox returns error when hardware timestamping enable > is requested. This patch fixes this issue. > > Fixes: 421572175ba5 ("octeontx2-af: Support to enable/disable HW timestamping") If it never worked for VFs but worked for PFs - it's not really a fix. New functionality enablement is -next material, even if the fact that it was previously disable was unintentional.
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Thursday, October 5, 2023 2:43 AM > To: Sai Krishna Gajula <saikrishnag@marvell.com> > Cc: davem@davemloft.net; edumazet@google.com; pabeni@redhat.com; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Sunil Kovvuri > Goutham <sgoutham@marvell.com>; Geethasowjanya Akula > <gakula@marvell.com>; richardcochran@gmail.com; Linu Cherian > <lcherian@marvell.com>; Jerin Jacob Kollanukkaran <jerinj@marvell.com>; > Hariprasad Kelam <hkelam@marvell.com>; Subbaraya Sundeep Bhatta > <sbhatta@marvell.com> > Subject: Re: [net PATCH] octeontx2-af: Enable hardware timestamping > for VFs > > On Fri, 29 Sep 2023 12:46:21 +0530 Sai Krishna wrote: > > Subject: [net PATCH] octeontx2-af: Enable hardware timestamping for > > VFs > > Date: Fri, 29 Sep 2023 12:46:21 +0530 > > X-Mailer: git-send-email 2.25.1 > > > > Currently for VFs, mailbox returns error when hardware timestamping > > enable is requested. This patch fixes this issue. > > > > Fixes: 421572175ba5 ("octeontx2-af: Support to enable/disable HW > > timestamping") > > If it never worked for VFs but worked for PFs - it's not really a fix. > New functionality enablement is -next material, even if the fact that it was > previously disable was unintentional. Ack, will submit the patch to net-next. > -- > pw-bot: cr
diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c index f2b1edf1bb43..aba0c530160c 100644 --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c @@ -756,12 +756,11 @@ static int rvu_cgx_ptp_rx_cfg(struct rvu *rvu, u16 pcifunc, bool enable) if (!is_mac_feature_supported(rvu, pf, RVU_LMAC_FEAT_PTP)) return 0; - /* This msg is expected only from PFs that are mapped to CGX LMACs, + /* This msg is expected only from PFs that are mapped to CGX/RPM LMACs, * if received from other PF/VF simply ACK, nothing to do. */ - if ((pcifunc & RVU_PFVF_FUNC_MASK) || - !is_pf_cgxmapped(rvu, pf)) - return -ENODEV; + if (!is_pf_cgxmapped(rvu, rvu_get_pf(pcifunc))) + return -EPERM; rvu_get_cgx_lmac_id(rvu->pf2cgxlmac_map[pf], &cgx_id, &lmac_id); cgxd = rvu_cgx_pdata(cgx_id, rvu);
Currently for VFs, mailbox returns error when hardware timestamping enable is requested. This patch fixes this issue. Fixes: 421572175ba5 ("octeontx2-af: Support to enable/disable HW timestamping") Signed-off-by: Subbaraya Sundeep <sbhatta@marvell.com> Signed-off-by: Sunil Kovvuri Goutham <sgoutham@marvell.com> Signed-off-by: Sai Krishna <saikrishnag@marvell.com> --- drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)