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

[Joe Perches <joe@perches.com>]

On Mon, 2014-06-09 at 19:14 -0700, Florian Fainelli wrote:
> 2014-06-09 18:08 GMT-07:00 Andy Green <andy.green@linaro.org>:
> > 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:

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

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