usb: ehci-fsl: Avoid -Wflex-array-member-not-at-end warning

[PATCH][next] usb: ehci-fsl: Avoid -Wflex-array-member-not-at-end warning

Posted by Gustavo A. R. Silva 8 months, 3 weeks ago

-Wflex-array-member-not-at-end was introduced in GCC-14, and we are
getting ready to enable it, globally.

Move the conflicting declaration to the end of the structure. Notice
that `struct ehci_hcd` is a flexible structure --a structure that
contains a flexible-array member.

Fix the following warning:

drivers/usb/host/ehci-fsl.c:414:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]

Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/usb/host/ehci-fsl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
index 26f13278d4d6..c720d55f4982 100644
--- a/drivers/usb/host/ehci-fsl.c
+++ b/drivers/usb/host/ehci-fsl.c
@@ -411,12 +411,13 @@ static int ehci_fsl_setup(struct usb_hcd *hcd)
 }
 
 struct ehci_fsl {
-	struct ehci_hcd	ehci;
-
 #ifdef CONFIG_PM
 	/* Saved USB PHY settings, need to restore after deep sleep. */
 	u32 usb_ctrl;
 #endif
+
+	/* Must be last --ends in a flexible-array member. */
+	struct ehci_hcd	ehci;
 };
 
 #ifdef CONFIG_PM
-- 
2.43.0

Re: [PATCH][next] usb: ehci-fsl: Avoid -Wflex-array-member-not-at-end warning

Posted by Alan Stern 8 months, 3 weeks ago

On Wed, Mar 26, 2025 at 04:17:41PM -0600, Gustavo A. R. Silva wrote:
> -Wflex-array-member-not-at-end was introduced in GCC-14, and we are
> getting ready to enable it, globally.
> 
> Move the conflicting declaration to the end of the structure. Notice
> that `struct ehci_hcd` is a flexible structure --a structure that
> contains a flexible-array member.
> 
> Fix the following warning:
> 
> drivers/usb/host/ehci-fsl.c:414:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> 
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
>  drivers/usb/host/ehci-fsl.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
> index 26f13278d4d6..c720d55f4982 100644
> --- a/drivers/usb/host/ehci-fsl.c
> +++ b/drivers/usb/host/ehci-fsl.c
> @@ -411,12 +411,13 @@ static int ehci_fsl_setup(struct usb_hcd *hcd)
>  }
>  
>  struct ehci_fsl {
> -	struct ehci_hcd	ehci;
> -
>  #ifdef CONFIG_PM
>  	/* Saved USB PHY settings, need to restore after deep sleep. */
>  	u32 usb_ctrl;
>  #endif
> +
> +	/* Must be last --ends in a flexible-array member. */
> +	struct ehci_hcd	ehci;
>  };
>  
>  #ifdef CONFIG_PM

While the sentiment is laudable, this mechanical change simply will not 
work.  The driver was written incorrectly to begin with, and the change 
will probably break it.

I'll try to find time soon to create a proper fix.  In short, the 
usb_ctrl field should have been stored in the .priv flex member of the 
ehci_hcd structure all along, and the .extra_priv_size member of 
ehci_fsl_overrides should have been set to the size of this u32 field, 
not the size of the entire ehci_fsl structure.

Alan Stern

[PATCH] usb: ehci-fsl: Fix use of private data to avoid -Wflex-array-member-not-at-end warning

Posted by Alan Stern 8 months, 3 weeks ago

In the course of fixing up the usages of flexible arrays, Gustavo
submitted a patch updating the ehci-fsl driver.  However, the patch
was wrong because the driver was using the .priv member of the
ehci_hcd structure incorrectly.  The private data is not supposed to
be a wrapper containing the ehci_hcd structure; it is supposed to be a
sub-structure stored in the .priv member.

Fix the problem by replacing the ehci_fsl structure with
ehci_fsl_priv, containing only the private data, along with a suitable
conversion macro for accessing it.  This removes the problem of having
data follow a flexible array member.

Reported-by: "Gustavo A. R. Silva" <gustavoars@kernel.org>
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Link: https://lore.kernel.org/linux-usb/Z-R9BcnSzrRv5FX_@kspp/

---

 drivers/usb/host/ehci-fsl.c |   25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

Index: usb-devel/drivers/usb/host/ehci-fsl.c
===================================================================
--- usb-devel.orig/drivers/usb/host/ehci-fsl.c
+++ usb-devel/drivers/usb/host/ehci-fsl.c
@@ -410,15 +410,13 @@ static int ehci_fsl_setup(struct usb_hcd
 	return retval;
 }
 
-struct ehci_fsl {
-	struct ehci_hcd	ehci;
-
-#ifdef CONFIG_PM
+struct ehci_fsl_priv {
 	/* Saved USB PHY settings, need to restore after deep sleep. */
 	u32 usb_ctrl;
-#endif
 };
 
+#define hcd_to_ehci_fsl_priv(h) ((struct ehci_fsl_priv *) hcd_to_ehci(h)->priv)
+
 #ifdef CONFIG_PM
 
 #ifdef CONFIG_PPC_MPC512x
@@ -566,17 +564,10 @@ static inline int ehci_fsl_mpc512x_drv_r
 }
 #endif /* CONFIG_PPC_MPC512x */
 
-static struct ehci_fsl *hcd_to_ehci_fsl(struct usb_hcd *hcd)
-{
-	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
-
-	return container_of(ehci, struct ehci_fsl, ehci);
-}
-
 static int ehci_fsl_drv_suspend(struct device *dev)
 {
 	struct usb_hcd *hcd = dev_get_drvdata(dev);
-	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
+	struct ehci_fsl_priv *priv = hcd_to_ehci_fsl_priv(hcd);
 	void __iomem *non_ehci = hcd->regs;
 
 	if (of_device_is_compatible(dev->parent->of_node,
@@ -589,14 +580,14 @@ static int ehci_fsl_drv_suspend(struct d
 	if (!fsl_deep_sleep())
 		return 0;
 
-	ehci_fsl->usb_ctrl = ioread32be(non_ehci + FSL_SOC_USB_CTRL);
+	priv->usb_ctrl = ioread32be(non_ehci + FSL_SOC_USB_CTRL);
 	return 0;
 }
 
 static int ehci_fsl_drv_resume(struct device *dev)
 {
 	struct usb_hcd *hcd = dev_get_drvdata(dev);
-	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
+	struct ehci_fsl_priv *priv = hcd_to_ehci_fsl_priv(hcd);
 	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
 	void __iomem *non_ehci = hcd->regs;
 
@@ -612,7 +603,7 @@ static int ehci_fsl_drv_resume(struct de
 	usb_root_hub_lost_power(hcd->self.root_hub);
 
 	/* Restore USB PHY settings and enable the controller. */
-	iowrite32be(ehci_fsl->usb_ctrl, non_ehci + FSL_SOC_USB_CTRL);
+	iowrite32be(priv->usb_ctrl, non_ehci + FSL_SOC_USB_CTRL);
 
 	ehci_reset(ehci);
 	ehci_fsl_reinit(ehci);
@@ -671,7 +662,7 @@ static int ehci_start_port_reset(struct
 #endif /* CONFIG_USB_OTG */
 
 static const struct ehci_driver_overrides ehci_fsl_overrides __initconst = {
-	.extra_priv_size = sizeof(struct ehci_fsl),
+	.extra_priv_size = sizeof(struct ehci_fsl_priv),
 	.reset = ehci_fsl_setup,
 };

Re: [PATCH] usb: ehci-fsl: Fix use of private data to avoid -Wflex-array-member-not-at-end warning

Posted by Kees Cook 8 months, 1 week ago

On Thu, Mar 27, 2025 at 03:31:15PM -0400, Alan Stern wrote:
> In the course of fixing up the usages of flexible arrays, Gustavo
> submitted a patch updating the ehci-fsl driver.  However, the patch
> was wrong because the driver was using the .priv member of the
> ehci_hcd structure incorrectly.  The private data is not supposed to
> be a wrapper containing the ehci_hcd structure; it is supposed to be a
> sub-structure stored in the .priv member.
> 
> Fix the problem by replacing the ehci_fsl structure with
> ehci_fsl_priv, containing only the private data, along with a suitable
> conversion macro for accessing it.  This removes the problem of having
> data follow a flexible array member.

Thanks for figuring out the right solution! :)

> 
> Reported-by: "Gustavo A. R. Silva" <gustavoars@kernel.org>
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> Link: https://lore.kernel.org/linux-usb/Z-R9BcnSzrRv5FX_@kspp/

Reviewed-by: Kees Cook <kees@kernel.org>

-Kees

> 
> ---
> 
>  drivers/usb/host/ehci-fsl.c |   25 ++++++++-----------------
>  1 file changed, 8 insertions(+), 17 deletions(-)
> 
> Index: usb-devel/drivers/usb/host/ehci-fsl.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/host/ehci-fsl.c
> +++ usb-devel/drivers/usb/host/ehci-fsl.c
> @@ -410,15 +410,13 @@ static int ehci_fsl_setup(struct usb_hcd
>  	return retval;
>  }
>  
> -struct ehci_fsl {
> -	struct ehci_hcd	ehci;
> -
> -#ifdef CONFIG_PM
> +struct ehci_fsl_priv {
>  	/* Saved USB PHY settings, need to restore after deep sleep. */
>  	u32 usb_ctrl;
> -#endif
>  };
>  
> +#define hcd_to_ehci_fsl_priv(h) ((struct ehci_fsl_priv *) hcd_to_ehci(h)->priv)
> +
>  #ifdef CONFIG_PM
>  
>  #ifdef CONFIG_PPC_MPC512x
> @@ -566,17 +564,10 @@ static inline int ehci_fsl_mpc512x_drv_r
>  }
>  #endif /* CONFIG_PPC_MPC512x */
>  
> -static struct ehci_fsl *hcd_to_ehci_fsl(struct usb_hcd *hcd)
> -{
> -	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
> -
> -	return container_of(ehci, struct ehci_fsl, ehci);
> -}
> -
>  static int ehci_fsl_drv_suspend(struct device *dev)
>  {
>  	struct usb_hcd *hcd = dev_get_drvdata(dev);
> -	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
> +	struct ehci_fsl_priv *priv = hcd_to_ehci_fsl_priv(hcd);
>  	void __iomem *non_ehci = hcd->regs;
>  
>  	if (of_device_is_compatible(dev->parent->of_node,
> @@ -589,14 +580,14 @@ static int ehci_fsl_drv_suspend(struct d
>  	if (!fsl_deep_sleep())
>  		return 0;
>  
> -	ehci_fsl->usb_ctrl = ioread32be(non_ehci + FSL_SOC_USB_CTRL);
> +	priv->usb_ctrl = ioread32be(non_ehci + FSL_SOC_USB_CTRL);
>  	return 0;
>  }
>  
>  static int ehci_fsl_drv_resume(struct device *dev)
>  {
>  	struct usb_hcd *hcd = dev_get_drvdata(dev);
> -	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
> +	struct ehci_fsl_priv *priv = hcd_to_ehci_fsl_priv(hcd);
>  	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
>  	void __iomem *non_ehci = hcd->regs;
>  
> @@ -612,7 +603,7 @@ static int ehci_fsl_drv_resume(struct de
>  	usb_root_hub_lost_power(hcd->self.root_hub);
>  
>  	/* Restore USB PHY settings and enable the controller. */
> -	iowrite32be(ehci_fsl->usb_ctrl, non_ehci + FSL_SOC_USB_CTRL);
> +	iowrite32be(priv->usb_ctrl, non_ehci + FSL_SOC_USB_CTRL);
>  
>  	ehci_reset(ehci);
>  	ehci_fsl_reinit(ehci);
> @@ -671,7 +662,7 @@ static int ehci_start_port_reset(struct
>  #endif /* CONFIG_USB_OTG */
>  
>  static const struct ehci_driver_overrides ehci_fsl_overrides __initconst = {
> -	.extra_priv_size = sizeof(struct ehci_fsl),
> +	.extra_priv_size = sizeof(struct ehci_fsl_priv),
>  	.reset = ehci_fsl_setup,
>  };
>  

-- 
Kees Cook