Hi Viral, thanks for review. Please see some comments below.
On Wednesday 26 May 2010 13:31, Viral Mehta wrote: > Hi, > >________________________________________ > >From: [email protected] [[email protected]] On > >Behalf Of Matthias Fuchs [[email protected]] > >Sent: Wednesday, May 26, 2010 2:44 PM > >To: [email protected] > >Cc: [email protected]; [email protected] > >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. > [...] > >+ > >+ 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. But please correct me when I am wrong. Your other comments will be fixed by version 4 of my patch. Matthias _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
