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

drivers by default (was Re: Another HID problem this merge window..)

[Adam Borowski]

On Sat, Oct 27, 2018 at 11:13:17AM -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.
> 
> Just don't do it.

Amen to that.  But, perhaps you could encourage people to do enable drivers
once they become very popular?  For example, I just (72a9c673636) got hit by
USB 3.0 being off in defconfigs, and not having keyboard is not that cool.

So what about a policy that says "almost all drivers are to be =n initially,
but patches enabling popular stuff by default are welcome"?  Preferably if
obsolete crap gets mass-disabled at least once per decade (think of the
floppy driver's importance on day one vs now).

> 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.

We don't know which drivers will become important and which won't, only time
can tell.  But not having such drivers on by default is also a problem.


Meow!
-- 
⢀⣴⠾⠻⢶⣦⠀ 
⣾⠁⢰⠒⠀⣿⡁ 10 people enter a bar: 1 who understands binary,
⢿⡄⠘⠷⠚⠋⠀ 1 who doesn't, D who prefer to write it as hex,
⠈⠳⣄⠀⠀⠀⠀ and 1 who narrowly avoided an off-by-one error.

Re: drivers by default (was Re: Another HID problem this merge window..)

[Linus Torvalds]

On Sun, Oct 28, 2018 at 4:16 AM Adam Borowski <kilobyte@angband.pl> wrote:
>
> Amen to that.  But, perhaps you could encourage people to do enable drivers
> once they become very popular?  For example, I just (72a9c673636) got hit by
> USB 3.0 being off in defconfigs, and not having keyboard is not that cool.

Yes, that seems reasonable.

I don't think we need much of an explicit "policy" for these things,
though. Things that get _so_ ubiquitous that they should be marked
default globally (as opposed to just being in a defconfig file for
some board) are pretty rare, and them being "new" things are rarer
still.

Yeah, usb3 sounds like it would count, but I think that's such a rare
exception that I don't think we need to make a policy for it, just a
"hey, once or twice in a decade we have a new bus that became so
common that we should update the 'default' to y".

               Linus

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
>