| Message ID | 20220903071543.2844698-1-dzm91@hust.edu.cn |
|---|---|
| State | New |
| Headers | show |
| Series | usb: misc: usb3503: call clk_disable_unprepare in the error handling | expand |
On Sat, Sep 03, 2022 at 03:15:40PM +0800, Dongliang Mu wrote: > From: Dongliang Mu <mudongliangabcd@gmail.com> > > Smatch reports the following warning: > > vers/usb/misc/usb3503.c:267 usb3503_probe() warn: 'hub->clk' > from clk_prepare_enable() not released on lines: 240,246,252 > > Fix this by adding a flag to indicate if hub->clk is prepared or not and > invoke clk_disable_unprepare in the error handling. > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > --- > drivers/usb/misc/usb3503.c | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/misc/usb3503.c b/drivers/usb/misc/usb3503.c > index 330f494cd158..add47dd964b2 100644 > --- a/drivers/usb/misc/usb3503.c > +++ b/drivers/usb/misc/usb3503.c > @@ -160,6 +160,7 @@ static int usb3503_probe(struct usb3503 *hub) > struct usb3503_platform_data *pdata = dev_get_platdata(dev); > struct device_node *np = dev->of_node; > int err; > + int is_clk_enable = 0; bool? > u32 mode = USB3503_MODE_HUB; > const u32 *property; > enum gpiod_flags flags; > @@ -217,6 +218,8 @@ static int usb3503_probe(struct usb3503 *hub) > return err; > } > > + // set a flag for successful clk_prepare_enable Comment isn't needed. > + is_clk_enable = 1; > property = of_get_property(np, "disabled-ports", &len); > if (property && (len / sizeof(u32)) > 0) { > int i; > @@ -236,20 +239,29 @@ static int usb3503_probe(struct usb3503 *hub) > else > flags = GPIOD_OUT_HIGH; > hub->intn = devm_gpiod_get_optional(dev, "intn", flags); > - if (IS_ERR(hub->intn)) > + if (IS_ERR(hub->intn)) { > + if (is_clk_enable) > + clk_disable_unprepare(hub->clk); > return PTR_ERR(hub->intn); This is getting messy, any way to make a common error handler at the bottom of the function instead? That's the common kernel coding style for this type of thing. thanks, greg k-h
On Wed, Sep 7, 2022 at 10:25 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Sat, Sep 03, 2022 at 03:15:40PM +0800, Dongliang Mu wrote: > > From: Dongliang Mu <mudongliangabcd@gmail.com> > > > > Smatch reports the following warning: > > > > vers/usb/misc/usb3503.c:267 usb3503_probe() warn: 'hub->clk' > > from clk_prepare_enable() not released on lines: 240,246,252 > > > > Fix this by adding a flag to indicate if hub->clk is prepared or not and > > invoke clk_disable_unprepare in the error handling. > > > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > > --- > > drivers/usb/misc/usb3503.c | 18 +++++++++++++++--- > > 1 file changed, 15 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/usb/misc/usb3503.c b/drivers/usb/misc/usb3503.c > > index 330f494cd158..add47dd964b2 100644 > > --- a/drivers/usb/misc/usb3503.c > > +++ b/drivers/usb/misc/usb3503.c > > @@ -160,6 +160,7 @@ static int usb3503_probe(struct usb3503 *hub) > > struct usb3503_platform_data *pdata = dev_get_platdata(dev); > > struct device_node *np = dev->of_node; > > int err; > > + int is_clk_enable = 0; > > bool? > > > u32 mode = USB3503_MODE_HUB; > > const u32 *property; > > enum gpiod_flags flags; > > @@ -217,6 +218,8 @@ static int usb3503_probe(struct usb3503 *hub) > > return err; > > } > > > > + // set a flag for successful clk_prepare_enable > > Comment isn't needed. > > > + is_clk_enable = 1; > > property = of_get_property(np, "disabled-ports", &len); > > if (property && (len / sizeof(u32)) > 0) { > > int i; > > @@ -236,20 +239,29 @@ static int usb3503_probe(struct usb3503 *hub) > > else > > flags = GPIOD_OUT_HIGH; > > hub->intn = devm_gpiod_get_optional(dev, "intn", flags); > > - if (IS_ERR(hub->intn)) > > + if (IS_ERR(hub->intn)) { > > + if (is_clk_enable) > > + clk_disable_unprepare(hub->clk); > > return PTR_ERR(hub->intn); > > This is getting messy, any way to make a common error handler at the > bottom of the function instead? That's the common kernel coding style > for this type of thing. I have addressed all the issues mentioned above: 1. change is_clk_enable to is_clk_enabled, and change its type to bool. 2. remove the comment and move the error handling code to the end of probe function. > > thanks, > > greg k-h
diff --git a/drivers/usb/misc/usb3503.c b/drivers/usb/misc/usb3503.c index 330f494cd158..add47dd964b2 100644 --- a/drivers/usb/misc/usb3503.c +++ b/drivers/usb/misc/usb3503.c @@ -160,6 +160,7 @@ static int usb3503_probe(struct usb3503 *hub) struct usb3503_platform_data *pdata = dev_get_platdata(dev); struct device_node *np = dev->of_node; int err; + int is_clk_enable = 0; u32 mode = USB3503_MODE_HUB; const u32 *property; enum gpiod_flags flags; @@ -217,6 +218,8 @@ static int usb3503_probe(struct usb3503 *hub) return err; } + // set a flag for successful clk_prepare_enable + is_clk_enable = 1; property = of_get_property(np, "disabled-ports", &len); if (property && (len / sizeof(u32)) > 0) { int i; @@ -236,20 +239,29 @@ static int usb3503_probe(struct usb3503 *hub) else flags = GPIOD_OUT_HIGH; hub->intn = devm_gpiod_get_optional(dev, "intn", flags); - if (IS_ERR(hub->intn)) + if (IS_ERR(hub->intn)) { + if (is_clk_enable) + clk_disable_unprepare(hub->clk); return PTR_ERR(hub->intn); + } if (hub->intn) gpiod_set_consumer_name(hub->intn, "usb3503 intn"); hub->connect = devm_gpiod_get_optional(dev, "connect", GPIOD_OUT_LOW); - if (IS_ERR(hub->connect)) + if (IS_ERR(hub->connect)) { + if (is_clk_enable) + clk_disable_unprepare(hub->clk); return PTR_ERR(hub->connect); + } if (hub->connect) gpiod_set_consumer_name(hub->connect, "usb3503 connect"); hub->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH); - if (IS_ERR(hub->reset)) + if (IS_ERR(hub->reset)) { + if (is_clk_enable) + clk_disable_unprepare(hub->clk); return PTR_ERR(hub->reset); + } if (hub->reset) { /* Datasheet defines a hardware reset to be at least 100us */ usleep_range(100, 10000);