| Message ID | 20241003105940.533921-1-danishanwar@ti.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [net] net: ti: icssg-prueth: Fix race condition for VLAN table access | expand |
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'
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.
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
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.
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.
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 {
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(+) base-commit: 1127c73a8d4f803bb3d9e3d024b0863191d52e03