[Nut-upsdev] [nut-commits] svn commit r1074 - in trunk: .

[Arjen de Korte]

>> The nasty side effect here, is that reading and parsing the report
>> descriptor requires claiming the device. If this happens to be *not* the
>> device we need to reconnect to, this means that the driver that *is*
>> connected to it will need to reclaim it. I have not found a way yet how
>> to do that cleanly.
> I could be wrong, but I thought that detaching a running driver only
> works for kernel drivers (keyboard, mouse). If a user level driver has
> claimed the device, then any other process claiming the device should
> simply fail. Have you observed otherwise?

Yes, although you'll only notice this in the syslog, the drivers will
appear to run normally. You can reproduce this quite easily. In 'ups.conf'
copy an existing entry for your (HID Power Device) UPS and name it
[duplicate] or some other easily remembered name and restart the lot.
Essentially, you fire up two drivers for the same UPS. The first one will
detach the existing kernel driver, claim the device and start normally. No
suriprise here. The second driver to start, won't have to detach the
kernel driver (which has been done already) but will still (need to) claim
the device. As a consequence (I don't know if this is Linux specific or
not), driver one will be detached and the kernel will flood the syslog
with warnings that process (insert PID of first driver) reads from the USB
device without claiming it. Not nice at all.

>> If you're worried about that, 'matching = delayed' is for you. I don't
>> think this should be the default though, since for devices that support
>> to be uniquely matched without reading the report descriptor, it would
>> mean a lot of additional effort for no good reason at all.
> I think the extra computational effort at runtime is negligible in
> real terms (remember that until recently, we used to parse the report
> descriptor each time any variable was accessed).

Parsing the report descriptor takes several seconds, at least for my
Evolution 650 (1700+ bytes, about 100 reports in total) and is totally
wasted, since it allows exact matching without reading the report
descriptor. As I said before, the fact that *some* UPS'es are broken,
should not mean that we should behave if *all* are broken. We're not
flooded by reports from people that suffer from this, so as long as this
doesn't change, I think we should keep this an option that needs to be
explicitly selected.

> An extra allocation/deallocation is unnecessary, provided that the report
> descriptor is an exact match (which the matcher should check).

We would probably want to calculate the CRC of the report descriptor and
if this matches, proceed without parsing it, right? That should probably
not take too much effort, I agree.

> So the
> main additional effort is a programming effort, which I understand you
> have already done.

Well, we have a callback function back to usbhid-ups that gives it an
option to accept/reject the device after reading the report descriptor.
Adding a CRC function would probably be trivial. Currently, we only do a
callback if 'matching = delayed' but there is no reason why we couldn't do
that always. Provided we don't parse the report descriptor, the additional
computational effort is likely to be insignificant.

> Note that in the case where there is a unique matching device,
> 'matching = delayed' never does anything bad for the user, i.e., its
> behavior, as visible to the user, is identical to the current default
> behavior (and therefore backwards compatible).

Not entirely. The delayed matching allows to involve results from feature
reports as well (the serial number for your Belkin UPS for instance), to
replace values that are not available through the usb_dev_handle.
Currently the vendor specific formatting routines mostly don't change
these results, but this may (and probably will) change, otherwise delayed
matching wouldn't make any sense. So regular expressions that currently
would match hd->Model 'Evolution' may not match on 'Evolution 650' for
instance. In your case, if you checked for an empty serial number, you'd
be surprised that it is now available. :-)

> The only situations in
> which the behaviors differ is when there are multiple similar devices
> present, in which case 'matching = delayed' is the safer option, isn't
> it? So perhaps one could argue that it would be a good default.

See above. I don't think we should change that, at least not without
giving ample warnings that people should look over their regular
expression to make sure they still match with 'matching = delayed'.

Best regards, Arjen