* [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric
@ 2017-02-01 2:19 Stefan Agner
2017-02-01 2:19 ` [PATCH 2/2] fs: configfs: use hexadecimal values and new line Stefan Agner
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Stefan Agner @ 2017-02-01 2:19 UTC (permalink / raw)
To: balbi; +Cc: gregkh, andrzej.p, linux-usb, linux-kernel, 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 2/2] fs: configfs: use hexadecimal values and new line
2017-02-01 2:19 [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric Stefan Agner
@ 2017-02-01 2:19 ` Stefan Agner
2017-02-01 8:07 ` Greg KH
2017-02-01 8:06 ` [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric Greg KH
2017-02-01 8:07 ` Greg KH
2 siblings, 1 reply; 17+ messages in thread
From: Stefan Agner @ 2017-02-01 2:19 UTC (permalink / raw)
To: balbi; +Cc: gregkh, andrzej.p, linux-usb, linux-kernel, 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 2/2] fs: configfs: use hexadecimal values and new line
2017-02-01 2:19 ` [PATCH 2/2] fs: configfs: use hexadecimal values and new line Stefan Agner
@ 2017-02-01 8:07 ` Greg KH
2017-02-01 9:02 ` Felipe Balbi
0 siblings, 1 reply; 17+ messages in thread
From: Greg KH @ 2017-02-01 8:07 UTC (permalink / raw)
To: Stefan Agner; +Cc: balbi, andrzej.p, linux-usb, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] fs: configfs: use hexadecimal values and new line
2017-02-01 8:07 ` Greg KH
@ 2017-02-01 9:02 ` Felipe Balbi
2017-02-01 16:51 ` Stefan Agner
0 siblings, 1 reply; 17+ messages in thread
From: Felipe Balbi @ 2017-02-01 9:02 UTC (permalink / raw)
To: Greg KH, Stefan Agner; +Cc: andrzej.p, linux-usb, linux-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] fs: configfs: use hexadecimal values and new line
2017-02-01 9:02 ` Felipe Balbi
@ 2017-02-01 16:51 ` Stefan Agner
0 siblings, 0 replies; 17+ messages in thread
From: Stefan Agner @ 2017-02-01 16:51 UTC (permalink / raw)
To: Felipe Balbi, Greg KH; +Cc: andrzej.p, linux-usb, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric
2017-02-01 2:19 [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric Stefan Agner
2017-02-01 2:19 ` [PATCH 2/2] fs: configfs: use hexadecimal values and new line Stefan Agner
@ 2017-02-01 8:06 ` Greg KH
2017-02-01 16:59 ` Stefan Agner
2017-02-01 8:07 ` Greg KH
2 siblings, 1 reply; 17+ messages in thread
From: Greg KH @ 2017-02-01 8:06 UTC (permalink / raw)
To: Stefan Agner; +Cc: balbi, andrzej.p, linux-usb, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric
2017-02-01 8:06 ` [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric Greg KH
@ 2017-02-01 16:59 ` Stefan Agner
2017-02-09 18:04 ` Stefan Agner
0 siblings, 1 reply; 17+ messages in thread
From: Stefan Agner @ 2017-02-01 16:59 UTC (permalink / raw)
To: Greg KH; +Cc: balbi, andrzej.p, linux-usb, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric
2017-02-01 16:59 ` Stefan Agner
@ 2017-02-09 18:04 ` Stefan Agner
2017-02-10 11:19 ` Greg KH
0 siblings, 1 reply; 17+ messages in thread
From: Stefan Agner @ 2017-02-09 18:04 UTC (permalink / raw)
To: Greg KH; +Cc: balbi, andrzej.p, linux-usb, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric
2017-02-09 18:04 ` Stefan Agner
@ 2017-02-10 11:19 ` Greg KH
2017-02-10 12:30 ` Felipe Balbi
0 siblings, 1 reply; 17+ messages in thread
From: Greg KH @ 2017-02-10 11:19 UTC (permalink / raw)
To: Stefan Agner; +Cc: balbi, andrzej.p, linux-usb, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric
2017-02-10 11:19 ` Greg KH
@ 2017-02-10 12:30 ` Felipe Balbi
2017-02-10 20:29 ` Stefan Agner
0 siblings, 1 reply; 17+ messages in thread
From: Felipe Balbi @ 2017-02-10 12:30 UTC (permalink / raw)
To: Greg KH, Stefan Agner; +Cc: andrzej.p, linux-usb, linux-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric
2017-02-10 12:30 ` Felipe Balbi
@ 2017-02-10 20:29 ` Stefan Agner
2017-02-13 10:46 ` Felipe Balbi
0 siblings, 1 reply; 17+ messages in thread
From: Stefan Agner @ 2017-02-10 20:29 UTC (permalink / raw)
To: Felipe Balbi; +Cc: Greg KH, andrzej.p, linux-usb, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric
2017-02-10 20:29 ` Stefan Agner
@ 2017-02-13 10:46 ` Felipe Balbi
0 siblings, 0 replies; 17+ messages in thread
From: Felipe Balbi @ 2017-02-13 10:46 UTC (permalink / raw)
To: Stefan Agner; +Cc: Greg KH, andrzej.p, linux-usb, linux-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric
2017-02-01 2:19 [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric Stefan Agner
2017-02-01 2:19 ` [PATCH 2/2] fs: configfs: use hexadecimal values and new line Stefan Agner
2017-02-01 8:06 ` [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric Greg KH
@ 2017-02-01 8:07 ` Greg KH
2 siblings, 0 replies; 17+ messages in thread
From: Greg KH @ 2017-02-01 8:07 UTC (permalink / raw)
To: Stefan Agner; +Cc: balbi, andrzej.p, linux-usb, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <CGME20170415013337epcas3p4871e4a671b352fed711acaf38803095f@epcas3p4.samsung.com>]
* [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric
@ 2017-04-15 1:35 ` Stefan Agner
2017-04-19 8:53 ` Krzysztof Opasiak
0 siblings, 1 reply; 17+ messages in thread
From: Stefan Agner @ 2017-04-15 1:35 UTC (permalink / raw)
To: balbi; +Cc: gregkh, k.opasiak, linux-usb, linux-kernel, 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric
2017-04-15 1:35 ` Stefan Agner
@ 2017-04-19 8:53 ` Krzysztof Opasiak
2017-05-25 8:33 ` Stefan Agner
0 siblings, 1 reply; 17+ messages in thread
From: Krzysztof Opasiak @ 2017-04-19 8:53 UTC (permalink / raw)
To: Stefan Agner, balbi; +Cc: gregkh, linux-usb, linux-kernel
On 04/15/2017 03:35 AM, 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.
>
> 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,
>
Code itself looks good to me and from libusbgx perspective it's also
fine to add this new line as we can just drop it like we do with other
newlines in case of gadget/config strings.
Reviewed-by: Krzysztof Opasiak <k.opasiak@samsung.com>
Best regards,
--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric
2017-04-19 8:53 ` Krzysztof Opasiak
@ 2017-05-25 8:33 ` Stefan Agner
2017-06-02 8:25 ` Felipe Balbi
0 siblings, 1 reply; 17+ messages in thread
From: Stefan Agner @ 2017-05-25 8:33 UTC (permalink / raw)
To: balbi; +Cc: Krzysztof Opasiak, gregkh, linux-usb, linux-kernel
Hi Felipe,
On 2017-04-19 01:53, Krzysztof Opasiak wrote:
> On 04/15/2017 03:35 AM, 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.
>>
>> 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,
>>
>
> Code itself looks good to me and from libusbgx perspective it's also
> fine to add this new line as we can just drop it like we do with other
> newlines in case of gadget/config strings.
>
> Reviewed-by: Krzysztof Opasiak <k.opasiak@samsung.com>
>
Any chance we get this in this merge window?
This still applies fine on v4.12-rc2.
--
Stefan
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric
2017-05-25 8:33 ` Stefan Agner
@ 2017-06-02 8:25 ` Felipe Balbi
0 siblings, 0 replies; 17+ messages in thread
From: Felipe Balbi @ 2017-06-02 8:25 UTC (permalink / raw)
To: Stefan Agner; +Cc: Krzysztof Opasiak, gregkh, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2353 bytes --]
Hi,
Stefan Agner <stefan@agner.ch> writes:
> Hi Felipe,
>
> On 2017-04-19 01:53, Krzysztof Opasiak wrote:
>> On 04/15/2017 03:35 AM, 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.
>>>
>>> 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,
>>>
>>
>> Code itself looks good to me and from libusbgx perspective it's also
>> fine to add this new line as we can just drop it like we do with other
>> newlines in case of gadget/config strings.
>>
>> Reviewed-by: Krzysztof Opasiak <k.opasiak@samsung.com>
>>
>
> Any chance we get this in this merge window?
>
> This still applies fine on v4.12-rc2.
I wouldn't consider this a fix. I'll get this into v4.13 merge window.
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2017-06-02 8:26 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-01 2:19 [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric Stefan Agner
2017-02-01 2:19 ` [PATCH 2/2] fs: configfs: use hexadecimal values and new line Stefan Agner
2017-02-01 8:07 ` Greg KH
2017-02-01 9:02 ` Felipe Balbi
2017-02-01 16:51 ` Stefan Agner
2017-02-01 8:06 ` [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric Greg KH
2017-02-01 16:59 ` Stefan Agner
2017-02-09 18:04 ` Stefan Agner
2017-02-10 11:19 ` Greg KH
2017-02-10 12:30 ` Felipe Balbi
2017-02-10 20:29 ` Stefan Agner
2017-02-13 10:46 ` Felipe Balbi
2017-02-01 8:07 ` Greg KH
[not found] <CGME20170415013337epcas3p4871e4a671b352fed711acaf38803095f@epcas3p4.samsung.com>
2017-04-15 1:35 ` Stefan Agner
2017-04-19 8:53 ` Krzysztof Opasiak
2017-05-25 8:33 ` Stefan Agner
2017-06-02 8:25 ` Felipe Balbi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.