-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Wolfgang Grandegger wrote: > Gole, Anant wrote: >>> -----Original Message----- >>> From: Wolfgang Grandegger [mailto:[email protected]] >>> Sent: Wednesday, September 23, 2009 9:05 PM >>> To: Gole, Anant >>> Cc: [email protected] >>> Subject: Re: [PATCH V3] net-next:can: add TI CAN (HECC) driver >>> >>> Hi Anant, >>> >>> here is my quick review. >> Appreciate the review. Ack on most comments - I will fix them before I head >> off for a vacation this weekend. > > I have a few more comments, which I will post in a second. > >> [snip] >> >>>> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile >>>> index 523a941..45d42dc 100644 >>>> --- a/drivers/net/can/Makefile >>>> +++ b/drivers/net/can/Makefile >>>> @@ -8,5 +8,6 @@ obj-$(CONFIG_CAN_DEV) += can-dev.o >>>> can-dev-y := dev.o >>>> >>>> obj-$(CONFIG_CAN_SJA1000) += sja1000/ >>>> +obj-$(CONFIG_CAN_TI_HECC) += ti_hecc.o >>> Die Patch will fail because two more drivers have been accepted for >>> net-next-2.6. They should show up soon. >>> >> Ok. I will take care while submitting the patch to netdev. >> >> [snip] >>>> +static netdev_tx_t ti_hecc_xmit(struct sk_buff *skb, struct net_device >>> *ndev) >>>> +{ >>>> + struct ti_hecc_priv *priv = netdev_priv(ndev); >>>> + struct can_frame *cf = (struct can_frame *)skb->data; >>>> + u32 mbxno = 0; >>>> + u32 data; >>>> + unsigned long flags; >>>> + >>>> + mbxno = get_tx_head_mb(priv); >>>> + spin_lock_irqsave(&priv->mbox_lock, flags); >>>> + if (unlikely(hecc_read(priv, HECC_CANME) & (1 << mbxno))) { >>>> + spin_unlock_irqrestore(&priv->mbox_lock, flags); >>>> + netif_stop_queue(ndev); >>>> + dev_err(priv->ndev->dev.parent, >>>> + "BUG: TX mbox not ready tx_head=%08X, tx_tail=%08X\n", >>>> + priv->tx_head, priv->tx_tail); >>>> + return NETDEV_TX_BUSY; >>> Can this happen? Will the hardware recover from the error? >> Technically it cannot happen but the check is put as a worst case to know if >> this really happened - it is difficult to predict if the driver will recover >> as this condition is really not expected but a error print probably can help >> debug. I checked other drivers with similar logic for TX mailbox (like at91) >> and the same error print was used which I thought would be useful. > > OK, I was just thinking if dropping the skb would not be the better > choice. Just keep it as-is.
What's the difference between dropping or not dropping the skb from the hardware point of view? I think there is none. So IMHO from the application point no packages should be dropped if this could be avoided. With returning NETDEV_TX_BUSY we give the skb back to the network stack it will be tried again later (if the netif_queue is running). cheers, Marc - -- Pengutronix e.K. | Marc Kleine-Budde | Linux Solutions for Science and Industry | Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917-5555 | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iEYEARECAAYFAkq7Jj8ACgkQjTAFq1RaXHOhUgCfVgSngoZPWW/LcFAy600+vjkK cZAAn0+uAS/axfTz4yBnROhoalmXH1Tr =40Qy -----END PGP SIGNATURE----- _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
