| Message ID | 20230530102617.3413183-3-jonas@kwiboo.se |
|---|---|
| State | Superseded |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | rockchip: rk3568: Use dwc3-generic driver | expand |
On Tue, May 30, 2023 at 3:56 PM Jonas Karlman <jonas@kwiboo.se> wrote: > > The call to device_find_first_child always return 0, change to return > early when there is no child node. Can you explain a little more about this? Maybe adding device-tree pipeline will give a better understanding of the issue. Jagan.
Hi Jagan, On 2023-06-01 06:10, Jagan Teki wrote: > On Tue, May 30, 2023 at 3:56 PM Jonas Karlman <jonas@kwiboo.se> wrote: >> >> The call to device_find_first_child always return 0, change to return >> early when there is no child node. > > Can you explain a little more about this? Maybe adding device-tree > pipeline will give a better understanding of the issue. The docs for device_find_first_child mention: @devp: Returns first child device, or NULL if none Return: 0 And the function does exactly this, always return 0. So, the current check for return value of this call is unnecessary. The following possible call to dwc3_glue_clk_init or dwc3_glue_reset_init may trigger a null pointer dereference further down the call chain. Looking closer this patch may have been more related to my rk3328 testing, where usb@ff600000 in rk3328.dtsi is missing child nodes and resets prop. For RK3568 this patch should not be needed, however it still fixes a possible null pointer dereference issue. Regards, Jonas > > Jagan.
On 5/30/23 12:26, Jonas Karlman wrote: > The call to device_find_first_child always return 0, change to return > early when there is no child node. > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se> > --- > drivers/usb/dwc3/dwc3-generic.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c > index 66da5a8d6f8c..c28ad47bddd8 100644 > --- a/drivers/usb/dwc3/dwc3-generic.c > +++ b/drivers/usb/dwc3/dwc3-generic.c > @@ -558,9 +558,9 @@ int dwc3_glue_probe(struct udevice *dev) > return ret; > } > > - ret = device_find_first_child(dev, &child); > - if (ret) > - return ret; > + device_find_first_child(dev, &child); > + if (!child) > + return 0; > > if (glue->clks.count == 0) { > ret = dwc3_glue_clk_init(child, glue); If this is a fix, then please send this separately, so I can pick it for current release.
Hi Marek, Sorry for a late reply. On 2023-06-05 12:12, Marek Vasut wrote: > On 5/30/23 12:26, Jonas Karlman wrote: >> The call to device_find_first_child always return 0, change to return >> early when there is no child node. >> >> Signed-off-by: Jonas Karlman <jonas@kwiboo.se> >> --- >> drivers/usb/dwc3/dwc3-generic.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c >> index 66da5a8d6f8c..c28ad47bddd8 100644 >> --- a/drivers/usb/dwc3/dwc3-generic.c >> +++ b/drivers/usb/dwc3/dwc3-generic.c >> @@ -558,9 +558,9 @@ int dwc3_glue_probe(struct udevice *dev) >> return ret; >> } >> >> - ret = device_find_first_child(dev, &child); >> - if (ret) >> - return ret; >> + device_find_first_child(dev, &child); >> + if (!child) >> + return 0; >> >> if (glue->clks.count == 0) { >> ret = dwc3_glue_clk_init(child, glue); > > If this is a fix, then please send this separately, so I can pick it for > current release. Current boards using this driver is not affected by this. To my knowledge only RK3328 may have been affected by this, DT does not have any resets. Adding support for RK3328 in this driver is part of the series "rockchip: Fix RK3328 USB support" by Jagan, a series that depend on patches in this series. Should I still re-send this patch as a separate patch? Regards, Jonas
On 7/13/23 11:51, Jonas Karlman wrote: > Hi Marek, > > Sorry for a late reply. > > On 2023-06-05 12:12, Marek Vasut wrote: >> On 5/30/23 12:26, Jonas Karlman wrote: >>> The call to device_find_first_child always return 0, change to return >>> early when there is no child node. >>> >>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se> >>> --- >>> drivers/usb/dwc3/dwc3-generic.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c >>> index 66da5a8d6f8c..c28ad47bddd8 100644 >>> --- a/drivers/usb/dwc3/dwc3-generic.c >>> +++ b/drivers/usb/dwc3/dwc3-generic.c >>> @@ -558,9 +558,9 @@ int dwc3_glue_probe(struct udevice *dev) >>> return ret; >>> } >>> >>> - ret = device_find_first_child(dev, &child); >>> - if (ret) >>> - return ret; >>> + device_find_first_child(dev, &child); >>> + if (!child) >>> + return 0; >>> >>> if (glue->clks.count == 0) { >>> ret = dwc3_glue_clk_init(child, glue); >> >> If this is a fix, then please send this separately, so I can pick it for >> current release. > > Current boards using this driver is not affected by this. To my knowledge > only RK3328 may have been affected by this, DT does not have any resets. > > Adding support for RK3328 in this driver is part of the series "rockchip: > Fix RK3328 USB support" by Jagan, a series that depend on patches in this > series. > > Should I still re-send this patch as a separate patch? Release is already out, so, no need.
diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c index 66da5a8d6f8c..c28ad47bddd8 100644 --- a/drivers/usb/dwc3/dwc3-generic.c +++ b/drivers/usb/dwc3/dwc3-generic.c @@ -558,9 +558,9 @@ int dwc3_glue_probe(struct udevice *dev) return ret; } - ret = device_find_first_child(dev, &child); - if (ret) - return ret; + device_find_first_child(dev, &child); + if (!child) + return 0; if (glue->clks.count == 0) { ret = dwc3_glue_clk_init(child, glue);
The call to device_find_first_child always return 0, change to return early when there is no child node. Signed-off-by: Jonas Karlman <jonas@kwiboo.se> --- drivers/usb/dwc3/dwc3-generic.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)