[net PATCH] octeontx2-af: Fix installation of PF multicast rules

[net PATCH] octeontx2-af: Fix installation of PF multicast rules

[Geetha sowjanya]

Due to target variable is being reassigned in npc_install_flow()
function, PF multicast rules are not getting installed.
This patch addresses the issue by fixing the "IF" condition
checks when rules are installed by AF.

Fixes: 6c40ca957fe5 ("octeontx2-pf: Adds TC offload support").
Signed-off-by: Geetha sowjanya <gakula@marvell.com>
---
 drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
index da69e454662a..8a2444a8b7d3 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
+++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
@@ -1457,14 +1457,14 @@ int rvu_mbox_handler_npc_install_flow(struct rvu *rvu,
 		target = req->vf;
 
 	/* PF installing for its VF */
-	if (!from_vf && req->vf && !from_rep_dev) {
+	else if (!from_vf && req->vf && !from_rep_dev) {
 		target = (req->hdr.pcifunc & ~RVU_PFVF_FUNC_MASK) | req->vf;
 		pf_set_vfs_mac = req->default_rule &&
 				(req->features & BIT_ULL(NPC_DMAC));
 	}
 
 	/* Representor device installing for a representee */
-	if (from_rep_dev && req->vf)
+	else if (from_rep_dev && req->vf)
 		target = req->vf;
 	else
 		/* msg received from PF/VF */
-- 
2.25.1

Re: [net PATCH] octeontx2-af: Fix installation of PF multicast rules

[Paolo Abeni]

On 11/27/24 12:48, Geetha sowjanya wrote:
> Due to target variable is being reassigned in npc_install_flow()
> function, PF multicast rules are not getting installed.
> This patch addresses the issue by fixing the "IF" condition
> checks when rules are installed by AF.
> 
> Fixes: 6c40ca957fe5 ("octeontx2-pf: Adds TC offload support").
> Signed-off-by: Geetha sowjanya <gakula@marvell.com>
> ---
>  drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
> index da69e454662a..8a2444a8b7d3 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
> @@ -1457,14 +1457,14 @@ int rvu_mbox_handler_npc_install_flow(struct rvu *rvu,
>  		target = req->vf;
>  
>  	/* PF installing for its VF */
> -	if (!from_vf && req->vf && !from_rep_dev) {
> +	else if (!from_vf && req->vf && !from_rep_dev) {

This IMHO makes the code quite unreadable and error-prone, as the else
branches are quite separate from the 'if' statement and easy to miss.

It also breaks the kernel style, as you must apply the curly brackets on
all the branches, if one of them is using them.

Please restructure the code a bit:

	if (!req->hdr.pcifunc) {
		/* AF installing for a PF/VF */
		target = req->vf;
	} else if (!from_vf && req->vf && !from_rep_dev) {
		/* PF installing for its VF */
		...

Thanks,

Paolo

RE: [EXTERNAL] Re: [net PATCH] octeontx2-af: Fix installation of PF multicast rules

[Geethasowjanya Akula]

>-----Original Message-----
>From: Paolo Abeni <pabeni@redhat.com>
>Sent: Tuesday, December 3, 2024 3:11 PM
>To: Geethasowjanya Akula <gakula@marvell.com>; netdev@vger.kernel.org;
>linux-kernel@vger.kernel.org
>Cc: kuba@kernel.org; davem@davemloft.net; horms@kernel.org;
>andrew+netdev@lunn.ch; edumazet@google.com; Sunil Kovvuri Goutham
><sgoutham@marvell.com>; Subbaraya Sundeep Bhatta
><sbhatta@marvell.com>; Hariprasad Kelam <hkelam@marvell.com>
>Subject: [EXTERNAL] Re: [net PATCH] octeontx2-af: Fix installation of PF
>multicast rules
>
>On 11/27/24 12:48, Geetha sowjanya wrote:
>> Due to target variable is being reassigned in npc_install_flow()
>> function, PF multicast rules are not getting installed.
>> This patch addresses the issue by fixing the "IF" condition checks
>> when rules are installed by AF.
>>
>> Fixes: 6c40ca957fe5 ("octeontx2-pf: Adds TC offload support").
>> Signed-off-by: Geetha sowjanya <gakula@marvell.com>
>> ---
>>  drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
>> b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
>> index da69e454662a..8a2444a8b7d3 100644
>> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
>> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
>> @@ -1457,14 +1457,14 @@ int rvu_mbox_handler_npc_install_flow(struct
>rvu *rvu,
>>  		target = req->vf;
>>
>>  	/* PF installing for its VF */
>> -	if (!from_vf && req->vf && !from_rep_dev) {
>> +	else if (!from_vf && req->vf && !from_rep_dev) {
>
>This IMHO makes the code quite unreadable and error-prone, as the else
>branches are quite separate from the 'if' statement and easy to miss.
>
>It also breaks the kernel style, as you must apply the curly brackets on all the
>branches, if one of them is using them.
>
>Please restructure the code a bit:
Ok will restructure in next version.

Thanks,
Geetha.
>
>	if (!req->hdr.pcifunc) {
>		/* AF installing for a PF/VF */
>		target = req->vf;
>	} else if (!from_vf && req->vf && !from_rep_dev) {
>		/* PF installing for its VF */
>		...
>
>Thanks,
>
>Paolo