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

On 10 June 2014 11:12, Joe Perches [off-list ref] wrote:

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

quoted

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

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.

OK.

quoted

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.

OK

quoted

 +               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.

Ah OK, missed it when porting from what was a whole abstracted logging system.

Thanks for the extra points.

-Andy