| Message ID | 20200107052502.803-1-sw0312.kim@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND] usbtty: fix possible alignment issues | expand |
On Tue, Jan 07, 2020 at 02:25:02PM +0900, Seung-Woo Kim wrote: > With gcc9, below warnings are shown: > drivers/serial/usbtty.c: In function 'usbtty_init_strings': > drivers/serial/usbtty.c:590:44: warning: taking address of packed member of 'struct usb_string_descriptor' may result in an unaligned pointer value [-Waddress-of-packed-member] > 590 | str2wide (CONFIG_USBD_MANUFACTURER, string->wData); > | ~~~~~~^~~~~~~ > drivers/serial/usbtty.c:596:44: warning: taking address of packed member of 'struct usb_string_descriptor' may result in an unaligned pointer value [-Waddress-of-packed-member] > 597 | str2wide (CONFIG_USBD_PRODUCT_NAME, string->wData); > | ~~~~~~^~~~~~~ > drivers/serial/usbtty.c:603:33: warning: taking address of packed member of 'struct usb_string_descriptor' may result in an unaligned pointer value [-Waddress-of-packed-member] > 604 | str2wide (serial_number, string->wData); > | ~~~~~~^~~~~~~ > drivers/serial/usbtty.c:610:49: warning: taking address of packed member of 'struct usb_string_descriptor' may result in an unaligned pointer value [-Waddress-of-packed-member] > 611 | str2wide (CONFIG_USBD_CONFIGURATION_STR, string->wData); > | ~~~~~~^~~~~~~ > drivers/serial/usbtty.c:617:50: warning: taking address of packed member of 'struct usb_string_descriptor' may result in an unaligned pointer value [-Waddress-of-packed-member] > 618 | str2wide (CONFIG_USBD_DATA_INTERFACE_STR, string->wData); > | ~~~~~~^~~~~~~ > drivers/serial/usbtty.c:623:50: warning: taking address of packed member of 'struct usb_string_descriptor' may result in an unaligned pointer value [-Waddress-of-packed-member] > 624 | str2wide (CONFIG_USBD_CTRL_INTERFACE_STR, string->wData); > | ~~~~~~^~~~~~~ > > Fix the issues by using packed structure. > > Ref: commit 616ebd8b9cb4 ("usb: composite: fix possible alignment issues") > Signed-off-by: Seung-Woo Kim <sw0312.kim at samsung.com> > --- > resend with proper e-mail address > --- > drivers/serial/usbtty.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/serial/usbtty.c b/drivers/serial/usbtty.c > index f1c1a260da..54e67dd0d1 100644 > --- a/drivers/serial/usbtty.c > +++ b/drivers/serial/usbtty.c > @@ -48,6 +48,8 @@ > #define CONFIG_USBD_DATA_INTERFACE_STR "Bulk Data Interface" > #define CONFIG_USBD_CTRL_INTERFACE_STR "Control Interface" > > +typedef struct { __le16 val; } __attribute__((aligned(16))) __le16_packed; > + > /* > * Buffers to hold input and output data > */ > @@ -372,14 +374,15 @@ static int fill_buffer (circbuf_t * buf); > void usbtty_poll (void); > > /* utility function for converting char* to wide string used by USB */ > -static void str2wide (char *str, u16 * wide) > +static void str2wide (char *str, void *wide) > { > int i; > + __le16_packed *tmp = wide; > for (i = 0; i < strlen (str) && str[i]; i++){ > #if defined(__LITTLE_ENDIAN) > - wide[i] = (u16) str[i]; > + tmp[i].val = (u16) str[i]; > #elif defined(__BIG_ENDIAN) > - wide[i] = ((u16)(str[i])<<8); > + tmp[i].val = ((u16)(str[i])<<8); > #else > #error "__LITTLE_ENDIAN or __BIG_ENDIAN undefined" > #endif We generally don't want packed structs and do need to disable this warning in general. Is this _really_ a problem, or are we just silencing the compiler here? Thanks!
Hi, On 2020년 01월 07일 22:22, Tom Rini wrote: > On Tue, Jan 07, 2020 at 02:25:02PM +0900, Seung-Woo Kim wrote: > ... >> --- >> drivers/serial/usbtty.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/serial/usbtty.c b/drivers/serial/usbtty.c >> index f1c1a260da..54e67dd0d1 100644 >> --- a/drivers/serial/usbtty.c >> +++ b/drivers/serial/usbtty.c >> @@ -48,6 +48,8 @@ >> #define CONFIG_USBD_DATA_INTERFACE_STR "Bulk Data Interface" >> #define CONFIG_USBD_CTRL_INTERFACE_STR "Control Interface" >> >> +typedef struct { __le16 val; } __attribute__((aligned(16))) __le16_packed; >> + >> /* >> * Buffers to hold input and output data >> */ >> @@ -372,14 +374,15 @@ static int fill_buffer (circbuf_t * buf); >> void usbtty_poll (void); >> >> /* utility function for converting char* to wide string used by USB */ >> -static void str2wide (char *str, u16 * wide) >> +static void str2wide (char *str, void *wide) >> { >> int i; >> + __le16_packed *tmp = wide; >> for (i = 0; i < strlen (str) && str[i]; i++){ >> #if defined(__LITTLE_ENDIAN) >> - wide[i] = (u16) str[i]; >> + tmp[i].val = (u16) str[i]; >> #elif defined(__BIG_ENDIAN) >> - wide[i] = ((u16)(str[i])<<8); >> + tmp[i].val = ((u16)(str[i])<<8); >> #else >> #error "__LITTLE_ENDIAN or __BIG_ENDIAN undefined" >> #endif > > We generally don't want packed structs and do need to disable this > warning in general. Is this _really_ a problem, or are we just > silencing the compiler here? Thanks! The change makes just silence for the warnings as like composite.c in usb gadget. Regards, - Seung-Woo Kim
On Wed, Jan 08, 2020 at 09:32:03AM +0900, Seung-Woo Kim wrote: > Hi, > > On 2020년 01월 07일 22:22, Tom Rini wrote: > > On Tue, Jan 07, 2020 at 02:25:02PM +0900, Seung-Woo Kim wrote: > > > ... > >> --- > >> drivers/serial/usbtty.c | 9 ++++++--- > >> 1 file changed, 6 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/serial/usbtty.c b/drivers/serial/usbtty.c > >> index f1c1a260da..54e67dd0d1 100644 > >> --- a/drivers/serial/usbtty.c > >> +++ b/drivers/serial/usbtty.c > >> @@ -48,6 +48,8 @@ > >> #define CONFIG_USBD_DATA_INTERFACE_STR "Bulk Data Interface" > >> #define CONFIG_USBD_CTRL_INTERFACE_STR "Control Interface" > >> > >> +typedef struct { __le16 val; } __attribute__((aligned(16))) __le16_packed; > >> + > >> /* > >> * Buffers to hold input and output data > >> */ > >> @@ -372,14 +374,15 @@ static int fill_buffer (circbuf_t * buf); > >> void usbtty_poll (void); > >> > >> /* utility function for converting char* to wide string used by USB */ > >> -static void str2wide (char *str, u16 * wide) > >> +static void str2wide (char *str, void *wide) > >> { > >> int i; > >> + __le16_packed *tmp = wide; > >> for (i = 0; i < strlen (str) && str[i]; i++){ > >> #if defined(__LITTLE_ENDIAN) > >> - wide[i] = (u16) str[i]; > >> + tmp[i].val = (u16) str[i]; > >> #elif defined(__BIG_ENDIAN) > >> - wide[i] = ((u16)(str[i])<<8); > >> + tmp[i].val = ((u16)(str[i])<<8); > >> #else > >> #error "__LITTLE_ENDIAN or __BIG_ENDIAN undefined" > >> #endif > > > > We generally don't want packed structs and do need to disable this > > warning in general. Is this _really_ a problem, or are we just > > silencing the compiler here? Thanks! > > The change makes just silence for the warnings as like composite.c in > usb gadget. OK, thanks. NAK on this and I will pick up the general change to disable this warning soon.
diff --git a/drivers/serial/usbtty.c b/drivers/serial/usbtty.c index f1c1a260da..54e67dd0d1 100644 --- a/drivers/serial/usbtty.c +++ b/drivers/serial/usbtty.c @@ -48,6 +48,8 @@ #define CONFIG_USBD_DATA_INTERFACE_STR "Bulk Data Interface" #define CONFIG_USBD_CTRL_INTERFACE_STR "Control Interface" +typedef struct { __le16 val; } __attribute__((aligned(16))) __le16_packed; + /* * Buffers to hold input and output data */ @@ -372,14 +374,15 @@ static int fill_buffer (circbuf_t * buf); void usbtty_poll (void); /* utility function for converting char* to wide string used by USB */ -static void str2wide (char *str, u16 * wide) +static void str2wide (char *str, void *wide) { int i; + __le16_packed *tmp = wide; for (i = 0; i < strlen (str) && str[i]; i++){ #if defined(__LITTLE_ENDIAN) - wide[i] = (u16) str[i]; + tmp[i].val = (u16) str[i]; #elif defined(__BIG_ENDIAN) - wide[i] = ((u16)(str[i])<<8); + tmp[i].val = ((u16)(str[i])<<8); #else #error "__LITTLE_ENDIAN or __BIG_ENDIAN undefined" #endif
With gcc9, below warnings are shown: drivers/serial/usbtty.c: In function 'usbtty_init_strings': drivers/serial/usbtty.c:590:44: warning: taking address of packed member of 'struct usb_string_descriptor' may result in an unaligned pointer value [-Waddress-of-packed-member] 590 | str2wide (CONFIG_USBD_MANUFACTURER, string->wData); | ~~~~~~^~~~~~~ drivers/serial/usbtty.c:596:44: warning: taking address of packed member of 'struct usb_string_descriptor' may result in an unaligned pointer value [-Waddress-of-packed-member] 597 | str2wide (CONFIG_USBD_PRODUCT_NAME, string->wData); | ~~~~~~^~~~~~~ drivers/serial/usbtty.c:603:33: warning: taking address of packed member of 'struct usb_string_descriptor' may result in an unaligned pointer value [-Waddress-of-packed-member] 604 | str2wide (serial_number, string->wData); | ~~~~~~^~~~~~~ drivers/serial/usbtty.c:610:49: warning: taking address of packed member of 'struct usb_string_descriptor' may result in an unaligned pointer value [-Waddress-of-packed-member] 611 | str2wide (CONFIG_USBD_CONFIGURATION_STR, string->wData); | ~~~~~~^~~~~~~ drivers/serial/usbtty.c:617:50: warning: taking address of packed member of 'struct usb_string_descriptor' may result in an unaligned pointer value [-Waddress-of-packed-member] 618 | str2wide (CONFIG_USBD_DATA_INTERFACE_STR, string->wData); | ~~~~~~^~~~~~~ drivers/serial/usbtty.c:623:50: warning: taking address of packed member of 'struct usb_string_descriptor' may result in an unaligned pointer value [-Waddress-of-packed-member] 624 | str2wide (CONFIG_USBD_CTRL_INTERFACE_STR, string->wData); | ~~~~~~^~~~~~~ Fix the issues by using packed structure. Ref: commit 616ebd8b9cb4 ("usb: composite: fix possible alignment issues") Signed-off-by: Seung-Woo Kim <sw0312.kim at samsung.com> --- resend with proper e-mail address --- drivers/serial/usbtty.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)