From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andreas Werner Subject: Re: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller driver Date: Mon, 8 Aug 2016 16:12:39 +0200 Message-ID: <20160808141239.GB1733@awelinux> References: <20160726091555.GA26227@awelinux> <9487fe77-dbd0-3a64-b648-cf76b897feda@grandegger.com> <20160808113938.GA17459@awelinux> <456c4f82-6a53-e821-3396-63d413db9eb7@grandegger.com> <20160808130633.GC12298@airbook.newtec.eu> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Return-path: Received: from mail1.bemta6.messagelabs.com ([85.158.143.250]:32733 "EHLO mail1.bemta6.messagelabs.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752014AbcHHOMn (ORCPT ); Mon, 8 Aug 2016 10:12:43 -0400 Content-Disposition: inline In-Reply-To: <20160808130633.GC12298@airbook.newtec.eu> Sender: linux-can-owner@vger.kernel.org List-ID: To: Kurt Van Dijck Cc: Wolfgang Grandegger , Andreas Werner , mkl@pengutronix.de, linux-can@vger.kernel.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, davem@davemloft.net, jthumshirn@suse.de, andy@wernerandy.de, michael.miehling@men.de On Mon, Aug 08, 2016 at 03:06:33PM +0200, Kurt Van Dijck wrote: > > --- Original message --- > > Date: Mon, 8 Aug 2016 14:28:39 +0200 > > From: Wolfgang Grandegger > > > [...] > > >>>+ > > >>>+ if (!(cf->can_id & CAN_RTR_FLAG)) { > > >>>+ writel(data[0], &cf_buf->data[0]); > > >>>+ writel(data[1], &cf_buf->data[1]); > > >> > > >>Why do you not check cf->can_dlc here as well. And is the extra copy > > >>necessary. > > >> > > > > > >Yes, I agree with you. The extra copy could be also avoided. > > > > > >>>+ > > >>>+ stats->tx_bytes += cf->can_dlc; > > >>>+ } > > >> > > >>If I look to other drivers, they write the data even in case of RTR. > > >> > > > > > >But why? > > > > > >A RTR does not have any data, therefore there is no need to write the data. > > >Only the length is required as the request size. > > > > Yes; I'm wondering as well. > > > > > > > >If there is a reason behind writing the data of a RTR frame, I can > > >change that, but for now there is no reason. > > > > Yep. > > I _think_ that copying the data without checking the RTR bit clearly > avoids a condition and might produce faster code on some machines. > In any case, it reads easier. > I'm not sure how that interacts with caches etc etc. > > On the other hand, giving unused data is a bad habit that may reveal > security information on some places, so better avoid it. > > Kurt Hi Kurt, thanks for your comment. In my opinion, I really prever to NOT copying such data if the RTR flag ist set. Regards Andy From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752214AbcHHOMq (ORCPT ); Mon, 8 Aug 2016 10:12:46 -0400 Received: from mail1.bemta6.messagelabs.com ([85.158.143.250]:32733 "EHLO mail1.bemta6.messagelabs.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752014AbcHHOMn (ORCPT ); Mon, 8 Aug 2016 10:12:43 -0400 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprJKsWRWlGSWpSXmKPExsUS8J9tom7k5BX hBk2feS1WXdvBaDHnfAuLxeKeHcwWj7tmsFqs+j6V2eLyrjlsFusXTWGxOLZAzGLpvZ2sDpwe W1beZPL4eOk2o0f/XwOPzaerPc4sOMHu8XmTnEf/pYeMAexRrJl5SfkVCawZkx+3shV85654e v0ZcwPjUs4uRi4OIYEGRomDU2cwdjFyckgI+Enc3tjIApGYxyix9c8adpAEi4CKxMXprWA2m4 COxLn2ZlYQW0TARKJr5nImkAZmgUVMEns7z4ElhAVCJPpPXmAGsXmBGv7PWMYEMfUVo8S7u4/ YIBKCEidnPmEBsZmBihbs/gQU5wCypSWW/+MACXMKmEt8+7iMGSQsCnTEq4P1IGEhASWJzy/6 mCGOlpfY27cYrFNCIE7iyx1TCNNaYuLmxAmMwrOQrJqFZNUshFULGJlXMaoXpxaVpRbpGuklF WWmZ5TkJmbm6BoamOnlphYXJ6an5iQmFesl5+duYgRGGQMQ7GBc9tfpEKMkB5OSKK+U0opwIb 6k/JTKjMTijPii0pzU4kOMMhwcShK8eycC5QSLUtNTK9Iyc4DxDpOW4OBREuFdD5LmLS5IzC3 OTIdInWJUlBLn3QKSEABJZJTmwbXBUswlRlkpYV5GoEOEeApSi3IzS1DlXzGKczAqCfP+AJnC k5lXAjcdGAdAN4vwJqmCLS5JREhJNTAK1YY0HuiWi/q6aapckPoUFhH7HROnz1LWe7os+Kd+5 vtKrVVPZxltS/m3pNdUlCFyz/KAT5Oktt0y+M4dEykgu+f9jjqZS9ZHn9q8P3XmzD6rZgntRy F+mp3XM59snWdm71X4Z5/afO/7PlF569nLRH9xzyyvao4/3xZz5o/EV6snu967dmoosRRnJBp qMRcVJwIA5ZoPPCwDAAA= X-Env-Sender: Andreas.Werner@men.de X-Msg-Ref: server-9.tower-193.messagelabs.com!1470665560!319688!1 X-Originating-IP: [80.255.6.145] X-StarScan-Received: X-StarScan-Version: 8.77; banners=-,-,- X-VirusChecked: Checked X-PGP-Universal: processed; by keys.men.de on Mon, 08 Aug 2016 16:12:41 +0200 Date: Mon, 8 Aug 2016 16:12:39 +0200 From: Andreas Werner To: Kurt Van Dijck CC: Wolfgang Grandegger , Andreas Werner , , , , , , , , Subject: Re: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller driver Message-ID: <20160808141239.GB1733@awelinux> References: <20160726091555.GA26227@awelinux> <9487fe77-dbd0-3a64-b648-cf76b897feda@grandegger.com> <20160808113938.GA17459@awelinux> <456c4f82-6a53-e821-3396-63d413db9eb7@grandegger.com> <20160808130633.GC12298@airbook.newtec.eu> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <20160808130633.GC12298@airbook.newtec.eu> User-Agent: Mutt/1.5.24 (2015-08-30) X-Originating-IP: [192.1.1.170] X-ClientProxiedBy: MEN-EX01.intra.men.de (192.168.1.1) To MEN-EX01.intra.men.de (192.168.1.1) X-EXCLAIMER-MD-CONFIG: e4841e51-7998-49c0-ba41-8b8a0e2d8962 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Aug 08, 2016 at 03:06:33PM +0200, Kurt Van Dijck wrote: > > --- Original message --- > > Date: Mon, 8 Aug 2016 14:28:39 +0200 > > From: Wolfgang Grandegger > > > [...] > > >>>+ > > >>>+ if (!(cf->can_id & CAN_RTR_FLAG)) { > > >>>+ writel(data[0], &cf_buf->data[0]); > > >>>+ writel(data[1], &cf_buf->data[1]); > > >> > > >>Why do you not check cf->can_dlc here as well. And is the extra copy > > >>necessary. > > >> > > > > > >Yes, I agree with you. The extra copy could be also avoided. > > > > > >>>+ > > >>>+ stats->tx_bytes += cf->can_dlc; > > >>>+ } > > >> > > >>If I look to other drivers, they write the data even in case of RTR. > > >> > > > > > >But why? > > > > > >A RTR does not have any data, therefore there is no need to write the data. > > >Only the length is required as the request size. > > > > Yes; I'm wondering as well. > > > > > > > >If there is a reason behind writing the data of a RTR frame, I can > > >change that, but for now there is no reason. > > > > Yep. > > I _think_ that copying the data without checking the RTR bit clearly > avoids a condition and might produce faster code on some machines. > In any case, it reads easier. > I'm not sure how that interacts with caches etc etc. > > On the other hand, giving unused data is a bad habit that may reveal > security information on some places, so better avoid it. > > Kurt Hi Kurt, thanks for your comment. In my opinion, I really prever to NOT copying such data if the RTR flag ist set. Regards Andy