Messages in this thread | | | From | "Tomoya MORINAGA" <> | Subject | Re: [PATCH net-next-2.6 v2] can: Topcliff: PCH_CAN driver: Fix build warnings | Date | Thu, 11 Nov 2010 18:56:24 +0900 |
| |
On Tuesday, November 09, 2010 9:59 PM, Marc Kleine-Budde wrote :
> > 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?
You are right. Each IF1 and IF2 is completely independent. All message objects can be accessed via both IF1 and IF2. There is a possibility that a message object is accessed by IF1 and IF2 simultaneously. But this driver separates message object by Tx/Rx completely.(Rx:1~26, Tx:27~32) Thus, these locks are unnecessary. I will delete these.
> > [...] > > >>>> 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. >
I understand.
> [...] > > >>>>> +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. >
I see. I will remove like above.
BTW, Let me know the reason. Could you explain the reason why you deny looping busy loop ?
Thanks,
Tomoya MORINAGA OKI SEMICONDUCTOR CO., LTD.
| |