Hi, > Gesendet: Mittwoch, 24. Mai 2017 um 11:06 Uhr > Von: "Stefan Wahren" <[email protected]> > An: "Lino Sanfilippo" <[email protected]>, "Rob Herring" > <[email protected]>, "Mark Rutland" <[email protected]>, "David S. > Miller" <[email protected]> > Cc: [email protected], "Jiri Slaby" <[email protected]>, "Greg > Kroah-Hartman" <[email protected]>, [email protected], > [email protected], "Jakub Kicinski" <[email protected]>, > [email protected] > Betreff: Re: [PATCH v6 net-next 17/17] net: qualcomm: add QCA7000 UART driver > > Am 23.05.2017 um 23:01 schrieb Lino Sanfilippo: > > On 23.05.2017 21:38, Stefan Wahren wrote: > >>> Lino Sanfilippo <[email protected]> hat am 23. Mai 2017 um 20:16 > >>> geschrieben: > >>> > >>> I suggest to avoid this possible race by first unregistering the > >>> netdevice and then > >>> calling cancel_work_sync(). > >> What makes you sure that's safe to unregister the netdev while the tx work > >> queue is possibly active? > > unregister_netdevice() calls netdev_close() if the interface is still up. > > netdev_close() calls flush_work() > > so the unregistration is delayed until the tx work function is finished. > > Furthermore both close() and > > tx work are synchronized by means of the qca->lock which also guarantees > > that unregister_netdevice() wont > > be finished until the tx work is done. > > > > Thanks for the explanation. I suspect there could be the same race > between serdev_device_close() and the tx work queue. > > So i would propose a variant of your original suggestion: > > unregister_netdev(qca->net_dev); > > /* Flush any pending characters in the driver. */ > serdev_device_close(serdev); > cancel_work_sync(&qca->tx_work); > > Since we have the same pattern in the error path of the probe function, > the same applies there.
Agreed, it is much cleaner to have the same cleanup pattern in remove() as we have in (error case of) probe(). Regards, Lino
