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

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]

Date:	Mon, 09 Jun 2014 20:12:06 -0700
From:	Joe Perches <joe@...ches.com>
To:	Florian Fainelli <f.fainelli@...il.com>
Cc:	Andy Green <andy.green@...aro.org>,
	netdev <netdev@...r.kernel.org>,
	Francois Romieu <romieu@...zoreil.com>, patches@...aro.org
Subject: 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 <andy.green@...aro.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.


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