linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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
* [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 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
* 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).