From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753759AbdKFRk3 (ORCPT ); Mon, 6 Nov 2017 12:40:29 -0500 Received: from 19.mo6.mail-out.ovh.net ([188.165.56.177]:48654 "EHLO 19.mo6.mail-out.ovh.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751890AbdKFRk2 (ORCPT ); Mon, 6 Nov 2017 12:40:28 -0500 X-Greylist: delayed 8390 seconds by postgrey-1.27 at vger.kernel.org; Mon, 06 Nov 2017 12:40:27 EST Subject: Re: [PATCH 3/3] HID: cp2112: fix broken gpio_direction_input callback To: Benjamin Tissoires Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org, Jiri Kosina , Julien Boibessot References: <1509621164-28830-1-git-send-email-sebastien.szymanski@armadeus.com> <1509621164-28830-3-git-send-email-sebastien.szymanski@armadeus.com> <20171106081120.GC9728@mail.corp.redhat.com> From: =?UTF-8?Q?S=c3=a9bastien_Szymanski?= Message-ID: Date: Mon, 6 Nov 2017 14:58:46 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171106081120.GC9728@mail.corp.redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Ovh-Tracer-Id: 7537618404588082206 X-VR-SPAMSTATE: OK X-VR-SPAMSCORE: -100 X-VR-SPAMCAUSE: gggruggvucftvghtrhhoucdtuddrgedttddrheeggdehvdcutefuodetggdotefrodftvfcurfhrohhfihhlvgemucfqggfjpdevjffgvefmvefgnecuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/06/2017 09:11 AM, Benjamin Tissoires wrote: > On Nov 02 2017 or thereabouts, Sébastien Szymanski wrote: >> When everything goes smoothly, ret is set to 0 which makes the function >> to return EIO error. >> >> Fixes: 8e9faa15469e ("HID: cp2112: fix gpio-callback error handling") >> Signed-off-by: Sébastien Szymanski >> --- >> drivers/hid/hid-cp2112.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c >> index 28e3c18..f7754a6 100644 >> --- a/drivers/hid/hid-cp2112.c >> +++ b/drivers/hid/hid-cp2112.c >> @@ -205,12 +205,13 @@ static int cp2112_gpio_direction_input(struct gpio_chip *chip, unsigned offset) >> ret = hid_hw_raw_request(hdev, CP2112_GPIO_CONFIG, buf, >> CP2112_GPIO_CONFIG_LENGTH, HID_FEATURE_REPORT, >> HID_REQ_SET_REPORT); >> - if (ret < 0) { >> + if (ret != CP2112_GPIO_CONFIG_LENGTH) { > > Ack for this. As explained in the interface specification, the device doesn't answer to set reports, so the transfer should be CP2112_GPIO_CONFIG_LENGTH (5) bytes. > >> hid_err(hdev, "error setting GPIO config: %d\n", ret); >> goto exit; >> } >> >> - ret = 0; >> + mutex_unlock(&dev->lock); >> + return 0; > > Wouldn't it be better to just turn > - return ret < 0 ? ret : -EIO; > into > + return ret <= 0 ? ret : -EIO; > at the end of the function? Well, the commit I mentioned in the Fixes tag, changes from - return ret <= 0 ? ret : -EIO; to + return ret < 0 ? ret : -EIO; because ret being 0 could mean that one of the hid_hw_raw_request returned 0. Regards, > > I'd rather keep the same exit path in both cases, error or success. > > Cheers, > Benjamin > > >> >> exit: >> mutex_unlock(&dev->lock); >> -- >> 2.7.3 >> -- Sébastien Szymanski Software engineer, Armadeus Systems Tel: +33 (0)9 72 29 41 44 Fax: +33 (0)9 72 28 79 26