From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marc Kleine-Budde Subject: Re: [PATCH net-next-2.6 v2] can: Topcliff: PCH_CAN driver: Fix build warnings Date: Tue, 09 Nov 2010 13:59:32 +0100 Message-ID: <4CD945B4.4060408@pengutronix.de> References: <005301cb7ffa$5b63cd90$66f8800a@maildom.okisemi.com> <00fe01cb8009$62e11410$66f8800a@maildom.okisemi.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============9021076400548837502==" Cc: andrew.chih.howe.khor-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org, Masayuki Ohtake , Samuel Ortiz , margie.foster-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org, netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, LKML , socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org, yong.y.wang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org, kok.howg.ewe-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org, Wolfgang Grandegger , joel.clark-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org, "David S. Miller" , Christian Pellegrin , qi.wang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org To: Tomoya MORINAGA Return-path: In-Reply-To: <00fe01cb8009$62e11410$66f8800a-a06+6cuVnkTSQfdrb5gaxUEOCMrvLtNR@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: socketcan-core-bounces-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org Errors-To: socketcan-core-bounces-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org List-Id: netdev.vger.kernel.org This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --===============9021076400548837502== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enigEC8162DD2057550F76EC6216" This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enigEC8162DD2057550F76EC6216 Content-Type: text/plain; charset=ISO-2022-JP Content-Transfer-Encoding: quoted-printable 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= =2E >>>> 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-w= rite >>> so that IF2 access not occurred, vice versa. >> >> Why is that needed? >=20 > 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. >=20 > 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 =3D netdev_priv(ndev); >>>>> + struct can_frame *cf =3D (struct can_frame *)skb->data; >>>>> + int tx_buffer_avail =3D 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 necess= ary ? >> >> Yes. >=20 > I can' understand your issue. > Please can you hear my opinion? >=20 > Please see the head of pch_xmit. >=20 >>> + if (priv->tx_obj =3D=3D (PCH_OBJ_NUM + 1)) { /* Point tail Obj + 1 = */ >>> + while (ioread32(&priv->regs->treq2) & 0xfc00) >>> + udelay(1); >=20 > 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 t= he number of tx message object. > Thus I think these code(netif_stop_queue/netif_wake_queue) are complete= ly redundant. Nope - please remove the waiting completely and convert your flow control to netif_stop_queue/netif_wake_queue. cheers, Marc --=20 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 | --------------enigEC8162DD2057550F76EC6216 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAkzZRbgACgkQjTAFq1RaXHPSNACfe9Z3OSyF7KYrgUd0zPJyd7JR 4L4Anj46U33eA0XBJ4SjrSp4q5XwhvUS =/mhx -----END PGP SIGNATURE----- --------------enigEC8162DD2057550F76EC6216-- --===============9021076400548837502== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Socketcan-core mailing list Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org https://lists.berlios.de/mailman/listinfo/socketcan-core --===============9021076400548837502==--