Hi, > -----Original Message----- > From: Matthias Fuchs [mailto:matthias.fuchs@xxxxxx] > Sent: Friday, May 28, 2010 8:56 PM > To: Viral Mehta > Cc: netdev@xxxxxxxxxxxxxxx; Socketcan-core@xxxxxxxxxxxxxxxx; linux- > usb@xxxxxxxxxxxxxxx > 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@xxxxxxxxxxxxxxx [linux-usb- > owner@xxxxxxxxxxxxxxx] On Behalf Of Matthias Fuchs > [matthias.fuchs@xxxxxx] > > >Sent: Wednesday, May 26, 2010 2:44 PM > > >To: netdev@xxxxxxxxxxxxxxx > > >Cc: Socketcan-core@xxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx > > >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 linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
