Marc Kleine-Budde wrote: > 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).
Yep. Wolfgang. _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
