From: Gustavo Padovan <[email protected]>
The uuid should be printed in the CPU endianness and not in little-endian.
Signed-off-by: Gustavo Padovan <[email protected]>
---
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
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
Johan Hedberg <[email protected]> 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
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
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 <[email protected]>
> ---
> 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