[PATCH] Bluetooth: Fix uuid output in debugfs

[PATCH] Bluetooth: Fix uuid output in debugfs

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

The uuid should be printed in the CPU endianness and not in little-endian.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 net/bluetooth/hci_sysfs.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
index 55cceee..05b78c7 100644
--- a/net/bluetooth/hci_sysfs.c
+++ b/net/bluetooth/hci_sysfs.c
@@ -461,19 +461,18 @@ static const struct file_operations blacklist_fops = {
 
 static void print_bt_uuid(struct seq_file *f, u8 *uuid)
 {
-	__be32 data0, data4;
-	__be16 data1, data2, data3, data5;
-
-	memcpy(&data0, &uuid[0], 4);
-	memcpy(&data1, &uuid[4], 2);
-	memcpy(&data2, &uuid[6], 2);
-	memcpy(&data3, &uuid[8], 2);
-	memcpy(&data4, &uuid[10], 4);
-	memcpy(&data5, &uuid[14], 2);
-
-	seq_printf(f, "%.8x-%.4x-%.4x-%.4x-%.8x%.4x\n",
-		   ntohl(data0), ntohs(data1), ntohs(data2), ntohs(data3),
-		   ntohl(data4), ntohs(data5));
+	u32 data0, data5;
+	u16 data1, data2, data3, data4;
+
+	data5 = le32_to_cpu(*(__le32 *)uuid);
+	data4 = le16_to_cpu(*(__le16 *)(uuid + 4));
+	data3 = le16_to_cpu(*(__le16 *)(uuid + 6));
+	data2 = le16_to_cpu(*(__le16 *)(uuid + 8));
+	data1 = le16_to_cpu(*(__le16 *)(uuid + 10));
+	data0 = le32_to_cpu(*(__le32 *)(uuid + 12));
+
+	seq_printf(f, "%.8x-%.4x-%.4x-%.4x-%.4x%.8x\n",
+		   data0, data1, data2, data3, data4, data5);
 }
 
 static int uuids_show(struct seq_file *f, void *p)
-- 
1.8.0.2

[PATCH] Bluetooth: Fix uuid output in debugfs

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

The uuid should be printed in the CPU endianness and not in little-endian.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 net/bluetooth/hci_sysfs.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
index 55cceee..05b78c7 100644
--- a/net/bluetooth/hci_sysfs.c
+++ b/net/bluetooth/hci_sysfs.c
@@ -461,19 +461,18 @@ static const struct file_operations blacklist_fops = {
 
 static void print_bt_uuid(struct seq_file *f, u8 *uuid)
 {
-	__be32 data0, data4;
-	__be16 data1, data2, data3, data5;
-
-	memcpy(&data0, &uuid[0], 4);
-	memcpy(&data1, &uuid[4], 2);
-	memcpy(&data2, &uuid[6], 2);
-	memcpy(&data3, &uuid[8], 2);
-	memcpy(&data4, &uuid[10], 4);
-	memcpy(&data5, &uuid[14], 2);
-
-	seq_printf(f, "%.8x-%.4x-%.4x-%.4x-%.8x%.4x\n",
-		   ntohl(data0), ntohs(data1), ntohs(data2), ntohs(data3),
-		   ntohl(data4), ntohs(data5));
+	u32 data0, data5;
+	u16 data1, data2, data3, data4;
+
+	data5 = le32_to_cpu(*(__le32 *)uuid);
+	data4 = le16_to_cpu(*(__le16 *)(uuid + 4));
+	data3 = le16_to_cpu(*(__le16 *)(uuid + 6));
+	data2 = le16_to_cpu(*(__le16 *)(uuid + 8));
+	data1 = le16_to_cpu(*(__le16 *)(uuid + 10));
+	data0 = le32_to_cpu(*(__le32 *)(uuid + 12));
+
+	seq_printf(f, "%.8x-%.4x-%.4x-%.4x-%.4x%.8x\n",
+		   data0, data1, data2, data3, data4, data5);
 }
 
 static int uuids_show(struct seq_file *f, void *p)
-- 
1.8.0.2

[PATCH] Bluetooth: Fix uuid output in debugfs

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

The uuid should be printed in the CPU endianness and not in little-endian.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 net/bluetooth/hci_sysfs.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
index 55cceee..05b78c7 100644
--- a/net/bluetooth/hci_sysfs.c
+++ b/net/bluetooth/hci_sysfs.c
@@ -461,19 +461,18 @@ static const struct file_operations blacklist_fops = {
 
 static void print_bt_uuid(struct seq_file *f, u8 *uuid)
 {
-	__be32 data0, data4;
-	__be16 data1, data2, data3, data5;
-
-	memcpy(&data0, &uuid[0], 4);
-	memcpy(&data1, &uuid[4], 2);
-	memcpy(&data2, &uuid[6], 2);
-	memcpy(&data3, &uuid[8], 2);
-	memcpy(&data4, &uuid[10], 4);
-	memcpy(&data5, &uuid[14], 2);
-
-	seq_printf(f, "%.8x-%.4x-%.4x-%.4x-%.8x%.4x\n",
-		   ntohl(data0), ntohs(data1), ntohs(data2), ntohs(data3),
-		   ntohl(data4), ntohs(data5));
+	u32 data0, data5;
+	u16 data1, data2, data3, data4;
+
+	data5 = le32_to_cpu(*(__le32 *)uuid);
+	data4 = le16_to_cpu(*(__le16 *)(uuid + 4));
+	data3 = le16_to_cpu(*(__le16 *)(uuid + 6));
+	data2 = le16_to_cpu(*(__le16 *)(uuid + 8));
+	data1 = le16_to_cpu(*(__le16 *)(uuid + 10));
+	data0 = le32_to_cpu(*(__le32 *)(uuid + 12));
+
+	seq_printf(f, "%.8x-%.4x-%.4x-%.4x-%.4x%.8x\n",
+		   data0, data1, data2, data3, data4, data5);
 }
 
 static int uuids_show(struct seq_file *f, void *p)
-- 
1.8.0.2

Re: [PATCH] Bluetooth: Fix uuid output in debugfs

[Bjørn Mork]

> diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
> index 55cceee..05b78c7 100644
> --- a/net/bluetooth/hci_sysfs.c
> +++ b/net/bluetooth/hci_sysfs.c
> @@ -461,19 +461,18 @@ static const struct file_operations blacklist_fops = {
>  
>  static void print_bt_uuid(struct seq_file *f, u8 *uuid)
>  {
> -	__be32 data0, data4;
> -	__be16 data1, data2, data3, data5;
> -
> -	memcpy(&data0, &uuid[0], 4);
> -	memcpy(&data1, &uuid[4], 2);
> -	memcpy(&data2, &uuid[6], 2);
> -	memcpy(&data3, &uuid[8], 2);
> -	memcpy(&data4, &uuid[10], 4);
> -	memcpy(&data5, &uuid[14], 2);
> -
> -	seq_printf(f, "%.8x-%.4x-%.4x-%.4x-%.8x%.4x\n",
> -		   ntohl(data0), ntohs(data1), ntohs(data2), ntohs(data3),
> -		   ntohl(data4), ntohs(data5));
> +	u32 data0, data5;
> +	u16 data1, data2, data3, data4;
> +
> +	data5 = le32_to_cpu(*(__le32 *)uuid);
> +	data4 = le16_to_cpu(*(__le16 *)(uuid + 4));
> +	data3 = le16_to_cpu(*(__le16 *)(uuid + 6));
> +	data2 = le16_to_cpu(*(__le16 *)(uuid + 8));
> +	data1 = le16_to_cpu(*(__le16 *)(uuid + 10));
> +	data0 = le32_to_cpu(*(__le32 *)(uuid + 12));
> +
> +	seq_printf(f, "%.8x-%.4x-%.4x-%.4x-%.4x%.8x\n",
> +		   data0, data1, data2, data3, data4, data5);
>  }
>  
>  static int uuids_show(struct seq_file *f, void *p)


Why can't all this be replaced with

static void print_bt_uuid(struct seq_file *f, u8 *uuid)
{
        seq_printf(f, "%pUl\n", uuid);
}

?


Bjørn

Re: [PATCH] Bluetooth: Fix uuid output in debugfs

[Johan Hedberg]

Hi Gustavo,

On Fri, Dec 21, 2012, Gustavo Padovan wrote:
> The uuid should be printed in the CPU endianness and not in little-endian.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  net/bluetooth/hci_sysfs.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
> index 55cceee..05b78c7 100644
> --- a/net/bluetooth/hci_sysfs.c
> +++ b/net/bluetooth/hci_sysfs.c
> @@ -461,19 +461,18 @@ static const struct file_operations blacklist_fops = {
>  
>  static void print_bt_uuid(struct seq_file *f, u8 *uuid)
>  {
> -	__be32 data0, data4;
> -	__be16 data1, data2, data3, data5;
> -
> -	memcpy(&data0, &uuid[0], 4);
> -	memcpy(&data1, &uuid[4], 2);
> -	memcpy(&data2, &uuid[6], 2);
> -	memcpy(&data3, &uuid[8], 2);
> -	memcpy(&data4, &uuid[10], 4);
> -	memcpy(&data5, &uuid[14], 2);
> -
> -	seq_printf(f, "%.8x-%.4x-%.4x-%.4x-%.8x%.4x\n",
> -		   ntohl(data0), ntohs(data1), ntohs(data2), ntohs(data3),
> -		   ntohl(data4), ntohs(data5));
> +	u32 data0, data5;
> +	u16 data1, data2, data3, data4;
> +
> +	data5 = le32_to_cpu(*(__le32 *)uuid);
> +	data4 = le16_to_cpu(*(__le16 *)(uuid + 4));
> +	data3 = le16_to_cpu(*(__le16 *)(uuid + 6));
> +	data2 = le16_to_cpu(*(__le16 *)(uuid + 8));
> +	data1 = le16_to_cpu(*(__le16 *)(uuid + 10));
> +	data0 = le32_to_cpu(*(__le32 *)(uuid + 12));

This looks prone to unaligned access violations if the "u8 *uuid"
pointer doesn't start off with a nicely aligned address. The use of the
unaligned getter macros would also look nicer since you wouldn't have to
do explicit type casting.

Johan

Re: [PATCH] Bluetooth: Fix uuid output in debugfs

[Johan Hedberg]

Hi Bjørn,

On Wed, Jan 02, 2013, Bjørn Mork wrote:
> > diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
> > index 55cceee..05b78c7 100644
> > --- a/net/bluetooth/hci_sysfs.c
> > +++ b/net/bluetooth/hci_sysfs.c
> > @@ -461,19 +461,18 @@ static const struct file_operations blacklist_fops = {
> >  
> >  static void print_bt_uuid(struct seq_file *f, u8 *uuid)
> >  {
> > -	__be32 data0, data4;
> > -	__be16 data1, data2, data3, data5;
> > -
> > -	memcpy(&data0, &uuid[0], 4);
> > -	memcpy(&data1, &uuid[4], 2);
> > -	memcpy(&data2, &uuid[6], 2);
> > -	memcpy(&data3, &uuid[8], 2);
> > -	memcpy(&data4, &uuid[10], 4);
> > -	memcpy(&data5, &uuid[14], 2);
> > -
> > -	seq_printf(f, "%.8x-%.4x-%.4x-%.4x-%.8x%.4x\n",
> > -		   ntohl(data0), ntohs(data1), ntohs(data2), ntohs(data3),
> > -		   ntohl(data4), ntohs(data5));
> > +	u32 data0, data5;
> > +	u16 data1, data2, data3, data4;
> > +
> > +	data5 = le32_to_cpu(*(__le32 *)uuid);
> > +	data4 = le16_to_cpu(*(__le16 *)(uuid + 4));
> > +	data3 = le16_to_cpu(*(__le16 *)(uuid + 6));
> > +	data2 = le16_to_cpu(*(__le16 *)(uuid + 8));
> > +	data1 = le16_to_cpu(*(__le16 *)(uuid + 10));
> > +	data0 = le32_to_cpu(*(__le32 *)(uuid + 12));
> > +
> > +	seq_printf(f, "%.8x-%.4x-%.4x-%.4x-%.4x%.8x\n",
> > +		   data0, data1, data2, data3, data4, data5);
> >  }
> >  
> >  static int uuids_show(struct seq_file *f, void *p)
> 
> 
> Why can't all this be replaced with
> 
> static void print_bt_uuid(struct seq_file *f, u8 *uuid)
> {
>         seq_printf(f, "%pUl\n", uuid);
> }
> 
> ?

I don't think there's any reason assuming that there are no unaligned
access considerations (which I pointed out in my other reply). I wasn't
aware of printk having such a nice extension to the usual format
specifiers (and neither was Gustavo as it seems). Thanks for making us
aware of it!

Johan

Re: [PATCH] Bluetooth: Fix uuid output in debugfs

[Bjørn Mork]

Johan Hedberg <johan.hedberg@gmail.com> writes:

>> Why can't all this be replaced with
>> 
>> static void print_bt_uuid(struct seq_file *f, u8 *uuid)
>> {
>>         seq_printf(f, "%pUl\n", uuid);
>> }
>> 
>> ?
>
> I don't think there's any reason assuming that there are no unaligned
> access considerations (which I pointed out in my other reply). I wasn't
> aware of printk having such a nice extension to the usual format
> specifiers (and neither was Gustavo as it seems). Thanks for making us
> aware of it!

lib/vsprintf.c:uuid_string() will only access "uuid" byte-by-byte so
alignment shouldn't be a problem.


Bjørn

Re: [PATCH] Bluetooth: Fix uuid output in debugfs

[Johan Hedberg]

Hi,

On Wed, Jan 02, 2013, Johan Hedberg wrote:
> > > +	u32 data0, data5;
> > > +	u16 data1, data2, data3, data4;
> > > +
> > > +	data5 = le32_to_cpu(*(__le32 *)uuid);
> > > +	data4 = le16_to_cpu(*(__le16 *)(uuid + 4));
> > > +	data3 = le16_to_cpu(*(__le16 *)(uuid + 6));
> > > +	data2 = le16_to_cpu(*(__le16 *)(uuid + 8));
> > > +	data1 = le16_to_cpu(*(__le16 *)(uuid + 10));
> > > +	data0 = le32_to_cpu(*(__le32 *)(uuid + 12));
> > > +
> > > +	seq_printf(f, "%.8x-%.4x-%.4x-%.4x-%.4x%.8x\n",
> > > +		   data0, data1, data2, data3, data4, data5);
> > >  }
> > >  
> > >  static int uuids_show(struct seq_file *f, void *p)
> > 
> > 
> > Why can't all this be replaced with
> > 
> > static void print_bt_uuid(struct seq_file *f, u8 *uuid)
> > {
> >         seq_printf(f, "%pUl\n", uuid);
> > }
> > 
> > ?
> 
> I don't think there's any reason assuming that there are no unaligned
> access considerations (which I pointed out in my other reply). I wasn't
> aware of printk having such a nice extension to the usual format
> specifiers (and neither was Gustavo as it seems). Thanks for making us
> aware of it!

Actually I'm not getting the expected results with %pU. The following is
the output of seq_printf(f, "%pUl %pUb\n", uuid, uuid):

$ cat /sys/kernel/debug/bluetooth/hci0/uuids
5f9b34fb-0080-8000-0010-00000e110000 fb349b5f-8000-0080-0010-00000e110000
5f9b34fb-0080-8000-0010-00000c110000 fb349b5f-8000-0080-0010-00000c110000
5f9b34fb-0080-8000-0010-000004a00000 fb349b5f-8000-0080-0010-000004a00000
5f9b34fb-0080-8000-0010-000001180000 fb349b5f-8000-0080-0010-000001180000
5f9b34fb-0080-8000-0010-000000180000 fb349b5f-8000-0080-0010-000000180000
5f9b34fb-0080-8000-0010-000000120000 fb349b5f-8000-0080-0010-000000120000

None of those are of the correct format which I do get with Gustavo's
patch:

$ cat /sys/kernel/debug/bluetooth/hci0/uuids
0000110e-0000-1000-8000-00805f9b34fb
0000110c-0000-1000-8000-00805f9b34fb
0000a004-0000-1000-8000-00805f9b34fb
00001801-0000-1000-8000-00805f9b34fb
00001800-0000-1000-8000-00805f9b34fb
00001200-0000-1000-8000-00805f9b34fb

So it seems to me that the %pU specifier is expecting something quite
different from how we store the UUIDs. Looking at the difference of %pUb
and %pUl it seems it doesn't do a complete byte order swap for the whole
value but only for half of it, and even that half doesn't see a proper
64-bit byte order swap but just internal swaps for the one 32-bit and
two 16-bit parts.

So assuming that potential alignment issues get sorted out it seems that
Gustavos patch is the way to go forward.

Johan