* [PATCH 1/3] HID: cp2112: fix interface specification URL
@ 2017-11-02 11:12 Sébastien Szymanski
2017-11-02 11:12 ` [PATCH 2/3] HID: cp2112: add HIDRAW dependency Sébastien Szymanski
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Sébastien Szymanski @ 2017-11-02 11:12 UTC (permalink / raw)
To: linux-input
Cc: linux-kernel, Benjamin Tissoires, Jiri Kosina, Julien Boibessot,
Sébastien Szymanski
Signed-off-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com>
---
drivers/hid/hid-cp2112.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 078026f..28e3c18 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -21,7 +21,7 @@
* Data Sheet:
* http://www.silabs.com/Support%20Documents/TechnicalDocs/CP2112.pdf
* Programming Interface Specification:
- * http://www.silabs.com/Support%20Documents/TechnicalDocs/AN495.pdf
+ * https://www.silabs.com/documents/public/application-notes/an495-cp2112-interface-specification.pdf
*/
#include <linux/gpio.h>
--
2.7.3
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 2/3] HID: cp2112: add HIDRAW dependency 2017-11-02 11:12 [PATCH 1/3] HID: cp2112: fix interface specification URL Sébastien Szymanski @ 2017-11-02 11:12 ` Sébastien Szymanski 2017-11-06 8:12 ` Benjamin Tissoires 2017-11-02 11:12 ` [PATCH 3/3] HID: cp2112: fix broken gpio_direction_input callback Sébastien Szymanski 2017-11-09 11:45 ` [PATCH 1/3] HID: cp2112: fix interface specification URL Jiri Kosina 2 siblings, 1 reply; 10+ messages in thread From: Sébastien Szymanski @ 2017-11-02 11:12 UTC (permalink / raw) To: linux-input Cc: linux-kernel, Benjamin Tissoires, Jiri Kosina, Julien Boibessot, Sébastien Szymanski Otherwise, with HIDRAW=n, the probe function crashes because of null dereference of hdev->hidraw. Fixes: 42cb6b35b9e6 ("HID: cp2112: use proper hidraw name with minor number") Signed-off-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com> --- drivers/hid/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index 374301f..8c7a0ce 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -230,7 +230,7 @@ config HID_CMEDIA config HID_CP2112 tristate "Silicon Labs CP2112 HID USB-to-SMBus Bridge support" - depends on USB_HID && I2C && GPIOLIB + depends on USB_HID && HIDRAW && I2C && GPIOLIB select GPIOLIB_IRQCHIP ---help--- Support for Silicon Labs CP2112 HID USB to SMBus Master Bridge. -- 2.7.3 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] HID: cp2112: add HIDRAW dependency 2017-11-02 11:12 ` [PATCH 2/3] HID: cp2112: add HIDRAW dependency Sébastien Szymanski @ 2017-11-06 8:12 ` Benjamin Tissoires 0 siblings, 0 replies; 10+ messages in thread From: Benjamin Tissoires @ 2017-11-06 8:12 UTC (permalink / raw) To: Sébastien Szymanski Cc: linux-input, linux-kernel, Jiri Kosina, Julien Boibessot On Nov 02 2017 or thereabouts, Sébastien Szymanski wrote: > Otherwise, with HIDRAW=n, the probe function crashes because of null > dereference of hdev->hidraw. > > Fixes: 42cb6b35b9e6 ("HID: cp2112: use proper hidraw name with minor number") > Signed-off-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com> Patches 1 and 2 (only) are: Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > --- > drivers/hid/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig > index 374301f..8c7a0ce 100644 > --- a/drivers/hid/Kconfig > +++ b/drivers/hid/Kconfig > @@ -230,7 +230,7 @@ config HID_CMEDIA > > config HID_CP2112 > tristate "Silicon Labs CP2112 HID USB-to-SMBus Bridge support" > - depends on USB_HID && I2C && GPIOLIB > + depends on USB_HID && HIDRAW && I2C && GPIOLIB > select GPIOLIB_IRQCHIP > ---help--- > Support for Silicon Labs CP2112 HID USB to SMBus Master Bridge. > -- > 2.7.3 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] HID: cp2112: fix broken gpio_direction_input callback 2017-11-02 11:12 [PATCH 1/3] HID: cp2112: fix interface specification URL Sébastien Szymanski 2017-11-02 11:12 ` [PATCH 2/3] HID: cp2112: add HIDRAW dependency Sébastien Szymanski @ 2017-11-02 11:12 ` Sébastien Szymanski 2017-11-06 8:11 ` Benjamin Tissoires 2017-11-09 11:45 ` [PATCH 1/3] HID: cp2112: fix interface specification URL Jiri Kosina 2 siblings, 1 reply; 10+ messages in thread From: Sébastien Szymanski @ 2017-11-02 11:12 UTC (permalink / raw) To: linux-input Cc: linux-kernel, Benjamin Tissoires, Jiri Kosina, Julien Boibessot, Sébastien Szymanski 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 <sebastien.szymanski@armadeus.com> --- 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) { hid_err(hdev, "error setting GPIO config: %d\n", ret); goto exit; } - ret = 0; + mutex_unlock(&dev->lock); + return 0; exit: mutex_unlock(&dev->lock); -- 2.7.3 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] HID: cp2112: fix broken gpio_direction_input callback 2017-11-02 11:12 ` [PATCH 3/3] HID: cp2112: fix broken gpio_direction_input callback Sébastien Szymanski @ 2017-11-06 8:11 ` Benjamin Tissoires 2017-11-06 13:58 ` Sébastien Szymanski 0 siblings, 1 reply; 10+ messages in thread From: Benjamin Tissoires @ 2017-11-06 8:11 UTC (permalink / raw) To: Sébastien Szymanski Cc: linux-input, linux-kernel, Jiri Kosina, Julien Boibessot 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 <sebastien.szymanski@armadeus.com> > --- > 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. > 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? I'd rather keep the same exit path in both cases, error or success. Cheers, Benjamin > > exit: > mutex_unlock(&dev->lock); > -- > 2.7.3 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] HID: cp2112: fix broken gpio_direction_input callback 2017-11-06 8:11 ` Benjamin Tissoires @ 2017-11-06 13:58 ` Sébastien Szymanski 2017-11-06 17:43 ` Benjamin Tissoires 0 siblings, 1 reply; 10+ messages in thread From: Sébastien Szymanski @ 2017-11-06 13:58 UTC (permalink / raw) To: Benjamin Tissoires Cc: linux-input, linux-kernel, Jiri Kosina, Julien Boibessot 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 <sebastien.szymanski@armadeus.com> >> --- >> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] HID: cp2112: fix broken gpio_direction_input callback 2017-11-06 13:58 ` Sébastien Szymanski @ 2017-11-06 17:43 ` Benjamin Tissoires 0 siblings, 0 replies; 10+ messages in thread From: Benjamin Tissoires @ 2017-11-06 17:43 UTC (permalink / raw) To: Sébastien Szymanski Cc: linux-input, linux-kernel, Jiri Kosina, Julien Boibessot On Nov 06 2017 or thereabouts, Sébastien Szymanski wrote: > 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 <sebastien.szymanski@armadeus.com> > >> --- > >> 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. True. So, commit 8e9faa15469e basically makes all transfert return -EIO and should be reverted, right? IMO, your commit should be: if (ret != CP2112_GPIO_CONFIG_LENGTH) { hid_err(hdev, "error setting GPIO config: %d\n", ret); if (ret >= 0) ret = -EIO; goto exit; } ret = 0; exit: mutex_unlock(&dev->lock); return ret; How does that sound? Cheers, Benjamin > > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] HID: cp2112: fix interface specification URL 2017-11-02 11:12 [PATCH 1/3] HID: cp2112: fix interface specification URL Sébastien Szymanski 2017-11-02 11:12 ` [PATCH 2/3] HID: cp2112: add HIDRAW dependency Sébastien Szymanski 2017-11-02 11:12 ` [PATCH 3/3] HID: cp2112: fix broken gpio_direction_input callback Sébastien Szymanski @ 2017-11-09 11:45 ` Jiri Kosina 2017-11-10 9:05 ` Sébastien Szymanski 2 siblings, 1 reply; 10+ messages in thread From: Jiri Kosina @ 2017-11-09 11:45 UTC (permalink / raw) To: Sébastien Szymanski Cc: linux-input, linux-kernel, Benjamin Tissoires, Julien Boibessot On Thu, 2 Nov 2017, Sébastien Szymanski wrote: > Signed-off-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com> > --- > drivers/hid/hid-cp2112.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c > index 078026f..28e3c18 100644 > --- a/drivers/hid/hid-cp2112.c > +++ b/drivers/hid/hid-cp2112.c > @@ -21,7 +21,7 @@ > * Data Sheet: > * http://www.silabs.com/Support%20Documents/TechnicalDocs/CP2112.pdf > * Programming Interface Specification: > - * http://www.silabs.com/Support%20Documents/TechnicalDocs/AN495.pdf > + * https://www.silabs.com/documents/public/application-notes/an495-cp2112-interface-specification.pdf > */ I've applied patches 1 and 2 (with stable anotation) for now. Thanks, -- Jiri Kosina SUSE Labs ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] HID: cp2112: fix interface specification URL 2017-11-09 11:45 ` [PATCH 1/3] HID: cp2112: fix interface specification URL Jiri Kosina @ 2017-11-10 9:05 ` Sébastien Szymanski 2017-11-10 10:04 ` Jiri Kosina 0 siblings, 1 reply; 10+ messages in thread From: Sébastien Szymanski @ 2017-11-10 9:05 UTC (permalink / raw) To: Jiri Kosina Cc: linux-input, linux-kernel, Benjamin Tissoires, Julien Boibessot On 11/09/2017 12:45 PM, Jiri Kosina wrote: > On Thu, 2 Nov 2017, Sébastien Szymanski wrote: > >> Signed-off-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com> >> --- >> drivers/hid/hid-cp2112.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c >> index 078026f..28e3c18 100644 >> --- a/drivers/hid/hid-cp2112.c >> +++ b/drivers/hid/hid-cp2112.c >> @@ -21,7 +21,7 @@ >> * Data Sheet: >> * http://www.silabs.com/Support%20Documents/TechnicalDocs/CP2112.pdf >> * Programming Interface Specification: >> - * http://www.silabs.com/Support%20Documents/TechnicalDocs/AN495.pdf >> + * https://www.silabs.com/documents/public/application-notes/an495-cp2112-interface-specification.pdf >> */ > > I've applied patches 1 and 2 (with stable anotation) for now. Thanks, > Thanks, I've send a v2 of patch 3. Regards, P.S: I don't see patch 1 on your cgit, is it normal? -- Sébastien Szymanski Software engineer, Armadeus Systems Tel: +33 (0)9 72 29 41 44 Fax: +33 (0)9 72 28 79 26 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] HID: cp2112: fix interface specification URL 2017-11-10 9:05 ` Sébastien Szymanski @ 2017-11-10 10:04 ` Jiri Kosina 0 siblings, 0 replies; 10+ messages in thread From: Jiri Kosina @ 2017-11-10 10:04 UTC (permalink / raw) To: Sébastien Szymanski Cc: linux-input, linux-kernel, Benjamin Tissoires, Julien Boibessot On Fri, 10 Nov 2017, Sébastien Szymanski wrote: > >> Signed-off-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com> > >> --- > >> drivers/hid/hid-cp2112.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c > >> index 078026f..28e3c18 100644 > >> --- a/drivers/hid/hid-cp2112.c > >> +++ b/drivers/hid/hid-cp2112.c > >> @@ -21,7 +21,7 @@ > >> * Data Sheet: > >> * http://www.silabs.com/Support%20Documents/TechnicalDocs/CP2112.pdf > >> * Programming Interface Specification: > >> - * http://www.silabs.com/Support%20Documents/TechnicalDocs/AN495.pdf > >> + * https://www.silabs.com/documents/public/application-notes/an495-cp2112-interface-specification.pdf > >> */ > > > > I've applied patches 1 and 2 (with stable anotation) for now. Thanks, > > > > Thanks, I've send a v2 of patch 3. > > Regards, > > P.S: I don't see patch 1 on your cgit, is it normal? It's been applied only locally, today I've actually pushed it out. Thanks, -- Jiri Kosina SUSE Labs ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-11-10 10:04 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-11-02 11:12 [PATCH 1/3] HID: cp2112: fix interface specification URL Sébastien Szymanski 2017-11-02 11:12 ` [PATCH 2/3] HID: cp2112: add HIDRAW dependency Sébastien Szymanski 2017-11-06 8:12 ` Benjamin Tissoires 2017-11-02 11:12 ` [PATCH 3/3] HID: cp2112: fix broken gpio_direction_input callback Sébastien Szymanski 2017-11-06 8:11 ` Benjamin Tissoires 2017-11-06 13:58 ` Sébastien Szymanski 2017-11-06 17:43 ` Benjamin Tissoires 2017-11-09 11:45 ` [PATCH 1/3] HID: cp2112: fix interface specification URL Jiri Kosina 2017-11-10 9:05 ` Sébastien Szymanski 2017-11-10 10:04 ` Jiri Kosina
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).