When dev_err_probe() is called, 'ret' holds the value of the previous
successful devm_request_irq() call.
'ret' should be assigned with a meaningful value before being used in
dev_err_probe().
While at it, use and return "PTR_ERR(ctrl->clk)" instead of a hard-coded
"-ENOENT" so that -EPROBE_DEFER is handled and propagated correctly.
Fixes: 81b63420564d ("video: fbdev: mmp: Make use of the helper function dev_err_probe()")
Signed-off-by: Christophe JAILLET <[email protected]>
---
drivers/video/fbdev/mmp/hw/mmp_ctrl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/mmp/hw/mmp_ctrl.c b/drivers/video/fbdev/mmp/hw/mmp_ctrl.c
index a9df8ee79810..51fbf02a0343 100644
--- a/drivers/video/fbdev/mmp/hw/mmp_ctrl.c
+++ b/drivers/video/fbdev/mmp/hw/mmp_ctrl.c
@@ -514,9 +514,9 @@ static int mmphw_probe(struct platform_device *pdev)
/* get clock */
ctrl->clk = devm_clk_get(ctrl->dev, mi->clk_name);
if (IS_ERR(ctrl->clk)) {
+ ret = PTR_ERR(ctrl->clk);
dev_err_probe(ctrl->dev, ret,
"unable to get clk %s\n", mi->clk_name);
- ret = -ENOENT;
goto failed;
}
clk_prepare_enable(ctrl->clk);
--
2.34.1
On Thu, Apr 13, 2023 at 09:33:17PM +0200, Christophe JAILLET wrote:
> When dev_err_probe() is called, 'ret' holds the value of the previous
> successful devm_request_irq() call.
> 'ret' should be assigned with a meaningful value before being used in
> dev_err_probe().
>
> While at it, use and return "PTR_ERR(ctrl->clk)" instead of a hard-coded
> "-ENOENT" so that -EPROBE_DEFER is handled and propagated correctly.
>
> Fixes: 81b63420564d ("video: fbdev: mmp: Make use of the helper function dev_err_probe()")
> Signed-off-by: Christophe JAILLET <[email protected]>
> ---
Presumably you already wrote a Coccinelle script for this but I've added
it to Smatch as well.
regards,
dan carpenter
diff --git a/check_zero_to_err_ptr.c b/check_zero_to_err_ptr.c
index 88ca0285948a..fa2a1f1603b2 100644
--- a/check_zero_to_err_ptr.c
+++ b/check_zero_to_err_ptr.c
@@ -157,11 +157,12 @@ static void match_err_ptr(const char *fn, struct expression *expr, void *data)
{
struct expression *arg_expr;
struct sm_state *sm, *tmp;
+ int arg = PTR_INT(data);
if (is_impossible_path())
return;
- arg_expr = get_argument_from_call_expr(expr->args, 0);
+ arg_expr = get_argument_from_call_expr(expr->args, arg);
sm = get_sm_state_expr(SMATCH_EXTRA, arg_expr);
if (!sm)
return;
@@ -194,13 +195,36 @@ static void match_err_ptr(const char *fn, struct expression *expr, void *data)
} END_FOR_EACH_PTR(tmp);
}
+static void match_err_ptr_or_zero(const char *fn, struct expression *expr, void *data)
+{
+ struct expression *arg;
+ struct range_list *rl;
+ char *name;
+
+ if (is_impossible_path())
+ return;
+
+ arg = get_argument_from_call_expr(expr->args, 0);
+ get_absolute_rl(arg, &rl);
+
+ if (rl_intersection(rl, valid_ptr_rl))
+ return;
+
+ name = expr_to_str(arg);
+ sm_warning("'%s' is never a valid pointer", name);
+ free_string(name);
+}
+
void check_zero_to_err_ptr(int id)
{
if (option_project != PROJ_KERNEL)
return;
my_id = id;
- add_function_hook("ERR_PTR", &match_err_ptr, NULL);
- add_function_hook("ERR_CAST", &match_err_ptr, NULL);
- add_function_hook("PTR_ERR", &match_err_ptr, NULL);
+ add_function_hook("ERR_PTR", &match_err_ptr, INT_PTR(0));
+ add_function_hook("ERR_CAST", &match_err_ptr, INT_PTR(0));
+ add_function_hook("PTR_ERR", &match_err_ptr, INT_PTR(0));
+ add_function_hook("dev_err_probe", &match_err_ptr, INT_PTR(1));
+
+ add_function_hook("ERR_PTR_OR_ZERO", &match_err_ptr_or_zero, NULL);
}
On Sat, Apr 15, 2023 at 04:09:03PM +0300, Dan Carpenter wrote:
> On Thu, Apr 13, 2023 at 09:33:17PM +0200, Christophe JAILLET wrote:
> > When dev_err_probe() is called, 'ret' holds the value of the previous
> > successful devm_request_irq() call.
> > 'ret' should be assigned with a meaningful value before being used in
> > dev_err_probe().
> >
> > While at it, use and return "PTR_ERR(ctrl->clk)" instead of a hard-coded
> > "-ENOENT" so that -EPROBE_DEFER is handled and propagated correctly.
> >
> > Fixes: 81b63420564d ("video: fbdev: mmp: Make use of the helper function dev_err_probe()")
> > Signed-off-by: Christophe JAILLET <[email protected]>
> > ---
>
> Presumably you already wrote a Coccinelle script for this but I've added
> it to Smatch as well.
Here is this warning:
drivers/video/fbdev/mmp/hw/mmp_ctrl.c:518 mmphw_probe() warn: passing zero to 'dev_err_probe'
Other warnings. All five are interesting.
drivers/power/supply/rt9467-charger.c:1026 rt9467_request_interrupt() warn: passing zero to 'dev_err_probe'
drivers/pci/controller/dwc/pcie-bt1.c:601 bt1_pcie_add_port() warn: passing zero to 'dev_err_probe'
drivers/spi/spi-sprd-adi.c:570 sprd_adi_probe() warn: passing zero to 'dev_err_probe'
drivers/soc/qcom/icc-bwmon.c:776 bwmon_probe() warn: passing zero to 'dev_err_probe'
drivers/soc/qcom/icc-bwmon.c:781 bwmon_probe() warn: passing zero to 'dev_err_probe'
regards,
dan carpenter
Le 19/04/2023 à 06:59, Dan Carpenter a écrit :
> On Sat, Apr 15, 2023 at 04:09:03PM +0300, Dan Carpenter wrote:
>> On Thu, Apr 13, 2023 at 09:33:17PM +0200, Christophe JAILLET wrote:
>>> When dev_err_probe() is called, 'ret' holds the value of the previous
>>> successful devm_request_irq() call.
>>> 'ret' should be assigned with a meaningful value before being used in
>>> dev_err_probe().
>>>
>>> While at it, use and return "PTR_ERR(ctrl->clk)" instead of a hard-coded
>>> "-ENOENT" so that -EPROBE_DEFER is handled and propagated correctly.
>>>
>>> Fixes: 81b63420564d ("video: fbdev: mmp: Make use of the helper function dev_err_probe()")
>>> Signed-off-by: Christophe JAILLET <[email protected]>
>>> ---
>>
>> Presumably you already wrote a Coccinelle script for this but I've added
>> it to Smatch as well.
No I haven't.
I've spotted it while looking at some devm_clk_get_optional() candidate
with grep.
git grep -A5 devm_clk_get | grep -B5 ENOENT
Not perfect, but sometimes this kind of approach can find interesting
things coccinelle would miss.
As an example, the bitmap_alloc patch on sh4 was found this way, with grep.
So nice to have it in smatch, ;-)
CJ
>
> Here is this warning:
> drivers/video/fbdev/mmp/hw/mmp_ctrl.c:518 mmphw_probe() warn: passing zero to 'dev_err_probe'
>
> Other warnings. All five are interesting.
> drivers/power/supply/rt9467-charger.c:1026 rt9467_request_interrupt() warn: passing zero to 'dev_err_probe'
> drivers/pci/controller/dwc/pcie-bt1.c:601 bt1_pcie_add_port() warn: passing zero to 'dev_err_probe'
> drivers/spi/spi-sprd-adi.c:570 sprd_adi_probe() warn: passing zero to 'dev_err_probe'
> drivers/soc/qcom/icc-bwmon.c:776 bwmon_probe() warn: passing zero to 'dev_err_probe'
> drivers/soc/qcom/icc-bwmon.c:781 bwmon_probe() warn: passing zero to 'dev_err_probe'
>
> regards,
> dan carpenter
>
>
On 4/13/23 21:33, Christophe JAILLET wrote:
> When dev_err_probe() is called, 'ret' holds the value of the previous
> successful devm_request_irq() call.
> 'ret' should be assigned with a meaningful value before being used in
> dev_err_probe().
>
> While at it, use and return "PTR_ERR(ctrl->clk)" instead of a hard-coded
> "-ENOENT" so that -EPROBE_DEFER is handled and propagated correctly.
>
> Fixes: 81b63420564d ("video: fbdev: mmp: Make use of the helper function dev_err_probe()")
> Signed-off-by: Christophe JAILLET <[email protected]>
> ---
> drivers/video/fbdev/mmp/hw/mmp_ctrl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
applied.
Thanks!
Helge
>
> diff --git a/drivers/video/fbdev/mmp/hw/mmp_ctrl.c b/drivers/video/fbdev/mmp/hw/mmp_ctrl.c
> index a9df8ee79810..51fbf02a0343 100644
> --- a/drivers/video/fbdev/mmp/hw/mmp_ctrl.c
> +++ b/drivers/video/fbdev/mmp/hw/mmp_ctrl.c
> @@ -514,9 +514,9 @@ static int mmphw_probe(struct platform_device *pdev)
> /* get clock */
> ctrl->clk = devm_clk_get(ctrl->dev, mi->clk_name);
> if (IS_ERR(ctrl->clk)) {
> + ret = PTR_ERR(ctrl->clk);
> dev_err_probe(ctrl->dev, ret,
> "unable to get clk %s\n", mi->clk_name);
> - ret = -ENOENT;
> goto failed;
> }
> clk_prepare_enable(ctrl->clk);