Messages in this thread | | | Date | Tue, 09 Nov 2010 13:59:32 +0100 | From | Marc Kleine-Budde <> | Subject | Re: [PATCH net-next-2.6 v2] can: Topcliff: PCH_CAN driver: Fix build warnings |
| |
On 11/09/2010 01:26 PM, Tomoya MORINAGA wrote: >>>> Can you please explain me your locking sheme? If I understand the >>>> documenation correctly the two message interfaces can be used mutual. >>>> And you use one for rx the other one for tx. >>> >>> I show our locking scheme. >>> When CPU accesses MessageRAM via IF1, CPU protect until read-modify-write >>> so that IF2 access not occurred, vice versa. >> >> Why is that needed? > > For MessageRAM data consistency.
As far as I understand the datasheet the access to IF1 and IF2 is completely independent. Why do you lock here?
[...]
>>>> Please use just "debug" level not warning here. Consider to use >>>> netdev_dbg() instead. IMHO the __func__ can be dropped and the >>>> "official" name for the error is "Error Warning". >>> >>> I want to know the reason. >>> Why is it not dev_warn but netdev_dbg ? >> >> If you use warning level it would end up on the console or and in the >> syslog. It's quite complicated (for programs) to get information from >> there. This is why we send CAN error frames. They hold the same >> information but int a binary form, thus it's easier to process. > > I understand the reason. > BTW, Why do you say not dev_dbg but netdev_dbg ?
Sorry - netdev_dbg() is easier to use, its first argument is the netdevice, while dev_dbg needs a device and that's deeply hidden in the netdevice.
[...]
>>>>> +static netdev_tx_t pch_xmit(struct sk_buff *skb, struct net_device *ndev) >>>>> +{ >>>>> + unsigned long flags; >>>>> + struct pch_can_priv *priv = netdev_priv(ndev); >>>>> + struct can_frame *cf = (struct can_frame *)skb->data; >>>>> + int tx_buffer_avail = 0; >>>> >>>> What I'm totally missing is the TX flow controll. Your driver has to >>>> ensure that the package leave the controller in the order that come >>>> into the xmit function. Further you have to stop your xmit queue if >>>> you're out of tx objects and reenable if you have a object free. >>>> >>>> Use netif_stop_queue() and netif_wake_queue() for this. >>> >>> In this code, I think "out of tx objects" cannot be occurred. >> >> It's not a matter of code it's the hardware. You cannot put more than a >> certain number of CAN frames into the hardware. If you have a CAN bus at >> a certain speed, you can only send a certain number of CAN frames in a >> second. So you cannot push more than this amount of frames/s into the >> hardware. >> >>> Nevertheless, are netif_stop_queue() and netif_wake_queue() is necessary ? >> >> Yes. > > I can' understand your issue. > Please can you hear my opinion? > > Please see the head of pch_xmit. > >>> + if (priv->tx_obj == (PCH_OBJ_NUM + 1)) { /* Point tail Obj + 1 */ >>> + while (ioread32(&priv->regs->treq2) & 0xfc00) >>> + udelay(1); > > When points tail of Tx message object, > this driver waits until completion of all tx messaeg objects.
Looping busy it not an option here.
> Thus, application/driver ought not to be able to put Tx object exceed the number of tx message object. > Thus I think these code(netif_stop_queue/netif_wake_queue) are completely redundant.
Nope - please remove the waiting completely and convert your flow control to netif_stop_queue/netif_wake_queue.
cheers, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions | Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917-5555 | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[unhandled content-type:application/pgp-signature] | |