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