On Mon, Jan 7, 2019 at 6:25 PM Nikolai Kondrashov <spbnick@xxxxxxxxx> wrote: > > Hi Benjamin, > > First of all, thanks a lot for the review :) > > > As a rule of thumb, we don't normally allow more than one file per > > vendor in the HID tree. But when the file gets out of control (or when > > it has a strong history) we can make exception. > > In your case, I think the split is justified, but I would only split > > the file in 2: hid-uclogic-rdesc.c and hid-uclogic-core.c. > > I don't really like the `params` one, especially if it it only the > > init sequence of the devices. I do not really see a benefit of > > splitting this init logic from the rest of the driver. > > I think I see what you mean. Yeah, the main file has no initialization, is > pretty short, and the initialization is in hid-uclogic-params.c. > > However, this one thing I'd like to defend. As you can see, there is a ton of > tablet models, and each with their own quirk, many of which cannot be > described with a report descriptor. There's also the thing where tablet > parameter discovery cannot be separated from the initialization, or I would've > done it. > > What I was trying to achieve by separating hid-uclogic-params.c is to narrow > down and explicitly and clearly define all the possible quirks a tablet can > have (see hid-uclogic-params.h). So that I could stop all the various ways > they can be detected/handled from spreading all over the code, and then > mutating. Things like those big switch statements checking VID:PID pairs in > multiple places. This was starting to happen as we tried to handle the new > Huion models (which wasn't contributed to the kernel). They were close enough, > and at the same time different enough, to cause a big mess and it was > difficult to disentangle the tablet description from tablet state. > > I think the goal of keeping the number of HID driver files to a minimum is a > worthy one, I sympathize. Yes, I can simply merge hid-uclogic-params.c into > hid-uclogic-core.c and it will work, of course. However, having tablet > description and feature discovery in a separate file from the actual > functionality makes it easier for me to think about them, and makes it easier > to prevent the code from regressing to quirk discovery all over the place, and > making the actual driver functionality code much bigger. This way we detect > them and describe them once. And I also get to be a little saner. Maybe that is just a naming issue then. Also one thing I learned with other subsystems (think drm) is that some time, having the same code path crippled with switch/case is much more of a burden than simply duplicating the code (and just one big switch case and function calls). This also prevent a device to regress when we introduce a modification for one of the latest version. Talking about regressions, I am currently running regressions tests at the HID protocol level through https://gitlab.freedesktop.org/libevdev/hid-tools (for the record, I have a local gitlab setup that runs this CI for me on every commit before I push). See https://gitlab.freedesktop.org/libevdev/hid-tools/blob/master/tests/test_multitouch.py for an example for hid-multitouch tests. I am hopping to have Wacom (or get the Wacom engineers) on board too, but having some hid-uclogic regeression tests would not be a bad idea. This tool focus on HID (thanks to uhid), so we can't check the USB get_str() calls, but at least we can test that the kernel emulates the proper in-range or any other protocols nitpicks we need to do. > > > While you are working on this (well attempt to at least), I'd like to > > know if we can simply drop the hid_have_special_driver related to > > hid-uclogic.ko. I am hesitant in blindly removing the full list, but > > reducing it to the bare minimum is something I'd like to achieve. > > I'm a little confused by this and your other comments regarding the > hid_have_special_driver entries. IIRC, it was that the driver wouldn't be used > at all without an entry there, unless you rebind it manually. I'm assuming > that has changed, will look at the fresh code to see what's going on, and if > it's true, will gladly drop the entries, but if you have a moment I would > appreciate an explanation :) TL;DR, since v4.16, hid-generic unbinds itself if you load an other driver on the bus that claims the device: commit e04a0442d33b8cf183bba38646447b891bb02123 upstream ("HID: core: remove the absolute need of hid_have_special_driver[]"). The code slightly evolved since this initial commit, but the TL;DR still applies. When a driver is added to the bus, hid-core checks if hid-generic is bound to one of the claimed devices, and it unbinds hid-generic. Likewise if the other driver is loaded, hid-generic will only bind itself to a new device if no other driver claims it. This allows to have LUKS password typed with hid-generic even if the hid-vendor-driver-for-this-keyboard module is not in the initrd :) > > > hid-uclogic is using direct calls to usb. Could you guard them by > > `hid_is_using_ll_driver(hdev, &usb_hid_driver)` before each calls or > > make .probe() refuses to bind if not using true USB? > > Hmm. I'll figure it out and will fix, thanks! Bonus point for you: if you can make hid-uclogic uhid capable, you can use it in the regression tests :) > > > Last, I understand the burden of rewriting the history. However, I'd > > like this patch to be split in 3 at least: > > - one patch to separate hid-uclogic.c into hid-uclogic-core and > > hid-uclogic-rdesc > > - one patch that rewrites the handling of the init sequence > > - one patch that adds the new devices > > Aaargh. It's painful, but I understand why you need this, and I'll try to do > it. Well, the bare minimum would be to split the reshuffle around (hid-uclogic-core+) and the code features. This would greatly help the review. And that should not take too much time, hopefully. > > I'll also look into fixing the other patches in the series, of course. Thanks. > > All of that likely on the weekend. :/ Cheers, Benjamin > > Thank you, Benjamin! > > Nick > > On 1/7/19 6:47 PM, Benjamin Tissoires wrote: > > Hi Nick, > > > > On Fri, Dec 28, 2018 at 1:59 PM Nikolai Kondrashov <spbnick@xxxxxxxxx> wrote: > >> > >> On 12/27/18 7:16 PM, Nikolai Kondrashov wrote: > >>> Rewrite the hid-uclogic driver by splitting the driver into three files: > >>> > >>> * hid-uclogic-core.c - the "front-end" of a normal HID driver, > >>> * hid-uclogic-rdesc.c - a collection of report descriptors, and > >>> * hid-uclogic-params.c - probing and initializing tablet parameters. > >> > >> Did this one get stuck somewhere in a moderation queue? > > > > Looks like the LKML didn't like it. It doesn't show up on > > https://patchwork.kernel.org/project/linux-input/list/?series=60781 > > > > I have a few comments on 4/4, they are general so I hope you don't > > mind I put them here: > > > > As a rule of thumb, we don't normally allow more than one file per > > vendor in the HID tree. But when the file gets out of control (or when > > it has a strong history) we can make exception. > > In your case, I think the split is justified, but I would only split > > the file in 2: hid-uclogic-rdesc.c and hid-uclogic-core.c. > > I don't really like the `params` one, especially if it it only the > > init sequence of the devices. I do not really see a benefit of > > splitting this init logic from the rest of the driver. > > > > While you are working on this (well attempt to at least), I'd like to > > know if we can simply drop the hid_have_special_driver related to > > hid-uclogic.ko. I am hesitant in blindly removing the full list, but > > reducing it to the bare minimum is something I'd like to achieve. > > > > hid-uclogic is using direct calls to usb. Could you guard them by > > `hid_is_using_ll_driver(hdev, &usb_hid_driver)` before each calls or > > make .probe() refuses to bind if not using true USB? > > > > Last, I understand the burden of rewriting the history. However, I'd > > like this patch to be split in 3 at least: > > - one patch to separate hid-uclogic.c into hid-uclogic-core and > > hid-uclogic-rdesc > > - one patch that rewrites the handling of the init sequence > > - one patch that adds the new devices > > > > I hope it is not too much to ask. > > > > Cheers, > > Benjamin > > > >> Don't see it in the archive. > >> > >> Nick >
