Re: [PATCH v6 net-next 17/17] net: qualcomm: add QCA7000 UART driver

[Lino Sanfilippo <LinoSanfilippo@gmx.de>]

On 23.05.2017 21:38, Stefan Wahren wrote:
> 
>> Lino Sanfilippo <LinoSanfilippo@gmx.de> 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