[PATCH v4 0/2] Get and Set Feature Reports on HIDRAW (USB and Bluetooth)

[PATCH v4 0/2] Get and Set Feature Reports on HIDRAW (USB and Bluetooth)

[Alan Ott]

This is version 4. Built against 2.6.35+ revision 320b2b8de12698 .

Alan Ott (2):
  HID: Add Support for Setting and Getting Feature Reports from hidraw
  Bluetooth: hidp: Add support for hidraw  HIDIOCGFEATURE  and
    HIDIOCSFEATURE

 drivers/hid/hidraw.c          |  105 ++++++++++++++++++++++++++++++++++++--
 drivers/hid/usbhid/hid-core.c |   37 +++++++++++++-
 include/linux/hid.h           |    3 +
 include/linux/hidraw.h        |    3 +
 net/bluetooth/hidp/core.c     |  114 +++++++++++++++++++++++++++++++++++++++--
 net/bluetooth/hidp/hidp.h     |    8 +++
 6 files changed, 260 insertions(+), 10 deletions(-)

[PATCH v4 1/2] HID: Add Support for Setting and Getting Feature Reports from hidraw

[Alan Ott]

Per the HID Specification, Feature reports must be sent and received on
the Configuration endpoint (EP 0) through the Set_Report/Get_Report
interfaces.  This patch adds two ioctls to hidraw to set and get feature
reports to and from the device.  Modifications were made to hidraw and
usbhid.

New hidraw ioctls:
  HIDIOCSFEATURE - Perform a Set_Report transfer of a Feature report.
  HIDIOCGFEATURE - Perform a Get_Report transfer of a Feature report.

Signed-off-by: Alan Ott <alan@signal11.us>
---
 drivers/hid/hidraw.c          |  105 +++++++++++++++++++++++++++++++++++++++--
 drivers/hid/usbhid/hid-core.c |   37 ++++++++++++++-
 include/linux/hid.h           |    3 +
 include/linux/hidraw.h        |    3 +
 4 files changed, 142 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index 47d70c5..7df1310 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -101,14 +101,14 @@ out:
 }
 
 /* the first byte is expected to be a report number */
-static ssize_t hidraw_write(struct file *file, const char __user *buffer, size_t count, loff_t *ppos)
+/* This function is to be called with the minors_lock mutex held */
+static ssize_t hidraw_send_report(struct file *file, const char __user *buffer, size_t count, unsigned char report_type)
 {
 	unsigned int minor = iminor(file->f_path.dentry->d_inode);
 	struct hid_device *dev;
 	__u8 *buf;
 	int ret = 0;
 
-	mutex_lock(&minors_lock);
 	dev = hidraw_table[minor]->hid;
 
 	if (!dev->hid_output_raw_report) {
@@ -141,14 +141,92 @@ static ssize_t hidraw_write(struct file *file, const char __user *buffer, size_t
 		goto out_free;
 	}
 
-	ret = dev->hid_output_raw_report(dev, buf, count, HID_OUTPUT_REPORT);
+	ret = dev->hid_output_raw_report(dev, buf, count, report_type);
 out_free:
 	kfree(buf);
 out:
+	return ret;
+}
+
+/* the first byte is expected to be a report number */
+static ssize_t hidraw_write(struct file *file, const char __user *buffer, size_t count, loff_t *ppos)
+{
+	ssize_t ret;
+	mutex_lock(&minors_lock);
+	ret = hidraw_send_report(file, buffer, count, HID_OUTPUT_REPORT);
 	mutex_unlock(&minors_lock);
 	return ret;
 }
 
+
+/* This function performs a Get_Report transfer over the control endpoint
+   per section 7.2.1 of the HID specification, version 1.1.  The first byte
+   of buffer is the report number to request, or 0x0 if the defice does not
+   use numbered reports. The report_type parameter can be HID_FEATURE_REPORT
+   or HID_INPUT_REPORT.  This function is to be called with the minors_lock
+   mutex held.  */
+static ssize_t hidraw_get_report(struct file *file, char __user *buffer, size_t count, unsigned char report_type)
+{
+	unsigned int minor = iminor(file->f_path.dentry->d_inode);
+	struct hid_device *dev;
+	__u8 *buf;
+	int ret = 0, len;
+	unsigned char report_number;
+
+	dev = hidraw_table[minor]->hid;
+
+	if (!dev->hid_get_raw_report) {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	if (count > HID_MAX_BUFFER_SIZE) {
+		printk(KERN_WARNING "hidraw: pid %d passed too large report\n",
+				task_pid_nr(current));
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (count < 2) {
+		printk(KERN_WARNING "hidraw: pid %d passed too short report\n",
+				task_pid_nr(current));
+		ret = -EINVAL;
+		goto out;
+	}
+
+	buf = kmalloc(count * sizeof(__u8), GFP_KERNEL);
+	if (!buf) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	/* Read the first byte from the user. This is the report number,
+	   which is passed to dev->hid_get_raw_report(). */
+	if (copy_from_user(&report_number, buffer, 1)) {
+		ret = -EFAULT;
+		goto out_free;
+	}
+
+	ret = dev->hid_get_raw_report(dev, report_number, buf, count, report_type);
+
+	if (ret < 0)
+		goto out_free;
+
+	len = (ret < count) ? ret : count;
+
+	if (copy_to_user(buffer, buf, len)) {
+		ret = -EFAULT;
+		goto out_free;
+	}
+
+	ret = len;
+
+out_free:
+	kfree(buf);
+out:
+	return ret;
+}
+
 static unsigned int hidraw_poll(struct file *file, poll_table *wait)
 {
 	struct hidraw_list *list = file->private_data;
@@ -281,7 +359,24 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,
 		default:
 			{
 				struct hid_device *hid = dev->hid;
-				if (_IOC_TYPE(cmd) != 'H' || _IOC_DIR(cmd) != _IOC_READ) {
+				if (_IOC_TYPE(cmd) != 'H') {
+					ret = -EINVAL;
+					break;
+				}
+
+				if (_IOC_NR(cmd) == _IOC_NR(HIDIOCSFEATURE(0))) {
+					int len = _IOC_SIZE(cmd);
+					ret = hidraw_send_report(file, user_arg, len, HID_FEATURE_REPORT);
+					break;
+				}
+				if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGFEATURE(0))) {
+					int len = _IOC_SIZE(cmd);
+					ret = hidraw_get_report(file, user_arg, len, HID_FEATURE_REPORT);
+					break;
+				}
+
+				/* Begin Read-only ioctls. */
+				if (_IOC_DIR(cmd) != _IOC_READ) {
 					ret = -EINVAL;
 					break;
 				}
@@ -313,7 +408,7 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,
 						-EFAULT : len;
 					break;
 				}
-		}
+			}
 
 		ret = -ENOTTY;
 	}
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index b729c02..93db724 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -798,6 +798,40 @@ static int hid_alloc_buffers(struct usb_device *dev, struct hid_device *hid)
 	return 0;
 }
 
+static int usbhid_get_raw_report(struct hid_device *hid,
+		unsigned char report_number, __u8 *buf, size_t count,
+		unsigned char report_type)
+{
+	struct usbhid_device *usbhid = hid->driver_data;
+	struct usb_device *dev = hid_to_usb_dev(hid);
+	struct usb_interface *intf = usbhid->intf;
+	struct usb_host_interface *interface = intf->cur_altsetting;
+	int skipped_report_id = 0;
+	int ret;
+
+	/* Byte 0 is the report number. Report data starts at byte 1.*/
+	buf[0] = report_number;
+	if (report_number == 0x0) {
+		/* Offset the return buffer by 1, so that the report ID
+		   will remain in byte 0. */
+		buf++;
+		count--;
+		skipped_report_id = 1;
+	}
+	ret = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
+		HID_REQ_GET_REPORT,
+		USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+		((report_type + 1) << 8) | report_number,
+		interface->desc.bInterfaceNumber, buf, count,
+		USB_CTRL_SET_TIMEOUT);
+
+	/* count also the report id */
+	if (ret > 0 && skipped_report_id)
+		ret++;
+
+	return ret;
+}
+
 static int usbhid_output_raw_report(struct hid_device *hid, __u8 *buf, size_t count,
 		unsigned char report_type)
 {
@@ -807,7 +841,7 @@ static int usbhid_output_raw_report(struct hid_device *hid, __u8 *buf, size_t co
 	struct usb_host_interface *interface = intf->cur_altsetting;
 	int ret;
 
-	if (usbhid->urbout) {
+	if (usbhid->urbout && report_type != HID_FEATURE_REPORT) {
 		int actual_length;
 		int skipped_report_id = 0;
 		if (buf[0] == 0x0) {
@@ -1149,6 +1183,7 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *
 
 	usb_set_intfdata(intf, hid);
 	hid->ll_driver = &usb_hid_driver;
+	hid->hid_get_raw_report = usbhid_get_raw_report;
 	hid->hid_output_raw_report = usbhid_output_raw_report;
 	hid->ff_init = hid_pidff_init;
 #ifdef CONFIG_USB_HIDDEV
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 42a0f1d..1b95b0b 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -503,6 +503,9 @@ struct hid_device {							/* device report descriptor */
 				  struct hid_usage *, __s32);
 	void (*hiddev_report_event) (struct hid_device *, struct hid_report *);
 
+	/* handler for raw input (Get_Report) data, used by hidraw */
+	int (*hid_get_raw_report) (struct hid_device *, unsigned char, __u8 *, size_t, unsigned char);
+
 	/* handler for raw output data, used by hidraw */
 	int (*hid_output_raw_report) (struct hid_device *, __u8 *, size_t, unsigned char);
 
diff --git a/include/linux/hidraw.h b/include/linux/hidraw.h
index dd8d692..4b88e69 100644
--- a/include/linux/hidraw.h
+++ b/include/linux/hidraw.h
@@ -35,6 +35,9 @@ struct hidraw_devinfo {
 #define HIDIOCGRAWINFO		_IOR('H', 0x03, struct hidraw_devinfo)
 #define HIDIOCGRAWNAME(len)     _IOC(_IOC_READ, 'H', 0x04, len)
 #define HIDIOCGRAWPHYS(len)     _IOC(_IOC_READ, 'H', 0x05, len)
+/* The first byte of SFEATURE and GFEATURE is the report number */
+#define HIDIOCSFEATURE(len)    _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x06, len)
+#define HIDIOCGFEATURE(len)    _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x07, len)
 
 #define HIDRAW_FIRST_MINOR 0
 #define HIDRAW_MAX_DEVICES 64
-- 
1.7.0.4

[PATCH v4 2/2] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE

[Alan Ott]

This patch adds support or getting and setting feature reports for bluetooth
HID devices from HIDRAW.

Signed-off-by: Alan Ott <alan@signal11.us>
---
 net/bluetooth/hidp/core.c |  114 +++++++++++++++++++++++++++++++++++++++++++--
 net/bluetooth/hidp/hidp.h |    8 +++
 2 files changed, 118 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index bfe641b..0e4880e 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -36,6 +36,7 @@
 #include <linux/file.h>
 #include <linux/init.h>
 #include <linux/wait.h>
+#include <linux/mutex.h>
 #include <net/sock.h>
 
 #include <linux/input.h>
@@ -313,6 +314,86 @@ static int hidp_send_report(struct hidp_session *session, struct hid_report *rep
 	return hidp_queue_report(session, buf, rsize);
 }
 
+static int hidp_get_raw_report(struct hid_device *hid,
+		unsigned char report_number,
+		unsigned char *data, size_t count,
+		unsigned char report_type)
+{
+	struct hidp_session *session = hid->driver_data;
+	struct sk_buff *skb;
+	size_t len;
+	int numbered_reports = hid->report_enum[report_type].numbered;
+
+	switch (report_type) {
+	case HID_FEATURE_REPORT:
+		report_type = HIDP_TRANS_GET_REPORT | HIDP_DATA_RTYPE_FEATURE;
+		break;
+	case HID_INPUT_REPORT:
+		report_type = HIDP_TRANS_GET_REPORT | HIDP_DATA_RTYPE_INPUT;
+		break;
+	case HID_OUTPUT_REPORT:
+		report_type = HIDP_TRANS_GET_REPORT | HIDP_DATA_RTYPE_OUPUT;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (mutex_lock_interruptible(&session->report_mutex))
+		return -ERESTARTSYS;
+
+	/* Set up our wait, and send the report request to the device. */
+	session->waiting_report_type = report_type & HIDP_DATA_RTYPE_MASK;
+	session->waiting_report_number = numbered_reports ? report_number : -1;
+	set_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
+	data[0] = report_number;
+	if (hidp_send_ctrl_message(hid->driver_data, report_type, data, 1))
+		goto err_eio;
+
+	/* Wait for the return of the report. The returned report
+	   gets put in session->report_return.  */
+	while (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags)) {
+		int res;
+
+		res = wait_event_interruptible_timeout(session->report_queue,
+			!test_bit(HIDP_WAITING_FOR_RETURN, &session->flags),
+			5*HZ);
+		if (res == 0) {
+			/* timeout */
+			goto err_eio;
+		}
+		if (res < 0) {
+			/* signal */
+			goto err_restartsys;
+		}
+	}
+
+	skb = session->report_return;
+	if (skb) {
+		len = skb->len < count ? skb->len : count;
+		memcpy(data, skb->data, len);
+
+		kfree_skb(skb);
+		session->report_return = NULL;
+	} else {
+		/* Device returned a HANDSHAKE, indicating  protocol error. */
+		len = -EIO;
+	}
+
+	clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
+	mutex_unlock(&session->report_mutex);
+
+	return len;
+
+err_restartsys:
+	clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
+	mutex_unlock(&session->report_mutex);
+	return -ERESTARTSYS;
+err_eio:
+	clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
+	mutex_unlock(&session->report_mutex);
+	return -EIO;
+}
+
 static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, size_t count,
 		unsigned char report_type)
 {
@@ -367,6 +448,10 @@ static void hidp_process_handshake(struct hidp_session *session,
 	case HIDP_HSHK_ERR_INVALID_REPORT_ID:
 	case HIDP_HSHK_ERR_UNSUPPORTED_REQUEST:
 	case HIDP_HSHK_ERR_INVALID_PARAMETER:
+		if (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags)) {
+			clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
+			wake_up_interruptible(&session->report_queue);
+		}
 		/* FIXME: Call into SET_ GET_ handlers here */
 		break;
 
@@ -403,9 +488,11 @@ static void hidp_process_hid_control(struct hidp_session *session,
 	}
 }
 
-static void hidp_process_data(struct hidp_session *session, struct sk_buff *skb,
+/* Returns true if the passed-in skb should be freed by the caller. */
+static int hidp_process_data(struct hidp_session *session, struct sk_buff *skb,
 				unsigned char param)
 {
+	int done_with_skb = 1;
 	BT_DBG("session %p skb %p len %d param 0x%02x", session, skb, skb->len, param);
 
 	switch (param) {
@@ -417,7 +504,6 @@ static void hidp_process_data(struct hidp_session *session, struct sk_buff *skb,
 
 		if (session->hid)
 			hid_input_report(session->hid, HID_INPUT_REPORT, skb->data, skb->len, 0);
-
 		break;
 
 	case HIDP_DATA_RTYPE_OTHER:
@@ -429,12 +515,27 @@ static void hidp_process_data(struct hidp_session *session, struct sk_buff *skb,
 		__hidp_send_ctrl_message(session,
 			HIDP_TRANS_HANDSHAKE | HIDP_HSHK_ERR_INVALID_PARAMETER, NULL, 0);
 	}
+
+	if (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags) &&
+				param == session->waiting_report_type) {
+		if (session->waiting_report_number < 0 ||
+		    session->waiting_report_number == skb->data[0]) {
+			/* hidp_get_raw_report() is waiting on this report. */
+			session->report_return = skb;
+			done_with_skb = 0;
+			clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
+			wake_up_interruptible(&session->report_queue);
+		}
+	}
+
+	return done_with_skb;
 }
 
 static void hidp_recv_ctrl_frame(struct hidp_session *session,
 					struct sk_buff *skb)
 {
 	unsigned char hdr, type, param;
+	int free_skb = 1;
 
 	BT_DBG("session %p skb %p len %d", session, skb, skb->len);
 
@@ -454,7 +555,7 @@ static void hidp_recv_ctrl_frame(struct hidp_session *session,
 		break;
 
 	case HIDP_TRANS_DATA:
-		hidp_process_data(session, skb, param);
+		free_skb = hidp_process_data(session, skb, param);
 		break;
 
 	default:
@@ -463,7 +564,8 @@ static void hidp_recv_ctrl_frame(struct hidp_session *session,
 		break;
 	}
 
-	kfree_skb(skb);
+	if (free_skb)
+		kfree_skb(skb);
 }
 
 static void hidp_recv_intr_frame(struct hidp_session *session,
@@ -797,6 +899,7 @@ static int hidp_setup_hid(struct hidp_session *session,
 	hid->dev.parent = hidp_get_device(session);
 	hid->ll_driver = &hidp_hid_driver;
 
+	hid->hid_get_raw_report = hidp_get_raw_report;
 	hid->hid_output_raw_report = hidp_output_raw_report;
 
 	err = hid_add_device(hid);
@@ -857,6 +960,9 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock,
 	skb_queue_head_init(&session->ctrl_transmit);
 	skb_queue_head_init(&session->intr_transmit);
 
+	mutex_init(&session->report_mutex);
+	init_waitqueue_head(&session->report_queue);
+
 	session->flags   = req->flags & (1 << HIDP_BLUETOOTH_VENDOR_ID);
 	session->idle_to = req->idle_to;
 
diff --git a/net/bluetooth/hidp/hidp.h b/net/bluetooth/hidp/hidp.h
index 8d934a1..00e71dd 100644
--- a/net/bluetooth/hidp/hidp.h
+++ b/net/bluetooth/hidp/hidp.h
@@ -80,6 +80,7 @@
 #define HIDP_VIRTUAL_CABLE_UNPLUG	0
 #define HIDP_BOOT_PROTOCOL_MODE		1
 #define HIDP_BLUETOOTH_VENDOR_ID	9
+#define	HIDP_WAITING_FOR_RETURN		10
 
 struct hidp_connadd_req {
 	int   ctrl_sock;	// Connected control socket
@@ -154,6 +155,13 @@ struct hidp_session {
 	struct sk_buff_head ctrl_transmit;
 	struct sk_buff_head intr_transmit;
 
+	/* Used in hidp_get_raw_report() */
+	int waiting_report_type; /* HIDP_DATA_RTYPE_* */
+	int waiting_report_number; /* -1 for not numbered */
+	struct mutex report_mutex;
+	struct sk_buff *report_return;
+	wait_queue_head_t report_queue;
+
 	/* Report descriptor */
 	__u8 *rd_data;
 	uint rd_size;
-- 
1.7.0.4

Re: [PATCH v4 0/2] Get and Set Feature Reports on HIDRAW (USB and Bluetooth)

[Jiri Kosina]

On Mon, 16 Aug 2010, Alan Ott wrote:

> This is version 4. Built against 2.6.35+ revision 320b2b8de12698 .
> 
> Alan Ott (2):
>   HID: Add Support for Setting and Getting Feature Reports from hidraw
>   Bluetooth: hidp: Add support for hidraw  HIDIOCGFEATURE  and
>     HIDIOCSFEATURE
> 
>  drivers/hid/hidraw.c          |  105 ++++++++++++++++++++++++++++++++++++--
>  drivers/hid/usbhid/hid-core.c |   37 +++++++++++++-
>  include/linux/hid.h           |    3 +
>  include/linux/hidraw.h        |    3 +
>  net/bluetooth/hidp/core.c     |  114 +++++++++++++++++++++++++++++++++++++++--
>  net/bluetooth/hidp/hidp.h     |    8 +++
>  6 files changed, 260 insertions(+), 10 deletions(-)

Marcel, as per our previous discussion -- what is your word on this? I'd 
be glad taking it once you Ack the bluetooth bits (which, as far as I 
understood from your last mail, don't have strong objections against any 
more).

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH v4 0/2] Get and Set Feature Reports on HIDRAW (USB and Bluetooth)

[Jiri Kosina]

On Mon, 23 Aug 2010, Jiri Kosina wrote:

> > This is version 4. Built against 2.6.35+ revision 320b2b8de12698 .
> > 
> > Alan Ott (2):
> >   HID: Add Support for Setting and Getting Feature Reports from hidraw
> >   Bluetooth: hidp: Add support for hidraw  HIDIOCGFEATURE  and
> >     HIDIOCSFEATURE
> > 
> >  drivers/hid/hidraw.c          |  105 ++++++++++++++++++++++++++++++++++++--
> >  drivers/hid/usbhid/hid-core.c |   37 +++++++++++++-
> >  include/linux/hid.h           |    3 +
> >  include/linux/hidraw.h        |    3 +
> >  net/bluetooth/hidp/core.c     |  114 +++++++++++++++++++++++++++++++++++++++--
> >  net/bluetooth/hidp/hidp.h     |    8 +++
> >  6 files changed, 260 insertions(+), 10 deletions(-)
> 
> Marcel, as per our previous discussion -- what is your word on this? I'd 
> be glad taking it once you Ack the bluetooth bits (which, as far as I 
> understood from your last mail, don't have strong objections against any 
> more).

... Marcel?

I'd really like not to miss 2.6.37 merge window with this.

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH v4 0/2] Get and Set Feature Reports on HIDRAW (USB and Bluetooth)

[Jiri Kosina]

On Thu, 2 Sep 2010, Jiri Kosina wrote:

> > > This is version 4. Built against 2.6.35+ revision 320b2b8de12698 .
> > > 
> > > Alan Ott (2):
> > >   HID: Add Support for Setting and Getting Feature Reports from hidraw
> > >   Bluetooth: hidp: Add support for hidraw  HIDIOCGFEATURE  and
> > >     HIDIOCSFEATURE
> > > 
> > >  drivers/hid/hidraw.c          |  105 ++++++++++++++++++++++++++++++++++++--
> > >  drivers/hid/usbhid/hid-core.c |   37 +++++++++++++-
> > >  include/linux/hid.h           |    3 +
> > >  include/linux/hidraw.h        |    3 +
> > >  net/bluetooth/hidp/core.c     |  114 +++++++++++++++++++++++++++++++++++++++--
> > >  net/bluetooth/hidp/hidp.h     |    8 +++
> > >  6 files changed, 260 insertions(+), 10 deletions(-)
> > 
> > Marcel, as per our previous discussion -- what is your word on this? I'd 
> > be glad taking it once you Ack the bluetooth bits (which, as far as I 
> > understood from your last mail, don't have strong objections against any 
> > more).
> 
> ... Marcel?
> 
> I'd really like not to miss 2.6.37 merge window with this.

Seemingly I have not enought powers to get statement from Marcel here 
these days/weeks.

Davem, would you perhaps be able to step in here?

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH v4 0/2] Get and Set Feature Reports on HIDRAW (USB and Bluetooth)

[Jiri Kosina]

On Wed, 22 Sep 2010, Jiri Kosina wrote:

> > > > This is version 4. Built against 2.6.35+ revision 320b2b8de12698 .
> > > > 
> > > > Alan Ott (2):
> > > >   HID: Add Support for Setting and Getting Feature Reports from hidraw
> > > >   Bluetooth: hidp: Add support for hidraw  HIDIOCGFEATURE  and
> > > >     HIDIOCSFEATURE
> > > > 
> > > >  drivers/hid/hidraw.c          |  105 ++++++++++++++++++++++++++++++++++++--
> > > >  drivers/hid/usbhid/hid-core.c |   37 +++++++++++++-
> > > >  include/linux/hid.h           |    3 +
> > > >  include/linux/hidraw.h        |    3 +
> > > >  net/bluetooth/hidp/core.c     |  114 +++++++++++++++++++++++++++++++++++++++--
> > > >  net/bluetooth/hidp/hidp.h     |    8 +++
> > > >  6 files changed, 260 insertions(+), 10 deletions(-)
> > > 
> > > Marcel, as per our previous discussion -- what is your word on this? I'd 
> > > be glad taking it once you Ack the bluetooth bits (which, as far as I 
> > > understood from your last mail, don't have strong objections against any 
> > > more).
> > 
> > ... Marcel?
> > 
> > I'd really like not to miss 2.6.37 merge window with this.
> 
> Seemingly I have not enought powers to get statement from Marcel here 
> these days/weeks.
> 
> Davem, would you perhaps be able to step in here?

Marcel, any word on this patchset by chance?

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH v4 0/2] Get and Set Feature Reports on HIDRAW (USB and Bluetooth)

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 1251 bytes --]

On Mon, 1 Nov 2010 15:23:34 -0400 (EDT)
Jiri Kosina <jkosina-AlSwsSmVLrQ@public.gmane.org> wrote:

> On Wed, 22 Sep 2010, Jiri Kosina wrote:
> 
> > > > > This is version 4. Built against 2.6.35+ revision 320b2b8de12698 .
> > > > > 
> > > > > Alan Ott (2):
> > > > >   HID: Add Support for Setting and Getting Feature Reports from hidraw
> > > > >   Bluetooth: hidp: Add support for hidraw  HIDIOCGFEATURE  and
> > > > >     HIDIOCSFEATURE
[...]
> > > ... Marcel?
> > > 
> > > I'd really like not to miss 2.6.37 merge window with this.
> > 
> > Seemingly I have not enought powers to get statement from Marcel here 
> > these days/weeks.
> > 
> > Davem, would you perhaps be able to step in here?
> 
> Marcel, any word on this patchset by chance?
> 

Hopefully Alan will manage to send a v5 sometime soon, Alan I don't want
to pressure you, just remember to CC Gustavo F. Padovan (see
MAINTAINERS) as he looks to be the most active bluetooth maintainer
these days.

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v4 0/2] Get and Set Feature Reports on HIDRAW (USB and Bluetooth)

[Ville Tervo]

Hi Alan,

On Mon, Aug 16, 2010 at 10:20:57PM +0200, ext Alan Ott wrote:
> This is version 4. Built against 2.6.35+ revision 320b2b8de12698 .


I gave a try to to this patch using your test tool [1] and very old BT
keyboard. I don't have anything else ATM to test with. Is there some BT hid
devices which support setting and getting features?

Shouldn't HIDIOCSFEATURE's bt version have similar wait as HIDIOCGFEATURE to
get report status back from the device? or is there even any status coming back
in successful case? Sorry I'm a newbie with HID and trying to understand how
this should work.

Now it just returns num of send bytes even if the remote device returned some
error. Which one is the expected behavior?

Other problem is that Get report is getting now handshake from set report.


2010-09-23 17:55:46.680612 < ACL data: handle 38 flags 0x02 dlen 9
    L2CAP(d): cid 0x008b len 5 [psm 17]
      HIDP: Set report: Feature report
      0000: 09 ff ff ff                                       ....
2010-09-23 17:55:46.680653 < ACL data: handle 38 flags 0x02 dlen 6
    L2CAP(d): cid 0x008b len 2 [psm 17]
      HIDP: Get report: Feature report
      0000: 09                                                .
2010-09-23 17:55:46.697577 > HCI Event: Number of Completed Packets (0x13) plen 5
    handle 38 packets 1
2010-09-23 17:55:46.698579 > HCI Event: Number of Completed Packets (0x13) plen 5
    handle 38 packets 1
2010-09-23 17:55:46.776827 > ACL data: handle 38 flags 0x02 dlen 5
    L2CAP(d): cid 0x0040 len 1 [psm 17]
      HIDP: Handshake: Invalid parameter
2010-09-23 17:55:46.777069 < ACL data: handle 38 flags 0x02 dlen 7
    L2CAP(d): cid 0x008b len 3 [psm 17]
      HIDP: Data: Output report
      0000: 01 77                                             .w
2010-09-23 17:55:46.797577 > HCI Event: Number of Completed Packets (0x13) plen 5
    handle 38 packets 1
2010-09-23 17:55:46.816826 > ACL data: handle 38 flags 0x02 dlen 5
    L2CAP(d): cid 0x0040 len 1 [psm 17]
      HIDP: Handshake: Invalid parameter
2010-09-23 17:55:46.856828 > ACL data: handle 38 flags 0x02 dlen 5
    L2CAP(d): cid 0x0040 len 1 [psm 17]
      HIDP: Handshake: Unsupported request



[1] http://lkml.org/lkml/2010/6/17/414

-- 
Ville the HID newbie

Re: [PATCH v4 0/2] Get and Set Feature Reports on HIDRAW (USB and Bluetooth)

[Ping Cheng]

On Thu, Sep 23, 2010 at 9:25 AM, Ville Tervo <ville.tervo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> wrote:
> Hi Alan,
>
> On Mon, Aug 16, 2010 at 10:20:57PM +0200, ext Alan Ott wrote:
>> This is version 4. Built against 2.6.35+ revision 320b2b8de12698 .
>
> I gave a try to to this patch using your test tool [1] and very old BT
> keyboard. I don't have anything else ATM to test with. Is there some BT hid
> devices which support setting and getting features?

As far as I know Wacom BT devices (Graphire and Intuos4) need to get
and set features.

Przemo,

Do you have time to test the patchset with your Graphire BT and
provide your result here?

Thank you,

Ping

Re: [PATCH v4 0/2] Get and Set Feature Reports on HIDRAW (USB and Bluetooth)

[Przemo Firszt]

Dnia 2010-09-23, czw o godzinie 10:07 -0700, Ping Cheng pisze:
> On Thu, Sep 23, 2010 at 9:25 AM, Ville Tervo <ville.tervo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> wrote:
> > Hi Alan,
> >
> > On Mon, Aug 16, 2010 at 10:20:57PM +0200, ext Alan Ott wrote:
> >> This is version 4. Built against 2.6.35+ revision 320b2b8de12698 .
> >
> > I gave a try to to this patch using your test tool [1] and very old BT
> > keyboard. I don't have anything else ATM to test with. Is there some BT hid
> > devices which support setting and getting features?
> 
> As far as I know Wacom BT devices (Graphire and Intuos4) need to get
> and set features.
> 
> Przemo,
> 
> Do you have time to test the patchset with your Graphire BT and
> provide your result here?
Hi Ping,
Speed switching works as expected.

Tested on HP NC4200 with internal and external bluetooth modules:

Linux pldmachine 2.6.35-07788-g320b2b8-dirty #15 Thu Sep 23 19:55:03 IST
2010 i686 Intel(R)_Pentium(R)_M_processor_1.86GHz PLD Linux

ID 03f0:011d Hewlett-Packard Integrated Bluetooth Module
ID 0a12:0001 Cambridge Silicon Radio, Ltd Bluetooth Dongle (HCI mode)

and Wacom Pen Tablet CTE-630BT

Tested-by: Przemo Firszt <przemo-q9SP4D9nreWHXe+LvDLADg@public.gmane.org>
-- 
Regards,
Przemo Firszt <przemo-q9SP4D9nreWHXe+LvDLADg@public.gmane.org>

Re: [PATCH v4 0/2] Get and Set Feature Reports on HIDRAW (USB and Bluetooth)

[Alan Ott]

On 09/23/2010 12:25 PM, Ville Tervo wrote:
> Hi Alan,
>
> On Mon, Aug 16, 2010 at 10:20:57PM +0200, ext Alan Ott wrote:
>    
>> This is version 4. Built against 2.6.35+ revision 320b2b8de12698 .
>>      
>
> I gave a try to to this patch using your test tool [1] and very old BT
> keyboard. I don't have anything else ATM to test with. Is there some BT hid
> devices which support setting and getting features?
>    

A keyboard is the only BT device I've used which has feature reports.

> Shouldn't HIDIOCSFEATURE's bt version have similar wait as HIDIOCGFEATURE to
> get report status back from the device? or is there even any status coming back
> in successful case? Sorry I'm a newbie with HID and trying to understand how
> this should work.
>
> Now it just returns num of send bytes even if the remote device returned some
> error. Which one is the expected behavior?
>    

I made it function the same as the existing hidp_output_raw_report(). 
Nothing in net/bluetooth/hidp/core.c is acked at all. I'm not sure of 
the reasons.

Jiri, Marcel, any ideas?

> Other problem is that Get report is getting now handshake from set report.
>
>
>    

Yeah, that makes sense. I hadn't considered the set_report failing. In 
that case, I guess it means we _must_ wait for an ack for sent packets.

Anyone else agree or disagree?

Alan.


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/2] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE

[Ville Tervo]

Hi Alan,

One comment.

On Mon, Aug 16, 2010 at 10:20:59PM +0200, ext Alan Ott wrote:
> This patch adds support or getting and setting feature reports for bluetooth
> HID devices from HIDRAW.
> 
> Signed-off-by: Alan Ott <alan-yzvJWuRpmD1zbRFIqnYvSA@public.gmane.org>
> ---
>  net/bluetooth/hidp/core.c |  114 +++++++++++++++++++++++++++++++++++++++++++--
>  net/bluetooth/hidp/hidp.h |    8 +++
>  2 files changed, 118 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index bfe641b..0e4880e 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -36,6 +36,7 @@
>  #include <linux/file.h>
>  #include <linux/init.h>
>  #include <linux/wait.h>
> +#include <linux/mutex.h>
>  #include <net/sock.h>
>  
>  #include <linux/input.h>
> @@ -313,6 +314,86 @@ static int hidp_send_report(struct hidp_session *session, struct hid_report *rep
>  	return hidp_queue_report(session, buf, rsize);
>  }
>  
> +static int hidp_get_raw_report(struct hid_device *hid,
> +		unsigned char report_number,
> +		unsigned char *data, size_t count,
> +		unsigned char report_type)
> +{
> +	struct hidp_session *session = hid->driver_data;
> +	struct sk_buff *skb;
> +	size_t len;
> +	int numbered_reports = hid->report_enum[report_type].numbered;
> +
> +	switch (report_type) {
> +	case HID_FEATURE_REPORT:
> +		report_type = HIDP_TRANS_GET_REPORT | HIDP_DATA_RTYPE_FEATURE;
> +		break;
> +	case HID_INPUT_REPORT:
> +		report_type = HIDP_TRANS_GET_REPORT | HIDP_DATA_RTYPE_INPUT;
> +		break;
> +	case HID_OUTPUT_REPORT:
> +		report_type = HIDP_TRANS_GET_REPORT | HIDP_DATA_RTYPE_OUPUT;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (mutex_lock_interruptible(&session->report_mutex))
> +		return -ERESTARTSYS;
> +
> +	/* Set up our wait, and send the report request to the device. */
> +	session->waiting_report_type = report_type & HIDP_DATA_RTYPE_MASK;
> +	session->waiting_report_number = numbered_reports ? report_number : -1;
> +	set_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> +	data[0] = report_number;
> +	if (hidp_send_ctrl_message(hid->driver_data, report_type, data, 1))
> +		goto err_eio;
> +
> +	/* Wait for the return of the report. The returned report
> +	   gets put in session->report_return.  */
> +	while (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags)) {
> +		int res;
> +
> +		res = wait_event_interruptible_timeout(session->report_queue,
> +			!test_bit(HIDP_WAITING_FOR_RETURN, &session->flags),
> +			5*HZ);
> +		if (res == 0) {
> +			/* timeout */
> +			goto err_eio;
> +		}
> +		if (res < 0) {
> +			/* signal */
> +			goto err_restartsys;
> +		}
> +	}
> +
> +	skb = session->report_return;
> +	if (skb) {
> +		len = skb->len < count ? skb->len : count;
> +		memcpy(data, skb->data, len);
> +
> +		kfree_skb(skb);
> +		session->report_return = NULL;
> +	} else {
> +		/* Device returned a HANDSHAKE, indicating  protocol error. */
> +		len = -EIO;
> +	}
> +
> +	clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> +	mutex_unlock(&session->report_mutex);
> +
> +	return len;
> +
> +err_restartsys:
> +	clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> +	mutex_unlock(&session->report_mutex);
> +	return -ERESTARTSYS;
> +err_eio:
> +	clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> +	mutex_unlock(&session->report_mutex);
> +	return -EIO;
> +}

How about a variable called ret and using that to return len or errno? It
would eliminate code dublication.

-- 
Ville

Re: [PATCH v4 2/2] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE

[Alan Ott]

On Sep 23, 2010, at 7:51 AM, Ville Tervo wrote:

> Hi Alan,
>
> One comment.
>
> How about a variable called ret and using that to return len or  
> errno? It
> would eliminate code dublication.
>

Hi Ville,

Where specifically? In which function? I've gone through it a couple  
of times and failed to find return statements which are superfluous.  
Maybe I'm missing something fundamental?

Alan.

Re: [PATCH v4 2/2] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 1055 bytes --]

On Thu, 23 Sep 2010 10:16:43 -0400
Alan Ott <alan@signal11.us> wrote:

> 
> On Sep 23, 2010, at 7:51 AM, Ville Tervo wrote:
> 
> > Hi Alan,
> >
> > One comment.
> >
> > How about a variable called ret and using that to return len or  
> > errno? It
> > would eliminate code dublication.
> >
> 
> Hi Ville,
> 
> Where specifically? In which function? I've gone through it a couple  
> of times and failed to find return statements which are superfluous.  
> Maybe I'm missing something fundamental?
> 

I guess he's referring to the return paths (normal, err_restartsys,
err_eio) in hidp_get_raw_report(), they could be merged into a
generic "out:" path by parametrizing the return value.

I don't know if Alan prefers explicit error paths, for me it's OK either
way.

Regards,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v4 1/2] HID: Add Support for Setting and Getting Feature Reports from hidraw

[Antonio Ospite]

[-- Attachment #1.1: Type: text/plain, Size: 2624 bytes --]

On Mon, 16 Aug 2010 16:20:58 -0400
Alan Ott <alan@signal11.us> wrote:

> Per the HID Specification, Feature reports must be sent and received on
> the Configuration endpoint (EP 0) through the Set_Report/Get_Report
> interfaces.  This patch adds two ioctls to hidraw to set and get feature
> reports to and from the device.  Modifications were made to hidraw and
> usbhid.
> 
> New hidraw ioctls:
>   HIDIOCSFEATURE - Perform a Set_Report transfer of a Feature report.
>   HIDIOCGFEATURE - Perform a Get_Report transfer of a Feature report.
> 
> Signed-off-by: Alan Ott <alan@signal11.us>

Hi Alan, I am doing some stress testing on hidraw, if I have a loop with
HIDIOCGFEATURE on a given report and I disconnect the device while the
loop is running I get this:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000028
IP: [<ffffffffa02c66b4>] hidraw_ioctl+0xfc/0x32c [hid]

Full log attached along with the test program, the device is a Sony PS3
Controller (sixaxis).

If my objdump analysis is right, hidraw_ioctl+0xfc should be around line
361 in hidraw.c (with your patch applied):

struct hid_device *hid = dev->hid;

It looks like 'dev' (which is hidraw_table[minor]) can be NULL
sometimes, can't it?
This is not introduced by your changes tho.

Just as a side note, the bug does not show up if the userspace program
handles return values properly and exits as soon as it gets an error
from the HID layer, see the XXX comment in test_hidraw_feature.c.

This fixes it, if it looks ok I will resend the patch rebased on
mainline code:

diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index 7df1310..3c040c6 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -322,6 +322,10 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,

        mutex_lock(&minors_lock);
        dev = hidraw_table[minor];
+       if (!dev) {
+               ret = -ENODEV;
+               goto out;
+       }

        switch (cmd) {
                case HIDIOCGRDESCSIZE:
@@ -412,6 +416,7 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,

                ret = -ENOTTY;
        }
+out:
        mutex_unlock(&minors_lock);
        return ret;
 }


this change covers also the other uses of hidraw_table[minor] in
hidraw_send_report/hidraw_get_report.

Regards,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: dmesg_hidraw_feature_bug.log --]
[-- Type: application/octet-stream, Size: 3914 bytes --]

[  111.645836] usb 4-1: USB disconnect, address 2
[  111.669466] BUG: unable to handle kernel NULL pointer dereference at 0000000000000028
[  111.669484] IP: [<ffffffffa02c66b4>] hidraw_ioctl+0xfc/0x32c [hid]
[  111.669529] PGD 5a953067 PUD 6d741067 PMD 0 
[  111.669539] Oops: 0000 [#1] SMP 
[  111.669545] last sysfs file: /sys/devices/pci0000:00/0000:00:04.0/usb4/idVendor
[  111.669556] CPU 0 
[  111.669559] Modules linked in: hidp powernow_k8 mperf cpufreq_powersave cpufreq_conservative cpufreq_stats cpufreq_userspace lirc_serial(C) lirc_dev ipt_MASQUERADE bridge stp ppdev lp sco bnep rfcomm l2cap crc16 tun sit tunnel4 kvm_amd kvm binfmt_misc uinput fuse nfsd ip6table_raw ip6table_mangle ip6t_REJECT exportfs nfs ip6t_LOG lockd fscache nf_conntrack_ipv6 nfs_acl auth_rpcgss ip6table_filter sunrpc ip6_tables xt_tcpudp ipt_REJECT ipt_ULOG xt_limit xt_state xt_multiport iptable_filter iptable_nat nf_nat nf_conntrack_ipv4 nf_conntrack nf_defrag_ipv4 iptable_mangle iptable_raw ip_tables x_tables hwmon_vid loop snd_hda_codec_nvhdmi snd_hda_codec_via snd_hda_intel snd_hda_codec snd_hwdep snd_pcm_oss snd_mixer_oss snd_pcm snd_seq_midi snd_rawmidi nvidia(P) snd_seq_midi_event snd_seq parport_pc btusb snd_timer snd_seq_device asus_atk0110 joydev video snd bluetooth rfkill hid_sony output wmi parport edac_core i2c_nforce2 tpm_tis shpchp pci_hotplug k8temp edac_mce_amd pcspkr tpm tpm_bios evdev soundcore snd_page_alloc i2c_core button processor ext3 jbd mbcache dm_mod sg sr_mod sd_mod crc_t10dif cdrom ata_generic usbhid hid ahci libahci pata_amd ohci_hcd libata ehci_hcd scsi_mod usbcore thermal forcedeth nls_base thermal_sys floppy [last unloaded: scsi_wait_scan]
[  111.669736] 
[  111.669746] Pid: 2969, comm: test_hidraw_fea Tainted: P         C  2.6.36-rc5-ao2+ #1 M3N78-VM/System Product Name
[  111.669753] RIP: 0010:[<ffffffffa02c66b4>]  [<ffffffffa02c66b4>] hidraw_ioctl+0xfc/0x32c [hid]
[  111.669771] RSP: 0018:ffff880056831e78  EFLAGS: 00010206
[  111.669776] RAX: 0000000000000048 RBX: 00000000c02d4807 RCX: 00000000022c7e50
[  111.669783] RDX: 0000000100000000 RSI: ffffffff810377e0 RDI: ffffffffa02ceaa0
[  111.669789] RBP: 00000000022c7e50 R08: 00007f983f1cbec8 R09: 0000000000400aca
[  111.669795] R10: ffffffff8100769f R11: ffff880037a78dd8 R12: ffff88005a85c300
[  111.669801] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[  111.669808] FS:  00007f983f3cc700(0000) GS:ffff880001a00000(0000) knlGS:0000000000000000
[  111.669815] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  111.669821] CR2: 0000000000000028 CR3: 000000005aba9000 CR4: 00000000000006f0
[  111.669827] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  111.669833] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[  111.669840] Process test_hidraw_fea (pid: 2969, threadinfo ffff880056830000, task ffff88005ab93680)
[  111.669844] Stack:
[  111.669848]  00000000010aad14 ffff88005ab936b8 ffff88005a85c300 ffff880037fc6700
[  111.669857] <0> 00000000022c7e50 00000000022c7e50 0000000000000000 ffffffff810f64a5
[  111.669865] <0> ffff880001a14880 ffff88005ab93680 ffffffff81302776 0000000000000000
[  111.669875] Call Trace:
[  111.669898]  [<ffffffff810f64a5>] ? do_vfs_ioctl+0x4a2/0x4ef
[  111.669915]  [<ffffffff81302776>] ? schedule+0x573/0x5b7
[  111.669923]  [<ffffffff810f653d>] ? sys_ioctl+0x4b/0x72
[  111.669933]  [<ffffffff810ea002>] ? sys_write+0x5f/0x6b
[  111.669947]  [<ffffffff81008a02>] ? system_call_fastpath+0x16/0x1b
[  111.669952] Code: 2c 66 89 44 24 06 e8 1a c2 03 e1 48 89 e6 ba 08 00 00 00 48 89 ef e8 cc 79 ec e0 85 c0 0f 84 18 02 00 00 e9 01 02 00 00 0f b6 c7 <49> 8b 7d 28 83 f8 48 0f 85 fa 01 00 00 0f b6 c3 83 f8 06 75 24 
[  111.670011] RIP  [<ffffffffa02c66b4>] hidraw_ioctl+0xfc/0x32c [hid]
[  111.670025]  RSP <ffff880056831e78>
[  111.670029] CR2: 0000000000000028
[  111.670036] ---[ end trace 9c7530cfc7009202 ]---

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.3: test_hidraw_feature.c --]
[-- Type: text/x-csrc; name="test_hidraw_feature.c", Size: 1265 bytes --]

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdint.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/ioctl.h>

/*
#include <linux/hidraw.h>
*/
#include "/home/ao2/Proj/linux/linux-2.6/include/linux/hidraw.h"

void dump_hex_string(unsigned char *buf, unsigned int len)
{
	unsigned int i;

	for (i = 0; i < len; i++)
		printf("%02x%c", buf[i], i < len - 1 ? ' ' : 0);
}

void dump_feature_report(int fd, uint8_t report_number, unsigned int len)
{
	unsigned char *buf;
	int ret;

	buf = calloc(len, sizeof(*buf));

	buf[0] = report_number;

	ret = ioctl(fd, HIDIOCGFEATURE(len), buf);
	if (ret < 0) {
		fprintf(stderr, "report: 0x%02x ret: %d\n", report_number, ret);
		/* XXX: if I put exit(1) here the bug is masked */
		return;
	}

	dump_hex_string(buf, len);
	printf("\r");
	fflush(stdout);

	free(buf);
}

int main(int argc, char *argv[])
{
	int fd = -1;

	if (argc != 2) {
		fprintf(stderr, "usage: %s </dev/hidrawX>\n", argv[0]);
		exit(1);
	}

	fd = open(argv[1], O_RDWR);
	if (fd < 0) {
		perror("hidraw open");
		exit(1);
	}

	while (1)
		dump_feature_report(fd, 0x01, 45);
	printf("\n");

	close(fd);
	exit(0);
}

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v4 1/2] HID: Add Support for Setting and Getting Feature Reports from hidraw

[Jiri Kosina]

On Tue, 28 Sep 2010, Antonio Ospite wrote:

> Hi Alan, I am doing some stress testing on hidraw, if I have a loop with
> HIDIOCGFEATURE on a given report and I disconnect the device while the
> loop is running I get this:
> 
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000028
> IP: [<ffffffffa02c66b4>] hidraw_ioctl+0xfc/0x32c [hid]
> 
> Full log attached along with the test program, the device is a Sony PS3
> Controller (sixaxis).
> 
> If my objdump analysis is right, hidraw_ioctl+0xfc should be around line
> 361 in hidraw.c (with your patch applied):
> 
> struct hid_device *hid = dev->hid;
> 
> It looks like 'dev' (which is hidraw_table[minor]) can be NULL
> sometimes, can't it?
> This is not introduced by your changes tho.
> 
> Just as a side note, the bug does not show up if the userspace program
> handles return values properly and exits as soon as it gets an error
> from the HID layer, see the XXX comment in test_hidraw_feature.c.
> 
> This fixes it, if it looks ok I will resend the patch rebased on
> mainline code:
> 
> diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
> index 7df1310..3c040c6 100644
> --- a/drivers/hid/hidraw.c
> +++ b/drivers/hid/hidraw.c
> @@ -322,6 +322,10 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,
> 
>         mutex_lock(&minors_lock);
>         dev = hidraw_table[minor];
> +       if (!dev) {
> +               ret = -ENODEV;
> +               goto out;
> +       }
> 
>         switch (cmd) {
>                 case HIDIOCGRDESCSIZE:
> @@ -412,6 +416,7 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,
> 
>                 ret = -ENOTTY;
>         }
> +out:
>         mutex_unlock(&minors_lock);
>         return ret;
>  }

Yes, this patch makes sense even for current mainline code. Could you 
please resend it to me with Signed-off-by: and changelog text, so that I 
could apply it?

Thanks!

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH v4 1/2] HID: Add Support for Setting and Getting Feature Reports from hidraw

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 2275 bytes --]

On Mon, 16 Aug 2010 16:20:58 -0400
Alan Ott <alan@signal11.us> wrote:

> Per the HID Specification, Feature reports must be sent and received on
> the Configuration endpoint (EP 0) through the Set_Report/Get_Report
> interfaces.  This patch adds two ioctls to hidraw to set and get feature
> reports to and from the device.  Modifications were made to hidraw and
> usbhid.
> 
> New hidraw ioctls:
>   HIDIOCSFEATURE - Perform a Set_Report transfer of a Feature report.
>   HIDIOCGFEATURE - Perform a Get_Report transfer of a Feature report.
> 
> Signed-off-by: Alan Ott <alan@signal11.us>
> ---

Some more testing with sixaxis and a question:

HIDIOCGFEATURE on report 0xf5 returns:
<-- 01 00 00 03 c9 87 34 fd
          ^^ a hw addr from here

with the latest six octets being a hw addr; now if I wanted to change
this, *naively*, I'd send this report with HIDIOCSFEATURE:
--> f5 01 00 66 55 44 33 22 11
    ^^ report ID _added_ in front of the whole previous buffer

but then when I get the report again to see if the operation succeeded I
get:
<-- 01 00 00 66 55 44 33 22

I can make it work if I send:
--> f5 00 66 55 44 33 22 11
    ^^ report ID _replaced_ buf[0]

then I get the expected:
<-- 01 00 66 55 44 33 22 11

Am I missing something about the differences about getting a feature
report and setting it?

In the HIDIOCGFEATURE doc I read:
	...
	The report will be returned starting at
	the first byte of the buffer (ie: the report number is not
	returned).

Someone (/me whistling, with hands in pockets) could interpret this
like a buf++ semantic on a quick read, even if this sentence means
"only" that _actual_data_ is from buf[1]; but one question stands: what
is that value returned in buf[0]? Is this unspecified?

Maybe the doc could be made even more explicit about buf[0] to overcame
non native speakers deficiencies.
And while at it, can we get rid of the ambiguity of "first byte" for
"byte 1", the former can even mean buf[0], can't it?

Thanks,
  Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v4 1/2] HID: Add Support for Setting and Getting Feature Reports from hidraw

[Alan Ott]

On 09/29/2010 07:51 PM, Antonio Ospite wrote:
> On Mon, 16 Aug 2010 16:20:58 -0400
> Alan Ott<alan@signal11.us>  wrote:
>
>    
>> Per the HID Specification, Feature reports must be sent and received on
>> the Configuration endpoint (EP 0) through the Set_Report/Get_Report
>> interfaces.  This patch adds two ioctls to hidraw to set and get feature
>> reports to and from the device.  Modifications were made to hidraw and
>> usbhid.
>>
>> New hidraw ioctls:
>>    HIDIOCSFEATURE - Perform a Set_Report transfer of a Feature report.
>>    HIDIOCGFEATURE - Perform a Get_Report transfer of a Feature report.
>>
>> Signed-off-by: Alan Ott<alan@signal11.us>
>> ---
>>      
> Some more testing with sixaxis and a question:
>
> HIDIOCGFEATURE on report 0xf5 returns:
> <-- 01 00 00 03 c9 87 34 fd
>            ^^ a hw addr from here
>    

Maybe I'm confused. Shouldn't the first byte of that be 0xf5? The report 
number should be the first byte of the returned buffer.

There's not a chance you're running an old version of this patch is there?

> with the latest six octets being a hw addr; now if I wanted to change
> this, *naively*, I'd send this report with HIDIOCSFEATURE:
> -->  f5 01 00 66 55 44 33 22 11
>      ^^ report ID _added_ in front of the whole previous buffer
>
> but then when I get the report again to see if the operation succeeded I
> get:
> <-- 01 00 00 66 55 44 33 22
>
> I can make it work if I send:
> -->  f5 00 66 55 44 33 22 11
>      ^^ report ID _replaced_ buf[0]
>
> then I get the expected:
> <-- 01 00 66 55 44 33 22 11
>
> Am I missing something about the differences about getting a feature
> report and setting it?
>
>    

Do you have patches 29129a98e6f and c29771c2d8ce applied? They are in 
2.6.36-rc6 (and some previous, but I'm note certain how far back). Those 
patches have to do with _sending_ the right bytes (in particular the 
report number) on the control endpoint. You seem to be having the 
opposite problem though. Let me know to be sure.

> In the HIDIOCGFEATURE doc I read:
> 	...
> 	The report will be returned starting at
> 	the first byte of the buffer (ie: the report number is not
> 	returned).
>
> Someone (/me whistling, with hands in pockets) could interpret this
> like a buf++ semantic on a quick read, even if this sentence means
> "only" that _actual_data_ is from buf[1]; but one question stands: what
> is that value returned in buf[0]? Is this unspecified?
>
>    

So that was written when I had an incorrect understanding of the 
standard. It is wrong. I had originally thought that Report ID's were 
not sent on the control endpoint as part of the payload (because it's 
redundant, since it's in wValue). I was wrong, and made those two 
patches above to correct it.

> Maybe the doc could be made even more explicit about buf[0] to overcame
> non native speakers deficiencies.
> And while at it, can we get rid of the ambiguity of "first byte" for
> "byte 1", the former can even mean buf[0], can't it?
>    

Your English is fine, my understanding was wrong :) .

Alan.

Re: [PATCH v4 1/2] HID: Add Support for Setting and Getting Feature Reports from hidraw

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 5310 bytes --]

On Wed, 29 Sep 2010 23:37:41 -0400
Alan Ott <alan@signal11.us> wrote:

> On 09/29/2010 07:51 PM, Antonio Ospite wrote:
> > On Mon, 16 Aug 2010 16:20:58 -0400
> > Alan Ott<alan@signal11.us>  wrote:
> >
> >    
> >> Per the HID Specification, Feature reports must be sent and received on
> >> the Configuration endpoint (EP 0) through the Set_Report/Get_Report
> >> interfaces.  This patch adds two ioctls to hidraw to set and get feature
> >> reports to and from the device.  Modifications were made to hidraw and
> >> usbhid.
> >>
> >> New hidraw ioctls:
> >>    HIDIOCSFEATURE - Perform a Set_Report transfer of a Feature report.
> >>    HIDIOCGFEATURE - Perform a Get_Report transfer of a Feature report.
> >>
> >> Signed-off-by: Alan Ott<alan@signal11.us>
> >> ---
> >>      
> > Some more testing with sixaxis and a question:
> >
> > HIDIOCGFEATURE on report 0xf5 returns:
> > <-- 01 00 00 03 c9 87 34 fd
> >            ^^ a hw addr from here
> >    
> 
> Maybe I'm confused. Shouldn't the first byte of that be 0xf5? The report 
> number should be the first byte of the returned buffer.
>
> There's not a chance you're running an old version of this patch is there?
>

v4 it is. About the first byte, is it very clear now than when _setting_
a report the first byte is the report descriptor even in the payload,
but when we are _getting_ it from the usb device, could a non conformant
device be mangling it?
 
> > with the latest six octets being a hw addr; now if I wanted to change
> > this, *naively*, I'd send this report with HIDIOCSFEATURE:
> > -->  f5 01 00 66 55 44 33 22 11
> >      ^^ report ID _added_ in front of the whole previous buffer
> >
> > but then when I get the report again to see if the operation succeeded I
> > get:
> > <-- 01 00 00 66 55 44 33 22
> >
> > I can make it work if I send:
> > -->  f5 00 66 55 44 33 22 11
> >      ^^ report ID _replaced_ buf[0]
> >
> > then I get the expected:
> > <-- 01 00 66 55 44 33 22 11
> >
> > Am I missing something about the differences about getting a feature
> > report and setting it?
> >
> >    
> 
> Do you have patches 29129a98e6f and c29771c2d8ce applied? They are in 
> 2.6.36-rc6 (and some previous, but I'm note certain how far back). Those 
> patches have to do with _sending_ the right bytes (in particular the 
> report number) on the control endpoint. You seem to be having the 
> opposite problem though. Let me know to be sure.
>

I confirm I have those changes applied, in fact I am working on top of
2.6.36-rc6, and yes, my doubt is about the buffer returned from a GET
FEATURE REPORT.

> > In the HIDIOCGFEATURE doc I read:
> > 	...
> > 	The report will be returned starting at
> > 	the first byte of the buffer (ie: the report number is not
> > 	returned).
> >
> > Someone (/me whistling, with hands in pockets) could interpret this
> > like a buf++ semantic on a quick read, even if this sentence means
> > "only" that _actual_data_ is from buf[1]; but one question stands: what
> > is that value returned in buf[0]? Is this unspecified?
> >
> 
> So that was written when I had an incorrect understanding of the 
> standard. It is wrong. I had originally thought that Report ID's were 
> not sent on the control endpoint as part of the payload (because it's 
> redundant, since it's in wValue). I was wrong, and made those two 
> patches above to correct it.
>

Alan I am still talking about GET_FEATURE here, not SET, the question
here is what value the first byte (buf[0]) is expected to have in a
buffer returned from a GET. If it has to be the report ID then I
suspect my device is changing it behind our backs, maybe a failsafe
measure like the following could do?

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index df80532..83e1b48 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -875,6 +875,9 @@ static int usbhid_output_raw_report(struct hid_device *hid, __u8 *buf, size_t co
                        ((report_type + 1) << 8) | report_id,
                        interface->desc.bInterfaceNumber, buf, count,
                        USB_CTRL_SET_TIMEOUT);
+               /* restore report ID into the buffer, some non conformant usb
+                * device could have mangled it */
+               buf[0] = report_id;
                /* count also the report id, if this was a numbered report. */
                if (ret > 0 && skipped_report_id)
                        ret++;


But I haven't even taken a look at the HID spec about this, so take it
just for the sake of discussion. Anyone could run some tests with other
devices?

> > Maybe the doc could be made even more explicit about buf[0] to overcame
> > non native speakers deficiencies.
> > And while at it, can we get rid of the ambiguity of "first byte" for
> > "byte 1", the former can even mean buf[0], can't it?
> >    
> 
> Your English is fine, my understanding was wrong :) .
> 
> Alan.
> 

Regards,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v4 1/2] HID: Add Support for Setting and Getting Feature Reports from hidraw

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 1792 bytes --]

On Fri, 1 Oct 2010 00:03:09 +0200
Antonio Ospite <ospite@studenti.unina.it> wrote:


> 
> Alan I am still talking about GET_FEATURE here, not SET, the question
> here is what value the first byte (buf[0]) is expected to have in a
> buffer returned from a GET. If it has to be the report ID then I
> suspect my device is changing it behind our backs, maybe a failsafe
> measure like the following could do?
> 
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index df80532..83e1b48 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -875,6 +875,9 @@ static int usbhid_output_raw_report(struct hid_device *hid, __u8 *buf, size_t co

Actually I meant to change the code in usbhid_get_raw_report() instead,
like the following:

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index df80532..c6648d3 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -825,6 +825,11 @@ static int usbhid_get_raw_report(struct hid_device *hid,
                interface->desc.bInterfaceNumber, buf, count,
                USB_CTRL_SET_TIMEOUT);

+       /* restore report number into buf[0],
+        * some non conformant device might be mangling it
+        */
+       buf[0] = report_number;
+
        /* count also the report id */
        if (ret > 0 && skipped_report_id)
                ret++;

But again, I haven't checked yet what the specs say about byte 0 in the
returned buffer.

Regards,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v4 1/2] HID: Add Support for Setting and Getting Feature Reports from hidraw

[Alan Ott]

On 10/01/2010 08:00 PM, Antonio Ospite wrote:
> But again, I haven't checked yet what the specs say about byte 0 in the
> returned buffer.

The HID spec is not crystal clear. The best I found was the following:

    The bit length of an item's data is obtained through the Report
    descriptor
    (Report Size * Report Count). Item data is ordered just as items are
    ordered in
    the Report descriptor. If a Report ID tag was used in the Report
    descriptor, all
    reports include a single byte ID prefix. If the Report ID tag was
    not used, all
    values are returned in a single report and a prefix ID is not
    included in that report.

I take it to mean, yes. It says "all reports." It doesn't specifically 
say "all reports on the interrupt endpoint," and it doesn't say "all 
reports on the control endpoint only." It seems to me like it's 
redundant to send it on the control endpoint, which is why I didn't 
originally implement it that way, but Amit Nagal had a device which 
required the report ID in byte zero on the control endpoint, so and 
that's why it sends it that way now.

Alan.

Re: [PATCH v4 1/2] HID: Add Support for Setting and Getting Feature Reports from hidraw

[Alan Ott]

On 09/30/2010 06:03 PM, Antonio Ospite wrote:
>
> v4 it is. About the first byte, is it very clear now than when _setting_
> a report the first byte is the report descriptor even in the payload,
> but when we are _getting_ it from the usb device, could a non conformant
> device be mangling it?
>    

Very possible. The sixaxis as we already know is non-conforming in other 
ways. From my custom firmware, I can send back reports with pretty much 
anything in byte 0.

>
>    
>>> with the latest six octets being a hw addr; now if I wanted to change
>>> this, *naively*, I'd send this report with HIDIOCSFEATURE:
>>> -->   f5 01 00 66 55 44 33 22 11
>>>       ^^ report ID _added_ in front of the whole previous buffer
>>>
>>> but then when I get the report again to see if the operation succeeded I
>>> get:
>>> <-- 01 00 00 66 55 44 33 22
>>>
>>> I can make it work if I send:
>>> -->   f5 00 66 55 44 33 22 11
>>>       ^^ report ID _replaced_ buf[0]
>>>
>>> then I get the expected:
>>> <-- 01 00 66 55 44 33 22 11
>>>
>>> Am I missing something about the differences about getting a feature
>>> report and setting it?
>>>
>>>
>>>        
>> Do you have patches 29129a98e6f and c29771c2d8ce applied? They are in
>> 2.6.36-rc6 (and some previous, but I'm note certain how far back). Those
>> patches have to do with _sending_ the right bytes (in particular the
>> report number) on the control endpoint. You seem to be having the
>> opposite problem though. Let me know to be sure.
>>
>>      
> I confirm I have those changes applied, in fact I am working on top of
> 2.6.36-rc6, and yes, my doubt is about the buffer returned from a GET
> FEATURE REPORT.
>    

Yes, but I was wondering if the data that you sent in the SET report was 
maybe shifted by one, causing the data read back properly from the GET 
report to look shifted. It was just a theory.

>    
>>> In the HIDIOCGFEATURE doc I read:
>>> 	...
>>> 	The report will be returned starting at
>>> 	the first byte of the buffer (ie: the report number is not
>>> 	returned).
>>>
>>> Someone (/me whistling, with hands in pockets) could interpret this
>>> like a buf++ semantic on a quick read, even if this sentence means
>>> "only" that _actual_data_ is from buf[1]; but one question stands: what
>>> is that value returned in buf[0]? Is this unspecified?
>>>
>>>        
>> So that was written when I had an incorrect understanding of the
>> standard. It is wrong. I had originally thought that Report ID's were
>> not sent on the control endpoint as part of the payload (because it's
>> redundant, since it's in wValue). I was wrong, and made those two
>> patches above to correct it.
>>
>>      
> Alan I am still talking about GET_FEATURE here, not SET, the question
> here is what value the first byte (buf[0]) is expected to have in a
> buffer returned from a GET. If it has to be the report ID then I
> suspect my device is changing it behind our backs, maybe a failsafe
> measure like the following could do?
>
>    

Like I said above, just a theory that your SET data was setting the data 
incorrectly, and that the GET data was reading back from the device 
exactly what the device sent you (but of course the hardware ID was 
shifted because it wasn't SET correctly).

> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index df80532..83e1b48 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -875,6 +875,9 @@ static int usbhid_output_raw_report(struct hid_device *hid, __u8 *buf, size_t co
>                          ((report_type + 1)<<  8) | report_id,
>                          interface->desc.bInterfaceNumber, buf, count,
>                          USB_CTRL_SET_TIMEOUT);
> +               /* restore report ID into the buffer, some non conformant usb
> +                * device could have mangled it */
> +               buf[0] = report_id;
>                  /* count also the report id, if this was a numbered report. */
>                  if (ret>  0&&  skipped_report_id)
>                          ret++;
>
>
> But I haven't even taken a look at the HID spec about this, so take it
> just for the sake of discussion. Anyone could run some tests with other
> devices?
>    

I actually don't like it (I did see your later email about how you 
really meant for it to be in usbhid_get_raw_report(), so consider this 
my response to that). I don't like it because hidraw is supposed to be 
raw. If the device sends back bogus stuff, then bogus stuff should be 
passed back to userspace. Likewise, if the host tries to send bogus 
stuff to the device, then it should be able to (or be able to try). The 
problem with enforcing rules here is determining which rules to enforce. 
For example, with hiddev, you can't get or set reports that aren't in 
the HID descriptor. That keeps you from being able to get report 0xf5 
which is required to use the sixaxis device. I think this goes against 
the idea of "raw," and could even mislead the user to think that a 
device is functioning properly (or has been implemented properly) when 
it is not.

Alan.

Re: [PATCH v4 1/2] HID: Add Support for Setting and Getting Feature Reports from hidraw

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 2556 bytes --]

On Fri, 01 Oct 2010 20:30:36 -0400
Alan Ott <alan@signal11.us> wrote:

> > diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> > index df80532..83e1b48 100644
> > --- a/drivers/hid/usbhid/hid-core.c
> > +++ b/drivers/hid/usbhid/hid-core.c
> > @@ -875,6 +875,9 @@ static int usbhid_output_raw_report(struct hid_device *hid, __u8 *buf, size_t co
> >                          ((report_type + 1)<<  8) | report_id,
> >                          interface->desc.bInterfaceNumber, buf, count,
> >                          USB_CTRL_SET_TIMEOUT);
> > +               /* restore report ID into the buffer, some non conformant usb
> > +                * device could have mangled it */
> > +               buf[0] = report_id;
> >                  /* count also the report id, if this was a numbered report. */
> >                  if (ret>  0&&  skipped_report_id)
> >                          ret++;
> >
> >
> > But I haven't even taken a look at the HID spec about this, so take it
> > just for the sake of discussion. Anyone could run some tests with other
> > devices?
> >    
> 
> I actually don't like it (I did see your later email about how you 
> really meant for it to be in usbhid_get_raw_report(), so consider this 
> my response to that). I don't like it because hidraw is supposed to be 
> raw. If the device sends back bogus stuff, then bogus stuff should be 
> passed back to userspace. Likewise, if the host tries to send bogus 
> stuff to the device, then it should be able to (or be able to try). The 
> problem with enforcing rules here is determining which rules to enforce. 
> For example, with hiddev, you can't get or set reports that aren't in 
> the HID descriptor. That keeps you from being able to get report 0xf5 
> which is required to use the sixaxis device. I think this goes against 
> the idea of "raw," and could even mislead the user to think that a 
> device is functioning properly (or has been implemented properly) when 
> it is not.
>

Ok, I see your point and you convinced me, let it be "raw" then.
If you are going through another iteration of the documentation patch
maybe you can mention that buf[0] in the returned descriptor can be
pretty much everything when dealing with deviated devices.

Regards,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v4 1/2] HID: Add Support for Setting and Getting Feature Reports from hidraw

[Alan Ott]

On 10/02/2010 03:50 AM, Antonio Ospite wrote:
> Ok, I see your point and you convinced me, let it be "raw" then.
> If you are going through another iteration of the documentation patch
> maybe you can mention that buf[0] in the returned descriptor can be
> pretty much everything when dealing with deviated devices.
>
> Regards,
>     Antonio
>    

Will do. I'm sure that document will need a good scrubbing by the time 
this hidraw feature stuff is applied to the kernel.

Alan.