usb: bdc: allocate phys with main struct

[PATCH] usb: bdc: allocate phys with main struct

Posted by Rosen Penev 2 months ago

Use a flexible array member to combine allocations and simplify code
slightly. No need for a branch deciding whether to allocate or not.

Add __counted_by for extra runtime analysis.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/usb/gadget/udc/bdc/bdc.h      |  2 +-
 drivers/usb/gadget/udc/bdc/bdc_core.c | 20 +++++++-------------
 2 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/gadget/udc/bdc/bdc.h b/drivers/usb/gadget/udc/bdc/bdc.h
index 2f4abf6f8f77..cc961161eb46 100644
--- a/drivers/usb/gadget/udc/bdc/bdc.h
+++ b/drivers/usb/gadget/udc/bdc/bdc.h
@@ -409,7 +409,6 @@ struct bdc {
 	spinlock_t	lock;
 
 	/* generic phy */
-	struct phy      **phys;
 	int num_phys;
 	/* num of endpoints for a particular instantiation of IP */
 	unsigned int num_eps;
@@ -453,6 +452,7 @@ struct bdc {
 	 */
 	struct delayed_work	func_wake_notify;
 	struct clk		*clk;
+	struct phy		*phys[] __counted_by(num_phys);
 };
 
 static inline u32 bdc_readl(void __iomem *base, u32 offset)
diff --git a/drivers/usb/gadget/udc/bdc/bdc_core.c b/drivers/usb/gadget/udc/bdc/bdc_core.c
index 438201dc96ca..4b16b85da450 100644
--- a/drivers/usb/gadget/udc/bdc/bdc_core.c
+++ b/drivers/usb/gadget/udc/bdc/bdc_core.c
@@ -487,14 +487,20 @@ static int bdc_probe(struct platform_device *pdev)
 	int irq;
 	u32 temp;
 	struct device *dev = &pdev->dev;
+	int num_phys;
 	int phy_num;
 
 	dev_dbg(dev, "%s()\n", __func__);
 
-	bdc = devm_kzalloc(dev, sizeof(*bdc), GFP_KERNEL);
+	num_phys = of_count_phandle_with_args(dev->of_node,
+						"phys", "#phy-cells");
+	bdc = devm_kzalloc(dev, struct_size(bdc, phys, num_phys), GFP_KERNEL);
 	if (!bdc)
 		return -ENOMEM;
 
+	bdc->num_phys = num_phys;
+	dev_info(dev, "Using %d phy(s)\n", bdc->num_phys);
+
 	bdc->regs = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(bdc->regs))
 		return PTR_ERR(bdc->regs);
@@ -508,18 +514,6 @@ static int bdc_probe(struct platform_device *pdev)
 	bdc->dev = dev;
 	dev_dbg(dev, "bdc->regs: %p irq=%d\n", bdc->regs, bdc->irq);
 
-	bdc->num_phys = of_count_phandle_with_args(dev->of_node,
-						"phys", "#phy-cells");
-	if (bdc->num_phys > 0) {
-		bdc->phys = devm_kcalloc(dev, bdc->num_phys,
-					sizeof(struct phy *), GFP_KERNEL);
-		if (!bdc->phys)
-			return -ENOMEM;
-	} else {
-		bdc->num_phys = 0;
-	}
-	dev_info(dev, "Using %d phy(s)\n", bdc->num_phys);
-
 	for (phy_num = 0; phy_num < bdc->num_phys; phy_num++) {
 		bdc->phys[phy_num] = devm_of_phy_get_by_index(
 			dev, dev->of_node, phy_num);
-- 
2.53.0

Re: [PATCH] usb: bdc: allocate phys with main struct

Posted by Justin Chen 2 months ago


On 4/10/26 9:12 PM, Rosen Penev wrote:
> Use a flexible array member to combine allocations and simplify code
> slightly. No need for a branch deciding whether to allocate or not.
> 
> Add __counted_by for extra runtime analysis.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>   drivers/usb/gadget/udc/bdc/bdc.h      |  2 +-
>   drivers/usb/gadget/udc/bdc/bdc_core.c | 20 +++++++-------------
>   2 files changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/bdc/bdc.h b/drivers/usb/gadget/udc/bdc/bdc.h
> index 2f4abf6f8f77..cc961161eb46 100644
> --- a/drivers/usb/gadget/udc/bdc/bdc.h
> +++ b/drivers/usb/gadget/udc/bdc/bdc.h
> @@ -409,7 +409,6 @@ struct bdc {
>   	spinlock_t	lock;
>   
>   	/* generic phy */
> -	struct phy      **phys;
>   	int num_phys;
>   	/* num of endpoints for a particular instantiation of IP */
>   	unsigned int num_eps;
> @@ -453,6 +452,7 @@ struct bdc {
>   	 */
>   	struct delayed_work	func_wake_notify;
>   	struct clk		*clk;
> +	struct phy		*phys[] __counted_by(num_phys);
>   };
>   
>   static inline u32 bdc_readl(void __iomem *base, u32 offset)
> diff --git a/drivers/usb/gadget/udc/bdc/bdc_core.c b/drivers/usb/gadget/udc/bdc/bdc_core.c
> index 438201dc96ca..4b16b85da450 100644
> --- a/drivers/usb/gadget/udc/bdc/bdc_core.c
> +++ b/drivers/usb/gadget/udc/bdc/bdc_core.c
> @@ -487,14 +487,20 @@ static int bdc_probe(struct platform_device *pdev)
>   	int irq;
>   	u32 temp;
>   	struct device *dev = &pdev->dev;
> +	int num_phys;
>   	int phy_num;
>   
>   	dev_dbg(dev, "%s()\n", __func__);
>   
> -	bdc = devm_kzalloc(dev, sizeof(*bdc), GFP_KERNEL);
> +	num_phys = of_count_phandle_with_args(dev->of_node,
> +						"phys", "#phy-cells");
> +	bdc = devm_kzalloc(dev, struct_size(bdc, phys, num_phys), GFP_KERNEL);
>   	if (!bdc)
>   		return -ENOMEM;
>   
> +	bdc->num_phys = num_phys;
> +	dev_info(dev, "Using %d phy(s)\n", bdc->num_phys);
> +

This feels like a step sideways instead of an improvement IMHO. And we 
are also moving the allocation and dev_info() print. Is there a reason 
to change the ordering?

Justin

>   	bdc->regs = devm_platform_ioremap_resource(pdev, 0);
>   	if (IS_ERR(bdc->regs))
>   		return PTR_ERR(bdc->regs);
> @@ -508,18 +514,6 @@ static int bdc_probe(struct platform_device *pdev)
>   	bdc->dev = dev;
>   	dev_dbg(dev, "bdc->regs: %p irq=%d\n", bdc->regs, bdc->irq);
>   
> -	bdc->num_phys = of_count_phandle_with_args(dev->of_node,
> -						"phys", "#phy-cells");
> -	if (bdc->num_phys > 0) {
> -		bdc->phys = devm_kcalloc(dev, bdc->num_phys,
> -					sizeof(struct phy *), GFP_KERNEL);
> -		if (!bdc->phys)
> -			return -ENOMEM;
> -	} else {
> -		bdc->num_phys = 0;
> -	}
> -	dev_info(dev, "Using %d phy(s)\n", bdc->num_phys);
> -
>   	for (phy_num = 0; phy_num < bdc->num_phys; phy_num++) {
>   		bdc->phys[phy_num] = devm_of_phy_get_by_index(
>   			dev, dev->of_node, phy_num);

Re: [PATCH] usb: bdc: allocate phys with main struct

Posted by Rosen Penev 2 months ago

On Mon, Apr 13, 2026 at 10:51 AM Justin Chen <justin.chen@broadcom.com> wrote:
>
>
>
> On 4/10/26 9:12 PM, Rosen Penev wrote:
> > Use a flexible array member to combine allocations and simplify code
> > slightly. No need for a branch deciding whether to allocate or not.
> >
> > Add __counted_by for extra runtime analysis.
> >
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
> >   drivers/usb/gadget/udc/bdc/bdc.h      |  2 +-
> >   drivers/usb/gadget/udc/bdc/bdc_core.c | 20 +++++++-------------
> >   2 files changed, 8 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/udc/bdc/bdc.h b/drivers/usb/gadget/udc/bdc/bdc.h
> > index 2f4abf6f8f77..cc961161eb46 100644
> > --- a/drivers/usb/gadget/udc/bdc/bdc.h
> > +++ b/drivers/usb/gadget/udc/bdc/bdc.h
> > @@ -409,7 +409,6 @@ struct bdc {
> >       spinlock_t      lock;
> >
> >       /* generic phy */
> > -     struct phy      **phys;
> >       int num_phys;
> >       /* num of endpoints for a particular instantiation of IP */
> >       unsigned int num_eps;
> > @@ -453,6 +452,7 @@ struct bdc {
> >        */
> >       struct delayed_work     func_wake_notify;
> >       struct clk              *clk;
> > +     struct phy              *phys[] __counted_by(num_phys);
> >   };
> >
> >   static inline u32 bdc_readl(void __iomem *base, u32 offset)
> > diff --git a/drivers/usb/gadget/udc/bdc/bdc_core.c b/drivers/usb/gadget/udc/bdc/bdc_core.c
> > index 438201dc96ca..4b16b85da450 100644
> > --- a/drivers/usb/gadget/udc/bdc/bdc_core.c
> > +++ b/drivers/usb/gadget/udc/bdc/bdc_core.c
> > @@ -487,14 +487,20 @@ static int bdc_probe(struct platform_device *pdev)
> >       int irq;
> >       u32 temp;
> >       struct device *dev = &pdev->dev;
> > +     int num_phys;
> >       int phy_num;
> >
> >       dev_dbg(dev, "%s()\n", __func__);
> >
> > -     bdc = devm_kzalloc(dev, sizeof(*bdc), GFP_KERNEL);
> > +     num_phys = of_count_phandle_with_args(dev->of_node,
> > +                                             "phys", "#phy-cells");
> > +     bdc = devm_kzalloc(dev, struct_size(bdc, phys, num_phys), GFP_KERNEL);
> >       if (!bdc)
> >               return -ENOMEM;
> >
> > +     bdc->num_phys = num_phys;
> > +     dev_info(dev, "Using %d phy(s)\n", bdc->num_phys);
> > +
>
> This feels like a step sideways instead of an improvement IMHO. And we
> are also moving the allocation and dev_info() print. Is there a reason
> to change the ordering?
Of the allocation, yes since we need the size.

Of the dev_info, no. I can move it back.
>
> Justin
>
> >       bdc->regs = devm_platform_ioremap_resource(pdev, 0);
> >       if (IS_ERR(bdc->regs))
> >               return PTR_ERR(bdc->regs);
> > @@ -508,18 +514,6 @@ static int bdc_probe(struct platform_device *pdev)
> >       bdc->dev = dev;
> >       dev_dbg(dev, "bdc->regs: %p irq=%d\n", bdc->regs, bdc->irq);
> >
> > -     bdc->num_phys = of_count_phandle_with_args(dev->of_node,
> > -                                             "phys", "#phy-cells");
> > -     if (bdc->num_phys > 0) {
> > -             bdc->phys = devm_kcalloc(dev, bdc->num_phys,
> > -                                     sizeof(struct phy *), GFP_KERNEL);
> > -             if (!bdc->phys)
> > -                     return -ENOMEM;
> > -     } else {
> > -             bdc->num_phys = 0;
> > -     }
> > -     dev_info(dev, "Using %d phy(s)\n", bdc->num_phys);
> > -
> >       for (phy_num = 0; phy_num < bdc->num_phys; phy_num++) {
> >               bdc->phys[phy_num] = devm_of_phy_get_by_index(
> >                       dev, dev->of_node, phy_num);
>