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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
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 <[email protected]>
> ---
> 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 <[email protected]>
Best regards,
--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
Hi,
Stefan Agner <[email protected]> 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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
>>
>
> 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