linux-kernel - Re: [PATCH 2/2] Add a device driver for the Apple Magic Mouse.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87d40cestn.fsf@troilus.org>
Date:	Wed, 10 Feb 2010 15:31:16 -0500
From:	Michael Poole <mdpoole@...ilus.org>
To:	Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:	Jiri Kosina <jkosina@...e.cz>, linux-input@...r.kernel.org,
	Marcel Holtmann <marcel@...tmann.org>,
	linux-bluetooth@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] Add a device driver for the Apple Magic Mouse.

I'll post a patch for these based on Jiri's for-next tree this evening.

Dmitry Torokhov writes:

> Hi Michael,
>
> On Tue, Feb 09, 2010 at 08:13:08AM -0500, Michael Poole wrote:
>> +
>> +static bool emulate_3button = 1;
>
> If it is a bool then values should be true/false.

Thanks.

>> +module_param(emulate_3button, bool, 0644);
>> +MODULE_PARM_DESC(emulate_3button, "Emulate a middle button");
>> +
>> +static int middle_button_start = -350;
>> +static int middle_button_stop = +350;
>> +
>> +static bool emulate_scroll_wheel = 1;
>> +module_param(emulate_scroll_wheel, bool, 0644);
>> +MODULE_PARM_DESC(emulate_scroll_wheel, "Emulate a scroll wheel");
>> +
>> +static bool report_touches = 1;
>> +module_param(report_touches, bool, 0644);
>> +MODULE_PARM_DESC(report_touches, "Emit touch records (otherwise, only use them for emulation)");
>> +
>> +static bool report_undeciphered = 0;
>
> No need to initialize statics to 0/false.

Okay, I'll remote that.

[snip]
>> +        int last_state = test_bit(BTN_LEFT, msc->input->key) << 0 |
>> +                test_bit(BTN_RIGHT, msc->input->key) << 1 |
>> +                test_bit(BTN_MIDDLE, msc->input->key) << 2;
>
> Indented with spaces, we prefer tabs.

Whoops -- one of my machines must be misconfigured.  I'll fix that.

[snip]
>> +		if (report_undeciphered) {
>> +			input_event(input, EV_MSC, MSC_RAW, tdata[7]);
>> +		}
>
> No need for braces for single statement.

Okay.

[snip]
>> +	if (emulate_scroll_wheel)
>> +		set_bit(REL_WHEEL, input->relbit);
>
> I'd use __set_bit() instead, no need to lock the bus.

Okay.  I wasn't aware of that semantic difference.

>> +
>> +	if (report_touches) {
>> +		set_bit(EV_ABS, input->evbit);
>> +
>> +		set_bit(ABS_MT_TRACKING_ID, input->absbit);
>> +		input->absmin[ABS_MT_TRACKING_ID] = 0;
>> +		input->absmax[ABS_MT_TRACKING_ID] = 15;
>> +		input->absfuzz[ABS_MT_TRACKING_ID] = 0;
>
> input_set_abs_params() is a bit more compact.

Thanks -- more idiomatic is always handy.

[snip]
>> +	ret = hdev->hid_output_raw_report(hdev, feature_1, sizeof(feature_1),
>> +			HID_FEATURE_REPORT);
>
> This will cause transmission of on-stack data through USB which it does
> not like.

Current Magic Mouse devices are Bluetooth-only.  Is this an issue for
that stack as well?  (hid-sony.c and hid-wacom.c also use
hid_output_raw_report() with on-stack buffers for Bluetooth devices --
if they need it too, I can write a patch for those.  I have a PS3
controller that I can test with, but don't have a Wacom tablet.)  Would
making those buffers static put them in an appropriate section, or does
the memory need to be allocated from a heap?

>> +	if (ret != sizeof(feature_1)) {
>> +		dev_err(&hdev->dev, "unable to request touch data (1:%d)\n",
>> +				ret);
>> +		goto err_free;
>> +	}
>> +	ret = hdev->hid_output_raw_report(hdev, feature_2,
>> +			sizeof(feature_2), HID_FEATURE_REPORT);
>
> Same here.
>
>> +	if (ret != sizeof(feature_2)) {
>> +		dev_err(&hdev->dev, "unable to request touch data (2:%d)\n",
>> +				ret);
>> +		goto err_free;
>> +	}
>> +
>> +	input = input_allocate_device();
>> +	if (!input) {
>> +		dev_err(&hdev->dev, "can't alloc input device\n");
>> +		ret = -ENOMEM;
>> +		goto err_free;
>> +	}
>> +	magicmouse_setup_input(input, hdev);
>> +
>> +	ret = input_register_device(input);
>> +	if (ret) {
>> +		dev_err(&hdev->dev, "input device registration failed\n");
>> +		goto err_both;
>> +	}
>> +	msc->input = input;
>> +
>> +	return 0;
>> + err_both:
>> +	input_free_device(input);
>> + err_free:
>> +	kfree(msc);
>> +	return ret;
>
> hid_hw_stop() is missing in error path. Also see question about freeing
> report below.

Thanks, I'll add the hid_hw_stop().

>> +}
>> +
>> +static void magicmouse_remove(struct hid_device *hdev)
>> +{
>
> Do we need to unregister report (I am not that familiar with HID, who is
> responsible for cleaning report lists?)?

hid-core.c's hid_device_release() frees all the reports that registered
with hid_register_report().

Michael Poole
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/