Re: [PATCH v3] Renesas Ethernet AVB driver

[Lino Sanfilippo <LinoSanfilippo@gmx.de>]

Hi,

On 20.04.2015 00:10, Sergei Shtylyov wrote:
> 
>> I recall a thread in which the use of bitfields for structs that are
>> shared with the hardware was considered a bad idea (because the compiler
>> is free to reorder the fields). Shift operations are probably a better
>> choice here.
> 
>     Well, it looks as the compiler is not free to reorder bit fields, and the 
> order is determined by the ABI. Will look into getting rid of them anyway...

I think that thread I was referring to was this one:
http://thread.gmane.org/gmane.linux.kernel/182862/focus=182986
(See the first comment from Benjamin Herrenschmidt).

>>> +/* Packet transmit function for Ethernet AVB */
>>> +static int ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>>> +{
>>> +	struct ravb_private *priv = netdev_priv(ndev);
>>> +	struct ravb_tstamp_skb *ts_skb = NULL;
>>> +	struct ravb_tx_desc *desc;
>>> +	unsigned long flags;
>>> +	void *buffer;
>>> +	u32 entry;
>>> +	u32 tccr;
>>> +	int q;
>>> +
>>> +	/* If skb needs TX timestamp, it is handled in network control queue */
>>> +	q = (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) ? RAVB_NC : RAVB_BE;
>>> +
>>> +	spin_lock_irqsave(&priv->lock, flags);
>>> +	if (priv->cur_tx[q] - priv->dirty_tx[q] >= priv->num_tx_ring[q] - 4) {
>>> +		if (!ravb_tx_free(ndev, q)) {
>>> +			netif_warn(priv, tx_queued, ndev, "TX FD exhausted.\n");
>>> +			netif_stop_queue(ndev);
>>> +			spin_unlock_irqrestore(&priv->lock, flags);
>>> +			return NETDEV_TX_BUSY;
>>> +		}
>>> +	}
>>> +	entry = priv->cur_tx[q] % priv->num_tx_ring[q];
>>> +	priv->cur_tx[q]++;
>>> +	spin_unlock_irqrestore(&priv->lock, flags);
>>> +
>>> +	if (skb_put_padto(skb, ETH_ZLEN))
>>> +		return NETDEV_TX_OK;
>>> +
>>> +	priv->tx_skb[q][entry] = skb;
>>> +	buffer = PTR_ALIGN(priv->tx_buffers[q][entry], RAVB_ALIGN);
>>> +	memcpy(buffer, skb->data, skb->len);
>>> +	desc = &priv->tx_ring[q][entry];
>>> +	desc->ds = skb->len;
>>> +	desc->dptr = dma_map_single(&ndev->dev, buffer, skb->len,
>>> +				    DMA_TO_DEVICE);
>>> +	if (dma_mapping_error(&ndev->dev, desc->dptr)) {
>>> +		dev_kfree_skb_any(skb);
>>> +		priv->tx_skb[q][entry] = NULL;
>>> +		return NETDEV_TX_OK;
>>> +	}
>>> +
>>> +	/* TX timestamp required */
>>> +	if (q == RAVB_NC) {
>>> +		ts_skb = kmalloc(sizeof(*ts_skb), GFP_ATOMIC);
>>> +		if (!ts_skb)
>>> +			return -ENOMEM;
> 
>> Dma mapping has to be undone.
> 
>     OK, fixed. Not sure what we should return in this case: error code or
> NETDEV_TX_OK...

NETDEV_TX_OK is the correct return value even in error case. The only
exception is NETDEV_TX_BUSY when the tx queue has been stopped. However
returning NETDEV_TX_OK also means that the skb has to be consumed (so
beside unmapping dma also the skb has to be freed in case that kmalloc
fails in ravb_start_xmit).

>> example all ptp related code could be put into its own file.
> 
>     OK, will try to split the driver back... Perhaps I should also split the 
> patch accordingly?

Yes, sounds like a good idea.

Regards,
Lino