Re: [net-next PATCH 2] net: ethernet driver: Fujitsu OGMA

On Mon, 2014-06-09 at 19:14 -0700, Florian Fainelli wrote:

2014-06-09 18:08 GMT-07:00 Andy Green [off-list ref]:

quoted

This driver adds support for "ogma", a Fujitsu Semiconductor Ltd IP Gigabit
Ethernet + PHY IP used in a variety of their ARM-based ASICs.

trivial notes:

quoted

+                                             desc->len * param->entries,
+                                             &desc->deschys_addr, GFP_KERNEL);
+       if (!desc->ring_vaddr) {
+               ret = -ENOMEM;
+               dev_err(priv->dev, "%s: failed to alloc\n", __func__);
+               goto err;
+       }
+
+       memset(desc->ring_vaddr, 0, (u32) desc->len * param->entries);
+       desc->frag = kmalloc(sizeof(*desc->frag) * param->entries,
+                            GFP_NOWAIT);

Is not GFP_KERNEL suitable here? If not, cannot you move the
allocation to a code-path that is allowed to sleep. e.g the ndo_open()
function for instance?

quoted

+       if (!desc->frag) {
+               ret = -ENOMEM;
+               dev_err(priv->dev, "%s: failed to alloc\n", __func__);
+               goto err;
+       }
+
+       memset(desc->frag, 0, sizeof(struct ogma_frag_info) * param->entries);

kzalloc() does a kmalloc() + memset().

kcalloc does that and checks for overflow too.

quoted

+       if (!desc->priv) {
+               ret = -ENOMEM;
+               dev_err(priv->dev, "%s: failed to alloc priv\n", __func__);

OOM messages are unnecessary as there's a generic one.

 +               ret = ogma_get_rx_pkt_data(ndev->priv, OGMA_RING_NRM_RX,

quoted

+                                          &rx_pkt_info, &frag, &len, &skb);
+               if (ret == -ENOMEM) {
+                       dev_err(ndev->priv->dev, "%s: rx fail %d",
+                               __func__, ret);
+                       net_device->stats.rx_dropped++;
+                       continue;
+               }

The hardware does not tell you whether the packet is oversized, or if
there any errors? That looks a little unusual.

Use netdev_err() here instead. Or even better netif_err() with
rx_status, this comment applies to the entire driver BTW.

That and please terminate all messages with \n to
avoid possible message interleaving from other modules.