[PATCH net] net: ti: icssg-prueth: Fix race condition for VLAN table access

[PATCH net] net: ti: icssg-prueth: Fix race condition for VLAN table access

[MD Danish Anwar]

The VLAN table is a shared memory between the two ports/slices
in a ICSSG cluster and this may lead to race condition when the
common code paths for both ports are executed in different CPUs.

Fix the race condition access by locking the shared memory access

Fixes: 487f7323f39a ("net: ti: icssg-prueth: Add helper functions to configure FDB")
Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
---
 drivers/net/ethernet/ti/icssg/icssg_config.c | 2 ++
 drivers/net/ethernet/ti/icssg/icssg_prueth.c | 1 +
 drivers/net/ethernet/ti/icssg/icssg_prueth.h | 1 +
 3 files changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.c b/drivers/net/ethernet/ti/icssg/icssg_config.c
index 72ace151d8e9..5d2491c2943a 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_config.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_config.c
@@ -735,6 +735,7 @@ void icssg_vtbl_modify(struct prueth_emac *emac, u8 vid, u8 port_mask,
 	u8 fid_c1;
 
 	tbl = prueth->vlan_tbl;
+	spin_lock(&prueth->vtbl_lock);
 	fid_c1 = tbl[vid].fid_c1;
 
 	/* FID_C1: bit0..2 port membership mask,
@@ -750,6 +751,7 @@ void icssg_vtbl_modify(struct prueth_emac *emac, u8 vid, u8 port_mask,
 	}
 
 	tbl[vid].fid_c1 = fid_c1;
+	spin_unlock(&prueth->vtbl_lock);
 }
 EXPORT_SYMBOL_GPL(icssg_vtbl_modify);
 
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
index 5fd9902ab181..5c20ceb164df 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
@@ -1442,6 +1442,7 @@ static int prueth_probe(struct platform_device *pdev)
 		icss_iep_init_fw(prueth->iep1);
 	}
 
+	spin_lock_init(&prueth->vtbl_lock);
 	/* setup netdev interfaces */
 	if (eth0_node) {
 		ret = prueth_netdev_init(prueth, eth0_node);
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
index bba6da2e6bd8..9a33e9ed2976 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
@@ -296,6 +296,7 @@ struct prueth {
 	bool is_switchmode_supported;
 	unsigned char switch_id[MAX_PHYS_ITEM_ID_LEN];
 	int default_vlan;
+	spinlock_t vtbl_lock; /* Lock for vtbl in shared memory */
 };
 
 struct emac_tx_ts_response {

base-commit: 1127c73a8d4f803bb3d9e3d024b0863191d52e03
-- 
2.34.1

Re: [PATCH net] net: ti: icssg-prueth: Fix race condition for VLAN table access

[Jakub Kicinski]

On Thu, 3 Oct 2024 16:29:40 +0530 MD Danish Anwar wrote:
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> index bba6da2e6bd8..9a33e9ed2976 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> @@ -296,6 +296,7 @@ struct prueth {
>  	bool is_switchmode_supported;
>  	unsigned char switch_id[MAX_PHYS_ITEM_ID_LEN];
>  	int default_vlan;
> +	spinlock_t vtbl_lock; /* Lock for vtbl in shared memory */

This needs to be kdoc, otherwise:

drivers/net/ethernet/ti/icssg/icssg_prueth.h:301: warning: Function parameter or struct member 'vtbl_lock' not described in 'prueth'
-- 
pw-bot: cr

Re: [PATCH net] net: ti: icssg-prueth: Fix race condition for VLAN table access

[MD Danish Anwar]

On 04/10/24 6:11 am, Jakub Kicinski wrote:
> On Thu, 3 Oct 2024 16:29:40 +0530 MD Danish Anwar wrote:
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>> index bba6da2e6bd8..9a33e9ed2976 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>> @@ -296,6 +296,7 @@ struct prueth {
>>  	bool is_switchmode_supported;
>>  	unsigned char switch_id[MAX_PHYS_ITEM_ID_LEN];
>>  	int default_vlan;
>> +	spinlock_t vtbl_lock; /* Lock for vtbl in shared memory */
> 
> This needs to be kdoc, otherwise:
> 
> drivers/net/ethernet/ti/icssg/icssg_prueth.h:301: warning: Function parameter or struct member 'vtbl_lock' not described in 'prueth'

Hi Jakub,

Removing the documentation from here and keeping it in kdoc results in
below checkpatch,

CHECK: spinlock_t definition without comment
#69: FILE: drivers/net/ethernet/ti/icssg/icssg_prueth.h:300:
+	spinlock_t vtbl_lock;


What should be done here? Should I,

1. Move the documentation to kdoc - This is will result in checkpatch
2. Keep the documentation in kdoc as well as inline - This will result
in no warnings but duplicate documentation which I don't think is good.

I was not sure which one takes more precedence check patch or kdoc, thus
put it inline thinking fixing checkpatch might have more weightage.

Let me know what should be done here.

-- 
Thanks and Regards,
Danish

Re: [PATCH net] net: ti: icssg-prueth: Fix race condition for VLAN table access

[Simon Horman]

On Fri, Oct 04, 2024 at 10:25:05AM +0530, MD Danish Anwar wrote:
> 
> 
> On 04/10/24 6:11 am, Jakub Kicinski wrote:
> > On Thu, 3 Oct 2024 16:29:40 +0530 MD Danish Anwar wrote:
> >> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> >> index bba6da2e6bd8..9a33e9ed2976 100644
> >> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> >> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> >> @@ -296,6 +296,7 @@ struct prueth {
> >>  	bool is_switchmode_supported;
> >>  	unsigned char switch_id[MAX_PHYS_ITEM_ID_LEN];
> >>  	int default_vlan;
> >> +	spinlock_t vtbl_lock; /* Lock for vtbl in shared memory */
> > 
> > This needs to be kdoc, otherwise:
> > 
> > drivers/net/ethernet/ti/icssg/icssg_prueth.h:301: warning: Function parameter or struct member 'vtbl_lock' not described in 'prueth'
> 
> Hi Jakub,
> 
> Removing the documentation from here and keeping it in kdoc results in
> below checkpatch,
> 
> CHECK: spinlock_t definition without comment
> #69: FILE: drivers/net/ethernet/ti/icssg/icssg_prueth.h:300:
> +	spinlock_t vtbl_lock;
> 
> 
> What should be done here? Should I,
> 
> 1. Move the documentation to kdoc - This is will result in checkpatch
> 2. Keep the documentation in kdoc as well as inline - This will result
> in no warnings but duplicate documentation which I don't think is good.
> 
> I was not sure which one takes more precedence check patch or kdoc, thus
> put it inline thinking fixing checkpatch might have more weightage.
> 
> Let me know what should be done here.

FWIIW, my preference would be for option 2.

I think it is important that Kernel doc is accurate, as it can end
up incorporated in documentation. And moreover, what is the point
if it is missing bits?

I feel less strongly about the checkpatch bit, but it does seem
to be worthwhile following that practice too.

Maybe you can avoid duplication by making the two location document
different aspects of the field. Or maybe that is silly 🤷

Re: [PATCH net] net: ti: icssg-prueth: Fix race condition for VLAN table access

[Jakub Kicinski]

On Fri, 4 Oct 2024 11:46:10 +0100 Simon Horman wrote:
> > 1. Move the documentation to kdoc - This is will result in checkpatch
> > 2. Keep the documentation in kdoc as well as inline - This will result
> > in no warnings but duplicate documentation which I don't think is good.
> > 
> > I was not sure which one takes more precedence check patch or kdoc, thus
> > put it inline thinking fixing checkpatch might have more weightage.
> > 
> > Let me know what should be done here.  
> 
> FWIIW, my preference would be for option 2.

Of the two options I'd pick 1, perhaps due to my deeply seated
"disappointment" in the quality of checkpatch warnings :)
Complaining about missing comment when there's a kdoc is a false
positive in my book. But option 2 works, too.

I haven't tested it but there's also the option 3 - providing 
the kdoc inline, something like:

+	/** @vtbl_lock: Lock for vtbl in shared memory */
+	spinlock_t vtbl_lock;

Again, no strong preference on which option you choose.
kdoc warnings may get emitted during builds so we should avoid them.

Re: [PATCH net] net: ti: icssg-prueth: Fix race condition for VLAN table access

[Anwar, Md Danish]

On 10/5/2024 1:37 AM, Jakub Kicinski wrote:
> On Fri, 4 Oct 2024 11:46:10 +0100 Simon Horman wrote:
>>> 1. Move the documentation to kdoc - This is will result in checkpatch
>>> 2. Keep the documentation in kdoc as well as inline - This will result
>>> in no warnings but duplicate documentation which I don't think is good.
>>>
>>> I was not sure which one takes more precedence check patch or kdoc, thus
>>> put it inline thinking fixing checkpatch might have more weightage.
>>>
>>> Let me know what should be done here.  
>>
>> FWIIW, my preference would be for option 2.
> 
> Of the two options I'd pick 1, perhaps due to my deeply seated
> "disappointment" in the quality of checkpatch warnings :)
> Complaining about missing comment when there's a kdoc is a false
> positive in my book. But option 2 works, too.
> 
> I haven't tested it but there's also the option 3 - providing 
> the kdoc inline, something like:
> 
> +	/** @vtbl_lock: Lock for vtbl in shared memory */
> +	spinlock_t vtbl_lock;
> 

Hi Jakub, I tested this and option 3 works. I don't see either kdoc or
checkpatch warning. I will go ahead and re spin the patch with option 3.

> Again, no strong preference on which option you choose.
> kdoc warnings may get emitted during builds so we should avoid them.

-- 
Thanks and Regards,
Md Danish Anwar