On Fri, 2022-02-11 at 14:38 -0800, Jakub Kicinski wrote:
>
> > +// Removes and unregisters a mctp-i2c netdev
> > +static void mctp_i2c_free_netdev(struct mctp_i2c_dev *midev)
> >
> You're doing a lot before the unregister call, this is likely racy.
> The usual flow is to unregister the netdev, then do uninit, then free.
> For instance you purge the queue but someone may Tx afterwards.
> needs_free_netdev is a footgun.
Thanks Jakub. I've reworked it here to do the work before register/after
unregister, without needs_free_netdev.
One question, the tx thread calls netif_wake_queue() - is it safe to call
that after unregister_netdev()? (before free_netdev)
I've moved the kthread_stop() to the post-unregister cleanup.
static int mctp_i2c_tx_thread(void *data)
{
struct mctp_i2c_dev *midev = data;
struct sk_buff *skb;
unsigned long flags;
for (;;) {
if (kthread_should_stop())
break;
spin_lock_irqsave(&midev->tx_queue.lock, flags);
skb = __skb_dequeue(&midev->tx_queue);
if (netif_queue_stopped(midev->ndev))
netif_wake_queue(midev->ndev); // <-------
spin_unlock_irqrestore(&midev->tx_queue.lock, flags);
> > + INIT_LIST_HEAD(&mi_driver_state.clients);
> > + mutex_init(&mi_driver_state.lock);
>
> I think there are static initializers for these.
*nod*
Thanks,
Matt
