[U-Boot] [PATCH 1/2] usb: composite: fix possible alignment issues

[Simon Goldschmidt]

Am 21.11.2019 um 20:36 schrieb Heinrich Schuchardt:
> On 11/12/19 10:26 PM, Simon Goldschmidt wrote:
>> Since upgrading to gcc9, warnings are issued:
>> "taking address of packed member of ‘...’ may result in an unaligned
>> pointer value"
>>
>> Fix this by converting two functions to use unaligned access since packed
>> structures may be on an unaligned address, depending on USB hardware.
>>
>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
>> ---
>>
>>    drivers/usb/gadget/composite.c | 24 +++++++++++++++++-------
>>    1 file changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>> index 618a7d5016..cfc9512caa 100644
>> --- a/drivers/usb/gadget/composite.c
>> +++ b/drivers/usb/gadget/composite.c
>> @@ -12,8 +12,16 @@
>>
>>    #define USB_BUFSIZ	4096
>>
>> +/* Helper type for accessing packed u16 pointers */
>> +typedef struct { __le16 val; } __packed __le16_packed;
>> +
>>    static struct usb_composite_driver *composite;
>>
>> +static inline void le16_add_cpu_packed(__le16_packed *var, u16 val)
>> +{
>> +	var->val = cpu_to_le16(le16_to_cpu(var->val) + val);
>> +}
>> +
>>    /**
>>     * usb_add_function() - add a function to a configuration
>>     * @config: the configuration
>> @@ -480,20 +488,20 @@ done:
>>     * the host side.
>>     */
>>
>> -static void collect_langs(struct usb_gadget_strings **sp, __le16 *buf)
>> +static void collect_langs(struct usb_gadget_strings **sp, void *buf)
> 
> With this patch and GCC 9.2.1 I get:
> 
> In file included from drivers/usb/gadget/g_dnl.c:24:
> drivers/usb/gadget/composite.c: In function ‘collect_langs’:
> drivers/usb/gadget/composite.c:500:41: error: dereferencing ‘void *’
> pointer [-Werror]
>     500 |   for (tmp = buf; tmp->val && tmp < &buf[126]; tmp++) {
>         |                                         ^
> drivers/usb/gadget/composite.c:500:35: error: comparison of distinct
> pointer types lacks a cast [-Werror]
>     500 |   for (tmp = buf; tmp->val && tmp < &buf[126]; tmp++) {
> 
> void does not have defined size and so cannot be subscripted.

Right, thanks for testing. I hate it that we don't have -Werror enabled, 
as this just slipped through with all the output on the screen.

I'll send v2 fixing that.

Regards,
Simon

> 
> Best regards
> 
> Heinrich
> 
>>    {
>>    	const struct usb_gadget_strings	*s;
>>    	u16				language;
>> -	__le16				*tmp;
>> +	__le16_packed			*tmp;
>>
>>    	while (*sp) {
>>    		s = *sp;
>>    		language = cpu_to_le16(s->language);
>> -		for (tmp = buf; *tmp && tmp < &buf[126]; tmp++) {
>> -			if (*tmp == language)
>> +		for (tmp = buf; tmp->val && tmp < &buf[126]; tmp++) {
>> +			if (tmp->val == language)
>>    				goto repeat;
>>    		}
>> -		*tmp++ = language;
>> +		tmp->val = language;
>>    repeat:
>>    		sp++;
>>    	}
>> @@ -705,7 +713,8 @@ static int bos_desc(struct usb_composite_dev *cdev)
>>    	 */
>>    	usb_ext = cdev->req->buf + le16_to_cpu(bos->wTotalLength);
>>    	bos->bNumDeviceCaps++;
>> -	le16_add_cpu(&bos->wTotalLength, USB_DT_USB_EXT_CAP_SIZE);
>> +	le16_add_cpu_packed((__le16_packed *)&bos->wTotalLength,
>> +			    USB_DT_USB_EXT_CAP_SIZE);
>>    	usb_ext->bLength = USB_DT_USB_EXT_CAP_SIZE;
>>    	usb_ext->bDescriptorType = USB_DT_DEVICE_CAPABILITY;
>>    	usb_ext->bDevCapabilityType = USB_CAP_TYPE_EXT;
>> @@ -721,7 +730,8 @@ static int bos_desc(struct usb_composite_dev *cdev)
>>
>>    		ss_cap = cdev->req->buf + le16_to_cpu(bos->wTotalLength);
>>    		bos->bNumDeviceCaps++;
>> -		le16_add_cpu(&bos->wTotalLength, USB_DT_USB_SS_CAP_SIZE);
>> +		le16_add_cpu_packed((__le16_packed *)&bos->wTotalLength,
>> +				    USB_DT_USB_SS_CAP_SIZE);
>>    		ss_cap->bLength = USB_DT_USB_SS_CAP_SIZE;
>>    		ss_cap->bDescriptorType = USB_DT_DEVICE_CAPABILITY;
>>    		ss_cap->bDevCapabilityType = USB_SS_CAP_TYPE;
>>
>