On 3/22/2022 7:24 PM, Dan Carpenter wrote:
Thanks for Your Time Dan!!!
On Tue, Mar 22, 2022 at 07:03:23PM +0530, Srinivasa Rao Mandadapu wrote:
9e3d83c52844f95 Srinivasa Rao Mandadapu 2022-02-26 14 struct lpass_macro *lpass_macro_pds_init(struct device *dev)
9e3d83c52844f95 Srinivasa Rao Mandadapu 2022-02-26 15 {
9e3d83c52844f95 Srinivasa Rao Mandadapu 2022-02-26 16 struct lpass_macro *l_pds;
9e3d83c52844f95 Srinivasa Rao Mandadapu 2022-02-26 17 int ret;
9e3d83c52844f95 Srinivasa Rao Mandadapu 2022-02-26 18
9e3d83c52844f95 Srinivasa Rao Mandadapu 2022-02-26 19 if (!of_find_property(dev->of_node, "power-domains", NULL))
9e3d83c52844f95 Srinivasa Rao Mandadapu 2022-02-26 20 return NULL;
Returning NULL here will lead to a crash in tx_macro_runtime_resume()
When a function returns a mix of NULL and error pointers, then NULL
means the feature is deliberately disabled. It's not an error, it's a
deliberate choice by the distro or sys admin. The caller has to
be written to allow the feature to be disabled.
An example of this might be LEDs. Maybe people don't want LEDs so code
has to asume that the led->ops pointer might be NULL and check for that
before dereferencing it.
Actually, it's optional here. For some targets, with lpass ADSP enabled,
power domains are not required.
So is the reason, returning NULL Here.
Unfortunately, the caller is not written to handle NULLs so it will
crash.
Yes agree. will add error check with in lpass_macro_pds_exit().
Will that make sense?
sound/soc/codecs/lpass-tx-macro.c
1913 static int tx_macro_remove(struct platform_device *pdev)
1914 {
1915 struct tx_macro *tx = dev_get_drvdata(&pdev->dev);
1916
1917 clk_disable_unprepare(tx->macro);
1918 clk_disable_unprepare(tx->dcodec);
1919 clk_disable_unprepare(tx->mclk);
1920 clk_disable_unprepare(tx->npl);
1921 clk_disable_unprepare(tx->fsgen);
1922
1923 lpass_macro_pds_exit(tx->pds);
^^^^^^^
Boom.
1924
1925 return 0;
1926 }
9e3d83c52844f95 Srinivasa Rao Mandadapu 2022-02-26 21
9e3d83c52844f95 Srinivasa Rao Mandadapu 2022-02-26 22 l_pds = devm_kzalloc(dev, sizeof(*l_pds), GFP_KERNEL);
9e3d83c52844f95 Srinivasa Rao Mandadapu 2022-02-26 23 if (!l_pds)
9e3d83c52844f95 Srinivasa Rao Mandadapu 2022-02-26 24 return ERR_PTR(-ENOMEM);
Good.
9e3d83c52844f95 Srinivasa Rao Mandadapu 2022-02-26 25
9e3d83c52844f95 Srinivasa Rao Mandadapu 2022-02-26 26 l_pds->macro_pd = dev_pm_domain_attach_by_name(dev, "macro");
9e3d83c52844f95 Srinivasa Rao Mandadapu 2022-02-26 27 if (IS_ERR_OR_NULL(l_pds->macro_pd))
9e3d83c52844f95 Srinivasa Rao Mandadapu 2022-02-26 28 return NULL;
If this feature is optional then it should be:
if (IS_ERR_OR_NULL(l_pds->macro_pd))
return ERR_CAST(l_pds->macro_pd);
The admin deliberately chose to enable the feature so we can't just
ignore errors and convert them to NULL.
Here it's not optional, if power domains feature is available then macro and
dcodec power domains should be present.
So will update it like below.
if (IS_ERR_OR_NULL(l_pds->macro_pd)) {
ret = PTR_ERR(l_pds->macro_pd);
goto macro_err;
}
I bet COMPILE_TEST allows this to compile without CONFIG_PM but there
isn't anything we can do about that...
regards,
dan carpenter
