On Sat, Sep 28, 2024 at 10:34:56AM GMT, Seshu Madhavi Puppala wrote:
> Under JEDEC specification ICE IP is tightly
> coupled with Storage. Qualcomm vendor HW
> implementation also ties the clock and power
> supply for ICE to corresponding storage clock and
> supplies. For a SoC supporting multiple storage
> types like UFS and eMMC the ICE physical address
> space is not shared and is always part of
> corresponding storage physical address space
> hence there is no need to independently probe ICE.
>
So, you're effectively saying that commit 2afbf43a4aec got system design
wrong, and it should never have been a dedicated device?
I presume this would be easy to spot, as there would be platforms with
multiple ICE device nodes...
If so, write that clearly and make sure you make sure that the author of
that change is among the addressed people in your patch.
> Cleanup commit 2afbf43a4aec ("soc: qcom: Make
> the Qualcomm UFS/SDCC ICE a dedicated driver")
> to remove dedicated ICE probe since there is no
> dedicated ICE IP block shared between UFS and
> SDCC as mentioned in 2afbf43a4aec.
>
> Storage probe will check for the corresponding
> ICE node by using of_qcom_ice_get to get ICE
> instance. Additional support added to
> of_qcom_ice_get to support ICE instance creation
> with new approach. Backward compatibility with
> old style device tree approach is untouched.
>
Add () suffix to function names, to make it clear that they are
functions.
Also, please read and follow:
https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format
> Signed-off-by: Seshu Madhavi Puppala <quic_spuppala@xxxxxxxxxxx>
> ---
> drivers/soc/qcom/ice.c | 44 +++++++-----------------------------------
> 1 file changed, 7 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
> index fbab7fe5c652..47f1b668dc86 100644
> --- a/drivers/soc/qcom/ice.c
> +++ b/drivers/soc/qcom/ice.c
> @@ -303,7 +303,13 @@ struct qcom_ice *of_qcom_ice_get(struct device *dev)
> goto out;
> }
>
> - ice = platform_get_drvdata(pdev);
> + base = devm_platform_ioremap_resource(pdev, 0);
So pdev here is the returned value of of_find_device_by_node() which
refers to a platform_device which now will never find a matching driver.
So no one will ever free this...
> + if (IS_ERR(base)) {
> + dev_warn(&pdev->dev, "ICE registers not found\n");
That's just one of the possible error cases. And iirc
devm_platform_ioremap_resource() already did print. Please double check
and update this accordingly.
> + return PTR_ERR(base);
> + }
> +
> + ice = qcom_ice_create(&pdev->dev, base);
This too will now allocate resources on a struct device that doesn't
have a driver and hence will never materialize - or clean up the devres
resources.
Regards,
Bjorn
> if (!ice) {
> dev_err(dev, "Cannot get ice instance from %s\n",
> dev_name(&pdev->dev));
> @@ -328,41 +334,5 @@ struct qcom_ice *of_qcom_ice_get(struct device *dev)
> }
> EXPORT_SYMBOL_GPL(of_qcom_ice_get);
>
> -static int qcom_ice_probe(struct platform_device *pdev)
> -{
> - struct qcom_ice *engine;
> - void __iomem *base;
> -
> - base = devm_platform_ioremap_resource(pdev, 0);
> - if (IS_ERR(base)) {
> - dev_warn(&pdev->dev, "ICE registers not found\n");
> - return PTR_ERR(base);
> - }
> -
> - engine = qcom_ice_create(&pdev->dev, base);
> - if (IS_ERR(engine))
> - return PTR_ERR(engine);
> -
> - platform_set_drvdata(pdev, engine);
> -
> - return 0;
> -}
> -
> -static const struct of_device_id qcom_ice_of_match_table[] = {
> - { .compatible = "qcom,inline-crypto-engine" },
> - { },
> -};
> -MODULE_DEVICE_TABLE(of, qcom_ice_of_match_table);
> -
> -static struct platform_driver qcom_ice_driver = {
> - .probe = qcom_ice_probe,
> - .driver = {
> - .name = "qcom-ice",
> - .of_match_table = qcom_ice_of_match_table,
> - },
> -};
> -
> -module_platform_driver(qcom_ice_driver);
> -
> MODULE_DESCRIPTION("Qualcomm Inline Crypto Engine driver");
> MODULE_LICENSE("GPL");
> --
> 2.17.1
>
