From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753737AbdBUQcI (ORCPT ); Tue, 21 Feb 2017 11:32:08 -0500 Received: from mail-pg0-f68.google.com ([74.125.83.68]:34485 "EHLO mail-pg0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751072AbdBUQb7 (ORCPT ); Tue, 21 Feb 2017 11:31:59 -0500 Date: Wed, 22 Feb 2017 00:31:42 +0800 From: Cheah Kok Cheong To: Valentin Rothberg Cc: Ian Abbott , hsweeten@visionengravers.com, gregkh@linuxfoundation.org, devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] Staging: comedi: drivers: comedi_test: Avoid multiple line dereference Message-ID: <20170221163141.GA22866@linux-Precision-WorkStation-T5500> References: <1487579315-14775-1-git-send-email-thrust73@gmail.com> <20170220160205.GA4017@linux-Precision-WorkStation-T5500> <675e00f6-2e6c-e486-898d-b942c76e26a0@mev.co.uk> <20170221093333.GA2775@linux-Precision-WorkStation-T5500> <4b35e656-8fc2-13a5-663b-c8db45bed832@mev.co.uk> <20170221102008.GA9653@costa.sra.uni-hannover.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170221102008.GA9653@costa.sra.uni-hannover.de> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Feb 21, 2017 at 11:20:08AM +0100, Valentin Rothberg wrote: > On Feb 21 '17 10:12, Ian Abbott wrote: > > On 21/02/2017 09:33, Cheah Kok Cheong wrote: > > > On Mon, Feb 20, 2017 at 05:36:52PM +0000, Ian Abbott wrote: > > > > On 20/02/17 16:02, Cheah Kok Cheong wrote: > > > > > On Mon, Feb 20, 2017 at 10:03:39AM +0000, Ian Abbott wrote: > > > > > > On 20/02/17 08:28, Cheah Kok Cheong wrote: > > > > > > > Fix checkpatch warning "Avoid multiple line dereference" > > > > > > > using a local variable to avoid line wrap. > > > > > > > > > > > > > > Signed-off-by: Cheah Kok Cheong > > > > > > > --- > > > > > > > drivers/staging/comedi/drivers/comedi_test.c | 6 ++---- > > > > > > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/staging/comedi/drivers/comedi_test.c b/drivers/staging/comedi/drivers/comedi_test.c > > > > > > > index 2a063f0..fde83e0 100644 > > > > > > > --- a/drivers/staging/comedi/drivers/comedi_test.c > > > > > > > +++ b/drivers/staging/comedi/drivers/comedi_test.c > > > > > > > @@ -480,11 +480,9 @@ static void waveform_ao_timer(unsigned long arg) > > > > > > > /* output the last scan */ > > > > > > > for (i = 0; i < cmd->scan_end_arg; i++) { > > > > > > > unsigned int chan = CR_CHAN(cmd->chanlist[i]); > > > > > > > + unsigned short d = devpriv->ao_loopbacks[chan]; > > > > > > > > > > > > > > - if (comedi_buf_read_samples(s, > > > > > > > - &devpriv-> > > > > > > > - ao_loopbacks[chan], > > > > > > > - 1) == 0) { > > > > > > > + if (!comedi_buf_read_samples(s, &d, 1)) { > > > > > > > /* unexpected underrun! (cancelled?) */ > > > > > > > async->events |= COMEDI_CB_OVERFLOW; > > > > > > > goto underrun; > > > > > > > > > > > > > > > > > > > NAK. This leaves devpriv->ao_loopbacks[chan] unchanged. > > > > > > > > > > > > > > > > Thanks for pointing this out. In that case will assigning the variable to > > > > > devpriv->ao_loopbacks[chan] be acceptable? Please review below snippet. > > > > > > > > > > Otherwise I'll just drop the variable and adjust the lines to avoid > > > > > checkpatch warning. > > > > > > > > > > Sorry for the inconvenience caused. > > > > > > > > > > [ Snip ] > > > > > > > > > > /* output the last scan */ > > > > > for (i = 0; i < cmd->scan_end_arg; i++) { > > > > > unsigned int chan = CR_CHAN(cmd->chanlist[i]); > > > > > unsigned short data; > > > > > > > > > > if (!comedi_buf_read_samples(s, &data, 1)) { > > > > > /* unexpected underrun! (cancelled?) */ > > > > > async->events |= COMEDI_CB_OVERFLOW; > > > > > goto underrun; > > > > > } > > > > > > > > > > devpriv->ao_loopbacks[chan] = data; > > > > > } > > > > > /* advance time of last scan */ > > > > > > > > > > [ Snip ] > > > > > > > > It will work, but you could just use a pointer variable set to > > > > &devpriv->ao_loopbacks[chan] and pass that to comedi_buf_read_samples(). > > > > > > > > > > Thanks for the suggestion. I tried below snippet 1 with the shortest pointer > > > name but 80 characters is exceeded. The declaration and initialisation > > > will have to be splitted. Will this be acceptable or am I doing it wrong > > > again? > > > > > > Sorry for the trouble. > > > > > > Snippet 1: > > > [ Snip ] > > > > > > /* output the last scan */ > > > for (i = 0; i < cmd->scan_end_arg; i++) { > > > unsigned int chan = CR_CHAN(cmd->chanlist[i]); > > > unsigned short *p = &devpriv->ao_loopbacks[chan]; > > > > > > if (!comedi_buf_read_samples(s, p, 1)) { > > > /* unexpected underrun! (cancelled?) */ > > > async->events |= COMEDI_CB_OVERFLOW; > > > goto underrun; > > > } > > > } > > > /* advance time of last scan */ > > > > > > [ Snip ] > > > > > > Snippet 2: > > > [ Snip ] > > > > > > /* output the last scan */ > > > for (i = 0; i < cmd->scan_end_arg; i++) { > > > unsigned int chan = CR_CHAN(cmd->chanlist[i]); > > > unsigned short *pd; > > > > > > pd = &devpriv->ao_loopbacks[chan]; > > > > > > if (!comedi_buf_read_samples(s, pd, 1)) { > > > /* unexpected underrun! (cancelled?) */ > > > async->events |= COMEDI_CB_OVERFLOW; > > > goto underrun; > > > } > > > } > > > > > > [ Snip ] > > > > Snippet 2 looks fine. Alternatives are to modify Snippet 1 to split the > > initialization of the pointer variable after the '=', or to shorten the the > > name of the 'chan' variable. I'm tempted to shorten the 'chan' variable but this will break consistency since it's also use in static int waveform_ai_insn_read() and static int waveform_ao_insn_write(). I'll send Snippet 2 as V2. > > Another option could be using the typedefs from include/linux/types.h, > e.g. ushort. However, this might require changing other declarations as > well to keep consistency. Thanks for the idea. I counted seven instances of 'unsigned short' in this file so it's not viable for our situation. Thks. Brgds, CheahKC