diff mbox series

[net] octeontx2-af: Enable hardware timestamping for VFs

Message ID 20230929071621.853570-1-saikrishnag@marvell.com
State New
Headers
Series [net] octeontx2-af: Enable hardware timestamping for VFs |

Commit Message

Sai Krishna Gajula Sept. 29, 2023, 7:16 a.m. UTC 
  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(-)

Comments

Jacob Keller Sept. 29, 2023, 5:54 p.m. UTC | #1 
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);
Sai Krishna Gajula Oct. 3, 2023, 7:21 a.m. UTC | #2 
> -----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);
Jakub Kicinski Oct. 4, 2023, 9:12 p.m. UTC | #3 
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.
Sai Krishna Gajula Oct. 11, 2023, 5:43 a.m. UTC | #4 
> -----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 mbox series

Patch

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