Building ehci drivers with gcc-10 results in a number of warnings like
when an zero-length array is accessed:
drivers/usb/host/ehci-hub.c: In function 'ehci_bus_suspend':
drivers/usb/host/ehci-hub.c:320:30: error: array subscript 14 is outside the bounds of an interior zero-length array 'u32[0]' {aka 'unsigned int[0]'} [-Werror=zero-length-bounds]
320 | u32 __iomem *hostpc_reg = &ehci->regs->hostpc[port];
| ^~~~~~~~~~~~~~~~~~~~~~~~~
In file included from drivers/usb/host/ehci.h:273,
from drivers/usb/host/ehci-hcd.c:96:
include/linux/usb/ehci_def.h:186:7: note: while referencing 'hostpc'
186 | u32 hostpc[0]; /* HOSTPC extension */
| ^~~~~~
In file included from drivers/usb/host/ehci-hcd.c:305:
drivers/usb/host/ehci-hub.c: In function 'ehci_hub_control':
drivers/usb/host/ehci-hub.c:892:15: error: array subscript 256 is outside the bounds of an interior zero-length array 'u32[0]' {aka 'unsigned int[0]'} [-Werror=zero-length-bounds]
892 | hostpc_reg = &ehci->regs->hostpc[temp];
| ^~~~~~~~~~~~~~~~~~~~~~~~~
In file included from drivers/usb/host/ehci.h:273,
from drivers/usb/host/ehci-hcd.c:96:
include/linux/usb/ehci_def.h:186:7: note: while referencing 'hostpc'
186 | u32 hostpc[0]; /* HOSTPC extension */
| ^~~~~~
All these fields are colocated with reserved fields that I guess
refer to the correct field length.
Change the two struct definition to use an unnamed union to define
both of these fields at the same location as the corresponding
reserved fields.
Signed-off-by: Arnd Bergmann <[email protected]>
---
include/linux/usb/ehci_def.h | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/include/linux/usb/ehci_def.h b/include/linux/usb/ehci_def.h
index 78e006355557..8777d8e56ef2 100644
--- a/include/linux/usb/ehci_def.h
+++ b/include/linux/usb/ehci_def.h
@@ -127,7 +127,8 @@ struct ehci_regs {
#define FLAG_CF (1<<0) /* true: we'll support "high speed" */
/* PORTSC: offset 0x44 */
- u32 port_status[0]; /* up to N_PORTS */
+ union {
+ u32 port_status[9]; /* up to N_PORTS */
/* EHCI 1.1 addendum */
#define PORTSC_SUSPEND_STS_ACK 0
#define PORTSC_SUSPEND_STS_NYET 1
@@ -165,7 +166,8 @@ struct ehci_regs {
#define PORT_CONNECT (1<<0) /* device connected */
#define PORT_RWC_BITS (PORT_CSC | PORT_PEC | PORT_OCC)
- u32 reserved3[9];
+ u32 reserved3[9];
+ };
/* USBMODE: offset 0x68 */
u32 usbmode; /* USB Device mode */
@@ -181,11 +183,13 @@ struct ehci_regs {
* PORTSCx
*/
/* HOSTPC: offset 0x84 */
- u32 hostpc[0]; /* HOSTPC extension */
+ union {
+ u32 hostpc[17]; /* HOSTPC extension */
#define HOSTPC_PHCD (1<<22) /* Phy clock disable */
#define HOSTPC_PSPD (3<<25) /* Port speed detection */
- u32 reserved5[17];
+ u32 reserved5[17];
+ };
/* USBMODE_EX: offset 0xc8 */
u32 usbmode_ex; /* USB Device mode extension */
--
2.26.0
On Thu, 30 Apr 2020, Arnd Bergmann wrote:
> Building ehci drivers with gcc-10 results in a number of warnings like
> when an zero-length array is accessed:
>
> drivers/usb/host/ehci-hub.c: In function 'ehci_bus_suspend':
> drivers/usb/host/ehci-hub.c:320:30: error: array subscript 14 is outside the bounds of an interior zero-length array 'u32[0]' {aka 'unsigned int[0]'} [-Werror=zero-length-bounds]
> 320 | u32 __iomem *hostpc_reg = &ehci->regs->hostpc[port];
> | ^~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from drivers/usb/host/ehci.h:273,
> from drivers/usb/host/ehci-hcd.c:96:
> include/linux/usb/ehci_def.h:186:7: note: while referencing 'hostpc'
> 186 | u32 hostpc[0]; /* HOSTPC extension */
> | ^~~~~~
> In file included from drivers/usb/host/ehci-hcd.c:305:
> drivers/usb/host/ehci-hub.c: In function 'ehci_hub_control':
> drivers/usb/host/ehci-hub.c:892:15: error: array subscript 256 is outside the bounds of an interior zero-length array 'u32[0]' {aka 'unsigned int[0]'} [-Werror=zero-length-bounds]
> 892 | hostpc_reg = &ehci->regs->hostpc[temp];
> | ^~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from drivers/usb/host/ehci.h:273,
> from drivers/usb/host/ehci-hcd.c:96:
> include/linux/usb/ehci_def.h:186:7: note: while referencing 'hostpc'
> 186 | u32 hostpc[0]; /* HOSTPC extension */
> | ^~~~~~
>
> All these fields are colocated with reserved fields that I guess
> refer to the correct field length.
No, they don't.
> Change the two struct definition to use an unnamed union to define
> both of these fields at the same location as the corresponding
> reserved fields.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> include/linux/usb/ehci_def.h | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/usb/ehci_def.h b/include/linux/usb/ehci_def.h
> index 78e006355557..8777d8e56ef2 100644
> --- a/include/linux/usb/ehci_def.h
> +++ b/include/linux/usb/ehci_def.h
> @@ -127,7 +127,8 @@ struct ehci_regs {
> #define FLAG_CF (1<<0) /* true: we'll support "high speed" */
>
> /* PORTSC: offset 0x44 */
> - u32 port_status[0]; /* up to N_PORTS */
> + union {
> + u32 port_status[9]; /* up to N_PORTS */
This array can have up to 15 elements, meaning that it can extend out
to offset 0x80.
> /* EHCI 1.1 addendum */
> #define PORTSC_SUSPEND_STS_ACK 0
> #define PORTSC_SUSPEND_STS_NYET 1
> @@ -165,7 +166,8 @@ struct ehci_regs {
> #define PORT_CONNECT (1<<0) /* device connected */
> #define PORT_RWC_BITS (PORT_CSC | PORT_PEC | PORT_OCC)
>
> - u32 reserved3[9];
> + u32 reserved3[9];
> + };
>
> /* USBMODE: offset 0x68 */
> u32 usbmode; /* USB Device mode */
As you see, this next field actually lies inside the preceding array.
It's not a real conflict; any hardware which supports the usbmode field
uses only the first element of the port_status array.
I don't know how you want to handle this. Doing
#define usbmode port_status[9]
doesn't seem like a very good approach, but I can't think of anything
better at the moment. Maybe just set the array size to 9, as you did,
but with a comment explaining what's really going on.
> @@ -181,11 +183,13 @@ struct ehci_regs {
> * PORTSCx
> */
> /* HOSTPC: offset 0x84 */
> - u32 hostpc[0]; /* HOSTPC extension */
> + union {
> + u32 hostpc[17]; /* HOSTPC extension */
Likewise, this array can have up to 15 elements. In fact, it's the
same size as the port_status array.
> #define HOSTPC_PHCD (1<<22) /* Phy clock disable */
> #define HOSTPC_PSPD (3<<25) /* Port speed detection */
>
> - u32 reserved5[17];
> + u32 reserved5[17];
> + };
>
> /* USBMODE_EX: offset 0xc8 */
> u32 usbmode_ex; /* USB Device mode extension */
Alan Stern
On Fri, May 1, 2020 at 4:42 AM Alan Stern <[email protected]> wrote:
> On Thu, 30 Apr 2020, Arnd Bergmann wrote:
>
> No, they don't.
> > /* PORTSC: offset 0x44 */
> > - u32 port_status[0]; /* up to N_PORTS */
> > + union {
> > + u32 port_status[9]; /* up to N_PORTS */
>
> This array can have up to 15 elements, meaning that it can extend out
> to offset 0x80.
Ok, thanks for the clarification!
> > /* EHCI 1.1 addendum */
> > #define PORTSC_SUSPEND_STS_ACK 0
> > #define PORTSC_SUSPEND_STS_NYET 1
> > @@ -165,7 +166,8 @@ struct ehci_regs {
> > #define PORT_CONNECT (1<<0) /* device connected */
> > #define PORT_RWC_BITS (PORT_CSC | PORT_PEC | PORT_OCC)
> >
> > - u32 reserved3[9];
> > + u32 reserved3[9];
> > + };
> >
> > /* USBMODE: offset 0x68 */
> > u32 usbmode; /* USB Device mode */
>
> As you see, this next field actually lies inside the preceding array.
> It's not a real conflict; any hardware which supports the usbmode field
> uses only the first element of the port_status array.
>
> I don't know how you want to handle this. Doing
>
> #define usbmode port_status[9]
>
> doesn't seem like a very good approach, but I can't think of anything
> better at the moment. Maybe just set the array size to 9, as you did,
> but with a comment explaining what's really going on.
The easiest change would be to use an anonymous struct like this
/* PORTSC: offset 0x44 */
union {
u32 port_status[15]; /* up to N_PORTS */
/* EHCI 1.1 addendum */
#define PORTSC_SUSPEND_STS_ACK 0
...
#define PORT_RWC_BITS (PORT_CSC | PORT_PEC | PORT_OCC)
struct {
u32 reserved3[9];
/* USBMODE: offset 0x68 */
u32 usbmode; /* USB Device mode */
#define USBMODE_SDIS (1<<3) /* Stream disable */
#define USBMODE_BE (1<<2) /* BE/LE endianness select */
#define USBMODE_CM_HC (3<<0) /* host controller mode */
#define USBMODE_CM_IDLE (0<<0) /* idle state */
u32 reserved4[5];
};
};
u32 reserved5;
It doesn't really improve readability, but it should correctly
reflect the layout as you described it.
Arnd
On Fri, 1 May 2020, Arnd Bergmann wrote:
> On Fri, May 1, 2020 at 4:42 AM Alan Stern <[email protected]> wrote:
> > On Thu, 30 Apr 2020, Arnd Bergmann wrote:
>
> >
> > No, they don't.
>
> > > /* PORTSC: offset 0x44 */
> > > - u32 port_status[0]; /* up to N_PORTS */
> > > + union {
> > > + u32 port_status[9]; /* up to N_PORTS */
> >
> > This array can have up to 15 elements, meaning that it can extend out
> > to offset 0x80.
>
> Ok, thanks for the clarification!
>
> > > /* EHCI 1.1 addendum */
> > > #define PORTSC_SUSPEND_STS_ACK 0
> > > #define PORTSC_SUSPEND_STS_NYET 1
> > > @@ -165,7 +166,8 @@ struct ehci_regs {
> > > #define PORT_CONNECT (1<<0) /* device connected */
> > > #define PORT_RWC_BITS (PORT_CSC | PORT_PEC | PORT_OCC)
> > >
> > > - u32 reserved3[9];
> > > + u32 reserved3[9];
> > > + };
> > >
> > > /* USBMODE: offset 0x68 */
> > > u32 usbmode; /* USB Device mode */
> >
> > As you see, this next field actually lies inside the preceding array.
> > It's not a real conflict; any hardware which supports the usbmode field
> > uses only the first element of the port_status array.
> >
> > I don't know how you want to handle this. Doing
> >
> > #define usbmode port_status[9]
> >
> > doesn't seem like a very good approach, but I can't think of anything
> > better at the moment. Maybe just set the array size to 9, as you did,
> > but with a comment explaining what's really going on.
>
> The easiest change would be to use an anonymous struct like this
>
> /* PORTSC: offset 0x44 */
> union {
> u32 port_status[15]; /* up to N_PORTS */
> /* EHCI 1.1 addendum */
> #define PORTSC_SUSPEND_STS_ACK 0
> ...
> #define PORT_RWC_BITS (PORT_CSC | PORT_PEC | PORT_OCC)
> struct {
> u32 reserved3[9];
>
> /* USBMODE: offset 0x68 */
> u32 usbmode; /* USB Device mode */
> #define USBMODE_SDIS (1<<3) /* Stream disable */
> #define USBMODE_BE (1<<2) /* BE/LE endianness select */
> #define USBMODE_CM_HC (3<<0) /* host controller mode */
> #define USBMODE_CM_IDLE (0<<0) /* idle state */
>
> u32 reserved4[5];
> };
> };
> u32 reserved5;
>
> It doesn't really improve readability, but it should correctly
> reflect the layout as you described it.
Sounds good. Please go ahead and update the patch.
Alan Stern