linux-kernel - Re: [PATCH] hid-multitouch: changes from the review process

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: <20110111131154.GC1961@polaris.bitmath.org>
Date:	Tue, 11 Jan 2011 14:11:54 +0100
From:	"Henrik Rydberg" <rydberg@...math.org>
To:	Benjamin Tissoires <benjamin.tissoires@...c.fr>
Cc:	Stephane Chatty <chatty@...c.fr>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Jiri Kosina <jkosina@...e.cz>, linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] hid-multitouch: changes from the review process

Hi Benjamin,

seems we are cloing in now, although there is still an outstanding
issue with the setting of curvalid, as replied in your previous
mail. Some comments on this patch, too.

>  struct mt_class mt_classes[] = {
> -	{ 0, 0, 0, 10 },                             /* MT_CLS_DEFAULT */
> -	{ MT_QUIRK_SLOT_IS_CONTACTID, 0, 0, 2 },     /* MT_CLS_DUAL1 */
> -	{ MT_QUIRK_SLOT_IS_CONTACTNUMBER, 0, 0, 10 },    /* MT_CLS_DUAL2 */
> -	{ MT_QUIRK_CYPRESS | MT_QUIRK_NOT_SEEN_MEANS_UP, 0, 0, 10 }, /* MT_CLS_CYPRESS */
> +	{ .name = MT_CLS_DEFAULT,
> +		.quirks = 0,

Please do not zero-initialize.

> +		.sn_move = 0,
> +		.sn_pressure = 0,
> +		.maxcontacts = 10 },
> +	{ .name = MT_CLS_DUAL1,
> +		.quirks = MT_QUIRK_SLOT_IS_CONTACTID,
> +		.sn_move = 0,
> +		.sn_pressure = 0,
> +		.maxcontacts = 2 },
> +	{ .name = MT_CLS_DUAL2,
> +		.quirks = MT_QUIRK_SLOT_IS_CONTACTNUMBER,
> +		.sn_move = 0,
> +		.sn_pressure = 0,
> +		.maxcontacts = 2 },
> +	{ .name = MT_CLS_CYPRESS,
> +		.quirks = MT_QUIRK_CYPRESS,
> +		.sn_move = 0,
> +		.sn_pressure = 0,
> +		.maxcontacts = 10 },
> +
> +	{ }
>  };

So no device is marked as NOT_SEEN_MEANS_UP, although allegedly some devices should...

> @@ -282,11 +297,10 @@ static void mt_emit_event(struct mt_device *td, struct input_dev *input)
>  
>  	for (i = 0; i < td->mtclass->maxcontacts; ++i) {
>  		struct mt_slot *s = &(td->slots[i]);
> -		if ((td->mtclass->quirks & MT_QUIRK_NOT_SEEN_MEANS_UP) &&
> -			!s->seen_in_this_frame) {
> +		if (!s->seen_in_this_frame) {
>  			/*
> -			 * this slot does not contain useful data,
> -			 * notify its closure
> +			 * FixMe: use MT_QUIRK_NOT_SEEN_MEANS_UP here.
> +			 * This requires to change the curvalid logic.
>  			 */
>  			s->touch_state = false;
>  		}

If we were to push this broken behavior, simply to avoid changing the
currently tested code, we would only push the problem of testing onto
the next cycle, with a larger risk of introducing regressions. I
really think we need to sort this out now.

> @@ -392,9 +407,16 @@ static void mt_set_input_mode(struct hid_device *hdev)
>  
>  static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  {
> -	int ret;
> +	int ret, i;
>  	struct mt_device *td;
> -	struct mt_class *mtclass = mt_classes + id->driver_data;
> +	struct mt_class *mtclass = mt_classes; /* MT_CLS_DEFAULT */
> +
> +	for (i = 0; mt_classes[i].name ; i++) {
> +		if (id->driver_data == mt_classes[i].name) {
> +			mtclass = &(mt_classes[i]);
> +			break;
> +		}
> +	}

Nice.

Thanks,
Henrik
--
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/