Messages in this thread | | | From | Linus Torvalds <> | Date | Fri, 5 May 2017 09:33:29 -0700 | Subject | Re: [GIT PULL] Char/Misc driver patches for 4.12-rc1 |
| |
On Fri, May 5, 2017 at 9:00 AM, James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > > I'm not going to defend the earlier coding, but you've lost the real > device_add() calls in the merge, meaning the tpm devices don't actually > get made visible at all. I suspect assuming device_add() is done by > cdev_device_add() because of the name is going to be our next anti > -pattern, so you're at least ahead of the game ...
Don't be silly. That's *exactly* what cdev_device_add() does.
That's the whole and only point of the whole function: it does *both* the cdev_add() _and_ the device_add(), and it handles errors sanely and unwinds things.
So removing the device_add() is what the code should do. It's also exactly what the commit I pointed you at does (the one that clashed with your addition of the new ":[c]devs" cases. Let me repeat:
8dbbf5825181 tpm-chip: utilize new cdev_device_add helper function
So your patch looks like complete garbage, and I think you're confused about what cdev_device_add() is.
I think you're confusing it with the nasty old "cdev_add()" function that indeed didn't do a "device_add()", but that's exactly why "cdev_device_add()" as added, so that you don't have to have that nasty pattern of having both.
That said, I do think that my merge is missing a "cdev_device_del()" in the [c]dev if the second cdev_device_add() (on [c]devs) fails.
Although I also considered just saying "maybe if the second device register fails, we still want to just succeed, and at least leave the first one available". Because why shouldn't you let people access at least the old interface even if the new interface couldn't be set up?
So that's part of why I'd like people to look at that resolutin. But more importantly it should be tested.
Linus
| |