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]

Message-ID: <CAAfg0W54QT9TrpH4AWOeA9EB-U2zMhvwygQyqDvWYs=WoWdmrA@mail.gmail.com>
Date:	Tue, 10 Jun 2014 12:04:11 +0800
From:	Andy Green <andy.green@...aro.org>
To:	Joe Perches <joe@...ches.com>
Cc:	Florian Fainelli <f.fainelli@...il.com>,
	netdev <netdev@...r.kernel.org>,
	Francois Romieu <romieu@...zoreil.com>,
	Patch Tracking <patches@...aro.org>
Subject: Re: [net-next PATCH 2] net: ethernet driver: Fujitsu OGMA

On 10 June 2014 11:12, Joe Perches <joe@...ches.com> wrote:
> 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.

OK.

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

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

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

Thanks for the extra points.

-Andy

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