[GIT PATCH] HID and USB HID update for 2.6.21-rc2

[GIT PATCH] HID and USB HID update for 2.6.21-rc2

[Jiri Kosina]

Linus,

could you please pull from 'for-linus' branch of

        git://git.kernel.org/pub/scm/linux/kernel/git/jikos/hid.git for-linus

or
        master.kernel.org:/pub/scm/linux/kernel/git/jikos/hid.git for-linus

to receive updates for HID core layer and USB HID for 2.6.21-rc2. These 
are mainly straightforward fixes for various broken devices (kernel 
bugzilla #8099, #7352) and other trivial things.

The diffstat looks larger because the usbhid code is moved from 
USB-specific directory to HID-specific directory, as discussed previously 
with Greg and Dmitry in order to ease maintainance, no functional changes 
here. This was held back until all related fixes from other people trees 
are upstream, so that no merge conflicts occur.

 MAINTAINERS                                   |    2 +
 drivers/hid/Kconfig                           |    2 +
 drivers/hid/Makefile                          |    2 +
 drivers/hid/hid-core.c                        |    7 +-
 drivers/hid/hid-debug.c                       |    1 +
 drivers/hid/hid-input.c                       |   37 +-
 drivers/hid/usbhid/Kconfig                    |  152 +++
 drivers/hid/usbhid/Makefile                   |   33 +
 drivers/hid/usbhid/hid-core.c                 | 1061 ++++++++++++++++++
 drivers/{usb/input => hid/usbhid}/hid-ff.c    |    0 
 drivers/{usb/input => hid/usbhid}/hid-lgff.c  |    0 
 drivers/{usb/input => hid/usbhid}/hid-pidff.c |    0 
 drivers/{usb/input => hid/usbhid}/hid-plff.c  |    0 
 drivers/{usb/input => hid/usbhid}/hid-tmff.c  |    0 
 drivers/{usb/input => hid/usbhid}/hid-zpff.c  |    0 
 drivers/{usb/input => hid/usbhid}/hiddev.c    |    0 
 drivers/hid/usbhid/usbhid.h                   |  504 +++++++++
 drivers/{usb/input => hid/usbhid}/usbkbd.c    |    0 
 drivers/{usb/input => hid/usbhid}/usbmouse.c  |    0 
 drivers/usb/input/Kconfig                     |  145 ---
 drivers/usb/input/Makefile                    |   24 -
 drivers/usb/input/hid-core.c                  | 1457 -------------------------
 drivers/usb/input/usbhid.h                    |   87 --
 include/linux/hid.h                           |    6 +-
 24 files changed, 1793 insertions(+), 1727 deletions(-)

Adrian Bunk (1):
      HID: hid-debug.c should #include <linux/hid-debug.h>

Jiri Kosina (8):
      USB HID: use CONFIG_HID_DEBUG for outputting report descriptor
      HID: fix bug in zeroing the last field byte in output reports
      HID: fix possible double-free on error path in hid parser
      HID: fix broken Logitech S510 keyboard report descriptor; make extra keys work
      USB HID: move usbhid to hid-specific directory
      USB HID: cleanup - move hid_blacklist to where it belongs
      HID: add git tree information to MAINTAINERS
      HID: fix Logitech DiNovo Edge touchwheel and Logic3 /SpectraVideo middle button

Julien BLACHE (1):
      USB HID: Fix USB vendor and product IDs endianness for USB HID devices

Re: [GIT PATCH] HID and USB HID update for 2.6.21-rc2

[Linus Torvalds]

On Wed, 28 Feb 2007, Jiri Kosina wrote:
>
> The diffstat looks larger because the usbhid code is moved from 
> USB-specific directory to HID-specific directory

No. The diffstat looks huge because you moved "hid_blacklist" into a 
header file, and that is a big enough change that git won't consider the 
rename to be just a rename any more (you basically moved the old 
usbdrivers/usb/input/hid-core.c file into *two* files: hid-core.c and 
usbhid.h).

Why do that? It now gets included INTO EVERY DAMN FILE that includes 
<usbhid.h>, since that one now has that

	static const struct hid_blacklist

definition in it. Yet *nothing* wants it, except for the one C file that 
it used to be in.

No way in hell will I include this kind of crap without major explanations 
for why the "crap" is not crap. And quite frankly, I don't think such 
explanations exist.

I'm also not going to pull it if you just add a new commit to undo the 
idiocy. That thing needs to be totally re-done, as far as I'm concerned. I 
don't want to touch anything that has EVER even *seen* anything that 
stupid.

Yes, I'm grumpy. I don't like big changes at this stage, and if they are 
also STUPID big changes, as this seems to be, I refuse to pull them 
entirely.

		Linus

Re: [GIT PATCH] HID and USB HID update for 2.6.21-rc2

[Jiri Kosina]

On Wed, 28 Feb 2007, Linus Torvalds wrote:

> > The diffstat looks larger because the usbhid code is moved from 
> > USB-specific directory to HID-specific directory
> No. The diffstat looks huge because you moved "hid_blacklist" into a 
> header file, and that is a big enough change that git won't consider the 
> rename to be just a rename any more (you basically moved the old 
> usbdrivers/usb/input/hid-core.c file into *two* files: hid-core.c and 
> usbhid.h).

Yes, but was done by two separate commits, diffstat -M for 
3d5af52d0997d545995d8747c8057be5dee49b14 shows hid-core.c as a 100% 
rename, but later commit b41ea57c01a1943ab36af0017cfc1329815af9ce splits 
it, so in cummulative diffstat it doesn't show it as a rename.

> Why do that? It now gets included INTO EVERY DAMN FILE that includes 
> <usbhid.h>, since that one now has that
> 	static const struct hid_blacklist
> definition in it. Yet *nothing* wants it, except for the one C file that 
> it used to be in.

You're right that usbhid.h is not a best place for it. The thing is that 
hid_blacklist[] and related things just badly needs cleanup - it has been 
for quite a long time placed on a really random place in the middle of a 
.c file. In addition to that, the corresponding #defines are scatthered 
around, interleaved with functions (see for example where 
USB_VENDOR_ID_PANJIT and USB_VENDOR_ID_TURBOX defines are).

> I'm also not going to pull it if you just add a new commit to undo the 
> idiocy. That thing needs to be totally re-done, as far as I'm concerned. I 
> don't want to touch anything that has EVER even *seen* anything that 
> stupid.

This IMHO just needs cleanup. Will you accept creating a separate header 
file solely for purposes of this blacklist and related defines?

Otherwise I will just drop this cleanup, but I still think that the 
current situation is horrible.

> Yes, I'm grumpy. I don't like big changes at this stage, and if they are 
> also STUPID big changes, as this seems to be, I refuse to pull them 
> entirely.

Are you also opposed to just the code movement? There are some bugfixes I 
think that really need merging, so just to know what would be acceptable 
for you at the time being.

Thanks,

-- 
Jiri Kosina

Re: [GIT PATCH] HID and USB HID update for 2.6.21-rc2

[Linus Torvalds]

On Wed, 28 Feb 2007, Jiri Kosina wrote:
> 
> You're right that usbhid.h is not a best place for it.

"Not the best place for it" is the understatement of the year.

It's totally idiotic. 

> This IMHO just needs cleanup. Will you accept creating a separate header 
> file solely for purposes of this blacklist and related defines?

*NO*.

Dammit, we don't put static initializers in header files. We don't 
duplicate the data in every single thing that includes a header file. If 
you want to duplicate the data, you export it as a real data structure, 
and you *still* put the data structure in a .c file.

> Otherwise I will just drop this cleanup, but I still think that the 
> current situation is horrible.

WHAT CLEANUP? The thing is the anti-thesis of a "cleanup". There is no 
excuse for putting a large array in a header file and including it 
millions of times. Or even just twice. The point of a header file is to 
*declare* things, not to have big data structures in.

		Linus

Re: [GIT PATCH] HID and USB HID update for 2.6.21-rc2

[Jiri Kosina]

On Wed, 28 Feb 2007, Linus Torvalds wrote:

> There is no excuse for putting a large array in a header file and 
> including it millions of times. Or even just twice. The point of a 
> header file is to *declare* things, not to have big data structures in.

The point was that noone else than hid/hid-core.c would ever be going to 
include this header with blacklist. The blacklist is growing in time quite 
rapidly and having it in the middle of the code just didn't look pretty - 
currently you have to perform some scrolling effort just to skip from 
usbhid_init_reports() to hid_find_max_report(), which just looks very sad 
to me.

But OK, I will leave it in there.

-- 
Jiri Kosina

Re: [GIT PATCH] HID and USB HID update for 2.6.21-rc2

[Linus Torvalds]

On Wed, 28 Feb 2007, Jiri Kosina wrote:

> On Wed, 28 Feb 2007, Linus Torvalds wrote:
> 
> > There is no excuse for putting a large array in a header file and 
> > including it millions of times. Or even just twice. The point of a 
> > header file is to *declare* things, not to have big data structures in.
> 
> The point was that noone else than hid/hid-core.c would ever be going to 
> include this header with blacklist.

So:
 - clearly other people *do* include it.
 - if no-one else incldues it WHY HAVE IT SEPARATE!
 - if you want to have it separate for other reasons, YOU MAKE IT A .c 
   FILE, SINCE IT'S CLEARLY NOT A HEADERFILE!

In other words, there is *zero* excuse for that braindamage.

> But OK, I will leave it in there.

No. You need to realize just WHY it was wrong. Not just an "But OK".

Because if you don't see why I'm complaining, I can't pull from you. You 
can send me patches, but for me to pull a git patch from you, I need to 
know that you know what you're doing, and I need to be able to trust 
things *without* then having to go and check every individual change by 
hand.

If I have to check changes like this by hand, I want you to send patches 
to the mailing list, so that other people can help me filter it out. It 
really is that simple, and that fundamental. It's about code flow, and 
it's about the fact that there's no way I can look at every change and vet 
it for being sane.

I simply *cannot* do git-to-git merges with people I can't trust to have 
good enough judgement that I don't need to do the micromanagement and 
check everything myself. I can't afford to. I don't have the bandwidth, 
but I also simply don't have the interest in micro-managing.

So people I do git merges with need to show that they have good taste and 
skills, otherwise it needs to be done as open patches where *other* people 
with good taste and skills can help me.

		Linus

Re: [GIT PATCH] HID and USB HID update for 2.6.21-rc2

[Linus Torvalds]

On Wed, 28 Feb 2007, Linus Torvalds wrote:
> 
> In other words, there is *zero* excuse for that braindamage.

To be clear:

 - in header files, we put "common definitions":

	* #defines
	* data structure declarations
	* external function and data declarations
	* inline functions ("nicer but otherwise equivalent to a #define")

 - but we do *not* put

	* actual real code
	* actual real data

   because those go into C files.

Yes, yes, all rules have exceptions, and sometimes we have ugly header 
files. For an example of a pre-existing ugly header file that breaks these 
rules, just look at <asm-i386/bugs.h> for example. Yeah, it only gets 
included from one place, but it *still* shouldn't have code in it. It grew 
over time, and none of the individual events were ever really big enough 
for anybody to say "ok, we should clean this up and create a bugs.c file 
in arch/i386/kernel".

I'm sure there are other examples of the exceptions too. But I do not want 
to add *new* ugly stuff, and I certainly refuse to do it after we're 
already long past a merge window.

		Linus

Re: [GIT PATCH] HID and USB HID update for 2.6.21-rc2

[Jiri Kosina]

On Wed, 28 Feb 2007, Linus Torvalds wrote:

> > But OK, I will leave it in there.
> No. You need to realize just WHY it was wrong. Not just an "But OK".

Yep, I totally agree that with the usbhid.h thing I really had a bad day, 
it was braindamage without excuse, sorry.

I still think that creating a separate header file solely for purpose of 
having the large hid blacklist and all related defines separate from the 
actual implementation is needed. The pages and pages of blacklist just 
pollute the hid-core.c needlessly. 

Of course hid-core.c must be the only user of this header. But seems like 
this solution oposes your taste ("The point of a header file is to 
*declare* things, not to have big data structures in"), so I would 
probably not go this way.

Thanks,

-- 
Jiri Kosina

Re: [GIT PATCH] HID and USB HID update for 2.6.21-rc2

[Randy Dunlap]

On Wed, 28 Feb 2007 10:28:10 -0800 (PST) Linus Torvalds wrote:

> 
> 
> On Wed, 28 Feb 2007, Linus Torvalds wrote:
> > 
> > In other words, there is *zero* excuse for that braindamage.
> 
> To be clear:
> 
>  - in header files, we put "common definitions":
> 
> 	* #defines
> 	* data structure declarations
> 	* external function and data declarations
> 	* inline functions ("nicer but otherwise equivalent to a #define")
> 
>  - but we do *not* put
> 
> 	* actual real code
> 	* actual real data
> 
>    because those go into C files.
> 
> Yes, yes, all rules have exceptions, and sometimes we have ugly header 
> files. For an example of a pre-existing ugly header file that breaks these 
> rules, just look at <asm-i386/bugs.h> for example. Yeah, it only gets 
> included from one place, but it *still* shouldn't have code in it. It grew 
> over time, and none of the individual events were ever really big enough 
> for anybody to say "ok, we should clean this up and create a bugs.c file 
> in arch/i386/kernel".

It's on my TODO list... just not high priority.

> I'm sure there are other examples of the exceptions too. But I do not want 
> to add *new* ugly stuff, and I certainly refuse to do it after we're 
> already long past a merge window.


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [GIT PATCH] HID and USB HID update for 2.6.21-rc2

[Linus Torvalds]

On Wed, 28 Feb 2007, Jiri Kosina wrote:
> 
> I still think that creating a separate header file solely for purpose of 
> having the large hid blacklist and all related defines separate from the 
> actual implementation is needed. The pages and pages of blacklist just 
> pollute the hid-core.c needlessly. 

BUT IT IS NOT A HEADER FILE!

Dammit!

If it has real code or data in it, it's a .c file, and you don't #include 
it, you *compile* it and you *link* it.

			Linus