Message ID | 20210929140854.28535-1-l.stelmach@samsung.com |
---|---|
Headers | show |
Series | AX88796C SPI Ethernet Adapter | expand |
On Wed, 29 Sep 2021 16:08:54 +0200 Łukasz Stelmach wrote: > +static char *no_regs_list = "80018001,e1918001,8001a001,fc0d0000"; static const char ... > +static int > +ax88796c_close(struct net_device *ndev) > +{ > + struct ax88796c_device *ax_local = to_ax88796c_device(ndev); > + > + netif_stop_queue(ndev); This can run concurrently with the work which restarts the queue. You should take the mutex and purge the queue here, so that there is no chance queue will get restarted by the work right after. > + phy_stop(ndev->phydev); > + > + mutex_lock(&ax_local->spi_lock); > +MODULE_AUTHOR("ASIX"); You can drop this, author should be human.
It was <2021-10-01 pią 15:39>, when Jakub Kicinski wrote: > On Wed, 29 Sep 2021 16:08:54 +0200 Łukasz Stelmach wrote: >> +static char *no_regs_list = "80018001,e1918001,8001a001,fc0d0000"; > > static const char ... > >> +static int >> +ax88796c_close(struct net_device *ndev) >> +{ >> + struct ax88796c_device *ax_local = to_ax88796c_device(ndev); >> + >> + netif_stop_queue(ndev); > > This can run concurrently with the work which restarts the queue. > You should take the mutex and purge the queue here, so that there > is no chance queue will get restarted by the work right after. >> + phy_stop(ndev->phydev); >> + >> + mutex_lock(&ax_local->spi_lock); This was a bit tricky. The function now looks like this (details in the comments). In theory there is a chance of dropping a packet if the ax88796c_work() runs between phy_stop() and mutex_lock(), but I'd say the overall risk is negligible. --8<---------------cut here---------------start------------->8--- static int ax88796c_close(struct net_device *ndev) { struct ax88796c_device *ax_local = to_ax88796c_device(ndev); phy_stop(ndev->phydev); /* We lock the mutex early not only to protect the device * against concurrent access, but also avoid waking up the * queue in ax88796c_work(). phy_stop() needs to be called * before because it locks the mutex to access SPI. */ mutex_lock(&ax_local->spi_lock); netif_stop_queue(ndev); /* No more work can be scheduled now. Make any pending work, * including one already waiting for the mutex to be unlocked, * NOP. */ clear_bit(EVENT_SET_MULTI, &ax_local->flags); clear_bit(EVENT_INTR, &ax_local->flags); clear_bit(EVENT_TX, &ax_local->flags); /* Disable MAC interrupts */ AX_WRITE(&ax_local->ax_spi, IMR_MASKALL, P0_IMR); __skb_queue_purge(&ax_local->tx_wait_q); ax88796c_soft_reset(ax_local); mutex_unlock(&ax_local->spi_lock); cancel_work_sync(&ax_local->ax_work); free_irq(ndev->irq, ndev); return 0; } --8<---------------cut here---------------end--------------->8--- >> +MODULE_AUTHOR("ASIX"); > > You can drop this, author should be human. I looked at diff --stat and decided it is OK to put my name there (-;