Another HID problem this merge window..

Another HID problem this merge window..

[Linus Torvalds]

Ok, so this is a much smaller issue than the i2c one that cause boot
problems, but it's annoying.

We do *not* enable new random drivers by default. And we most
*definitely* don't do it when they are odd-ball ones that most people
have never heard of.

Yet the new "BigBen Interactive" driver that was added this merge
window did exactly that.

Just don't do it.

Yes, yes, every developer always thinks that _their_ driver is so
special and so magically important that it should be enabled by
default. But no. When we have thousands of drivers, we don't randomly
pick one new driver to be enabled by default just because some
developer thinks it is special. It's not.

So the

        default !EXPERT

was completely wrong in commit 256a90ed9e46 ("HID: hid-bigbenff:
driver for BigBen Interactive PS3OFMINIPAD gamepad"). Please don't do
things like this.

                         Linus

Re: Another HID problem this merge window..

[Joe Perches]

On Sat, 2018-10-27 at 11:13 -0700, Linus Torvalds wrote:
> Ok, so this is a much smaller issue than the i2c one that cause boot
> problems, but it's annoying.
> 
> We do *not* enable new random drivers by default. And we most
> *definitely* don't do it when they are odd-ball ones that most people
> have never heard of.
> 
> Yet the new "BigBen Interactive" driver that was added this merge
> window did exactly that.

In fairness, it seems many of the HID drivers do exactly that
and this could have been a "copy from example" addition.

$ git grep -P -i -B2 'default\s+\!EXPERT' -- drivers/hid/Kconfig
drivers/hid/Kconfig-    tristate "A4 tech mice"
drivers/hid/Kconfig-    depends on HID
drivers/hid/Kconfig:    default !EXPERT
--
drivers/hid/Kconfig-    tristate "Apple {i,Power,Mac}Books"
drivers/hid/Kconfig-    depends on HID
drivers/hid/Kconfig:    default !EXPERT
--
drivers/hid/Kconfig-    tristate "Belkin Flip KVM and Wireless keyboard"
drivers/hid/Kconfig-    depends on HID
drivers/hid/Kconfig:    default !EXPERT
--
drivers/hid/Kconfig-    tristate "Cherry Cymotion keyboard"
drivers/hid/Kconfig-    depends on HID
drivers/hid/Kconfig:    default !EXPERT
--
drivers/hid/Kconfig-    tristate "Chicony devices"
drivers/hid/Kconfig-    depends on HID
drivers/hid/Kconfig:    default !EXPERT
--
drivers/hid/Kconfig-    tristate "Cypress mouse and barcode readers"
drivers/hid/Kconfig-    depends on HID
drivers/hid/Kconfig:    default !EXPERT
--
drivers/hid/Kconfig-    tristate "Ezkey BTC 8193 keyboard"
drivers/hid/Kconfig-    depends on HID
drivers/hid/Kconfig:    default !EXPERT
--
drivers/hid/Kconfig-    tristate "ITE devices"
drivers/hid/Kconfig-    depends on HID
drivers/hid/Kconfig:    default !EXPERT
--
drivers/hid/Kconfig-    tristate "Kensington Slimblade Trackball"
drivers/hid/Kconfig-    depends on HID
drivers/hid/Kconfig:    default !EXPERT
--
drivers/hid/Kconfig-    tristate "Logitech devices"
drivers/hid/Kconfig-    depends on HID
drivers/hid/Kconfig:    default !EXPERT

Re: Another HID problem this merge window..

[Linus Torvalds]

On Sat, Oct 27, 2018 at 12:36 PM Joe Perches <joe@perches.com> wrote:
>
> In fairness, it seems many of the HID drivers do exactly that
> and this could have been a "copy from example" addition.

Interesting, and I think you're right.

I wonder why I haven't noticed this before. Some of those might be
hidden by other dependencies - what I do is to just check how "make
oldconfig" changes my normal fairly minimal configuration, and then
complain when I notice that somebody enables some odd new hardware.

Anyway, those other "!EXPERT" defaults look pretty questionable too.
HID itself beind default y and the HID_GENERIC driver defaulting on
seems eminently sane. And HID_LOGITECH might fall under both "very
common" and "enables other config options".

But even then, the !EXPERT seems an odd choice. Why not just 'y' for
the truly common cases?

I wonder if there is some truly old historical legacy there, ie the
old PC keyboard support would have been configurable out only for
expert users to avoid errors, and maybe the HID Kconfig file started
getting ideas from that...

                Linus

Re: Another HID problem this merge window..

[Jiri Kosina]

On Sat, 27 Oct 2018, Linus Torvalds wrote:

> I wonder if there is some truly old historical legacy there, ie the old 
> PC keyboard support would have been configurable out only for expert 
> users to avoid errors, and maybe the HID Kconfig file started getting 
> ideas from that...

This really goes waaay back to times when we extracted all the quirks from 
the generic driver (which became unmaintainable exactly because quirks 
being sprinkled left and right) into specialized drivers, but didn't want 
to cause too many user surprises that all of a sudden their configuration 
regressed when it comes to hardware support.

We've had exactly this discussion multiple times before, see for example

	https://lkml.org/lkml/2010/5/20/227

So I guess there is no need for replaying it, I think we're in complete 
agreement.

That being said, benff Kconfig setting definitely escaped attention. That 
should never ever have been set to default y, I take blame for not 
noticing that while applying the patch.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: Another HID problem this merge window..

[Benjamin Tissoires]

Hi,

On Sat, Oct 27, 2018 at 11:45 PM Jiri Kosina <jikos@kernel.org> wrote:
>
> On Sat, 27 Oct 2018, Linus Torvalds wrote:
>
> > I wonder if there is some truly old historical legacy there, ie the old
> > PC keyboard support would have been configurable out only for expert
> > users to avoid errors, and maybe the HID Kconfig file started getting
> > ideas from that...
>
> This really goes waaay back to times when we extracted all the quirks from
> the generic driver (which became unmaintainable exactly because quirks
> being sprinkled left and right) into specialized drivers, but didn't want
> to cause too many user surprises that all of a sudden their configuration
> regressed when it comes to hardware support.
>
> We've had exactly this discussion multiple times before, see for example
>
>         https://lkml.org/lkml/2010/5/20/227
>
> So I guess there is no need for replaying it, I think we're in complete
> agreement.

On the things I have on my plate, I'll try to remove all of the tiny
HID drivers that does nothing but some small remapping. This will
probably need help from userspace ("firmware" or bpf loading), and I
have not settled my plans yet.
I also think we should probably clean up the Kconfig now that
hid-generic can unbind itself if there is a special driver coming in.
And that means that we should probably start removing the blacklist of
devices that have special modules, to let them be taken by hid-generic
on boot until their driver is loaded.

>
> That being said, benff Kconfig setting definitely escaped attention. That
> should never ever have been set to default y, I take blame for not
> noticing that while applying the patch.

Not sure I have actually reviewed this one (I don't think so), but
I'll keep this in my head for next drivers.

Cheers,
Benjamin

>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
>