Re: [bug report] clk: mxl: Switch from direct readl/writel based IO to regmap based IO

[Rahul Tanwar <rtanwar@maxlinear.com>]

Hi Dan,


On 24/10/2022 4:48 pm, Dan Carpenter wrote:
> This email was sent from outside of MaxLinear.
> 
> 
> Hello Rahul Tanwar,
> 
> The patch 036177310bac: "clk: mxl: Switch from direct readl/writel
> based IO to regmap based IO" from Oct 13, 2022, leads to the
> following Smatch static checker warning:
> 
>          drivers/clk/x86/clk-lgm.c:441 lgm_cgu_probe()
>          warn: passing zero to 'PTR_ERR'
> 
> drivers/clk/x86/clk-lgm.c
>      424 static int lgm_cgu_probe(struct platform_device *pdev)
>      425 {
>      426         struct lgm_clk_provider *ctx;
>      427         struct device *dev = &pdev->dev;
>      428         struct device_node *np = dev->of_node;
>      429         int ret;
>      430
>      431         ctx = devm_kzalloc(dev, struct_size(ctx, clk_data.hws, CLK_NR_CLKS),
>      432                            GFP_KERNEL);
>      433         if (!ctx)
>      434                 return -ENOMEM;
>      435
>      436         ctx->clk_data.num = CLK_NR_CLKS;
>      437
>      438         ctx->membase = syscon_node_to_regmap(np);
>      439         if (IS_ERR_OR_NULL(ctx->membase)) {
>      440                 dev_err(dev, "Failed to get clk CGU iomem\n");
> --> 441                 return PTR_ERR(ctx->membase);
> 
> Generally when a function returns NULL that is a special kind of success
> path where the feature has been deliberately disabled.  If for example,
> LEDs have been disabled then the ethernet driver or whatever should just
> handle that and continue functioning.  (If there is an error in the LEDs
> then report the error and do not ignore it).
> 
> Can this driver not continue without ctx->membase?  Is it useful to
> have a no-op probe function that just sets ctx->clk_data.num and returns
> success?  If it's not useful then it's better to handle it in the
> Kconfig instead of waiting for the user to find it the hard way.
> 
> If CONFIG_MFD_SYSCON is turned off then syscon_node_to_regmap() does
> not return NULL, it returns ERR_PTR(-ENOTSUPP).  So the IS_ERR_OR_NULL()
> doesn't make sense.
> 

Thanks for catching it. I agree with you that IS_ERR_OR_NULL() does not 
make sense here. It should have been IS_ERR(). I will send a fixup patch 
to fix it.

IMHO, it is not useful to have a no-op probe function without 
ctx->membase. Firstly, i have checked the syscon_node_to_regmap() 
function. It never returns NULL in any/all possible cases whether 
success or failure. Either it returns a valid membase pointer or error 
code. Secondly, if i approach to consider NULL return as special kind of 
success then i will have to return NULL for all clk_hw for all 
ctx->clk_data.num clks in order to make it a successfully probed & 
loaded driver but which offers no clk services at all i.e. makes all 
clks as dummy clks. That approach will need much more unnecessary code 
changes because the driver dynamically allocates all clk structures post 
syscon_node_to_regmap() call. Also, if the regbase is returned NULL then 
i do not see any point in loading the driver at all.

I will send a fixup patch to fix this static checker warning by 
replacing IS_ERR_OR_NULL() with IS_ERR().

Regards,
Rahul




>      442         }
>      443
>      444
>      445         ctx->np = np;
>      446         ctx->dev = dev;
>      447
>      448         ret = lgm_clk_register_plls(ctx, lgm_pll_clks,
>      449                                     ARRAY_SIZE(lgm_pll_clks));
>      450         if (ret)
>      451                 return ret;
>      452
>      453         ret = lgm_clk_register_branches(ctx, lgm_branch_clks,
>      454                                         ARRAY_SIZE(lgm_branch_clks));
>      455         if (ret)
>      456                 return ret;
>      457
>      458         ret = lgm_clk_register_ddiv(ctx, lgm_ddiv_clks,
>      459                                     ARRAY_SIZE(lgm_ddiv_clks));
>      460         if (ret)
>      461                 return ret;
>      462
>      463         return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
>      464                                            &ctx->clk_data);
>      465 }
> 
> regards,
> dan carpenter
> 
>