Re: [PATCH V6 1/1] can: Add support for esd CAN PCIe/402 card

["Ahmed S. Darwish" <darwish.07@gmail.com>]

On Tue, Mar 17, 2015 at 09:33:09AM -0400, Ahmed S. Darwish wrote:
> Hi,
> 
> [ Adding Marc to the CC list. Kindly add him in future submissions.. ]
> 
> On Tue, Mar 17, 2015 at 12:38:28PM +0100, Thomas Körper wrote:
> ...
> > +netdev_tx_t acc_start_xmit(struct sk_buff *skb, struct net_device *netdev)
> > +{
> > +	struct acc_net_priv *priv = netdev_priv(netdev);
> > +	struct acc_core *core = priv->core;
> > +	struct can_frame *cf = (struct can_frame *)skb->data;
> > +	u8 new_fifo_head = (core->tx_fifo_head + 1) % core->tx_fifo_size;
> > +	u32 esd_id;
> > +	u8 esd_len;
> > +
> > +	if ((new_fifo_head == core->tx_fifo_tail) || !acc_txq_isready(core)) {
> > +		netif_stop_queue(netdev);
> > +		return NETDEV_TX_BUSY;
> > +	}
> > +
> 
> In that case, an error or warning message is in order. Quoting from
> Documentation/networking/{driver.txt,netdevices.txt}:
> 
>   When this happens, it usually means start/stop flow control
>   is broken in the driver. This is a hard error that should be logged:
>   netdev_err("BUG: Tx ring full when queue awake!\n");
> 
> [ Actually it would be nice to print such message and _heavily_ test
>   the driver tx/rx pathes afterwards, making sure no practical race
>   conditions exist regarding the netif tx queue .. ]
> 
> On another topic, I've searched the code for the part where it stops
> the queue because "we've taken the last slot in the ring, and we will
> not be able to handle further transmissions" but couldn't find it at
> all. AFAIK, that part calls netif_stop_queue(), but _always_ return
> with NETDEV_TX_OK. Am I missing something obvious?
> 

s/_always_/should always