Hi, > Gesendet: Mittwoch, 24. Mai 2017 um 11:06 Uhr > Von: "Stefan Wahren" <stefan.wah...@i2se.com> > An: "Lino Sanfilippo" <linosanfili...@gmx.de>, "Rob Herring" > <robh...@kernel.org>, "Mark Rutland" <mark.rutl...@arm.com>, "David S. > Miller" <da...@davemloft.net> > Cc: linux-ser...@vger.kernel.org, "Jiri Slaby" <jsl...@suse.com>, "Greg > Kroah-Hartman" <gre...@linuxfoundation.org>, netdev@vger.kernel.org, > linux-ker...@vger.kernel.org, "Jakub Kicinski" <kubak...@wp.pl>, > devicet...@vger.kernel.org > 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 <linosanfili...@gmx.de> 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
