On Mon, Oct 29, 2007 at 10:49:03AM -0400, Jeff Garzik wrote:
> Dmitry Torokhov wrote:
> > On 10/29/07, Jiri Kosina <[EMAIL PROTECTED]> wrote:
> >> On Mon, 29 Oct 2007, Dirk Hohndel wrote:
> >>
> >>> [INPUT] hidinput_connect incorrectly ignored return value from
> >>> input_register_device
> >>> Signed-off-by: Dirk Hohndel <[EMAIL PROTECTED]>
> >> Will apply
> > Please don't - the fix is completely broken for multi-input devices -
> > if 2nd device fails to register we bail out of hidinput_connect and
> > thus never set HID_CLAIMED_INPUT bit. So when we disconnect device we
> > never call hidinput_disconnect and who knows what will happen after
> > that.
> > hidinput_connect() should properly unwind already registered devices
> > after failure.
>
> Then the existing code to handle hidinput and input_dev allocation failure
> probably also wants fixing... Dirk's patch was largely following the same
> logic.
I was wondering about that. If I didn't get lost in the structures again, I
think it isn't too hard to simply call out directly to hidinput_disconnect to
do the cleanup / unwind; the &hid->inputs should contain those devices that
have successfully been registered before we failed.
Actually, the more I look at the code that bails when it runs out of memory,
the more I wonder about that.
hidinput = kzalloc(sizeof(*hidinput), GFP_KERNEL);
input_dev = input_allocate_device();
if (!hidinput || !input_dev) {
kfree(hidinput);
input_free_device(input_dev);
This either passes a NULL pointer to kfree or to input_free_device. That's
not nice.
Would something like this work?
[PATCH] hidinput_connect ignores retval from input_register_device
Signed-off-by: Dirk Hohndel <[EMAIL PROTECTED]>
---
drivers/hid/hid-input.c | 24 ++++++++++++++++++------
1 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index dd332f2..5bff5cc 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1149,10 +1149,12 @@ int hidinput_connect(struct hid_device *hid)
hidinput = kzalloc(sizeof(*hidinput),
GFP_KERNEL);
input_dev = input_allocate_device();
if (!hidinput || !input_dev) {
- kfree(hidinput);
- input_free_device(input_dev);
+ if (hidinput)
+ kfree(hidinput);
+ if (input_dev)
+ input_free_device(input_dev);
err_hid("Out of memory during hid input
probe");
- return -1;
+ goto out_unwind;
}
input_set_drvdata(input_dev, hid);
@@ -1186,15 +1188,25 @@ int hidinput_connect(struct hid_device *hid)
* UGCI) cram a lot of unrelated inputs into the
* same interface. */
hidinput->report = report;
- input_register_device(hidinput->input);
+ if (input_register_device(hidinput->input))
+ goto out_cleanup;
hidinput = NULL;
}
}
- if (hidinput)
- input_register_device(hidinput->input);
+ if (hidinput && input_register_device(hidinput->input))
+ goto out_cleanup;
return 0;
+
+out_cleanup:
+ input_free_device(hidinput->input);
+ kfree(hidinput);
+out_unwind:
+ /* unwind the ones we already registered */
+ hidinput_disconnect(hid);
+
+ return -1;
}
EXPORT_SYMBOL_GPL(hidinput_connect);
--
gitgui.0.8.4.g8d863
