Remove HID ignore quirk for MGE UPS devices

Remove HID ignore quirk for MGE UPS devices

[Richard Hughes]

[-- Attachment #1: Type: text/plain, Size: 2234 bytes --]

The attached patch reverts the change made four years ago here:
http://www.vg.kernel.org/pub/linux/kernel/people/gregkh/usb/2.4/usb-hid-2.4.25-pre7.patch

UPS's made by MGE can be used with usbhid just fine, and by removing the
ignore quirk allows them to be used with HAL so they just work when plugged
in, without needing to be manually configured.

With the ignore quirk in place a user would have to configure NUT before the
UPS could be used, as NUT uses it's own internal USB matching framework
to match against the USB devices, do low level control messages on the
device and then parse the HID tables all in userspace.

This is not needed, as allowing the device to be claimed as a usbhid device
allows it to be used like any other USB UPS device. The devices correctly
advertise the power device page which can be queried for the device state.

I assume the quirk was changed so that people using < libusb 0.1.8 could
still use NUT's internal HID code to manage the UPS.
libusb 0.1.8 was released quite some time ago: 2004-02-11.

This patch does not break NUT as in drivers/libusb.c the device is force
unbound from the kernel driver using usb_detach_kernel_driver_np () where
it can be controlled like normal.

Signed-off-by: Richard Hughes <rhughes@redhat.com>

diff --git a/drivers/hid/usbhid/hid-quirks.c b/drivers/hid/usbhid/hid-quirks.c
index 61e78a4..faf3f4e 100644
--- a/drivers/hid/usbhid/hid-quirks.c
+++ b/drivers/hid/usbhid/hid-quirks.c
@@ -568,8 +568,8 @@ static const struct hid_blacklist {
 	{ USB_VENDOR_ID_LD, USB_DEVICE_ID_LD_MACHINETEST, HID_QUIRK_IGNORE },
 	{ USB_VENDOR_ID_MCC, USB_DEVICE_ID_MCC_PMD1024LS, HID_QUIRK_IGNORE },
 	{ USB_VENDOR_ID_MCC, USB_DEVICE_ID_MCC_PMD1208LS, HID_QUIRK_IGNORE },
-	{ USB_VENDOR_ID_MGE, USB_DEVICE_ID_MGE_UPS, HID_QUIRK_IGNORE },
-	{ USB_VENDOR_ID_MGE, USB_DEVICE_ID_MGE_UPS1, HID_QUIRK_IGNORE },
+	{ USB_VENDOR_ID_MGE, USB_DEVICE_ID_MGE_UPS, HID_QUIRK_HIDDEV },
+	{ USB_VENDOR_ID_MGE, USB_DEVICE_ID_MGE_UPS1, HID_QUIRK_HIDDEV },
 	{ USB_VENDOR_ID_ONTRAK, USB_DEVICE_ID_ONTRAK_ADU100, HID_QUIRK_IGNORE },
 	{ USB_VENDOR_ID_ONTRAK, USB_DEVICE_ID_ONTRAK_ADU100 + 20, HID_QUIRK_IGNORE },
 	{ USB_VENDOR_ID_ONTRAK, USB_DEVICE_ID_ONTRAK_ADU100 + 30, HID_QUIRK_IGNORE },



[-- Attachment #2: 0001-remove-hid-ignore-quirk-for-mge-ups-devices.patch --]
[-- Type: text/x-patch, Size: 2510 bytes --]

commit 0abf9c8395f83ba1396574ee4ca0efa3541b56b1
Author: Richard Hughes <richard@hughsie.com>
Date:   Wed Aug 13 09:43:02 2008 +0100

    remove hid ignore quirk for MGE UPS devices
    
    This patch reverts the change made four years ago here:
    http://www.vg.kernel.org/pub/linux/kernel/people/gregkh/usb/2.4/usb-hid-2.4.25-pre7.patch
    
    UPS's made by MGE can be used with usbhid just fine, and by removing the
    ignore quirk allows them to be used with HAL so they just work when plugged
    in, without needing to be manually configured.
    
    With the ignore quirk in place a user would have to configure NUT before the
    UPS could be used, as NUT uses it's own internal USB matching framework
    to match against the USB devices, do low level control messages on the
    device and then parse the HID tables all in userspace.
    
    This is not needed, as allowing the device to be claimed as a usbhid device
    allows it to be used like any other USB UPS device. The devices correctly
    advertise the power device page which can be queried for the device state.
    
    I assume the quirk was changed so that people using < libusb 0.1.8 could
    still use NUT's internal HID code to manage the UPS.
    libusb 0.1.8 was released quite some time ago: 2004-02-11.
    
    This patch does not break NUT as in drivers/libusb.c the device is force
    unbound from the kernel driver using usb_detach_kernel_driver_np () where
    it can be controlled like normal.
    
    Signed-off-by: Richard Hughes <rhughes@redhat.com>

diff --git a/drivers/hid/usbhid/hid-quirks.c b/drivers/hid/usbhid/hid-quirks.c
index 61e78a4..faf3f4e 100644
--- a/drivers/hid/usbhid/hid-quirks.c
+++ b/drivers/hid/usbhid/hid-quirks.c
@@ -568,8 +568,8 @@ static const struct hid_blacklist {
 	{ USB_VENDOR_ID_LD, USB_DEVICE_ID_LD_MACHINETEST, HID_QUIRK_IGNORE },
 	{ USB_VENDOR_ID_MCC, USB_DEVICE_ID_MCC_PMD1024LS, HID_QUIRK_IGNORE },
 	{ USB_VENDOR_ID_MCC, USB_DEVICE_ID_MCC_PMD1208LS, HID_QUIRK_IGNORE },
-	{ USB_VENDOR_ID_MGE, USB_DEVICE_ID_MGE_UPS, HID_QUIRK_IGNORE },
-	{ USB_VENDOR_ID_MGE, USB_DEVICE_ID_MGE_UPS1, HID_QUIRK_IGNORE },
+	{ USB_VENDOR_ID_MGE, USB_DEVICE_ID_MGE_UPS, HID_QUIRK_HIDDEV },
+	{ USB_VENDOR_ID_MGE, USB_DEVICE_ID_MGE_UPS1, HID_QUIRK_HIDDEV },
 	{ USB_VENDOR_ID_ONTRAK, USB_DEVICE_ID_ONTRAK_ADU100, HID_QUIRK_IGNORE },
 	{ USB_VENDOR_ID_ONTRAK, USB_DEVICE_ID_ONTRAK_ADU100 + 20, HID_QUIRK_IGNORE },
 	{ USB_VENDOR_ID_ONTRAK, USB_DEVICE_ID_ONTRAK_ADU100 + 30, HID_QUIRK_IGNORE },

Re: Remove HID ignore quirk for MGE UPS devices

[Jiri Kosina]

On Fri, 15 Aug 2008, Richard Hughes wrote:

> With the ignore quirk in place a user would have to configure NUT before the
> UPS could be used, as NUT uses it's own internal USB matching framework
> to match against the USB devices, do low level control messages on the
> device and then parse the HID tables all in userspace.
> This is not needed, as allowing the device to be claimed as a usbhid device
> allows it to be used like any other USB UPS device. The devices correctly
> advertise the power device page which can be queried for the device state.

Applied, thanks Richard.

-- 
Jiri Kosina
SUSE Labs

Re: Remove HID ignore quirk for MGE UPS devices

[Richard Hughes]

On Mon, 2008-09-01 at 13:50 +0200, Arnaud Quette wrote:
> first, note that I'm the originator of the below mentioned (2.4.25)
> patch, and the one that has developed the USB UPS support code (helped
> only for the kernel part, and done the userland one).

After reading al the kernel and NUT HID stuff, I'm really not convinced
any of the low level HID table parsing should be done in userspace at
all.

> 2008/8/15 Richard Hughes <rhughes@redhat.com>:
> > The attached patch reverts the change made four years ago here:
> > http://www.vg.kernel.org/pub/linux/kernel/people/gregkh/usb/2.4/usb-hid-2.4.25-pre7.patch
> >
> > UPS's made by MGE can be used with usbhid just fine, and by removing the
> > ignore quirk allows them to be used with HAL so they just work when plugged
> > in, without needing to be manually configured.
> 
> this is true for recent units, but hiddev (not itself but something in
> the USB stack at that time) was breaking the communication with some
> of MGE *UPS* small devices. Note that this might have been solved
> since then.

Right, so it should be a tiny patch to the kernel HID parser, if it's
not been fixed already.

> the assertion that is false is that without this, HAL support can't work.
> the code to support USB UPSs that I've developed since this oldish
> kernel since that allows this (Richard, you have missed to compile nut
> with --with-hal...). This latter is more complete (support all USB HID
> UPS + all USB devices, along with more features, more specific
> handling, more data, more ... everything) and is *portable* (or
> ported) on other USB enabled Unix (*bsd, solaris and darwin). So as to
> the HAL support code using this exact same NUT code.

Sure, I've used --with-hal myself. The number of desktop users who
install NUT and use the hal-nut sub package is virtually nil in the real
world. It also excludes server machines that don't want or need HAL, and
doesn't really work well when DeviceKit starts being used instead. I've
got thousands of lshal outputs from desktop machines, and the majority
of USB UPS devices are not managed by NUT as the users probably think it
"doesn't work with Linux".

I appreciate some of the older more random serial cable units probably
belong in a userspace library, but when the new standard USB HID stuff
in the kernel makes this just work without any drivers, libraries or
daemons, then I really think we should use that.

> > With the ignore quirk in place a user would have to configure NUT before the
> > UPS could be used, as NUT uses it's own internal USB matching framework
> > to match against the USB devices, do low level control messages on the
> > device and then parse the HID tables all in userspace.
> 
> wrong, see above.

I don't see why a user should have to install a multi-megabyte library
and daemon just to use the power page device that the kernel already
supports. This stuff can _just work_, and does for all the other of
thousands of UPS devices that support the HID power page.

> The only thing I can say, as in 2004 is that in kernel UPS handling is
> too fat, incomplete and unuseful.

Right, so the proper way to do this is to work with the subsystem
maintainers, and push the quirk patches lower into the kernel as they
are probably useful for other devices.

> the userland support is there, and for some time mature, propose more
> features, best device handling (and there are many devices specifics
> out there)

Right, there are a ton of HID quirks right now, I don't think adding a
couple more for obsolete devices there would be a problem.

> , support all USB UPSs (not only HID ones), support HAL and
> hotplug / 0 config things and is portable.
> 
> the PDC support is hiddev should simply be removed! that would give a
> small diet to the kernel too...

I don't agree. After all the work I've done with userspace drivers the
conclusion I've come to is that they are more complex, slower and often
are not as stable as in kernel drivers. If the PDC page is removed, then
NUT has to be installed by default on every machine, just in case a USB
UPS is ever inserted. I don't think that's a good idea.

Richard.

Re: Remove HID ignore quirk for MGE UPS devices

[Jiri Kosina]

On Mon, 1 Sep 2008, Richard Hughes wrote:

> > the userland support is there, and for some time mature, propose more 
> > features, best device handling (and there are many devices specifics 
> > out there)
> Right, there are a ton of HID quirks right now, I don't think adding a
> couple more for obsolete devices there would be a problem.

Agreed. Even the "quirk support" is now more mature -- there are hidbus 
patches queued in my tree, that allow for creation of separate drivers for 
every non-standard HID device, so that quirks don't have to be sprinkled 
randomly all over the HID code.
If you are going to add any new more or less complex quirks, I'd like to 
ask you to look into this infrastructure.

Thanks,

-- 
Jiri Kosina
SUSE Labs