netdev - RE: [PATCH v3] can: Add driver for esd CAN-USB/2 device

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] [day] [month] [year] [list]

Message-ID: <2B8BB46015B5984383F155EB036F018E3E760E39@VSHINMSMBX01.vshodc.lntinfotech.com>
Date:	Mon, 31 May 2010 12:05:50 +0530
From:	Viral Mehta <Viral.Mehta@...infotech.com>
To:	Matthias Fuchs <matthias.fuchs@....eu>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"Socketcan-core@...ts.berlios.de" <Socketcan-core@...ts.berlios.de>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>
Subject: RE: [PATCH v3] can: Add driver for esd CAN-USB/2 device

Hi,

> -----Original Message-----
> From: Matthias Fuchs [mailto:matthias.fuchs@....eu]
> Sent: Friday, May 28, 2010 8:56 PM
> To: Viral Mehta
> Cc: netdev@...r.kernel.org; Socketcan-core@...ts.berlios.de; linux-
> usb@...r.kernel.org
> Subject: Re: [PATCH v3] can: Add driver for esd CAN-USB/2 device
>
> Hi Viral,
>
> thanks for review. Please see some comments below.
>
> On Wednesday 26 May 2010 13:31, Viral Mehta wrote:
> > Hi,
> > >________________________________________
> > >From: linux-usb-owner@...r.kernel.org [linux-usb-
> owner@...r.kernel.org] On Behalf Of Matthias Fuchs
> [matthias.fuchs@....eu]
> > >Sent: Wednesday, May 26, 2010 2:44 PM
> > >To: netdev@...r.kernel.org
> > >Cc: Socketcan-core@...ts.berlios.de; linux-usb@...r.kernel.org
> > >Subject: [PATCH v3] can: Add driver for esd CAN-USB/2 device
> > >+
> > >+       BUG_ON(!context);
> >
> > It is preferred to used WARN_ON and avoid using BUG_ON and thus dont
> kill the whole system....
> Really? Even when next line will reference a NULL pointer in this case?
> Ok. Will be changed.

How about,
if(!context)
        return;

Look at the other drivers for example.
I, personally, dont care since I will not be using your driver but I am sure that other people, too, wont like BUG_ON....

>
> > [...]
> > >+
> > >+       priv = context->priv;
> > >+       netdev = priv->netdev;
> > >+       dev = priv->usb2;
> > >+       err = usb_submit_urb(urb, GFP_ATOMIC);
> > >+       if (err) {
> > >+               can_free_echo_skb(netdev, context->echo_index);
> > >+
> > >+               atomic_dec(&priv->active_tx_jobs);
> > >+               usb_unanchor_urb(urb);
> > >+
> > >+               stats->tx_dropped++;
> > >+
> > >+               if (err == -ENODEV)
> > >+                       netif_device_detach(netdev);
> > >+               else
> > >+                       dev_warn(netdev->dev.parent, "failed tx_urb
> %d\n", err);
> > >+
> > >+               goto releasebuf;
> >
> > You probably want to set "ret" here or do you really want to return
> NETDEV_TX_OK
> As far as I can see netword device drivers xmit_start() return
> NETDEV_TX_OK or _BUSY.
> There are no real alternatives.
Yup. this is okie....

>But please correct me when I am wrong.
>
> Your other comments will be fixed by version 4 of my patch.
>
> Matthias
>
> ______________________________________________________________________

This Email may contain confidential or privileged information for the intended recipient (s) If you are not the intended recipient, please do not use or disseminate the information, notify the sender and delete it from your system.

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