[PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Stefan Agner]

Currently qw_sign requires UTF-8 character to set, but returns UTF-16
when read. This isn't obvious when simply using cat since the null
characters are not visible, but hexdump unveils the true string:

  # echo MSFT100 > os_desc/qw_sign
  # hexdump -C os_desc/qw_sign
  00000000  4d 00 53 00 46 00 54 00  31 00 30 00 30 00        |M.S.F.T.1.0.0.|

Make qw_sign symmetric by returning an UTF-8 string too. Also follow
common convention and add a new line at the end.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/usb/gadget/configfs.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 78c44979dde3..ea1a40f5b23d 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -786,9 +786,13 @@ static ssize_t os_desc_b_vendor_code_store(struct config_item *item,
 static ssize_t os_desc_qw_sign_show(struct config_item *item, char *page)
 {
 	struct gadget_info *gi = os_desc_item_to_gadget_info(item);
+	int res;
 
-	memcpy(page, gi->qw_sign, OS_STRING_QW_SIGN_LEN);
-	return OS_STRING_QW_SIGN_LEN;
+	res = utf16s_to_utf8s((wchar_t *) gi->qw_sign, OS_STRING_QW_SIGN_LEN,
+			      UTF16_LITTLE_ENDIAN, page, PAGE_SIZE - 1);
+	page[res++] = '\n';
+
+	return res;
 }
 
 static ssize_t os_desc_qw_sign_store(struct config_item *item, const char *page,
-- 
2.11.0

[PATCH 2/2] fs: configfs: use hexadecimal values and new line

[Stefan Agner]

Other unsigned properties return hexadecimal values, follow this
convention when printing b_vendor_code too. Also add newlines to
the OS Descriptor support related properties, like other sysfs
files use.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/usb/gadget/configfs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index ea1a40f5b23d..8e9adcfff748 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -737,7 +737,7 @@ static inline struct gadget_info *os_desc_item_to_gadget_info(
 
 static ssize_t os_desc_use_show(struct config_item *item, char *page)
 {
-	return sprintf(page, "%d",
+	return sprintf(page, "%d\n",
 			os_desc_item_to_gadget_info(item)->use_os_desc);
 }
 
@@ -761,7 +761,7 @@ static ssize_t os_desc_use_store(struct config_item *item, const char *page,
 
 static ssize_t os_desc_b_vendor_code_show(struct config_item *item, char *page)
 {
-	return sprintf(page, "%d",
+	return sprintf(page, "0x%02x\n",
 			os_desc_item_to_gadget_info(item)->b_vendor_code);
 }
 
@@ -903,7 +903,7 @@ static inline struct usb_os_desc_ext_prop
 
 static ssize_t ext_prop_type_show(struct config_item *item, char *page)
 {
-	return sprintf(page, "%d", to_usb_os_desc_ext_prop(item)->type);
+	return sprintf(page, "%d\n", to_usb_os_desc_ext_prop(item)->type);
 }
 
 static ssize_t ext_prop_type_store(struct config_item *item,
-- 
2.11.0

Re: [PATCH 2/2] fs: configfs: use hexadecimal values and new line

[Greg KH]

On Tue, Jan 31, 2017 at 06:19:17PM -0800, Stefan Agner wrote:
> Other unsigned properties return hexadecimal values, follow this
> convention when printing b_vendor_code too. Also add newlines to
> the OS Descriptor support related properties, like other sysfs
> files use.

configfs is not sysfs, so watch out, you might not need/want those new
lines as tools read the values and are not expecting them.

Have you verified that nothing will break with this?  How have you
tested it?

thanks,

greg k-h

Re: [PATCH 2/2] fs: configfs: use hexadecimal values and new line

[Felipe Balbi]

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


Hi,

Greg KH <gregkh@linuxfoundation.org> writes:
> On Tue, Jan 31, 2017 at 06:19:17PM -0800, Stefan Agner wrote:
>> Other unsigned properties return hexadecimal values, follow this
>> convention when printing b_vendor_code too. Also add newlines to
>> the OS Descriptor support related properties, like other sysfs
>> files use.
>
> configfs is not sysfs, so watch out, you might not need/want those new
> lines as tools read the values and are not expecting them.
>
> Have you verified that nothing will break with this?  How have you
> tested it?

I would rather not change things like that, specially since Android guys
are alreadying using configfs.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 2/2] fs: configfs: use hexadecimal values and new line

[Stefan Agner]

On 2017-02-01 01:02, Felipe Balbi wrote:
> Hi,
> 
> Greg KH <gregkh@linuxfoundation.org> writes:
>> On Tue, Jan 31, 2017 at 06:19:17PM -0800, Stefan Agner wrote:
>>> Other unsigned properties return hexadecimal values, follow this
>>> convention when printing b_vendor_code too. Also add newlines to
>>> the OS Descriptor support related properties, like other sysfs
>>> files use.
>>
>> configfs is not sysfs, so watch out, you might not need/want those new
>> lines as tools read the values and are not expecting them.
>>
>> Have you verified that nothing will break with this?  How have you
>> tested it?

I am actually writing OS Descriptor support for libusbgx (fork of
libusbg) when I stumbled upon this. The helper function there do not
care about the new line, but I just thought we should fix it for the
sake of coherency to the other properties. I am also fine dropping this
patch.

> 
> I would rather not change things like that, specially since Android guys
> are alreadying using configfs.

Do you happen to know where the source of that part is? So we could
check if they make use of any OS Descriptor support yet...

--
Stefan

Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Greg KH]

On Tue, Jan 31, 2017 at 06:19:16PM -0800, Stefan Agner wrote:
> Currently qw_sign requires UTF-8 character to set, but returns UTF-16
> when read. This isn't obvious when simply using cat since the null
> characters are not visible, but hexdump unveils the true string:
> 
>   # echo MSFT100 > os_desc/qw_sign
>   # hexdump -C os_desc/qw_sign
>   00000000  4d 00 53 00 46 00 54 00  31 00 30 00 30 00        |M.S.F.T.1.0.0.|
> 
> Make qw_sign symmetric by returning an UTF-8 string too. Also follow
> common convention and add a new line at the end.

Doesn't USB require that strings be in UTF-16?  So why have the kernel
convert them?

thanks,

greg k-h

Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Stefan Agner]

On 2017-02-01 00:06, Greg KH wrote:
> On Tue, Jan 31, 2017 at 06:19:16PM -0800, Stefan Agner wrote:
>> Currently qw_sign requires UTF-8 character to set, but returns UTF-16
>> when read. This isn't obvious when simply using cat since the null
>> characters are not visible, but hexdump unveils the true string:
>>
>>   # echo MSFT100 > os_desc/qw_sign
>>   # hexdump -C os_desc/qw_sign
>>   00000000  4d 00 53 00 46 00 54 00  31 00 30 00 30 00        |M.S.F.T.1.0.0.|
>>
>> Make qw_sign symmetric by returning an UTF-8 string too. Also follow
>> common convention and add a new line at the end.
> 
> Doesn't USB require that strings be in UTF-16?  So why have the kernel
> convert them?

That is a discussion we should have had when the write side of this has
been added:

static ssize_t os_desc_qw_sign_store(struct config_item *item, const
char *page,
				     size_t len)
{
	struct gadget_info *gi = os_desc_item_to_gadget_info(item);
	int res, l;

	l = min((int)len, OS_STRING_QW_SIGN_LEN >> 1);
	if (page[l - 1] == '\n')
		--l;

	mutex_lock(&gi->lock);
	res = utf8s_to_utf16s(page, l,
			      UTF16_LITTLE_ENDIAN, (wchar_t *) gi->qw_sign,
			      OS_STRING_QW_SIGN_LEN);
	if (res > 0)
		res = len;
	mutex_unlock(&gi->lock);

	return res;
}


The store function is definitely already in use today, e.g. this script
used for ev3dev:
https://github.com/ev3dev/ev3-systemd/blob/ev3dev-jessie/scripts/ev3-usb.sh

Changing it to UTF-16 would break that script... So changing the store
part is the lesser of two evils.

Regarding new line: Just following what other attributes are doing by
using the GS_STRINGS_R macro.

--
Stefan

Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Stefan Agner]

On 2017-02-01 08:59, Stefan Agner wrote:
> On 2017-02-01 00:06, Greg KH wrote:
>> On Tue, Jan 31, 2017 at 06:19:16PM -0800, Stefan Agner wrote:
>>> Currently qw_sign requires UTF-8 character to set, but returns UTF-16
>>> when read. This isn't obvious when simply using cat since the null
>>> characters are not visible, but hexdump unveils the true string:
>>>
>>>   # echo MSFT100 > os_desc/qw_sign
>>>   # hexdump -C os_desc/qw_sign
>>>   00000000  4d 00 53 00 46 00 54 00  31 00 30 00 30 00        |M.S.F.T.1.0.0.|
>>>
>>> Make qw_sign symmetric by returning an UTF-8 string too. Also follow
>>> common convention and add a new line at the end.
>>
>> Doesn't USB require that strings be in UTF-16?  So why have the kernel
>> convert them?
> 
> That is a discussion we should have had when the write side of this has
> been added:
> 
> static ssize_t os_desc_qw_sign_store(struct config_item *item, const
> char *page,
> 				     size_t len)
> {
> 	struct gadget_info *gi = os_desc_item_to_gadget_info(item);
> 	int res, l;
> 
> 	l = min((int)len, OS_STRING_QW_SIGN_LEN >> 1);
> 	if (page[l - 1] == '\n')
> 		--l;
> 
> 	mutex_lock(&gi->lock);
> 	res = utf8s_to_utf16s(page, l,
> 			      UTF16_LITTLE_ENDIAN, (wchar_t *) gi->qw_sign,
> 			      OS_STRING_QW_SIGN_LEN);
> 	if (res > 0)
> 		res = len;
> 	mutex_unlock(&gi->lock);
> 
> 	return res;
> }
> 
> 
> The store function is definitely already in use today, e.g. this script
> used for ev3dev:
> https://github.com/ev3dev/ev3-systemd/blob/ev3dev-jessie/scripts/ev3-usb.sh
> 
> Changing it to UTF-16 would break that script... So changing the store
> part is the lesser of two evils.
> 
> Regarding new line: Just following what other attributes are doing by
> using the GS_STRINGS_R macro.

Any comment on this? In my opinion especially this first patch really
fixes a bug and should get applied... I can remove the newline if
preferred.

--
Stefan

Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Greg KH]

On Thu, Feb 09, 2017 at 10:04:43AM -0800, Stefan Agner wrote:
> On 2017-02-01 08:59, Stefan Agner wrote:
> > On 2017-02-01 00:06, Greg KH wrote:
> >> On Tue, Jan 31, 2017 at 06:19:16PM -0800, Stefan Agner wrote:
> >>> Currently qw_sign requires UTF-8 character to set, but returns UTF-16
> >>> when read. This isn't obvious when simply using cat since the null
> >>> characters are not visible, but hexdump unveils the true string:
> >>>
> >>>   # echo MSFT100 > os_desc/qw_sign
> >>>   # hexdump -C os_desc/qw_sign
> >>>   00000000  4d 00 53 00 46 00 54 00  31 00 30 00 30 00        |M.S.F.T.1.0.0.|
> >>>
> >>> Make qw_sign symmetric by returning an UTF-8 string too. Also follow
> >>> common convention and add a new line at the end.
> >>
> >> Doesn't USB require that strings be in UTF-16?  So why have the kernel
> >> convert them?
> > 
> > That is a discussion we should have had when the write side of this has
> > been added:
> > 
> > static ssize_t os_desc_qw_sign_store(struct config_item *item, const
> > char *page,
> > 				     size_t len)
> > {
> > 	struct gadget_info *gi = os_desc_item_to_gadget_info(item);
> > 	int res, l;
> > 
> > 	l = min((int)len, OS_STRING_QW_SIGN_LEN >> 1);
> > 	if (page[l - 1] == '\n')
> > 		--l;
> > 
> > 	mutex_lock(&gi->lock);
> > 	res = utf8s_to_utf16s(page, l,
> > 			      UTF16_LITTLE_ENDIAN, (wchar_t *) gi->qw_sign,
> > 			      OS_STRING_QW_SIGN_LEN);
> > 	if (res > 0)
> > 		res = len;
> > 	mutex_unlock(&gi->lock);
> > 
> > 	return res;
> > }
> > 
> > 
> > The store function is definitely already in use today, e.g. this script
> > used for ev3dev:
> > https://github.com/ev3dev/ev3-systemd/blob/ev3dev-jessie/scripts/ev3-usb.sh
> > 
> > Changing it to UTF-16 would break that script... So changing the store
> > part is the lesser of two evils.
> > 
> > Regarding new line: Just following what other attributes are doing by
> > using the GS_STRINGS_R macro.
> 
> Any comment on this? In my opinion especially this first patch really
> fixes a bug and should get applied... I can remove the newline if
> preferred.

It's up to Felipe, give him a chance to catch up on patches...

thanks,

greg k-h

Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Felipe Balbi]

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


Hi,

Greg KH <gregkh@linuxfoundation.org> writes:
> On Thu, Feb 09, 2017 at 10:04:43AM -0800, Stefan Agner wrote:
>> On 2017-02-01 08:59, Stefan Agner wrote:
>> > On 2017-02-01 00:06, Greg KH wrote:
>> >> On Tue, Jan 31, 2017 at 06:19:16PM -0800, Stefan Agner wrote:
>> >>> Currently qw_sign requires UTF-8 character to set, but returns UTF-16
>> >>> when read. This isn't obvious when simply using cat since the null
>> >>> characters are not visible, but hexdump unveils the true string:
>> >>>
>> >>>   # echo MSFT100 > os_desc/qw_sign
>> >>>   # hexdump -C os_desc/qw_sign
>> >>>   00000000  4d 00 53 00 46 00 54 00  31 00 30 00 30 00        |M.S.F.T.1.0.0.|
>> >>>
>> >>> Make qw_sign symmetric by returning an UTF-8 string too. Also follow
>> >>> common convention and add a new line at the end.
>> >>
>> >> Doesn't USB require that strings be in UTF-16?  So why have the kernel
>> >> convert them?
>> > 
>> > That is a discussion we should have had when the write side of this has
>> > been added:
>> > 
>> > static ssize_t os_desc_qw_sign_store(struct config_item *item, const
>> > char *page,
>> > 				     size_t len)
>> > {
>> > 	struct gadget_info *gi = os_desc_item_to_gadget_info(item);
>> > 	int res, l;
>> > 
>> > 	l = min((int)len, OS_STRING_QW_SIGN_LEN >> 1);
>> > 	if (page[l - 1] == '\n')
>> > 		--l;
>> > 
>> > 	mutex_lock(&gi->lock);
>> > 	res = utf8s_to_utf16s(page, l,
>> > 			      UTF16_LITTLE_ENDIAN, (wchar_t *) gi->qw_sign,
>> > 			      OS_STRING_QW_SIGN_LEN);
>> > 	if (res > 0)
>> > 		res = len;
>> > 	mutex_unlock(&gi->lock);
>> > 
>> > 	return res;
>> > }
>> > 
>> > 
>> > The store function is definitely already in use today, e.g. this script
>> > used for ev3dev:
>> > https://github.com/ev3dev/ev3-systemd/blob/ev3dev-jessie/scripts/ev3-usb.sh
>> > 
>> > Changing it to UTF-16 would break that script... So changing the store
>> > part is the lesser of two evils.
>> > 
>> > Regarding new line: Just following what other attributes are doing by
>> > using the GS_STRINGS_R macro.
>> 
>> Any comment on this? In my opinion especially this first patch really
>> fixes a bug and should get applied... I can remove the newline if
>> preferred.
>
> It's up to Felipe, give him a chance to catch up on patches...

I really don't know what to do here :-) Either way have the potential of
breaking userspace. Maybe returning UTF8 as in write is the lesser of
two evils.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Stefan Agner]

On 2017-02-10 04:30, Felipe Balbi wrote:
> Hi,
> 
> Greg KH <gregkh@linuxfoundation.org> writes:
>> On Thu, Feb 09, 2017 at 10:04:43AM -0800, Stefan Agner wrote:
>>> On 2017-02-01 08:59, Stefan Agner wrote:
>>> > On 2017-02-01 00:06, Greg KH wrote:
>>> >> On Tue, Jan 31, 2017 at 06:19:16PM -0800, Stefan Agner wrote:
>>> >>> Currently qw_sign requires UTF-8 character to set, but returns UTF-16
>>> >>> when read. This isn't obvious when simply using cat since the null
>>> >>> characters are not visible, but hexdump unveils the true string:
>>> >>>
>>> >>>   # echo MSFT100 > os_desc/qw_sign
>>> >>>   # hexdump -C os_desc/qw_sign
>>> >>>   00000000  4d 00 53 00 46 00 54 00  31 00 30 00 30 00        |M.S.F.T.1.0.0.|
>>> >>>
>>> >>> Make qw_sign symmetric by returning an UTF-8 string too. Also follow
>>> >>> common convention and add a new line at the end.
>>> >>
>>> >> Doesn't USB require that strings be in UTF-16?  So why have the kernel
>>> >> convert them?
>>> >
>>> > That is a discussion we should have had when the write side of this has
>>> > been added:
>>> >
>>> > static ssize_t os_desc_qw_sign_store(struct config_item *item, const
>>> > char *page,
>>> > 				     size_t len)
>>> > {
>>> > 	struct gadget_info *gi = os_desc_item_to_gadget_info(item);
>>> > 	int res, l;
>>> >
>>> > 	l = min((int)len, OS_STRING_QW_SIGN_LEN >> 1);
>>> > 	if (page[l - 1] == '\n')
>>> > 		--l;
>>> >
>>> > 	mutex_lock(&gi->lock);
>>> > 	res = utf8s_to_utf16s(page, l,
>>> > 			      UTF16_LITTLE_ENDIAN, (wchar_t *) gi->qw_sign,
>>> > 			      OS_STRING_QW_SIGN_LEN);
>>> > 	if (res > 0)
>>> > 		res = len;
>>> > 	mutex_unlock(&gi->lock);
>>> >
>>> > 	return res;
>>> > }
>>> >
>>> >
>>> > The store function is definitely already in use today, e.g. this script
>>> > used for ev3dev:
>>> > https://github.com/ev3dev/ev3-systemd/blob/ev3dev-jessie/scripts/ev3-usb.sh
>>> >
>>> > Changing it to UTF-16 would break that script... So changing the store
>>> > part is the lesser of two evils.
>>> >
>>> > Regarding new line: Just following what other attributes are doing by
>>> > using the GS_STRINGS_R macro.
>>>
>>> Any comment on this? In my opinion especially this first patch really
>>> fixes a bug and should get applied... I can remove the newline if
>>> preferred.
>>
>> It's up to Felipe, give him a chance to catch up on patches...
> 
> I really don't know what to do here :-) Either way have the potential of
> breaking userspace. Maybe returning UTF8 as in write is the lesser of
> two evils.

So far libusbg and libusbgx do not have support for OS Descriptors.

I came across the issue while writing support for libusbgx (not upstream
yet). And it is rather ugly to have to do it on the read side and not to
do on the write side. It would also extend the dependencies of the
library (I implemented a prototype using iconv).

Also reading the old interface with UTF-8 does not crash the library, it
just return only the first character (since the second character is
\0...).

--
Stefan

Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Felipe Balbi]

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


Hi,

Stefan Agner <stefan@agner.ch> writes:
>>> On Thu, Feb 09, 2017 at 10:04:43AM -0800, Stefan Agner wrote:
>>>> On 2017-02-01 08:59, Stefan Agner wrote:
>>>> > On 2017-02-01 00:06, Greg KH wrote:
>>>> >> On Tue, Jan 31, 2017 at 06:19:16PM -0800, Stefan Agner wrote:
>>>> >>> Currently qw_sign requires UTF-8 character to set, but returns UTF-16
>>>> >>> when read. This isn't obvious when simply using cat since the null
>>>> >>> characters are not visible, but hexdump unveils the true string:
>>>> >>>
>>>> >>>   # echo MSFT100 > os_desc/qw_sign
>>>> >>>   # hexdump -C os_desc/qw_sign
>>>> >>>   00000000  4d 00 53 00 46 00 54 00  31 00 30 00 30 00        |M.S.F.T.1.0.0.|
>>>> >>>
>>>> >>> Make qw_sign symmetric by returning an UTF-8 string too. Also follow
>>>> >>> common convention and add a new line at the end.
>>>> >>
>>>> >> Doesn't USB require that strings be in UTF-16?  So why have the kernel
>>>> >> convert them?
>>>> >
>>>> > That is a discussion we should have had when the write side of this has
>>>> > been added:
>>>> >
>>>> > static ssize_t os_desc_qw_sign_store(struct config_item *item, const
>>>> > char *page,
>>>> > 				     size_t len)
>>>> > {
>>>> > 	struct gadget_info *gi = os_desc_item_to_gadget_info(item);
>>>> > 	int res, l;
>>>> >
>>>> > 	l = min((int)len, OS_STRING_QW_SIGN_LEN >> 1);
>>>> > 	if (page[l - 1] == '\n')
>>>> > 		--l;
>>>> >
>>>> > 	mutex_lock(&gi->lock);
>>>> > 	res = utf8s_to_utf16s(page, l,
>>>> > 			      UTF16_LITTLE_ENDIAN, (wchar_t *) gi->qw_sign,
>>>> > 			      OS_STRING_QW_SIGN_LEN);
>>>> > 	if (res > 0)
>>>> > 		res = len;
>>>> > 	mutex_unlock(&gi->lock);
>>>> >
>>>> > 	return res;
>>>> > }
>>>> >
>>>> >
>>>> > The store function is definitely already in use today, e.g. this script
>>>> > used for ev3dev:
>>>> > https://github.com/ev3dev/ev3-systemd/blob/ev3dev-jessie/scripts/ev3-usb.sh
>>>> >
>>>> > Changing it to UTF-16 would break that script... So changing the store
>>>> > part is the lesser of two evils.
>>>> >
>>>> > Regarding new line: Just following what other attributes are doing by
>>>> > using the GS_STRINGS_R macro.
>>>>
>>>> Any comment on this? In my opinion especially this first patch really
>>>> fixes a bug and should get applied... I can remove the newline if
>>>> preferred.
>>>
>>> It's up to Felipe, give him a chance to catch up on patches...
>> 
>> I really don't know what to do here :-) Either way have the potential of
>> breaking userspace. Maybe returning UTF8 as in write is the lesser of
>> two evils.
>
> So far libusbg and libusbgx do not have support for OS Descriptors.
>
> I came across the issue while writing support for libusbgx (not upstream
> yet). And it is rather ugly to have to do it on the read side and not to
> do on the write side. It would also extend the dependencies of the
> library (I implemented a prototype using iconv).
>
> Also reading the old interface with UTF-8 does not crash the library, it
> just return only the first character (since the second character is
> \0...).

Okay, then let's go with your original patch. Can you resend once -rc1
is out?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Greg KH]

On Tue, Jan 31, 2017 at 06:19:16PM -0800, Stefan Agner wrote:
> Currently qw_sign requires UTF-8 character to set, but returns UTF-16
> when read. This isn't obvious when simply using cat since the null
> characters are not visible, but hexdump unveils the true string:
> 
>   # echo MSFT100 > os_desc/qw_sign
>   # hexdump -C os_desc/qw_sign
>   00000000  4d 00 53 00 46 00 54 00  31 00 30 00 30 00        |M.S.F.T.1.0.0.|
> 
> Make qw_sign symmetric by returning an UTF-8 string too. Also follow
> common convention and add a new line at the end.

The newline is not part of the descriptor, so why add it?

thanks,

greg k-h

[PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Stefan Agner]

Currently qw_sign requires UTF-8 character to set, but returns UTF-16
when read. This isn't obvious when simply using cat since the null
characters are not visible, but hexdump unveils the true string:

  # echo MSFT100 > os_desc/qw_sign
  # hexdump -C os_desc/qw_sign
  00000000  4d 00 53 00 46 00 54 00  31 00 30 00 30 00        |M.S.F.T.1.0.0.|

Make qw_sign symmetric by returning an UTF-8 string too. Also follow
common convention and add a new line at the end.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
Resend as discussed here:
https://patchwork.kernel.org/patch/9548869/

Sorry, a bit later than we discussed... Hope still not too late?

--
Stefan

 drivers/usb/gadget/configfs.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index cbff3b02840d..863ca4ded1be 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -787,9 +787,13 @@ static ssize_t os_desc_b_vendor_code_store(struct config_item *item,
 static ssize_t os_desc_qw_sign_show(struct config_item *item, char *page)
 {
 	struct gadget_info *gi = os_desc_item_to_gadget_info(item);
+	int res;
 
-	memcpy(page, gi->qw_sign, OS_STRING_QW_SIGN_LEN);
-	return OS_STRING_QW_SIGN_LEN;
+	res = utf16s_to_utf8s((wchar_t *) gi->qw_sign, OS_STRING_QW_SIGN_LEN,
+			      UTF16_LITTLE_ENDIAN, page, PAGE_SIZE - 1);
+	page[res++] = '\n';
+
+	return res;
 }
 
 static ssize_t os_desc_qw_sign_store(struct config_item *item, const char *page,
-- 
2.12.1

[PATCH 2/2] fs: configfs: use hexadecimal values and new line

[Stefan Agner]

Other unsigned properties return hexadecimal values, follow this
convention when printing b_vendor_code too. Also add newlines to
the OS Descriptor support related properties, like other sysfs
files use.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/usb/gadget/configfs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 863ca4ded1be..a22a892de7b7 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -738,7 +738,7 @@ static inline struct gadget_info *os_desc_item_to_gadget_info(
 
 static ssize_t os_desc_use_show(struct config_item *item, char *page)
 {
-	return sprintf(page, "%d",
+	return sprintf(page, "%d\n",
 			os_desc_item_to_gadget_info(item)->use_os_desc);
 }
 
@@ -762,7 +762,7 @@ static ssize_t os_desc_use_store(struct config_item *item, const char *page,
 
 static ssize_t os_desc_b_vendor_code_show(struct config_item *item, char *page)
 {
-	return sprintf(page, "%d",
+	return sprintf(page, "0x%02x\n",
 			os_desc_item_to_gadget_info(item)->b_vendor_code);
 }
 
@@ -904,7 +904,7 @@ static inline struct usb_os_desc_ext_prop
 
 static ssize_t ext_prop_type_show(struct config_item *item, char *page)
 {
-	return sprintf(page, "%d", to_usb_os_desc_ext_prop(item)->type);
+	return sprintf(page, "%d\n", to_usb_os_desc_ext_prop(item)->type);
 }
 
 static ssize_t ext_prop_type_store(struct config_item *item,
-- 
2.12.1

Re: [PATCH 2/2] fs: configfs: use hexadecimal values and new line

[Krzysztof Opasiak]

On 04/15/2017 03:35 AM, Stefan Agner wrote:
> Other unsigned properties return hexadecimal values, follow this
> convention when printing b_vendor_code too. Also add newlines to
> the OS Descriptor support related properties, like other sysfs
> files use.
>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  drivers/usb/gadget/configfs.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> index 863ca4ded1be..a22a892de7b7 100644
> --- a/drivers/usb/gadget/configfs.c
> +++ b/drivers/usb/gadget/configfs.c
> @@ -738,7 +738,7 @@ static inline struct gadget_info *os_desc_item_to_gadget_info(
>
>  static ssize_t os_desc_use_show(struct config_item *item, char *page)
>  {
> -	return sprintf(page, "%d",
> +	return sprintf(page, "%d\n",
>  			os_desc_item_to_gadget_info(item)->use_os_desc);
>  }
>
> @@ -762,7 +762,7 @@ static ssize_t os_desc_use_store(struct config_item *item, const char *page,
>
>  static ssize_t os_desc_b_vendor_code_show(struct config_item *item, char *page)
>  {
> -	return sprintf(page, "%d",
> +	return sprintf(page, "0x%02x\n",
>  			os_desc_item_to_gadget_info(item)->b_vendor_code);
>  }
>
> @@ -904,7 +904,7 @@ static inline struct usb_os_desc_ext_prop
>
>  static ssize_t ext_prop_type_show(struct config_item *item, char *page)
>  {
> -	return sprintf(page, "%d", to_usb_os_desc_ext_prop(item)->type);
> +	return sprintf(page, "%d\n", to_usb_os_desc_ext_prop(item)->type);
>  }
>
>  static ssize_t ext_prop_type_store(struct config_item *item,
>
looks good to me:

Reviewed-by: Krzysztof Opasiak <k.opasiak@samsung.com>
-- 
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics