netdev - Re: [PATCH 3/3] net: hisilicon: new hip04 ethernet 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]

Message-ID: <532CE4EB.6010305@linaro.org>
Date:	Sat, 22 Mar 2014 09:18:35 +0800
From:	zhangfei <zhangfei.gao@...aro.org>
To:	Arnd Bergmann <arnd@...db.de>
CC:	"David S. Miller" <davem@...emloft.net>, linux@....linux.org.uk,
	f.fainelli@...il.com, mark.rutland@....com,
	sergei.shtylyov@...entembedded.com,
	linux-arm-kernel@...ts.infradead.org, netdev@...r.kernel.org,
	devicetree@...r.kernel.org
Subject: Re: [PATCH 3/3] net: hisilicon: new hip04 ethernet driver

Dear Arnd

On 03/21/2014 11:27 PM, Arnd Bergmann wrote:
> On Friday 21 March 2014 23:09:30 Zhangfei Gao wrote:
>
>> +
>> +static void __iomem *ppebase;
>
> Any reason why you still have this, rather than using a separate
> driver for it as we discussed? If you have comments that you still
> plan to address, please mention those in the introductory mail,
> so you don't get the same review comments multiple times.

Sorry for my bad understanding.
I thought you agreed to use this.
Will check your earlier comments more carefully.

>
>
>> +static void hip04_tx_reclaim(struct net_device *ndev, bool force)
>> +{
>> +	struct hip04_priv *priv = netdev_priv(ndev);
>> +	unsigned tx_head = priv->tx_head;
>> +	unsigned tx_tail = priv->tx_tail;
>> +	struct tx_desc *desc = &priv->tx_desc[priv->tx_tail];
>> +
>> +	while (tx_tail != tx_head) {
>> +		if (desc->send_addr != 0) {
>> +			if (force)
>> +				desc->send_addr = 0;
>> +			else
>> +				break;
>> +		}
>> +		if (priv->tx_phys[tx_tail]) {
>> +			dma_unmap_single(&ndev->dev, priv->tx_phys[tx_tail],
>> +				priv->tx_skb[tx_tail]->len, DMA_TO_DEVICE);
>> +			priv->tx_phys[tx_tail] = 0;
>> +		}
>> +		dev_kfree_skb_irq(priv->tx_skb[tx_tail]);
>> +		priv->tx_skb[tx_tail] = NULL;
>> +		tx_tail = TX_NEXT(tx_tail);
>> +		priv->tx_count--;
>> +	}
>> +	priv->tx_tail = tx_tail;
>> +}
>
> You call this function from start_xmit(), which may be too early, causing the
> dma_unmap_single() and dev_kfree_skb_irq() functions to be called while the
> device is still accessing the data. This is bad.

There is a protection.
Only after xmit done, desc->send_addr = 0, which is cleared by hardware.

>
> You have to ensure that you only ever clean up tx buffers that have been
> successfully transmitted. Also, you should use an interrupt to notify you
> of this in case there is no further xmit packet. Otherwise you may have
> a user space program waiting indefinitely for a single packet to get sent
> on a socket.
>
> It's ok to also call the cleanup from start_xmit, but calling it from the
> poll() function or another appropriate place is required.

There is no transmit done interrupt, so relying on every xmit to reclaim 
finished buffer.
I thought it would be enough, since there are TX_DESC_NUM descs.
It is a good idea also put reclaim in poll, then will add spin lock etc.

>
>> +	priv->id = of_alias_get_id(node, "ethernet");
>> +	if (priv->id < 0) {
>> +		dev_warn(d, "no ethernet alias\n");
>> +		ret = -EINVAL;
>> +		goto init_fail;
>> +	}
>
> Apparently you try to rely on the alias to refer to a specific piece
> of hardware, which is not correct. The alias is meant to be selectable
> to match e.g. the numbering written on the external connector, which
> is totally independent of the internal hardware.

Thanks for clarifying alisa.
The id will be used for start channel in ppe, RX_DESC_NUM * priv->id;
Is it suitable directly use id in the dts, or other name such as start-chan?

Thanks
--
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