Re: [PATCH v4 net-next 04/14] net: enetc: add MAC filter for i.MX95 ENETC PF

[Vladimir Oltean <vladimir.oltean@nxp.com>]

Hi Claudiu,

On Tue, Mar 18, 2025 at 10:08:24AM +0200, Claudiu Manoil wrote:
> 
> > -----Original Message-----
> > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > Sent: Monday, March 17, 2025 4:18 PM
> [...]
> > Subject: Re: [PATCH v4 net-next 04/14] net: enetc: add MAC filter for i.MX95
> > ENETC PF
> > 
> > On Tue, Mar 11, 2025 at 01:38:20PM +0800, Wei Fang wrote:
> [...]
> > > +static void enetc4_pf_destroy_mac_list(struct enetc_pf *pf)
> > > +{
> > > +	struct enetc_mac_list_entry *entry;
> > > +	struct hlist_node *tmp;
> > > +
> > > +	mutex_lock(&pf->mac_list_lock);
> > 
> > The mutex_lock() usage here should raise serious questions. This is
> > running right before mutex_destroy(). So if there were any concurrent
> > attempt to acquire this lock, that concurrent code would have been broken
> > any time it would have lost arbitration, by the fact that it would
> > attempt to acquire a destroyed mutex.
> > 
> > But there's no such concurrent thread, because we run after destroy_workqueue()
> > which flushes those concurrent calls and prevents new ones. So the mutex
> > usage here is not necessary.
> > 
> > [ same thing with mutex_init() immediately followed by mutex_lock().
> >   It is an incorrect pattern most of the time. ]
> > 
> 
> This is not as bad as it seems. In the final version of the code, mutex 'mac_list_lock'
> serializes the access btw the thread programming the filter for the PF instance,
> and the threads programming the filter on behalf of underlying VFs (triggered by async
> request from VF). But since VF support is not included in this patch set (as Wei
> already mentioned) the lock can/should be added later, with the VF patches.

We all agree that the lock is unnecessary as far as the logic presented
in this patch set is concerned, and thus should not be added.

When Wei will present new code that will make explicit serialization
necessary, we can comment more. But, I don't see how any extra logic
can change the basic fact I was pointing out in the portion that you've
quoted. Before you destroy the mutex, you need to do something that
ensures concurrent threads can no longer execute. Then, there's no point
in locking in enetc4_pf_destroy_mac_list(). I expect not to see such
locking there in future patch sets.