| Message ID | 1487152516-12335-1-git-send-email-climbbb.kim@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jiri, Benjamin Could you please review my hiddev patch? Thanks, jaejoong 2017-02-15 18:55 GMT+09:00 Jaejoong Kim <climbbb.kim@gmail.com>: > 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 <climbbb.kim@gmail.com> > --- > 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-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <climbbb.kim@gmail.com> > --- 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 <benjamin.tissoires@redhat.com> 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-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Benjamin Thanks for the review my patch :) 2017-03-01 2:05 GMT+09:00 Benjamin Tissoires <benjamin.tissoires@redhat.com>: > 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 <climbbb.kim@gmail.com> >> --- > > 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 <benjamin.tissoires@redhat.com> > > 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-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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 <climbbb.kim@gmail.com> --- Changes in v2: - fix typo in commit message --- drivers/hid/usbhid/hiddev.c | 1 + 1 file changed, 1 insertion(+)