Re: [PATCH v2 2/3] can: cc770: Stop queue on NETDEV_TX_BUSY

[Wolfgang Grandegger <wg@grandegger.com>]

Hello,

Am 12.02.2018 um 20:40 schrieb Richard Weinberger:
> Andri,
> 
> Am Dienstag, 30. Januar 2018, 17:56:48 CET schrieb Andri Yngvason:
>> If the queue is not stopped, the start_xmit function will continue to be
>> called until the driver or the system is reset.
>>
>> Signed-off-by: Andri Yngvason <andri.yngvason@marel.com>
>> ---
>>   drivers/net/can/cc770/cc770.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/can/cc770/cc770.c b/drivers/net/can/cc770/cc770.c
>> index 9fed163..12d3b89 100644
>> --- a/drivers/net/can/cc770/cc770.c
>> +++ b/drivers/net/can/cc770/cc770.c
>> @@ -405,6 +405,7 @@ static netdev_tx_t cc770_start_xmit(struct sk_buff *skb,
>> struct net_device *dev)
>>
>>   	if ((cc770_read_reg(priv,
>>   			    msgobj[mo].ctrl1) & TXRQST_UNC) == TXRQST_SET) {
>> +		netif_stop_queue(dev);
>>   		netdev_err(dev, "TX register is still occupied!\n");
>>   		return NETDEV_TX_BUSY;
>>   	}
> 
> I see that some can drivers stop the queue, others not.
> What exactly is the problem in this case?

Stopping the queue and returning NETDEV_TX_BUSY does make little sense. 
NETDEV_TX_BUSY will tell the upper layer to retry asap. In general that 
case should be avoided by stopping the queue earlier. Actually, 
netif_stop_queue(dev) is called directly after the if block above,
which means that "TX register is still occupied" indicates an error/bug. 
I think there is a race somewhere.

Wolfgang.