From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753186AbdBJLay (ORCPT ); Fri, 10 Feb 2017 06:30:54 -0500 Received: from mail.linuxfoundation.org ([140.211.169.12]:49660 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752088AbdBJLaw (ORCPT ); Fri, 10 Feb 2017 06:30:52 -0500 Date: Fri, 10 Feb 2017 12:19:29 +0100 From: Greg KH To: Stefan Agner Cc: balbi@kernel.org, andrzej.p@samsung.com, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric Message-ID: <20170210111929.GA22347@kroah.com> References: <20170201021917.27398-1-stefan@agner.ch> <20170201080616.GA17968@kroah.com> <32e2fa73fdbd8b52abe442365fd1c514@agner.ch> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <32e2fa73fdbd8b52abe442365fd1c514@agner.ch> User-Agent: Mutt/1.7.2 (2016-11-26) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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