From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1162547AbdEWVNx (ORCPT ); Tue, 23 May 2017 17:13:53 -0400 Received: from mout.gmx.net ([212.227.17.20]:53480 "EHLO mout.gmx.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1034015AbdEWVBY (ORCPT ); Tue, 23 May 2017 17:01:24 -0400 Subject: Re: [PATCH v6 net-next 17/17] net: qualcomm: add QCA7000 UART driver To: Stefan Wahren , Rob Herring , Mark Rutland , "David S. Miller" Cc: linux-serial@vger.kernel.org, Jiri Slaby , Greg Kroah-Hartman , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Jakub Kicinski , devicetree@vger.kernel.org References: <1495545173-22150-1-git-send-email-stefan.wahren@i2se.com> <1495545173-22150-18-git-send-email-stefan.wahren@i2se.com> <053235ad-a963-6a09-ccb0-b643115dee00@gmx.de> <1059621060.236992.1495568289051@email.1und1.de> From: Lino Sanfilippo Message-ID: <41c7302e-b25c-9f57-470a-dd95200a060f@gmx.de> Date: Tue, 23 May 2017 23:01:01 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: <1059621060.236992.1495568289051@email.1und1.de> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Provags-ID: V03:K0:3ZCFvfJ3fPTNlamEmeH2yTATroyQbPG+hjuuwo50FrqBiTxBzPd bf54PGv29EI9owclfk3/CddAaM1ToqHtPYo9j30oY4ys7duajlVN/9apP5aW2W9aoXjV0ow 641b99CLq7aOrkX3Xw/+TlNOTtWFOpjmOtjBZrAUsF+4essd5rK8QnuvK9Cv9rgm9Am56xN 4g/ZULgtxSCfeiThd1CMw== X-UI-Out-Filterresults: notjunk:1;V01:K0:Vl3+7DkKr9M=:fQpPaA2FmxZdh4GB3SJ3gq OHDFQg65wH1hNfP+5hbf0gSSUAFDSI7xT9mtbZkEeSHgPpOiS1ZEISRDVOkn9tR8IYzLWLalY kJA4S9tsGCDmBqG4Wn0jN1xuv+K+YeavaVKYjjTvPqmPjlX8m4kr4xdmJqXTCaYWV/TOE5waf GRcCFk/PjWHTj9yIlzzWW00sqEnkouvmQ4RktW3J9xD6qrP7q1Xz6GCq6jJtQ72zY2ClLsOy2 FbUdl9L4iIqA4qSQT3zxJrSPXqt1mhQx9RoQK7RQ4Z1wwYyiP5zTSh7gxFKRL8XZmt5LBtKun ekWvAT47uFonrjs7RvI31lB9FTKwGxW32k4bsvzHUsCnnaWwGsRpEKP4a/Ust/JxtgFEcJg5a Jri9Z+eSHDxpMKvvgJTdeO2vLYn0lmOT9CFP1FhBp+U0Ux58fqWiFHcTZNtaRV6CuuPenfQ/w Zwh8KyMgplOkUQ7/q2SJhwnuNn5TeYq8gBxqXW8T/UVtpvPLRrJK6Rsu6jdheJeBBDEmwl6y/ Iy8VG04Wz2EFg5Ot4v1O42+8BpZoGUBryobFnGoxcjmcMSrSqvao21iaIFtAXvpGA1ki5g1Jg XUba5/pxSgif2MqplwYxRFwzGMnup0EGmoZyDhtyowWI4ArR0KBvkjZoxOaLbzS3JYBS++fIE oeqjVe5Sjzk2Go0nEtzkHCD6/u+r2SIyvnMOU3260gSji8Wp6IDO/CpJeOmoqzAPwEa508BEO mf4NpqtyvEVOAjIO17NA92nqScyajYkejOYUHRM90LotCdly/tTSujV3oWz6QshgYAR/DJahP J9Yt7Qy Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 23.05.2017 21:38, Stefan Wahren wrote: > >> Lino Sanfilippo hat am 23. Mai 2017 um 20:16 geschrieben: >> >> >> Hi, >> >> On 23.05.2017 15:12, Stefan Wahren wrote: >> >> >>> +} >>> + >>> +static void qca_uart_remove(struct serdev_device *serdev) >>> +{ >>> + struct qcauart *qca = serdev_device_get_drvdata(serdev); >>> + >>> + netif_carrier_off(qca->net_dev); >>> + cancel_work_sync(&qca->tx_work); >>> + unregister_netdev(qca->net_dev); >> >> Note that it is still possible that the tx work is queued right after cancel_work_sync() >> returned and before the net device is unregistered (and thus the check for the net device >> being up at the beginning of the tx work function is passed and the function is executed). > > Even if the carrier is off? Since i see this pattern in some drivers, can you please point me to a reference like a thread or something else? > The check in the tx work function is against the "running" state not against the carrier. So why should the carrier matter in this case? >> 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. But I may have missed something and if unregistering the device while the tx work could be running worries you, we could first close and later unregister the device like in the following sequence: dev_close(); /* the tx work wont be scheduled any more now, however we have to wait for a potentially earlier scheduled work */ cancel_work_sync(&qca->tx_work); /* we can be sure that the tx work will neither be running nor be started again, so it is safe to unregister the netdev */ unregister_netdev(qca->net_dev); serdev_device_close(serdev); free_netdev(qca->net_dev); What do you think? Regards, Lino