[PATCH 0/6] HID: Add a stable method for retrieving the client MAC address of a HID device

[PATCH 0/6] HID: Add a stable method for retrieving the client MAC address of a HID device

[Frank Praznik]

Currently there is no reliable way for a device module or hidraw application to
retrieve the client MAC address of the associated wireless device.  This series
of patches adds a stable way of retrieving this information.

The first patch adds a new client_addr member to the hid_device struct.  This
member, when populated, will be guaranteed to contain the client hardware
address of a connected wireless device.

The second patch modifies HIDP to populate the client_addr member of the 
hid_device struct with the client MAC address of the connected wireless device.

The third patch adds support for setting the client_addr value in a uhid
device.

The final patches add support for retrieving the client_addr value as a string
in the hidraw interface via a new ioctl called HIDIOCGRAWCLIENTADDR.  The
hidraw documentation and example program are also updated.

[PATCH 1/6] HID: Add the client_addr member to the hid_device struct

[Frank Praznik]

Add the client_addr member to the hid_device struct for storing the client
address of a connected device.

Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
---
 include/linux/hid.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/hid.h b/include/linux/hid.h
index 31b9d29..283fc6e 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -495,6 +495,7 @@ struct hid_device {							/* device report descriptor */
 	char name[128];							/* Device name */
 	char phys[64];							/* Device physical location */
 	char uniq[64];							/* Device unique identifier (serial #) */
+	__u8 client_addr[6];						/* Device client address (if wireless) */
 
 	void *driver_data;
 
-- 
1.8.5.3

Re: [PATCH 1/6] HID: Add the client_addr member to the hid_device struct

[Benjamin Tissoires]

On Fri, Jan 31, 2014 at 12:32 PM, Frank Praznik <frank.praznik@oh.rr.com> wrote:
> Add the client_addr member to the hid_device struct for storing the client
> address of a connected device.
>
> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
> ---
>  include/linux/hid.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 31b9d29..283fc6e 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -495,6 +495,7 @@ struct hid_device {                                                 /* device report descriptor */
>         char name[128];                                                 /* Device name */
>         char phys[64];                                                  /* Device physical location */
>         char uniq[64];                                                  /* Device unique identifier (serial #) */
> +       __u8 client_addr[6];                                            /* Device client address (if wireless) */

It is good to make small patches, but this one should definitively be
squashed with 2/6.
You should be able to say why you add a field in a struct by looking
at the code in the patch.

Cheers,
Benjamin

>
>         void *driver_data;
>
> --
> 1.8.5.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/6] HID: hidp: Store the device MAC in client_addr.

[Frank Praznik]

Have hidp store the client device MAC address in client_addr

Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
---
 net/bluetooth/hidp/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 292e619..f256f33 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -772,6 +772,9 @@ static int hidp_setup_hid(struct hidp_session *session,
 	snprintf(hid->uniq, sizeof(hid->uniq), "%pMR",
 		 &l2cap_pi(session->ctrl_sock->sk)->chan->dst);
 
+	memcpy(hid->client_addr, &l2cap_pi(session->ctrl_sock->sk)->chan->dst,
+		sizeof(hid->client_addr));
+
 	hid->dev.parent = &session->conn->hcon->dev;
 	hid->ll_driver = &hidp_hid_driver;
 
-- 
1.8.5.3

[PATCH 3/6] HID: Add support for setting client_addr in uhid.

[Frank Praznik]

Add support for setting the client address of a device in uhid.

Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
---
 drivers/hid/uhid.c        | 5 +++++
 include/uapi/linux/uhid.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index cedc6da..cfd2b93 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -259,6 +259,7 @@ struct uhid_create_req_compat {
 	__u8 name[128];
 	__u8 phys[64];
 	__u8 uniq[64];
+	__u8 client_addr[6];
 
 	compat_uptr_t rd_data;
 	__u16 rd_size;
@@ -308,6 +309,8 @@ static int uhid_event_from_user(const char __user *buffer, size_t len,
 				sizeof(compat->phys));
 			memcpy(event->u.create.uniq, compat->uniq,
 				sizeof(compat->uniq));
+			memcpy(event->u.create.client_addr, compat->client_addr,
+				sizeof(compat->client_addr));
 
 			event->u.create.rd_data = compat_ptr(compat->rd_data);
 			event->u.create.rd_size = compat->rd_size;
@@ -375,6 +378,8 @@ static int uhid_dev_create(struct uhid_device *uhid,
 	hid->phys[63] = 0;
 	strncpy(hid->uniq, ev->u.create.uniq, 63);
 	hid->uniq[63] = 0;
+	memcpy(hid->client_addr, ev->u.create.client_addr,
+		sizeof(hid->client_addr));
 
 	hid->ll_driver = &uhid_hid_driver;
 	hid->hid_get_raw_report = uhid_hid_get_raw;
diff --git a/include/uapi/linux/uhid.h b/include/uapi/linux/uhid.h
index 414b74b..5e48acc 100644
--- a/include/uapi/linux/uhid.h
+++ b/include/uapi/linux/uhid.h
@@ -40,6 +40,7 @@ struct uhid_create_req {
 	__u8 name[128];
 	__u8 phys[64];
 	__u8 uniq[64];
+	__u8 client_addr[6];
 	__u8 __user *rd_data;
 	__u16 rd_size;
 
-- 
1.8.5.3

Re: [PATCH 3/6] HID: Add support for setting client_addr in uhid.

[Benjamin Tissoires]

On Fri, Jan 31, 2014 at 12:32 PM, Frank Praznik <frank.praznik@oh.rr.com> wrote:
> Add support for setting the client address of a device in uhid.
>
> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
> ---
>  drivers/hid/uhid.c        | 5 +++++
>  include/uapi/linux/uhid.h | 1 +
>  2 files changed, 6 insertions(+)
>
> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
> index cedc6da..cfd2b93 100644
> --- a/drivers/hid/uhid.c
> +++ b/drivers/hid/uhid.c
> @@ -259,6 +259,7 @@ struct uhid_create_req_compat {
>         __u8 name[128];
>         __u8 phys[64];
>         __u8 uniq[64];
> +       __u8 client_addr[6];
>
>         compat_uptr_t rd_data;
>         __u16 rd_size;
> @@ -308,6 +309,8 @@ static int uhid_event_from_user(const char __user *buffer, size_t len,
>                                 sizeof(compat->phys));
>                         memcpy(event->u.create.uniq, compat->uniq,
>                                 sizeof(compat->uniq));
> +                       memcpy(event->u.create.client_addr, compat->client_addr,
> +                               sizeof(compat->client_addr));
>
>                         event->u.create.rd_data = compat_ptr(compat->rd_data);
>                         event->u.create.rd_size = compat->rd_size;
> @@ -375,6 +378,8 @@ static int uhid_dev_create(struct uhid_device *uhid,
>         hid->phys[63] = 0;
>         strncpy(hid->uniq, ev->u.create.uniq, 63);
>         hid->uniq[63] = 0;
> +       memcpy(hid->client_addr, ev->u.create.client_addr,
> +               sizeof(hid->client_addr));
>
>         hid->ll_driver = &uhid_hid_driver;
>         hid->hid_get_raw_report = uhid_hid_get_raw;
> diff --git a/include/uapi/linux/uhid.h b/include/uapi/linux/uhid.h
> index 414b74b..5e48acc 100644
> --- a/include/uapi/linux/uhid.h
> +++ b/include/uapi/linux/uhid.h
> @@ -40,6 +40,7 @@ struct uhid_create_req {
>         __u8 name[128];
>         __u8 phys[64];
>         __u8 uniq[64];
> +       __u8 client_addr[6];

This is definitively wrong. By that I mean adding a field in the
middle of a struct in a user-space API without handling the case where
a user-space program will use the old API.

You are breaking the API and the ABI here, and you will lead to kernel
crash if the program using this has not been recompiled.
You can add fields in a struct, don't misinterpret me, but you have to
be sure that you will support both old and new API/ABI.

And yes, there are some users (programs) of uhid which would be broken by this.

Cheers,
Benjamin

>         __u8 __user *rd_data;
>         __u16 rd_size;
>
> --
> 1.8.5.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/6] HID: Add the HIDIOCGRAWCLIENTADDR ioctl to hidraw

[Frank Praznik]

Add a new HIDIOCGRAWCLIENTADDR ioctl to hidraw for retrieving the client
address.

Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
---
 drivers/hid/hidraw.c        | 19 +++++++++++++++++++
 include/uapi/linux/hidraw.h |  1 +
 2 files changed, 20 insertions(+)

diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index cb0137b..07c328b 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -447,6 +447,25 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,
 						-EFAULT : len;
 					break;
 				}
+
+				if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGRAWCLIENTADDR(0))) {
+					char addr_str[32];
+					int len;
+					ret = snprintf(addr_str, sizeof(addr_str),
+							"%pMR", hid->client_addr);
+					if (ret < 0)
+						break;
+					else if (ret > sizeof(addr_str)-1)
+						len = sizeof(addr_str);
+					else
+						len = ret + 1;
+
+					if (len > _IOC_SIZE(cmd))
+						len = _IOC_SIZE(cmd);
+					ret = copy_to_user(user_arg, addr_str, len) ?
+						-EFAULT : len;
+					break;
+				}
 			}
 
 		ret = -ENOTTY;
diff --git a/include/uapi/linux/hidraw.h b/include/uapi/linux/hidraw.h
index f5b7329..5b6f4fa 100644
--- a/include/uapi/linux/hidraw.h
+++ b/include/uapi/linux/hidraw.h
@@ -38,6 +38,7 @@ struct hidraw_devinfo {
 /* 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 HIDIOCGRAWCLIENTADDR(len) _IOC(_IOC_READ, 'H', 0x08, len)
 
 #define HIDRAW_FIRST_MINOR 0
 #define HIDRAW_MAX_DEVICES 64
-- 
1.8.5.3

Re: [PATCH 4/6] HID: Add the HIDIOCGRAWCLIENTADDR ioctl to hidraw

[Benjamin Tissoires]

On Fri, Jan 31, 2014 at 12:32 PM, Frank Praznik <frank.praznik@oh.rr.com> wrote:
> Add a new HIDIOCGRAWCLIENTADDR ioctl to hidraw for retrieving the client
> address.
>
> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
> ---
>  drivers/hid/hidraw.c        | 19 +++++++++++++++++++
>  include/uapi/linux/hidraw.h |  1 +
>  2 files changed, 20 insertions(+)
>
> diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
> index cb0137b..07c328b 100644
> --- a/drivers/hid/hidraw.c
> +++ b/drivers/hid/hidraw.c
> @@ -447,6 +447,25 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,
>                                                 -EFAULT : len;
>                                         break;
>                                 }
> +
> +                               if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGRAWCLIENTADDR(0))) {

Maybe we are just lacking an ioctl to retrieve hid->uniq...

Cheers,
Benjamin

> +                                       char addr_str[32];
> +                                       int len;
> +                                       ret = snprintf(addr_str, sizeof(addr_str),
> +                                                       "%pMR", hid->client_addr);
> +                                       if (ret < 0)
> +                                               break;
> +                                       else if (ret > sizeof(addr_str)-1)
> +                                               len = sizeof(addr_str);
> +                                       else
> +                                               len = ret + 1;
> +
> +                                       if (len > _IOC_SIZE(cmd))
> +                                               len = _IOC_SIZE(cmd);
> +                                       ret = copy_to_user(user_arg, addr_str, len) ?
> +                                               -EFAULT : len;
> +                                       break;
> +                               }
>                         }
>
>                 ret = -ENOTTY;
> diff --git a/include/uapi/linux/hidraw.h b/include/uapi/linux/hidraw.h
> index f5b7329..5b6f4fa 100644
> --- a/include/uapi/linux/hidraw.h
> +++ b/include/uapi/linux/hidraw.h
> @@ -38,6 +38,7 @@ struct hidraw_devinfo {
>  /* 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 HIDIOCGRAWCLIENTADDR(len) _IOC(_IOC_READ, 'H', 0x08, len)
>
>  #define HIDRAW_FIRST_MINOR 0
>  #define HIDRAW_MAX_DEVICES 64
> --
> 1.8.5.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/6] HID: Add the HIDIOCGRAWCLIENTADDR ioctl to the hidraw documentation

[Frank Praznik]

Add the HIDIOCGRAWCLIENTADDR ioctl to the hidraw documentation.

Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
---
 Documentation/hid/hidraw.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/hid/hidraw.txt b/Documentation/hid/hidraw.txt
index 029e6cb..737fd1f 100644
--- a/Documentation/hid/hidraw.txt
+++ b/Documentation/hid/hidraw.txt
@@ -93,6 +93,11 @@ For USB devices, the string contains the physical path to the device (the
 USB controller, hubs, ports, etc).  For Bluetooth devices, the string
 contains the hardware (MAC) address of the device.
 
+HIDIOCGRAWCLIENTADDR(len): Get Client Address
+This ioctl returns a string representing the client address of the device.
+For Bluetooth devices, the string contains the hardware (MAC) address of the
+device.  For USB devices, the returned string is always 00:00:00:00:00:00.
+
 HIDIOCSFEATURE(len): Send a Feature Report
 This ioctl will send a feature report to the device.  Per the HID
 specification, feature reports are always sent using the control endpoint.
-- 
1.8.5.3

Re: [PATCH 5/6] HID: Add the HIDIOCGRAWCLIENTADDR ioctl to the hidraw documentation

[Benjamin Tissoires]

On Fri, Jan 31, 2014 at 12:32 PM, Frank Praznik <frank.praznik@oh.rr.com> wrote:
> Add the HIDIOCGRAWCLIENTADDR ioctl to the hidraw documentation.

This one should be squashed with the previous one (same reason in 1/6)

>
> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
> ---
>  Documentation/hid/hidraw.txt | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/hid/hidraw.txt b/Documentation/hid/hidraw.txt
> index 029e6cb..737fd1f 100644
> --- a/Documentation/hid/hidraw.txt
> +++ b/Documentation/hid/hidraw.txt
> @@ -93,6 +93,11 @@ For USB devices, the string contains the physical path to the device (the
>  USB controller, hubs, ports, etc).  For Bluetooth devices, the string
>  contains the hardware (MAC) address of the device.
>
> +HIDIOCGRAWCLIENTADDR(len): Get Client Address
> +This ioctl returns a string representing the client address of the device.
> +For Bluetooth devices, the string contains the hardware (MAC) address of the
> +device.  For USB devices, the returned string is always 00:00:00:00:00:00.
> +
>  HIDIOCSFEATURE(len): Send a Feature Report
>  This ioctl will send a feature report to the device.  Per the HID
>  specification, feature reports are always sent using the control endpoint.
> --
> 1.8.5.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/6] HID: Update the hidraw sample with the usage of HIDIOCGRAWCLIENTADDR

[Frank Praznik]

Add the HIDIOCGRAWCLIENTADDR ioctl to the hidraw sample program.

Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
---
 samples/hidraw/hid-example.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/samples/hidraw/hid-example.c b/samples/hidraw/hid-example.c
index 512a7e5..dbf072d 100644
--- a/samples/hidraw/hid-example.c
+++ b/samples/hidraw/hid-example.c
@@ -23,6 +23,10 @@
 #define HIDIOCSFEATURE(len)    _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x06, len)
 #define HIDIOCGFEATURE(len)    _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x07, len)
 #endif
+#ifndef HIDIOCGRAWCLIENTADDR
+#warning Please have your distro update the userspace kernel headers
+#define HIDIOCGRAWCLIENTADDR(len) _IOC(_IOC_READ, 'H', 0x08, len)
+#endif
 
 /* Unix */
 #include <sys/ioctl.h>
@@ -93,6 +97,13 @@ int main(int argc, char **argv)
 	else
 		printf("Raw Phys: %s\n", buf);
 
+	/* Get Client Address */
+	res = ioctl(fd, HIDIOCGRAWCLIENTADDR(256), buf);
+	if (res < 0)
+		perror("HIDIOCGRAWCLIENTADDR");
+	else
+		printf("Raw Client Address: %s\n", buf);
+
 	/* Get Raw Info */
 	res = ioctl(fd, HIDIOCGRAWINFO, &info);
 	if (res < 0) {
-- 
1.8.5.3

Re: [PATCH 6/6] HID: Update the hidraw sample with the usage of HIDIOCGRAWCLIENTADDR

[Benjamin Tissoires]

On Fri, Jan 31, 2014 at 12:32 PM, Frank Praznik <frank.praznik@oh.rr.com> wrote:
> Add the HIDIOCGRAWCLIENTADDR ioctl to the hidraw sample program.
>

I would personally squash this one with the previous one, but this is
more a matter of taste IMO.

> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
> ---
>  samples/hidraw/hid-example.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/samples/hidraw/hid-example.c b/samples/hidraw/hid-example.c
> index 512a7e5..dbf072d 100644
> --- a/samples/hidraw/hid-example.c
> +++ b/samples/hidraw/hid-example.c
> @@ -23,6 +23,10 @@
>  #define HIDIOCSFEATURE(len)    _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x06, len)
>  #define HIDIOCGFEATURE(len)    _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x07, len)
>  #endif
> +#ifndef HIDIOCGRAWCLIENTADDR
> +#warning Please have your distro update the userspace kernel headers
> +#define HIDIOCGRAWCLIENTADDR(len) _IOC(_IOC_READ, 'H', 0x08, len)
> +#endif
>
>  /* Unix */
>  #include <sys/ioctl.h>
> @@ -93,6 +97,13 @@ int main(int argc, char **argv)
>         else
>                 printf("Raw Phys: %s\n", buf);
>
> +       /* Get Client Address */
> +       res = ioctl(fd, HIDIOCGRAWCLIENTADDR(256), buf);
> +       if (res < 0)
> +               perror("HIDIOCGRAWCLIENTADDR");
> +       else
> +               printf("Raw Client Address: %s\n", buf);
> +
>         /* Get Raw Info */
>         res = ioctl(fd, HIDIOCGRAWINFO, &info);
>         if (res < 0) {
> --
> 1.8.5.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/6] HID: Add a stable method for retrieving the client MAC address of a HID device

[Benjamin Tissoires]

Hi Frank,

just a quick review:

On Fri, Jan 31, 2014 at 12:32 PM, Frank Praznik <frank.praznik@oh.rr.com> wrote:
> Currently there is no reliable way for a device module or hidraw application to
> retrieve the client MAC address of the associated wireless device.  This series
> of patches adds a stable way of retrieving this information.

Well, if I look at the uevent of a Bluetooth mouse I have:

$ cat /sys/devices/pci0000\:00/0000\:00\:14.0/usb3/3-2/3-2\:1.0/bluetooth/hci0/hci0\:43/0005\:046D\:B00D.001F/uevent
DRIVER=hid-generic
HID_ID=0005:0000046D:0000B00D
HID_NAME=Ultrathin Touch Mouse
HID_PHYS=00:10:60:ea:df:ae
HID_UNIQ=00:1f:20:96:33:47
MODALIAS=hid:b0005g0001v0000046Dp0000B00D

I would say that HID_UNIQ is the client MAC address set by hidp, no?
So you don't need to duplicate the info by adding a new field in
hid_device.

I also have few general comments on the patches, I'll comment in them.

Cheers,
Benjamin

>
> The first patch adds a new client_addr member to the hid_device struct.  This
> member, when populated, will be guaranteed to contain the client hardware
> address of a connected wireless device.
>
> The second patch modifies HIDP to populate the client_addr member of the
> hid_device struct with the client MAC address of the connected wireless device.
>
> The third patch adds support for setting the client_addr value in a uhid
> device.
>
> The final patches add support for retrieving the client_addr value as a string
> in the hidraw interface via a new ioctl called HIDIOCGRAWCLIENTADDR.  The
> hidraw documentation and example program are also updated.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/6] HID: Add a stable method for retrieving the client MAC address of a HID device

[Frank Praznik]

On 1/31/2014 14:10, Benjamin Tissoires wrote:
> Hi Frank,
>
> just a quick review:
>
> On Fri, Jan 31, 2014 at 12:32 PM, Frank Praznik <frank.praznik@oh.rr.com> wrote:
>> Currently there is no reliable way for a device module or hidraw application to
>> retrieve the client MAC address of the associated wireless device.  This series
>> of patches adds a stable way of retrieving this information.
> Well, if I look at the uevent of a Bluetooth mouse I have:
>
> $ cat /sys/devices/pci0000\:00/0000\:00\:14.0/usb3/3-2/3-2\:1.0/bluetooth/hci0/hci0\:43/0005\:046D\:B00D.001F/uevent
> DRIVER=hid-generic
> HID_ID=0005:0000046D:0000B00D
> HID_NAME=Ultrathin Touch Mouse
> HID_PHYS=00:10:60:ea:df:ae
> HID_UNIQ=00:1f:20:96:33:47
> MODALIAS=hid:b0005g0001v0000046Dp0000B00D
>
> I would say that HID_UNIQ is the client MAC address set by hidp, no?
> So you don't need to duplicate the info by adding a new field in
> hid_device.
>
In a patch I recently submitted I was using the UNIQ field for 
retrieving a Bluetooth device MAC address and was warned against it 
because "UNIQ is a way to provide unique identifiers for devices, but 
it's not guaranteed to stay the same".  HIDP happens to store the MAC in 
the UNIQ data, but there is no guarantee that it will always be there.  
With these patches you can be completely sure that the data in 
client_addr is the client device MAC address.

Re: [PATCH 0/6] HID: Add a stable method for retrieving the client MAC address of a HID device

[Benjamin Tissoires]

On Fri, Jan 31, 2014 at 3:04 PM, Frank Praznik <frank.praznik@oh.rr.com> wrote:
> On 1/31/2014 14:10, Benjamin Tissoires wrote:
>>
>> Hi Frank,
>>
>> just a quick review:
>>
>> On Fri, Jan 31, 2014 at 12:32 PM, Frank Praznik <frank.praznik@oh.rr.com>
>> wrote:
>>>
>>> Currently there is no reliable way for a device module or hidraw
>>> application to
>>> retrieve the client MAC address of the associated wireless device.  This
>>> series
>>> of patches adds a stable way of retrieving this information.
>>
>> Well, if I look at the uevent of a Bluetooth mouse I have:
>>
>> $ cat
>> /sys/devices/pci0000\:00/0000\:00\:14.0/usb3/3-2/3-2\:1.0/bluetooth/hci0/hci0\:43/0005\:046D\:B00D.001F/uevent
>> DRIVER=hid-generic
>> HID_ID=0005:0000046D:0000B00D
>> HID_NAME=Ultrathin Touch Mouse
>> HID_PHYS=00:10:60:ea:df:ae
>> HID_UNIQ=00:1f:20:96:33:47
>> MODALIAS=hid:b0005g0001v0000046Dp0000B00D
>>
>> I would say that HID_UNIQ is the client MAC address set by hidp, no?
>> So you don't need to duplicate the info by adding a new field in
>> hid_device.
>>
> In a patch I recently submitted I was using the UNIQ field for retrieving a
> Bluetooth device MAC address and was warned against it because "UNIQ is a
> way to provide unique identifiers for devices, but it's not guaranteed to
> stay the same".  HIDP happens to store the MAC in the UNIQ data, but there
> is no guarantee that it will always be there.  With these patches you can be
> completely sure that the data in client_addr is the client device MAC
> address.

ok. But still, adding a transport-specific information to hid_device
is IMO a bad idea. We fought to make HID agnostic of the transport
layer enough.

David, could you elaborate why you think that UNIQ may change in the
future regarding BT?
If the BT MAC address is the same principle of an ethernet MAC
address, UNIQ seems to fit perfectly well.

Otherwise, you may be able to retrieve the MAC address by using the
parent of the current device.

Cheers,
Benjamin

Re: [PATCH 0/6] HID: Add a stable method for retrieving the client MAC address of a HID device

[Frank Praznik]

On 1/31/2014 15:45, Benjamin Tissoires wrote:
> On Fri, Jan 31, 2014 at 3:04 PM, Frank Praznik <frank.praznik@oh.rr.com> wrote:
>> On 1/31/2014 14:10, Benjamin Tissoires wrote:
>>> Hi Frank,
>>>
>>> just a quick review:
>>>
>>> On Fri, Jan 31, 2014 at 12:32 PM, Frank Praznik <frank.praznik@oh.rr.com>
>>> wrote:
>>>> Currently there is no reliable way for a device module or hidraw
>>>> application to
>>>> retrieve the client MAC address of the associated wireless device.  This
>>>> series
>>>> of patches adds a stable way of retrieving this information.
>>> Well, if I look at the uevent of a Bluetooth mouse I have:
>>>
>>> $ cat
>>> /sys/devices/pci0000\:00/0000\:00\:14.0/usb3/3-2/3-2\:1.0/bluetooth/hci0/hci0\:43/0005\:046D\:B00D.001F/uevent
>>> DRIVER=hid-generic
>>> HID_ID=0005:0000046D:0000B00D
>>> HID_NAME=Ultrathin Touch Mouse
>>> HID_PHYS=00:10:60:ea:df:ae
>>> HID_UNIQ=00:1f:20:96:33:47
>>> MODALIAS=hid:b0005g0001v0000046Dp0000B00D
>>>
>>> I would say that HID_UNIQ is the client MAC address set by hidp, no?
>>> So you don't need to duplicate the info by adding a new field in
>>> hid_device.
>>>
>> In a patch I recently submitted I was using the UNIQ field for retrieving a
>> Bluetooth device MAC address and was warned against it because "UNIQ is a
>> way to provide unique identifiers for devices, but it's not guaranteed to
>> stay the same".  HIDP happens to store the MAC in the UNIQ data, but there
>> is no guarantee that it will always be there.  With these patches you can be
>> completely sure that the data in client_addr is the client device MAC
>> address.
> ok. But still, adding a transport-specific information to hid_device
> is IMO a bad idea. We fought to make HID agnostic of the transport
> layer enough.
>
> David, could you elaborate why you think that UNIQ may change in the
> future regarding BT?
> If the BT MAC address is the same principle of an ethernet MAC
> address, UNIQ seems to fit perfectly well.
>
> Otherwise, you may be able to retrieve the MAC address by using the
> parent of the current device.
>
> Cheers,
> Benjamin
Are you referring to using the hid_device::dev.parent pointer?  I know 
that hidp sets it to point to the hci_conn struct from which src address 
for the Bluetooth connection can be obtained, but making assumptions 
about an opaque pointer like that seems dangerous.  Is the parent 
pointer guaranteed to point to the hci_conn struct as long as the bus 
type is Bluetooth?

Re: [PATCH 0/6] HID: Add a stable method for retrieving the client MAC address of a HID device

[Benjamin Tissoires]

On Sat, Feb 1, 2014 at 11:06 AM, Frank Praznik <frank.praznik@gmail.com> wrote:
> On 1/31/2014 15:45, Benjamin Tissoires wrote:
>>
>> On Fri, Jan 31, 2014 at 3:04 PM, Frank Praznik <frank.praznik@oh.rr.com>
>> wrote:
>>>
>>> On 1/31/2014 14:10, Benjamin Tissoires wrote:
>>>>
>>>> Hi Frank,
>>>>
>>>> just a quick review:
>>>>
>>>> On Fri, Jan 31, 2014 at 12:32 PM, Frank Praznik
>>>> <frank.praznik@oh.rr.com>
>>>> wrote:
>>>>>
>>>>> Currently there is no reliable way for a device module or hidraw
>>>>> application to
>>>>> retrieve the client MAC address of the associated wireless device.
>>>>> This
>>>>> series
>>>>> of patches adds a stable way of retrieving this information.
>>>>
>>>> Well, if I look at the uevent of a Bluetooth mouse I have:
>>>>
>>>> $ cat
>>>>
>>>> /sys/devices/pci0000\:00/0000\:00\:14.0/usb3/3-2/3-2\:1.0/bluetooth/hci0/hci0\:43/0005\:046D\:B00D.001F/uevent
>>>> DRIVER=hid-generic
>>>> HID_ID=0005:0000046D:0000B00D
>>>> HID_NAME=Ultrathin Touch Mouse
>>>> HID_PHYS=00:10:60:ea:df:ae
>>>> HID_UNIQ=00:1f:20:96:33:47
>>>> MODALIAS=hid:b0005g0001v0000046Dp0000B00D
>>>>
>>>> I would say that HID_UNIQ is the client MAC address set by hidp, no?
>>>> So you don't need to duplicate the info by adding a new field in
>>>> hid_device.
>>>>
>>> In a patch I recently submitted I was using the UNIQ field for retrieving
>>> a
>>> Bluetooth device MAC address and was warned against it because "UNIQ is a
>>> way to provide unique identifiers for devices, but it's not guaranteed to
>>> stay the same".  HIDP happens to store the MAC in the UNIQ data, but
>>> there
>>> is no guarantee that it will always be there.  With these patches you can
>>> be
>>> completely sure that the data in client_addr is the client device MAC
>>> address.
>>
>> ok. But still, adding a transport-specific information to hid_device
>> is IMO a bad idea. We fought to make HID agnostic of the transport
>> layer enough.
>>
>> David, could you elaborate why you think that UNIQ may change in the
>> future regarding BT?
>> If the BT MAC address is the same principle of an ethernet MAC
>> address, UNIQ seems to fit perfectly well.
>>
>> Otherwise, you may be able to retrieve the MAC address by using the
>> parent of the current device.
>>
>> Cheers,
>> Benjamin
>
> Are you referring to using the hid_device::dev.parent pointer?  I know that

Yes

> hidp sets it to point to the hci_conn struct from which src address for the
> Bluetooth connection can be obtained, but making assumptions about an opaque
> pointer like that seems dangerous.  Is the parent pointer guaranteed to
> point to the hci_conn struct as long as the bus type is Bluetooth?

And nope. If you use uhid, then the parent will not be a hci_conn.

With enough guards, you should be able to use it, but it's not ideal I agree.
I really want to have David's opinion regarding the UNIQ field. IMO,
this seems to be the most transport-agnostic way of doing it.

Cheers,
Benjamin

Re: [PATCH 0/6] HID: Add a stable method for retrieving the client MAC address of a HID device

[David Herrmann]

Hi

On Sat, Feb 1, 2014 at 5:28 PM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> On Sat, Feb 1, 2014 at 11:06 AM, Frank Praznik <frank.praznik@gmail.com> wrote:
>> On 1/31/2014 15:45, Benjamin Tissoires wrote:
>>>
>>> On Fri, Jan 31, 2014 at 3:04 PM, Frank Praznik <frank.praznik@oh.rr.com>
>>> wrote:
>>>>
>>>> On 1/31/2014 14:10, Benjamin Tissoires wrote:
>>>>>
>>>>> Hi Frank,
>>>>>
>>>>> just a quick review:
>>>>>
>>>>> On Fri, Jan 31, 2014 at 12:32 PM, Frank Praznik
>>>>> <frank.praznik@oh.rr.com>
>>>>> wrote:
>>>>>>
>>>>>> Currently there is no reliable way for a device module or hidraw
>>>>>> application to
>>>>>> retrieve the client MAC address of the associated wireless device.
>>>>>> This
>>>>>> series
>>>>>> of patches adds a stable way of retrieving this information.
>>>>>
>>>>> Well, if I look at the uevent of a Bluetooth mouse I have:
>>>>>
>>>>> $ cat
>>>>>
>>>>> /sys/devices/pci0000\:00/0000\:00\:14.0/usb3/3-2/3-2\:1.0/bluetooth/hci0/hci0\:43/0005\:046D\:B00D.001F/uevent
>>>>> DRIVER=hid-generic
>>>>> HID_ID=0005:0000046D:0000B00D
>>>>> HID_NAME=Ultrathin Touch Mouse
>>>>> HID_PHYS=00:10:60:ea:df:ae
>>>>> HID_UNIQ=00:1f:20:96:33:47
>>>>> MODALIAS=hid:b0005g0001v0000046Dp0000B00D
>>>>>
>>>>> I would say that HID_UNIQ is the client MAC address set by hidp, no?
>>>>> So you don't need to duplicate the info by adding a new field in
>>>>> hid_device.
>>>>>
>>>> In a patch I recently submitted I was using the UNIQ field for retrieving
>>>> a
>>>> Bluetooth device MAC address and was warned against it because "UNIQ is a
>>>> way to provide unique identifiers for devices, but it's not guaranteed to
>>>> stay the same".  HIDP happens to store the MAC in the UNIQ data, but
>>>> there
>>>> is no guarantee that it will always be there.  With these patches you can
>>>> be
>>>> completely sure that the data in client_addr is the client device MAC
>>>> address.
>>>
>>> ok. But still, adding a transport-specific information to hid_device
>>> is IMO a bad idea. We fought to make HID agnostic of the transport
>>> layer enough.
>>>
>>> David, could you elaborate why you think that UNIQ may change in the
>>> future regarding BT?
>>> If the BT MAC address is the same principle of an ethernet MAC
>>> address, UNIQ seems to fit perfectly well.
>>>
>>> Otherwise, you may be able to retrieve the MAC address by using the
>>> parent of the current device.
>>>
>>> Cheers,
>>> Benjamin
>>
>> Are you referring to using the hid_device::dev.parent pointer?  I know that
>
> Yes
>
>> hidp sets it to point to the hci_conn struct from which src address for the
>> Bluetooth connection can be obtained, but making assumptions about an opaque
>> pointer like that seems dangerous.  Is the parent pointer guaranteed to
>> point to the hci_conn struct as long as the bus type is Bluetooth?
>
> And nope. If you use uhid, then the parent will not be a hci_conn.
>
> With enough guards, you should be able to use it, but it's not ideal I agree.
> I really want to have David's opinion regarding the UNIQ field. IMO,
> this seems to be the most transport-agnostic way of doing it.

UNIQ is definitely wrong. It is used to assign a run-time *unique*
value to the connection. So ideally it should be different if a device
is reconnected. The PHYS field is actually used to identify a physical
device. So please, if we're doing this, then we should do it via PHYS.

I'm fine with hard-coding PHYS as bt-address for BT-HID, but please
add a comment to hidp_setup_hid() in net/bluetooth/hidp/core.c before
doing the snprintf() there.

The reason why I disliked hard-coding this behavior is that if a
single BT-device is connected twice to us, we usually want two
different PHYS entries for both depending on which service this
connection provides. However, this seems like an unlikely and
overengineered problem so lets not do that. Furthermore, while BT-HID
would allow such setups with some hacks, it isn't supported in a clean
way. So yeah, I'm actually fine with using PHYS for that.

Thanks
David

Re: [PATCH 0/6] HID: Add a stable method for retrieving the client MAC address of a HID device

[Frank Praznik]

On Mon, Feb 3, 2014 at 11:24 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Sat, Feb 1, 2014 at 5:28 PM, Benjamin Tissoires
> <benjamin.tissoires@gmail.com> wrote:
>> On Sat, Feb 1, 2014 at 11:06 AM, Frank Praznik <frank.praznik@gmail.com> wrote:
>>> On 1/31/2014 15:45, Benjamin Tissoires wrote:
>>>>
>>>> On Fri, Jan 31, 2014 at 3:04 PM, Frank Praznik <frank.praznik@oh.rr.com>
>>>> wrote:
>>>>>
>>>>> On 1/31/2014 14:10, Benjamin Tissoires wrote:
>>>>>>
>>>>>> Hi Frank,
>>>>>>
>>>>>> just a quick review:
>>>>>>
>>>>>> On Fri, Jan 31, 2014 at 12:32 PM, Frank Praznik
>>>>>> <frank.praznik@oh.rr.com>
>>>>>> wrote:
>>>>>>>
>>>>>>> Currently there is no reliable way for a device module or hidraw
>>>>>>> application to
>>>>>>> retrieve the client MAC address of the associated wireless device.
>>>>>>> This
>>>>>>> series
>>>>>>> of patches adds a stable way of retrieving this information.
>>>>>>
>>>>>> Well, if I look at the uevent of a Bluetooth mouse I have:
>>>>>>
>>>>>> $ cat
>>>>>>
>>>>>> /sys/devices/pci0000\:00/0000\:00\:14.0/usb3/3-2/3-2\:1.0/bluetooth/hci0/hci0\:43/0005\:046D\:B00D.001F/uevent
>>>>>> DRIVER=hid-generic
>>>>>> HID_ID=0005:0000046D:0000B00D
>>>>>> HID_NAME=Ultrathin Touch Mouse
>>>>>> HID_PHYS=00:10:60:ea:df:ae
>>>>>> HID_UNIQ=00:1f:20:96:33:47
>>>>>> MODALIAS=hid:b0005g0001v0000046Dp0000B00D
>>>>>>
>>>>>> I would say that HID_UNIQ is the client MAC address set by hidp, no?
>>>>>> So you don't need to duplicate the info by adding a new field in
>>>>>> hid_device.
>>>>>>
>>>>> In a patch I recently submitted I was using the UNIQ field for retrieving
>>>>> a
>>>>> Bluetooth device MAC address and was warned against it because "UNIQ is a
>>>>> way to provide unique identifiers for devices, but it's not guaranteed to
>>>>> stay the same".  HIDP happens to store the MAC in the UNIQ data, but
>>>>> there
>>>>> is no guarantee that it will always be there.  With these patches you can
>>>>> be
>>>>> completely sure that the data in client_addr is the client device MAC
>>>>> address.
>>>>
>>>> ok. But still, adding a transport-specific information to hid_device
>>>> is IMO a bad idea. We fought to make HID agnostic of the transport
>>>> layer enough.
>>>>
>>>> David, could you elaborate why you think that UNIQ may change in the
>>>> future regarding BT?
>>>> If the BT MAC address is the same principle of an ethernet MAC
>>>> address, UNIQ seems to fit perfectly well.
>>>>
>>>> Otherwise, you may be able to retrieve the MAC address by using the
>>>> parent of the current device.
>>>>
>>>> Cheers,
>>>> Benjamin
>>>
>>> Are you referring to using the hid_device::dev.parent pointer?  I know that
>>
>> Yes
>>
>>> hidp sets it to point to the hci_conn struct from which src address for the
>>> Bluetooth connection can be obtained, but making assumptions about an opaque
>>> pointer like that seems dangerous.  Is the parent pointer guaranteed to
>>> point to the hci_conn struct as long as the bus type is Bluetooth?
>>
>> And nope. If you use uhid, then the parent will not be a hci_conn.
>>
>> With enough guards, you should be able to use it, but it's not ideal I agree.
>> I really want to have David's opinion regarding the UNIQ field. IMO,
>> this seems to be the most transport-agnostic way of doing it.
>
> UNIQ is definitely wrong. It is used to assign a run-time *unique*
> value to the connection. So ideally it should be different if a device
> is reconnected. The PHYS field is actually used to identify a physical
> device. So please, if we're doing this, then we should do it via PHYS.
>
> I'm fine with hard-coding PHYS as bt-address for BT-HID, but please
> add a comment to hidp_setup_hid() in net/bluetooth/hidp/core.c before
> doing the snprintf() there.
>
> The reason why I disliked hard-coding this behavior is that if a
> single BT-device is connected twice to us, we usually want two
> different PHYS entries for both depending on which service this
> connection provides. However, this seems like an unlikely and
> overengineered problem so lets not do that. Furthermore, while BT-HID
> would allow such setups with some hacks, it isn't supported in a clean
> way. So yeah, I'm actually fine with using PHYS for that.
>
> Thanks
> David

PHYS should definitely be changed if this is the case because right
now it is set to the MAC of the host adapter which means that it's the
same for every Bluetooth device and connection.  I believe that the
hidraw documentation also specifies that for Bluetooth devices the
HIDIOCGRAWPHYS ioctl should return a string with the MAC address of
the associated device rather than that of the host adapter as the
current behavior does.

Re: [PATCH 0/6] HID: Add a stable method for retrieving the client MAC address of a HID device

[David Herrmann]

Hi

Adding Dmitry+Jiri, maybe they can clarify this.

[snip]
>>>
>>> Yes
>>>
>>>> hidp sets it to point to the hci_conn struct from which src address for the
>>>> Bluetooth connection can be obtained, but making assumptions about an opaque
>>>> pointer like that seems dangerous.  Is the parent pointer guaranteed to
>>>> point to the hci_conn struct as long as the bus type is Bluetooth?
>>>
>>> And nope. If you use uhid, then the parent will not be a hci_conn.
>>>
>>> With enough guards, you should be able to use it, but it's not ideal I agree.
>>> I really want to have David's opinion regarding the UNIQ field. IMO,
>>> this seems to be the most transport-agnostic way of doing it.
>>
>> UNIQ is definitely wrong. It is used to assign a run-time *unique*
>> value to the connection. So ideally it should be different if a device
>> is reconnected. The PHYS field is actually used to identify a physical
>> device. So please, if we're doing this, then we should do it via PHYS.
>>
>> I'm fine with hard-coding PHYS as bt-address for BT-HID, but please
>> add a comment to hidp_setup_hid() in net/bluetooth/hidp/core.c before
>> doing the snprintf() there.
>>
>> The reason why I disliked hard-coding this behavior is that if a
>> single BT-device is connected twice to us, we usually want two
>> different PHYS entries for both depending on which service this
>> connection provides. However, this seems like an unlikely and
>> overengineered problem so lets not do that. Furthermore, while BT-HID
>> would allow such setups with some hacks, it isn't supported in a clean
>> way. So yeah, I'm actually fine with using PHYS for that.
>>
>> Thanks
>> David
>
> PHYS should definitely be changed if this is the case because right
> now it is set to the MAC of the host adapter which means that it's the
> same for every Bluetooth device and connection.  I believe that the
> hidraw documentation also specifies that for Bluetooth devices the
> HIDIOCGRAWPHYS ioctl should return a string with the MAC address of
> the associated device rather than that of the host adapter as the
> current behavior does.

Oh, yes, nice catch!
Ok, maybe we need to clear up what PHYS and UNIQ do before relying on
them. I thought, they were defined as:

PHYS: A string describing the physical device. It should be the same
if a device reconnects. It can be used by user-space to track devices
across disconnects

UNIQ: A string describing the current connection to a device. If the
device reconnects, the UNIQ string should change. It can be used by
user-space to track a single device-session.

afaik both strings have no common format and drivers are free to
provide any kind of information, as long as it follows the given
rules. That's why I disliked the idea of relying on them and parsing
them. But maybe I just have no idea what the original intention was
behind them?

Thanks
David

Re: [PATCH 0/6] HID: Add a stable method for retrieving the client MAC address of a HID device

[Dmitry Torokhov]

On Mon, Feb 03, 2014 at 06:45:52PM +0100, David Herrmann wrote:
> Hi
> 
> Adding Dmitry+Jiri, maybe they can clarify this.
> 
> [snip]
> >>>
> >>> Yes
> >>>
> >>>> hidp sets it to point to the hci_conn struct from which src address for the
> >>>> Bluetooth connection can be obtained, but making assumptions about an opaque
> >>>> pointer like that seems dangerous.  Is the parent pointer guaranteed to
> >>>> point to the hci_conn struct as long as the bus type is Bluetooth?
> >>>
> >>> And nope. If you use uhid, then the parent will not be a hci_conn.
> >>>
> >>> With enough guards, you should be able to use it, but it's not ideal I agree.
> >>> I really want to have David's opinion regarding the UNIQ field. IMO,
> >>> this seems to be the most transport-agnostic way of doing it.
> >>
> >> UNIQ is definitely wrong. It is used to assign a run-time *unique*
> >> value to the connection. So ideally it should be different if a device
> >> is reconnected. The PHYS field is actually used to identify a physical
> >> device. So please, if we're doing this, then we should do it via PHYS.
> >>
> >> I'm fine with hard-coding PHYS as bt-address for BT-HID, but please
> >> add a comment to hidp_setup_hid() in net/bluetooth/hidp/core.c before
> >> doing the snprintf() there.
> >>
> >> The reason why I disliked hard-coding this behavior is that if a
> >> single BT-device is connected twice to us, we usually want two
> >> different PHYS entries for both depending on which service this
> >> connection provides. However, this seems like an unlikely and
> >> overengineered problem so lets not do that. Furthermore, while BT-HID
> >> would allow such setups with some hacks, it isn't supported in a clean
> >> way. So yeah, I'm actually fine with using PHYS for that.
> >>
> >> Thanks
> >> David
> >
> > PHYS should definitely be changed if this is the case because right
> > now it is set to the MAC of the host adapter which means that it's the
> > same for every Bluetooth device and connection.  I believe that the
> > hidraw documentation also specifies that for Bluetooth devices the
> > HIDIOCGRAWPHYS ioctl should return a string with the MAC address of
> > the associated device rather than that of the host adapter as the
> > current behavior does.
> 
> Oh, yes, nice catch!
> Ok, maybe we need to clear up what PHYS and UNIQ do before relying on
> them. I thought, they were defined as:
> 
> PHYS: A string describing the physical device. It should be the same
> if a device reconnects. It can be used by user-space to track devices
> across disconnects
> 
> UNIQ: A string describing the current connection to a device. If the
> device reconnects, the UNIQ string should change. It can be used by
> user-space to track a single device-session.
> 
> afaik both strings have no common format and drivers are free to
> provide any kind of information, as long as it follows the given
> rules. That's why I disliked the idea of relying on them and parsing
> them. But maybe I just have no idea what the original intention was
> behind them?

PHYS: describes physical connection of the device in a given box,
supposed to be unique within a box.

UNIQ: unique identifier (if exists) assigned to the device, should
ideally be unique globally and should not change when device is moved
within a box out between boxes. Think serial number in USB descriptors.

Thanks.

-- 
Dmitry

Re: [PATCH 0/6] HID: Add a stable method for retrieving the client MAC address of a HID device

[David Herrmann]

Hi

On Mon, Feb 3, 2014 at 6:57 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Mon, Feb 03, 2014 at 06:45:52PM +0100, David Herrmann wrote:
>> Hi
>>
>> Adding Dmitry+Jiri, maybe they can clarify this.
>>
>> [snip]
>> >>>
>> >>> Yes
>> >>>
>> >>>> hidp sets it to point to the hci_conn struct from which src address for the
>> >>>> Bluetooth connection can be obtained, but making assumptions about an opaque
>> >>>> pointer like that seems dangerous.  Is the parent pointer guaranteed to
>> >>>> point to the hci_conn struct as long as the bus type is Bluetooth?
>> >>>
>> >>> And nope. If you use uhid, then the parent will not be a hci_conn.
>> >>>
>> >>> With enough guards, you should be able to use it, but it's not ideal I agree.
>> >>> I really want to have David's opinion regarding the UNIQ field. IMO,
>> >>> this seems to be the most transport-agnostic way of doing it.
>> >>
>> >> UNIQ is definitely wrong. It is used to assign a run-time *unique*
>> >> value to the connection. So ideally it should be different if a device
>> >> is reconnected. The PHYS field is actually used to identify a physical
>> >> device. So please, if we're doing this, then we should do it via PHYS.
>> >>
>> >> I'm fine with hard-coding PHYS as bt-address for BT-HID, but please
>> >> add a comment to hidp_setup_hid() in net/bluetooth/hidp/core.c before
>> >> doing the snprintf() there.
>> >>
>> >> The reason why I disliked hard-coding this behavior is that if a
>> >> single BT-device is connected twice to us, we usually want two
>> >> different PHYS entries for both depending on which service this
>> >> connection provides. However, this seems like an unlikely and
>> >> overengineered problem so lets not do that. Furthermore, while BT-HID
>> >> would allow such setups with some hacks, it isn't supported in a clean
>> >> way. So yeah, I'm actually fine with using PHYS for that.
>> >>
>> >> Thanks
>> >> David
>> >
>> > PHYS should definitely be changed if this is the case because right
>> > now it is set to the MAC of the host adapter which means that it's the
>> > same for every Bluetooth device and connection.  I believe that the
>> > hidraw documentation also specifies that for Bluetooth devices the
>> > HIDIOCGRAWPHYS ioctl should return a string with the MAC address of
>> > the associated device rather than that of the host adapter as the
>> > current behavior does.
>>
>> Oh, yes, nice catch!
>> Ok, maybe we need to clear up what PHYS and UNIQ do before relying on
>> them. I thought, they were defined as:
>>
>> PHYS: A string describing the physical device. It should be the same
>> if a device reconnects. It can be used by user-space to track devices
>> across disconnects
>>
>> UNIQ: A string describing the current connection to a device. If the
>> device reconnects, the UNIQ string should change. It can be used by
>> user-space to track a single device-session.
>>
>> afaik both strings have no common format and drivers are free to
>> provide any kind of information, as long as it follows the given
>> rules. That's why I disliked the idea of relying on them and parsing
>> them. But maybe I just have no idea what the original intention was
>> behind them?
>
> PHYS: describes physical connection of the device in a given box,
> supposed to be unique within a box.
>
> UNIQ: unique identifier (if exists) assigned to the device, should
> ideally be unique globally and should not change when device is moved
> within a box out between boxes. Think serial number in USB descriptors.

Thanks, so it's basically the other way around as I thought. So I
think using UNIQ is the way to go, sorry for the confusion.

Thanks
David

Re: [PATCH 0/6] HID: Add a stable method for retrieving the client MAC address of a HID device

[Frank Praznik]

On Mon, Feb 3, 2014 at 1:06 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Mon, Feb 3, 2014 at 6:57 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> On Mon, Feb 03, 2014 at 06:45:52PM +0100, David Herrmann wrote:
>>> Hi
>>>
>>> Adding Dmitry+Jiri, maybe they can clarify this.
>>>
>>> [snip]
>>> >>>
>>> >>> Yes
>>> >>>
>>> >>>> hidp sets it to point to the hci_conn struct from which src address for the
>>> >>>> Bluetooth connection can be obtained, but making assumptions about an opaque
>>> >>>> pointer like that seems dangerous.  Is the parent pointer guaranteed to
>>> >>>> point to the hci_conn struct as long as the bus type is Bluetooth?
>>> >>>
>>> >>> And nope. If you use uhid, then the parent will not be a hci_conn.
>>> >>>
>>> >>> With enough guards, you should be able to use it, but it's not ideal I agree.
>>> >>> I really want to have David's opinion regarding the UNIQ field. IMO,
>>> >>> this seems to be the most transport-agnostic way of doing it.
>>> >>
>>> >> UNIQ is definitely wrong. It is used to assign a run-time *unique*
>>> >> value to the connection. So ideally it should be different if a device
>>> >> is reconnected. The PHYS field is actually used to identify a physical
>>> >> device. So please, if we're doing this, then we should do it via PHYS.
>>> >>
>>> >> I'm fine with hard-coding PHYS as bt-address for BT-HID, but please
>>> >> add a comment to hidp_setup_hid() in net/bluetooth/hidp/core.c before
>>> >> doing the snprintf() there.
>>> >>
>>> >> The reason why I disliked hard-coding this behavior is that if a
>>> >> single BT-device is connected twice to us, we usually want two
>>> >> different PHYS entries for both depending on which service this
>>> >> connection provides. However, this seems like an unlikely and
>>> >> overengineered problem so lets not do that. Furthermore, while BT-HID
>>> >> would allow such setups with some hacks, it isn't supported in a clean
>>> >> way. So yeah, I'm actually fine with using PHYS for that.
>>> >>
>>> >> Thanks
>>> >> David
>>> >
>>> > PHYS should definitely be changed if this is the case because right
>>> > now it is set to the MAC of the host adapter which means that it's the
>>> > same for every Bluetooth device and connection.  I believe that the
>>> > hidraw documentation also specifies that for Bluetooth devices the
>>> > HIDIOCGRAWPHYS ioctl should return a string with the MAC address of
>>> > the associated device rather than that of the host adapter as the
>>> > current behavior does.
>>>
>>> Oh, yes, nice catch!
>>> Ok, maybe we need to clear up what PHYS and UNIQ do before relying on
>>> them. I thought, they were defined as:
>>>
>>> PHYS: A string describing the physical device. It should be the same
>>> if a device reconnects. It can be used by user-space to track devices
>>> across disconnects
>>>
>>> UNIQ: A string describing the current connection to a device. If the
>>> device reconnects, the UNIQ string should change. It can be used by
>>> user-space to track a single device-session.
>>>
>>> afaik both strings have no common format and drivers are free to
>>> provide any kind of information, as long as it follows the given
>>> rules. That's why I disliked the idea of relying on them and parsing
>>> them. But maybe I just have no idea what the original intention was
>>> behind them?
>>
>> PHYS: describes physical connection of the device in a given box,
>> supposed to be unique within a box.
>>
>> UNIQ: unique identifier (if exists) assigned to the device, should
>> ideally be unique globally and should not change when device is moved
>> within a box out between boxes. Think serial number in USB descriptors.
>
> Thanks, so it's basically the other way around as I thought. So I
> think using UNIQ is the way to go, sorry for the confusion.
>
> Thanks
> David

Thanks for clearing this up.  It sounds like UNIQ should be safe for
getting the client MAC of a Bluetooth device as long as the
appropriate sanity checks are performed to make sure that it's not a
custom string from a uhid device.  Or, going back to the earlier
question, is there still some reason as to why HIDP might change the
behavior of UNIQ in the future?