linux-kernel - Re: [PATCH 2/2] hid-ntrig: calibration

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: <20110321171550.GA4602@polaris.bitmath.org>
Date:	Mon, 21 Mar 2011 18:15:50 +0100
From:	"Henrik Rydberg" <rydberg@...omail.se>
To:	Rafi Rubin <rafi@...s.upenn.edu>
Cc:	jkosina@...e.cz, linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org, micki@...rig.com, chatty@...c.fr,
	dmitry.torokhov@...il.com
Subject: Re: [PATCH 2/2] hid-ntrig: calibration

Hi Rafi,

On Fri, Mar 11, 2011 at 02:37:52AM -0500, Rafi Rubin wrote:
> Adding a function to tell the device to run its calibration routine.
> A number written to the sysfs specifies the duration of the calibration
> in milliseconds
> 
> Signed-off-by: Rafi Rubin <rafi@...s.upenn.edu>
> ---

This is great functionality, and from what it seems, it
works. However, the poking at the usb layer makes me wonder if there
is another way... Jiri? Dmitry? Awaiting answers from someone more
knowledgeable, please find some comments inline.

> diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
> index 24ab6a5..ddf2c76 100644
> --- a/drivers/hid/hid-ntrig.c
> +++ b/drivers/hid/hid-ntrig.c
> @@ -490,6 +490,53 @@ static ssize_t store_mode(struct device *dev,
>  }
>  static DEVICE_ATTR(mode, S_IWUSR | S_IRUGO, show_mode, store_mode);
>  
> +static ssize_t ntrig_calibrate(struct device *dev,
> +			       struct device_attribute *attr,
> +			       const char *buf, size_t count)
> +{
> +	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> +	struct usb_device *usb_dev = hid_to_usb_dev(hdev);
> +	struct usbhid_device *usbhid = hdev->driver_data;
> +	int ret;
> +	unsigned long t;
> +	unsigned char *data;
> +
> +	if (strict_strtoul(buf, 0, &t))
> +		return -EINVAL;
> +
> +	data = kmalloc(4, GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	spin_lock(&usbhid->lock);
> +	set_bit(HID_DISCONNECTED, &usbhid->iofl);
> +	spin_unlock(&usbhid->lock);

Perhaps an addition to the internal HID api instead of the above?

> +
> +	ret = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
> +			      USB_REQ_CLEAR_FEATURE, USB_TYPE_CLASS
> +			      | USB_RECIP_INTERFACE | USB_DIR_IN,
> +			      0x30b, 1, data, 4, USB_CTRL_GET_TIMEOUT);
> +	if (ret < 0)
> +		goto fail;

How about launching restoration work here, instead of waiting?

> +
> +	msleep(t);
> +
> +	ret = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
> +			      USB_REQ_CLEAR_FEATURE, USB_TYPE_CLASS
> +			      | USB_RECIP_INTERFACE | USB_DIR_IN,
> +			      0x311, 1, data, 4, USB_CTRL_GET_TIMEOUT);
> +
> +fail:
> +	kfree(data);
> +	spin_lock(&usbhid->lock);
> +	clear_bit(HID_DISCONNECTED, &usbhid->iofl);
> +	spin_unlock(&usbhid->lock);
> +	schedule_work(&usbhid->reset_work);
> +
> +	return (ret < 0) ? ret : count;
> +}
> +static DEVICE_ATTR(calibrate, S_IWUSR, NULL, ntrig_calibrate);
> +
>  static struct attribute *sysfs_attrs[] = {
>  	&dev_attr_sensor_physical_width.attr,
>  	&dev_attr_sensor_physical_height.attr,
> @@ -502,6 +549,7 @@ static struct attribute *sysfs_attrs[] = {
>  	&dev_attr_activation_height.attr,
>  	&dev_attr_deactivate_slack.attr,
>  	&dev_attr_mode.attr,
> +	&dev_attr_calibrate.attr,

Maybe one could actually run the calibration at module load, or even
at start/stop? A few seconds of unresponsiveness might be ok at that
time.

>  	NULL
>  };

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/