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 🤷