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 devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
