video: fbdev: mmp: Fix deferred clk handling in mmphw_probe()

  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 <christophe.jaillet@wanadoo.fr>
---
 drivers/video/fbdev/mmp/hw/mmp_ctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

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 <christophe.jaillet@wanadoo.fr>
> ---

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 <christophe.jaillet@wanadoo.fr>
> > ---
> 
> 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 <christophe.jaillet@wanadoo.fr>
>>> ---
>>
>> 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 <christophe.jaillet@wanadoo.fr>
> ---
>   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);