| Message ID | 83e985f2f60a68f89fac777aa29c55b1edb8769f.1541054985.git.matti.vaittinen@fi.rohmeurope.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: clkdev: managed clk lookup and provider registrations | expand |
On Thu, 1 Nov 2018 at 08:19, Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> wrote: > > clk-max77686 never clean clkdev lookup at remove. This can cause > oops if clk-max77686 is removed and inserted again. Fix leak by > using new devm clkdev lookup registration. Simplify also error > path by using new devm_of_clk_add_parent_hw_provider. > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > --- > drivers/clk/clk-max77686.c | 25 ++++--------------------- > 1 file changed, 4 insertions(+), 21 deletions(-) > > diff --git a/drivers/clk/clk-max77686.c b/drivers/clk/clk-max77686.c > index 02551fe4b87c..b1920c1d9b76 100644 > --- a/drivers/clk/clk-max77686.c > +++ b/drivers/clk/clk-max77686.c > @@ -235,7 +235,7 @@ static int max77686_clk_probe(struct platform_device *pdev) > return ret; > } > > - ret = clk_hw_register_clkdev(&max_clk_data->hw, > + ret = devm_clk_hw_register_clkdev(dev, &max_clk_data->hw, > max_clk_data->clk_idata.name, NULL); You need to re-align the next line. > if (ret < 0) { > dev_err(dev, "Failed to clkdev register: %d\n", ret); > @@ -244,8 +244,8 @@ static int max77686_clk_probe(struct platform_device *pdev) > } > > if (parent->of_node) { > - ret = of_clk_add_hw_provider(parent->of_node, of_clk_max77686_get, > - drv_data); > + ret = devm_of_clk_add_parent_hw_provider(dev, > + of_clk_max77686_get, drv_data); The same, please. Rest looks good, so with these changes: Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> Best regards, Krzysztof
Thanks for taking the time and reviewing this! On Fri, Nov 02, 2018 at 09:15:17AM +0100, Krzysztof Kozlowski wrote: > On Thu, 1 Nov 2018 at 08:19, Matti Vaittinen > <matti.vaittinen@fi.rohmeurope.com> wrote: > > > > clk-max77686 never clean clkdev lookup at remove. This can cause > > oops if clk-max77686 is removed and inserted again. Fix leak by > > using new devm clkdev lookup registration. Simplify also error > > path by using new devm_of_clk_add_parent_hw_provider. > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > --- > > drivers/clk/clk-max77686.c | 25 ++++--------------------- > > 1 file changed, 4 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/clk/clk-max77686.c b/drivers/clk/clk-max77686.c > > index 02551fe4b87c..b1920c1d9b76 100644 > > --- a/drivers/clk/clk-max77686.c > > +++ b/drivers/clk/clk-max77686.c > > @@ -235,7 +235,7 @@ static int max77686_clk_probe(struct platform_device *pdev) > > return ret; > > } > > > > - ret = clk_hw_register_clkdev(&max_clk_data->hw, > > + ret = devm_clk_hw_register_clkdev(dev, &max_clk_data->hw, > > max_clk_data->clk_idata.name, NULL); > > You need to re-align the next line. I'll change this to ret = devm_clk_hw_register_clkdev(dev, &max_clk_data->hw, max_clk_data->clk_idata.name, NULL); > > if (ret < 0) { > > dev_err(dev, "Failed to clkdev register: %d\n", ret); > > @@ -244,8 +244,8 @@ static int max77686_clk_probe(struct platform_device *pdev) > > } > > > > if (parent->of_node) { > > - ret = of_clk_add_hw_provider(parent->of_node, of_clk_max77686_get, > > - drv_data); > > + ret = devm_of_clk_add_parent_hw_provider(dev, > > + of_clk_max77686_get, drv_data); > > The same, please. I will change this to ret = devm_of_clk_add_parent_hw_provider(dev, of_clk_max77686_get, drv_data); > > Rest looks good, so with these changes: > Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> > > Best regards, > Krzysztof
diff --git a/drivers/clk/clk-max77686.c b/drivers/clk/clk-max77686.c index 02551fe4b87c..b1920c1d9b76 100644 --- a/drivers/clk/clk-max77686.c +++ b/drivers/clk/clk-max77686.c @@ -235,7 +235,7 @@ static int max77686_clk_probe(struct platform_device *pdev) return ret; } - ret = clk_hw_register_clkdev(&max_clk_data->hw, + ret = devm_clk_hw_register_clkdev(dev, &max_clk_data->hw, max_clk_data->clk_idata.name, NULL); if (ret < 0) { dev_err(dev, "Failed to clkdev register: %d\n", ret); @@ -244,8 +244,8 @@ static int max77686_clk_probe(struct platform_device *pdev) } if (parent->of_node) { - ret = of_clk_add_hw_provider(parent->of_node, of_clk_max77686_get, - drv_data); + ret = devm_of_clk_add_parent_hw_provider(dev, + of_clk_max77686_get, drv_data); if (ret < 0) { dev_err(dev, "Failed to register OF clock provider: %d\n", @@ -261,27 +261,11 @@ static int max77686_clk_probe(struct platform_device *pdev) 1 << MAX77802_CLOCK_LOW_JITTER_SHIFT); if (ret < 0) { dev_err(dev, "Failed to config low-jitter: %d\n", ret); - goto remove_of_clk_provider; + return ret; } } return 0; - -remove_of_clk_provider: - if (parent->of_node) - of_clk_del_provider(parent->of_node); - - return ret; -} - -static int max77686_clk_remove(struct platform_device *pdev) -{ - struct device *parent = pdev->dev.parent; - - if (parent->of_node) - of_clk_del_provider(parent->of_node); - - return 0; } static const struct platform_device_id max77686_clk_id[] = { @@ -297,7 +281,6 @@ static struct platform_driver max77686_clk_driver = { .name = "max77686-clk", }, .probe = max77686_clk_probe, - .remove = max77686_clk_remove, .id_table = max77686_clk_id, };
clk-max77686 never clean clkdev lookup at remove. This can cause oops if clk-max77686 is removed and inserted again. Fix leak by using new devm clkdev lookup registration. Simplify also error path by using new devm_of_clk_add_parent_hw_provider. Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> --- drivers/clk/clk-max77686.c | 25 ++++--------------------- 1 file changed, 4 insertions(+), 21 deletions(-)