On Mon, Oct 29, 2007 at 10:49:03AM -0400, Jeff Garzik wrote:

 Dmitry Torokhov wrote:

quoted

On 10/29/07, Jiri Kosina [off-list ref] wrote:

quoted

On Mon, 29 Oct 2007, Dirk Hohndel wrote:

quoted

[INPUT] hidinput_connect incorrectly ignored return value from
input_register_device
Signed-off-by: Dirk Hohndel <redacted>

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

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