* [PATCH] mailbox: imx: Fix clk handling in imx_mu_probe()
@ 2018-12-15 23:01 Alexey Khoroshilov
2018-12-17 6:01 ` Oleksij Rempel
0 siblings, 1 reply; 4+ messages in thread
From: Alexey Khoroshilov @ 2018-12-15 23:01 UTC (permalink / raw)
To: Oleksij Rempel, Jassi Brar; +Cc: Alexey Khoroshilov, linux-kernel, ldv-project
Handling of devm_clk_get() suggests that the driver should support
lack of priv->clk. But imx_mu_probe() fails on clk_prepare_enable(NULL)
in that case.
The patch removes the try to enable absent clk and adds error handling
for mbox_controller_register().
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
drivers/mailbox/imx-mailbox.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
index 363d35d5e49d..ddde398f576e 100644
--- a/drivers/mailbox/imx-mailbox.c
+++ b/drivers/mailbox/imx-mailbox.c
@@ -292,10 +292,12 @@ static int imx_mu_probe(struct platform_device *pdev)
priv->clk = NULL;
}
- ret = clk_prepare_enable(priv->clk);
- if (ret) {
- dev_err(dev, "Failed to enable clock\n");
- return ret;
+ if (priv->clk) {
+ ret = clk_prepare_enable(priv->clk);
+ if (ret) {
+ dev_err(dev, "Failed to enable clock\n");
+ return ret;
+ }
}
for (i = 0; i < IMX_MU_CHANS; i++) {
@@ -324,7 +326,13 @@ static int imx_mu_probe(struct platform_device *pdev)
imx_mu_init_generic(priv);
- return mbox_controller_register(&priv->mbox);
+ ret = mbox_controller_register(&priv->mbox);
+ if (ret) {
+ clk_disable_unprepare(priv->clk);
+ return ret;
+ }
+
+ return 0;
}
static int imx_mu_remove(struct platform_device *pdev)
--
2.7.4
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] mailbox: imx: Fix clk handling in imx_mu_probe()
2018-12-15 23:01 [PATCH] mailbox: imx: Fix clk handling in imx_mu_probe() Alexey Khoroshilov
@ 2018-12-17 6:01 ` Oleksij Rempel
2018-12-17 7:24 ` Alexey Khoroshilov
0 siblings, 1 reply; 4+ messages in thread
From: Oleksij Rempel @ 2018-12-17 6:01 UTC (permalink / raw)
To: Alexey Khoroshilov; +Cc: Jassi Brar, linux-kernel, ldv-project
[-- Attachment #1: Type: text/plain, Size: 2845 bytes --]
Hi Alexey,
On Sun, Dec 16, 2018 at 02:01:44AM +0300, Alexey Khoroshilov wrote:
> Handling of devm_clk_get() suggests that the driver should support
> lack of priv->clk. But imx_mu_probe() fails on clk_prepare_enable(NULL)
> in that case.
>
> The patch removes the try to enable absent clk and adds error handling
> for mbox_controller_register().
>
> Found by Linux Driver Verification project (linuxtesting.org).
>
> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> ---
> drivers/mailbox/imx-mailbox.c | 18 +++++++++++++-----
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> index 363d35d5e49d..ddde398f576e 100644
> --- a/drivers/mailbox/imx-mailbox.c
> +++ b/drivers/mailbox/imx-mailbox.c
> @@ -292,10 +292,12 @@ static int imx_mu_probe(struct platform_device *pdev)
> priv->clk = NULL;
> }
>
> - ret = clk_prepare_enable(priv->clk);
> - if (ret) {
> - dev_err(dev, "Failed to enable clock\n");
> - return ret;
> + if (priv->clk) {
> + ret = clk_prepare_enable(priv->clk);
> + if (ret) {
> + dev_err(dev, "Failed to enable clock\n");
> + return ret;
> + }
> }
>
> for (i = 0; i < IMX_MU_CHANS; i++) {
> @@ -324,7 +326,13 @@ static int imx_mu_probe(struct platform_device *pdev)
>
> imx_mu_init_generic(priv);
>
> - return mbox_controller_register(&priv->mbox);
> + ret = mbox_controller_register(&priv->mbox);
> + if (ret) {
> + clk_disable_unprepare(priv->clk);
> + return ret;
> + }
> +
> + return 0;
> }
>
> static int imx_mu_remove(struct platform_device *pdev)
> --
> 2.7.4
>
>
NACK for this patch.
Here are code snippets from clk framework:
============================================================================
/* clk_prepare_enable helps cases using clk_enable in non-atomic context. */
static inline int clk_prepare_enable(struct clk *clk)
{
int ret;
ret = clk_prepare(clk);
if (ret)
return ret;
ret = clk_enable(clk);
if (ret)
clk_unprepare(clk);
return ret;
}
int clk_prepare(struct clk *clk)
{
if (!clk)
return 0;
return clk_core_prepare_lock(clk->core);
}
int clk_enable(struct clk *clk)
{
if (!clk)
return 0;
return clk_core_enable_lock(clk->core);
}
============================================================================
As you can see, complete code path is NULL resistant. Are you trying to
fix some real issue, or it is a theoretical work?
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] mailbox: imx: Fix clk handling in imx_mu_probe()
2018-12-17 6:01 ` Oleksij Rempel
@ 2018-12-17 7:24 ` Alexey Khoroshilov
2018-12-17 7:34 ` Oleksij Rempel
0 siblings, 1 reply; 4+ messages in thread
From: Alexey Khoroshilov @ 2018-12-17 7:24 UTC (permalink / raw)
To: Oleksij Rempel; +Cc: Jassi Brar, linux-kernel, ldv-project
Hi Oleksij,
By chance I took a look at another implementations:
arch/arm/mach-ep93xx/clock.c#L266
int clk_enable(struct clk *clk)
{
unsigned long flags;
if (!clk)
return -EINVAL;
...
arch/c6x/platforms/pll.c#L48
int clk_enable(struct clk *clk)
{
unsigned long flags;
if (clk == NULL || IS_ERR(clk))
return -EINVAL;
So, I am not sure the NULL resistance is a part of the clk_enable()
contract?
--
Alexey
On 17.12.2018 9:01, Oleksij Rempel wrote:
> Hi Alexey,
>
> On Sun, Dec 16, 2018 at 02:01:44AM +0300, Alexey Khoroshilov wrote:
>> Handling of devm_clk_get() suggests that the driver should support
>> lack of priv->clk. But imx_mu_probe() fails on clk_prepare_enable(NULL)
>> in that case.
>>
>> The patch removes the try to enable absent clk and adds error handling
>> for mbox_controller_register().
>>
>> Found by Linux Driver Verification project (linuxtesting.org).
>>
>> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
>> ---
>> drivers/mailbox/imx-mailbox.c | 18 +++++++++++++-----
>> 1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
>> index 363d35d5e49d..ddde398f576e 100644
>> --- a/drivers/mailbox/imx-mailbox.c
>> +++ b/drivers/mailbox/imx-mailbox.c
>> @@ -292,10 +292,12 @@ static int imx_mu_probe(struct platform_device *pdev)
>> priv->clk = NULL;
>> }
>>
>> - ret = clk_prepare_enable(priv->clk);
>> - if (ret) {
>> - dev_err(dev, "Failed to enable clock\n");
>> - return ret;
>> + if (priv->clk) {
>> + ret = clk_prepare_enable(priv->clk);
>> + if (ret) {
>> + dev_err(dev, "Failed to enable clock\n");
>> + return ret;
>> + }
>> }
>>
>> for (i = 0; i < IMX_MU_CHANS; i++) {
>> @@ -324,7 +326,13 @@ static int imx_mu_probe(struct platform_device *pdev)
>>
>> imx_mu_init_generic(priv);
>>
>> - return mbox_controller_register(&priv->mbox);
>> + ret = mbox_controller_register(&priv->mbox);
>> + if (ret) {
>> + clk_disable_unprepare(priv->clk);
>> + return ret;
>> + }
>> +
>> + return 0;
>> }
>>
>> static int imx_mu_remove(struct platform_device *pdev)
>> --
>> 2.7.4
>>
>>
>
> NACK for this patch.
>
> Here are code snippets from clk framework:
> ============================================================================
> /* clk_prepare_enable helps cases using clk_enable in non-atomic context. */
> static inline int clk_prepare_enable(struct clk *clk)
> {
> int ret;
>
> ret = clk_prepare(clk);
> if (ret)
> return ret;
> ret = clk_enable(clk);
> if (ret)
> clk_unprepare(clk);
>
> return ret;
> }
>
> int clk_prepare(struct clk *clk)
> {
> if (!clk)
> return 0;
>
> return clk_core_prepare_lock(clk->core);
> }
>
> int clk_enable(struct clk *clk)
> {
> if (!clk)
> return 0;
>
> return clk_core_enable_lock(clk->core);
> }
> ============================================================================
>
> As you can see, complete code path is NULL resistant. Are you trying to
> fix some real issue, or it is a theoretical work?
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] mailbox: imx: Fix clk handling in imx_mu_probe()
2018-12-17 7:24 ` Alexey Khoroshilov
@ 2018-12-17 7:34 ` Oleksij Rempel
0 siblings, 0 replies; 4+ messages in thread
From: Oleksij Rempel @ 2018-12-17 7:34 UTC (permalink / raw)
To: Alexey Khoroshilov; +Cc: Jassi Brar, linux-kernel, ldv-project
On 17.12.18 08:24, Alexey Khoroshilov wrote:
> Hi Oleksij,
>
> By chance I took a look at another implementations:
>
> arch/arm/mach-ep93xx/clock.c#L266
>
> int clk_enable(struct clk *clk)
> {
> unsigned long flags;
>
> if (!clk)
> return -EINVAL;
> ...
>
> arch/c6x/platforms/pll.c#L48
>
> int clk_enable(struct clk *clk)
> {
> unsigned long flags;
>
> if (clk == NULL || IS_ERR(clk))
> return -EINVAL;
>
> So, I am not sure the NULL resistance is a part of the clk_enable()
> contract?
Main framework is always preferred. If some local code provide
replacement of a functions already provided by the clk framework and not
follow same rules, then this code should be fixed. If this code is even
exporting this function for global use, then it is probably buggy.
And if i see it correctly:
1d81eedb8f6c1 (Lennert Buytenhek 2006-06-24 10:33:02 +0100 266) int
clk_enable(struct clk *clk)
It is ancient code... it just can't be correct.
> --
> Alexey
>
>
> On 17.12.2018 9:01, Oleksij Rempel wrote:
>> Hi Alexey,
>>
>> On Sun, Dec 16, 2018 at 02:01:44AM +0300, Alexey Khoroshilov wrote:
>>> Handling of devm_clk_get() suggests that the driver should support
>>> lack of priv->clk. But imx_mu_probe() fails on clk_prepare_enable(NULL)
>>> in that case.
>>>
>>> The patch removes the try to enable absent clk and adds error handling
>>> for mbox_controller_register().
>>>
>>> Found by Linux Driver Verification project (linuxtesting.org).
>>>
>>> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
>>> ---
>>> drivers/mailbox/imx-mailbox.c | 18 +++++++++++++-----
>>> 1 file changed, 13 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
>>> index 363d35d5e49d..ddde398f576e 100644
>>> --- a/drivers/mailbox/imx-mailbox.c
>>> +++ b/drivers/mailbox/imx-mailbox.c
>>> @@ -292,10 +292,12 @@ static int imx_mu_probe(struct platform_device *pdev)
>>> priv->clk = NULL;
>>> }
>>>
>>> - ret = clk_prepare_enable(priv->clk);
>>> - if (ret) {
>>> - dev_err(dev, "Failed to enable clock\n");
>>> - return ret;
>>> + if (priv->clk) {
>>> + ret = clk_prepare_enable(priv->clk);
>>> + if (ret) {
>>> + dev_err(dev, "Failed to enable clock\n");
>>> + return ret;
>>> + }
>>> }
>>>
>>> for (i = 0; i < IMX_MU_CHANS; i++) {
>>> @@ -324,7 +326,13 @@ static int imx_mu_probe(struct platform_device *pdev)
>>>
>>> imx_mu_init_generic(priv);
>>>
>>> - return mbox_controller_register(&priv->mbox);
>>> + ret = mbox_controller_register(&priv->mbox);
>>> + if (ret) {
>>> + clk_disable_unprepare(priv->clk);
>>> + return ret;
>>> + }
>>> +
>>> + return 0;
>>> }
>>>
>>> static int imx_mu_remove(struct platform_device *pdev)
>>> --
>>> 2.7.4
>>>
>>>
>>
>> NACK for this patch.
>>
>> Here are code snippets from clk framework:
>> ============================================================================
>> /* clk_prepare_enable helps cases using clk_enable in non-atomic context. */
>> static inline int clk_prepare_enable(struct clk *clk)
>> {
>> int ret;
>>
>> ret = clk_prepare(clk);
>> if (ret)
>> return ret;
>> ret = clk_enable(clk);
>> if (ret)
>> clk_unprepare(clk);
>>
>> return ret;
>> }
>>
>> int clk_prepare(struct clk *clk)
>> {
>> if (!clk)
>> return 0;
>>
>> return clk_core_prepare_lock(clk->core);
>> }
>>
>> int clk_enable(struct clk *clk)
>> {
>> if (!clk)
>> return 0;
>>
>> return clk_core_enable_lock(clk->core);
>> }
>> ============================================================================
>>
>> As you can see, complete code path is NULL resistant. Are you trying to
>> fix some real issue, or it is a theoretical work?
>>
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-12-17 7:34 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-15 23:01 [PATCH] mailbox: imx: Fix clk handling in imx_mu_probe() Alexey Khoroshilov
2018-12-17 6:01 ` Oleksij Rempel
2018-12-17 7:24 ` Alexey Khoroshilov
2018-12-17 7:34 ` Oleksij Rempel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).