Re: [PATCH v2] HID: hiddev: allocate minor number hiddev's USB interface is bound to

[Kim Jaejoong]

Hi Benjamin

Thanks for the review my patch :)

2017-03-01 2:05 GMT+09:00 Benjamin Tissoires :
> On Feb 15 2017 or thereabouts, Jaejoong Kim wrote:
>> When HID device connected to the PC, HID device driver announces which
>> driver is loaded with a kernel info message. In this case, hiddev's minor
>> number is always '0' even though hiddev's real minor number is not zero.
>>
>> To display hiddev with minor number asked from usb core, we need
>> to fill hiddev's minor number this interface is bound to.
>>
>> Signed-off-by: Jaejoong Kim 
>> ---
>
> That's a single line of code, and I get into some headaches :)
>
> So, the commit that broke this (.minor not being set) is actually commit
> bd25f4dd69727555 ("HID: hiddev: use usb_find_interface, get rid of
> BKL"), from 2010-07-11...
>
> And this patch reverts to the intended behavior.

Could you explain more about 'intended behavior'?

> But I am wondering if we
> should really store the minor in struct hid_device if it is only used by
> hiddev. hidraw does use a minor too, but stores it in struct hidraw
> directly, so IMO it would make sense to store this in struct hiddev. The
> problem is that this struct is not exported, and it's going to be some
> refactoring work to do so.
>

I agree with you. My patch is just allocate 'right' minor number in
struct hid_device.
After got your review mail,I also think struct hiddev and struct hid_device
need some refactoring work for minor number handling as you said.

For example, hid-cp2112.c uses a minor number of struct hid_device.
Based on comment
for minor in struct hid_device, it is for hiddev not hidraw. But,
hid-cp2112's connect_mask
for hid_hw_start is HIDRAW.

I will do some work for this one, so please ignore this patch :)

Thanks, jaejoong

> So, in a way, I am tempted to give my:
> Acked-by: Benjamin Tissoires 
>
> But if Jiri feels that a cleanup of hiddev would be required instead, I
> would follow him :)
>
> Cheers,
> Benjamin
>
>> Changes in v2:
>>  - fix typo in commit message
>> ---
>>
>>  drivers/hid/usbhid/hiddev.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
>> index 700145b..27e1f8d 100644
>> --- a/drivers/hid/usbhid/hiddev.c
>> +++ b/drivers/hid/usbhid/hiddev.c
>> @@ -910,6 +910,7 @@ int hiddev_connect(struct hid_device *hid, unsigned int 
>> force)
>>   kfree(hiddev);
>>   return -1;
>>   }
>> + hid->minor = usbhid->intf->minor;
>>   return 0;
>>  }
>>
>> --
>> 2.7.4
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] HID: hiddev: allocate minor number hiddev's USB interface is bound to

[Benjamin Tissoires]

On Feb 15 2017 or thereabouts, Jaejoong Kim wrote:
> When HID device connected to the PC, HID device driver announces which
> driver is loaded with a kernel info message. In this case, hiddev's minor
> number is always '0' even though hiddev's real minor number is not zero.
> 
> To display hiddev with minor number asked from usb core, we need
> to fill hiddev's minor number this interface is bound to.
> 
> Signed-off-by: Jaejoong Kim 
> ---

That's a single line of code, and I get into some headaches :)

So, the commit that broke this (.minor not being set) is actually commit
bd25f4dd69727555 ("HID: hiddev: use usb_find_interface, get rid of
BKL"), from 2010-07-11...

And this patch reverts to the intended behavior. But I am wondering if we
should really store the minor in struct hid_device if it is only used by
hiddev. hidraw does use a minor too, but stores it in struct hidraw
directly, so IMO it would make sense to store this in struct hiddev. The
problem is that this struct is not exported, and it's going to be some
refactoring work to do so.

So, in a way, I am tempted to give my:
Acked-by: Benjamin Tissoires 

But if Jiri feels that a cleanup of hiddev would be required instead, I
would follow him :)

Cheers,
Benjamin

> Changes in v2:
>  - fix typo in commit message
> ---
> 
>  drivers/hid/usbhid/hiddev.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
> index 700145b..27e1f8d 100644
> --- a/drivers/hid/usbhid/hiddev.c
> +++ b/drivers/hid/usbhid/hiddev.c
> @@ -910,6 +910,7 @@ int hiddev_connect(struct hid_device *hid, unsigned int 
> force)
>   kfree(hiddev);
>   return -1;
>   }
> + hid->minor = usbhid->intf->minor;
>   return 0;
>  }
>  
> -- 
> 2.7.4
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] HID: hiddev: allocate minor number hiddev's USB interface is bound to

[Kim Jaejoong]

Hi Jiri, Benjamin

Could you please review my hiddev patch?

Thanks, jaejoong

2017-02-15 18:55 GMT+09:00 Jaejoong Kim :
> When HID device connected to the PC, HID device driver announces which
> driver is loaded with a kernel info message. In this case, hiddev's minor
> number is always '0' even though hiddev's real minor number is not zero.
>
> To display hiddev with minor number asked from usb core, we need
> to fill hiddev's minor number this interface is bound to.
>
> Signed-off-by: Jaejoong Kim 
> ---
> Changes in v2:
>  - fix typo in commit message
> ---
>
>  drivers/hid/usbhid/hiddev.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
> index 700145b..27e1f8d 100644
> --- a/drivers/hid/usbhid/hiddev.c
> +++ b/drivers/hid/usbhid/hiddev.c
> @@ -910,6 +910,7 @@ int hiddev_connect(struct hid_device *hid, unsigned int 
> force)
> kfree(hiddev);
> return -1;
> }
> +   hid->minor = usbhid->intf->minor;
> return 0;
>  }
>
> --
> 2.7.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] HID: hiddev: allocate minor number hiddev's USB interface is bound to

[Jaejoong Kim]

When HID device connected to the PC, HID device driver announces which
driver is loaded with a kernel info message. In this case, hiddev's minor
number is always '0' even though hiddev's real minor number is not zero.

To display hiddev with minor number asked from usb core, we need
to fill hiddev's minor number this interface is bound to.

Signed-off-by: Jaejoong Kim 
---
Changes in v2:
 - fix typo in commit message
---

 drivers/hid/usbhid/hiddev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index 700145b..27e1f8d 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -910,6 +910,7 @@ int hiddev_connect(struct hid_device *hid, unsigned int 
force)
kfree(hiddev);
return -1;
}
+   hid->minor = usbhid->intf->minor;
return 0;
 }
 
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html