netdev - Re: [PATCH v3] Renesas Ethernet AVB driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 20 Apr 2015 01:45:23 +0200
From:	Lino Sanfilippo <LinoSanfilippo@....de>
To:	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>,
	robh+dt@...nel.org, pawel.moll@....com, mark.rutland@....com,
	ijc+devicetree@...lion.org.uk, devicetree@...r.kernel.org,
	galak@...eaurora.org, netdev@...r.kernel.org,
	richardcochran@...il.com
CC:	linux-sh@...r.kernel.org
Subject: Re: [PATCH v3] Renesas Ethernet AVB driver

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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html