From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751936AbdK0KVW (ORCPT ); Mon, 27 Nov 2017 05:21:22 -0500 Received: from mail-lf0-f67.google.com ([209.85.215.67]:45689 "EHLO mail-lf0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751668AbdK0KVV (ORCPT ); Mon, 27 Nov 2017 05:21:21 -0500 X-Google-Smtp-Source: AGs4zMY1K4FTJHXwRAKxeU/SoY/y5JfFRDYPumfuST+hW6b9gG4gNQMFD6d0+vjzdEuNYxTSVutobw== Date: Mon, 27 Nov 2017 11:21:15 +0100 From: Johan Hovold To: Gimcuan Hui Cc: Johan Hovold , gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] USB: serial: Correct return value on read Message-ID: <20171127102115.GB9862@localhost> References: <20171126161851.25650-1-gimcuan@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171126161851.25650-1-gimcuan@gmail.com> User-Agent: Mutt/1.7.2 (2016-11-26) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Nov 26, 2017 at 04:18:51PM +0000, Gimcuan Hui wrote: > It's meaningless to return buf[0] on read. Because the caller of this > interface checks the return value negative or not. Instead, we should > return the result variable. It's not really meaningless, it's just that the return value is no longer being used the way it was originally intended. I think "redundant" and/or "confusing" would be a more appropriate description. > Signed-off-by: Gimcuan Hui > --- > drivers/usb/serial/ark3116.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/usb/serial/ark3116.c b/drivers/usb/serial/ark3116.c > index 3c544782f60b..bfdbc7164e7b 100644 > --- a/drivers/usb/serial/ark3116.c > +++ b/drivers/usb/serial/ark3116.c > @@ -101,11 +101,9 @@ static int ark3116_read_reg(struct usb_serial *serial, > reg, result); > if (result >= 0) > result = -EIO; > - > - return result; > } > > - return buf[0]; > + return result; Also we do not want to return the value of result on success as that would always be 1 (i.e. the buffer size). You could change this function, and also the write_reg one, to return 0 on success since this is a more common pattern. Please also rephrase your commit summary (Subject) since you're not really "correcting" (as in fixing) anything here. This is more a of nice clean up, even if it could potentially also prevent future bugs. Thanks, Johan