* [PATCH v2 0/4] soc: qcom: ice: Remove platform_driver support and expose as a pure library
@ 2026-02-10 6:56 Manivannan Sadhasivam via B4 Relay
2026-02-10 6:56 ` [PATCH v2 1/4] " Manivannan Sadhasivam via B4 Relay
2026-02-10 13:43 ` [PATCH v2 0/4] " Ulf Hansson
0 siblings, 2 replies; 10+ messages in thread
From: Manivannan Sadhasivam via B4 Relay @ 2026-02-10 6:56 UTC (permalink / raw)
To: Bjorn Andersson, Konrad Dybcio, Abel Vesa, Adrian Hunter,
Ulf Hansson, Manivannan Sadhasivam, James E.J. Bottomley,
Martin K. Petersen
Cc: linux-arm-msm, linux-kernel, linux-mmc, linux-scsi, Sumit Garg,
mani, Manivannan Sadhasivam, stable, Abel Vesa
Hi,
This series removes the platform_driver support from Qcom ICE driver and
exposes it as a pure library to the clients to avoid race conditions with ICE
SCM call availability.
Merge Strategy
==============
ICE patches (1,2) through Qcom tree and MMC/UFS patches (3,4) through respective
subsystem trees as there is no dependency.
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
Changes in v2:
* Added MODULE_* macros back
* Removed spurious platform_device_put()
* Added patches to remove NULL return
---
Manivannan Sadhasivam (4):
soc: qcom: ice: Remove platform_driver support and expose as a pure library
soc: qcom: ice: Return proper error codes from devm_of_qcom_ice_get() instead of NULL
mmc: sdhci-msm: Remove NULL check from devm_of_qcom_ice_get()
scsi: ufs: ufs-qcom: Remove NULL check from devm_of_qcom_ice_get()
drivers/mmc/host/sdhci-msm.c | 10 ++--
drivers/soc/qcom/ice.c | 127 ++++++++++++++++---------------------------
drivers/ufs/host/ufs-qcom.c | 10 ++--
3 files changed, 58 insertions(+), 89 deletions(-)
---
base-commit: 8f0b4cce4481fb22653697cced8d0d04027cb1e8
change-id: 20260210-qcom-ice-fix-d2a3a045b32d
Best regards,
--
Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v2 1/4] soc: qcom: ice: Remove platform_driver support and expose as a pure library 2026-02-10 6:56 [PATCH v2 0/4] soc: qcom: ice: Remove platform_driver support and expose as a pure library Manivannan Sadhasivam via B4 Relay @ 2026-02-10 6:56 ` Manivannan Sadhasivam via B4 Relay 2026-02-10 9:39 ` Konrad Dybcio 2026-02-13 1:02 ` Eric Biggers 2026-02-10 13:43 ` [PATCH v2 0/4] " Ulf Hansson 1 sibling, 2 replies; 10+ messages in thread From: Manivannan Sadhasivam via B4 Relay @ 2026-02-10 6:56 UTC (permalink / raw) To: Bjorn Andersson, Konrad Dybcio, Abel Vesa, Adrian Hunter, Ulf Hansson, Manivannan Sadhasivam, James E.J. Bottomley, Martin K. Petersen Cc: linux-arm-msm, linux-kernel, linux-mmc, linux-scsi, Sumit Garg, mani, Manivannan Sadhasivam, stable, Abel Vesa From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com> The current platform driver design causes probe ordering races with consumers (UFS, eMMC) due to ICE's dependency on SCM firmware calls. If ICE probe fails (missing ICE SCM or DT registers), devm_of_qcom_ice_get() loops with -EPROBE_DEFER, leaving consumers non-functional even when ICE should be gracefully disabled. devm_of_qcom_ice_get() cannot know if the ICE driver probe has failed due to above reasons or it is waiting for the SCM driver. Moreover, there is no devlink dependency between ICE and consumer drivers as 'qcom,ice' is not considered as a DT 'supplier'. So the consumer drivers have no idea of when the ICE driver is going to probe. To avoid all this hassle, remove the platform driver support altogether and just expose the ICE driver as a pure library to consumer drivers. With this design, when devm_of_qcom_ice_get() is called, it will check if the ICE instance is available or not. If not, it will create one based on the ICE DT node, increase the refcount and return the handle. When the next consumer calls the API again, the ICE instance would be available. So this function will just increment the refcount and return the instance. Finally, when the consumer devices get destroyed, refcount will be decremented and finally the cleanup will happen once the last consumer goes away. For the consumers using the old DT binding of providing the separate 'ice' register range in their node, this change has no impact. This change also warrants rewording the kernel-doc of devm_of_qcom_ice_get() API. While at it, remove the duplicate kernel-doc for of_qcom_ice_get() static helper as it provides no value. Cc: stable@vger.kernel.org Cc: Abel Vesa <abel.vesa@oss.qualcomm.com> Reported-by: Sumit Garg <sumit.garg@oss.qualcomm.com> Fixes: 2afbf43a4aec ("soc: qcom: Make the Qualcomm UFS/SDCC ICE a dedicated driver") Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com> --- drivers/soc/qcom/ice.c | 118 ++++++++++++++++++------------------------------- 1 file changed, 44 insertions(+), 74 deletions(-) diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c index b203bc685cad..8e25609c7e7b 100644 --- a/drivers/soc/qcom/ice.c +++ b/drivers/soc/qcom/ice.c @@ -107,12 +107,16 @@ struct qcom_ice { struct device *dev; void __iomem *base; + struct kref refcount; struct clk *core_clk; bool use_hwkm; bool hwkm_init_complete; u8 hwkm_version; }; +static DEFINE_MUTEX(ice_mutex); +struct qcom_ice *ice_handle; + static bool qcom_ice_check_supported(struct qcom_ice *ice) { u32 regval = qcom_ice_readl(ice, QCOM_ICE_REG_VERSION); @@ -592,30 +596,18 @@ static struct qcom_ice *qcom_ice_create(struct device *dev, return engine; } -/** - * of_qcom_ice_get() - get an ICE instance from a DT node - * @dev: device pointer for the consumer device - * - * This function will provide an ICE instance either by creating one for the - * consumer device if its DT node provides the 'ice' reg range and the 'ice' - * clock (for legacy DT style). On the other hand, if consumer provides a - * phandle via 'qcom,ice' property to an ICE DT, the ICE instance will already - * be created and so this function will return that instead. - * - * Return: ICE pointer on success, NULL if there is no ICE data provided by the - * consumer or ERR_PTR() on error. - */ static struct qcom_ice *of_qcom_ice_get(struct device *dev) { struct platform_device *pdev = to_platform_device(dev); struct qcom_ice *ice; struct resource *res; void __iomem *base; - struct device_link *link; if (!dev || !dev->of_node) return ERR_PTR(-ENODEV); + guard(mutex)(&ice_mutex); + /* * In order to support legacy style devicetree bindings, we need * to create the ICE instance using the consumer device and the reg @@ -631,6 +623,16 @@ static struct qcom_ice *of_qcom_ice_get(struct device *dev) return qcom_ice_create(&pdev->dev, base); } + /* + * If the ICE node has been initialized already, just increase the + * refcount and return the handle. + */ + if (ice_handle) { + kref_get(&ice_handle->refcount); + + return ice_handle; + } + /* * If the consumer node does not provider an 'ice' reg range * (legacy DT binding), then it must at least provide a phandle @@ -643,41 +645,42 @@ static struct qcom_ice *of_qcom_ice_get(struct device *dev) pdev = of_find_device_by_node(node); if (!pdev) { - dev_err(dev, "Cannot find device node %s\n", node->name); + dev_err(dev, "Cannot find ICE platform device\n"); return ERR_PTR(-EPROBE_DEFER); } - ice = platform_get_drvdata(pdev); - if (!ice) { - dev_err(dev, "Cannot get ice instance from %s\n", - dev_name(&pdev->dev)); + base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(base)) { + dev_warn(&pdev->dev, "ICE registers not found\n"); platform_device_put(pdev); - return ERR_PTR(-EPROBE_DEFER); + return base; } - link = device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_SUPPLIER); - if (!link) { - dev_err(&pdev->dev, - "Failed to create device link to consumer %s\n", - dev_name(dev)); + ice = qcom_ice_create(&pdev->dev, base); + if (IS_ERR(ice)) { platform_device_put(pdev); - ice = ERR_PTR(-EINVAL); + return ice_handle; } - return ice; + ice_handle = ice; + kref_init(&ice_handle->refcount); + + return ice_handle; } -static void qcom_ice_put(const struct qcom_ice *ice) +static void qcom_ice_put(struct kref *kref) { - struct platform_device *pdev = to_platform_device(ice->dev); - - if (!platform_get_resource_byname(pdev, IORESOURCE_MEM, "ice")) - platform_device_put(pdev); + platform_device_put(to_platform_device(ice_handle->dev)); + ice_handle = NULL; } static void devm_of_qcom_ice_put(struct device *dev, void *res) { - qcom_ice_put(*(struct qcom_ice **)res); + const struct qcom_ice *ice = *(struct qcom_ice **)res; + struct platform_device *pdev = to_platform_device(ice->dev); + + if (!platform_get_resource_byname(pdev, IORESOURCE_MEM, "ice")) + kref_put(&ice_handle->refcount, qcom_ice_put); } /** @@ -685,11 +688,14 @@ static void devm_of_qcom_ice_put(struct device *dev, void *res) * a DT node. * @dev: device pointer for the consumer device. * - * This function will provide an ICE instance either by creating one for the - * consumer device if its DT node provides the 'ice' reg range and the 'ice' - * clock (for legacy DT style). On the other hand, if consumer provides a - * phandle via 'qcom,ice' property to an ICE DT, the ICE instance will already - * be created and so this function will return that instead. + * This function will create the ICE instance (for the first time) and increase + * its refcount if the consumer device has either 'ice' reg range (legacy DT + * binding) or the 'qcom,ice' property pointing to the ICE DT node. If the ICE + * instance was already created, it will just increase its refcount and return + * the handle. + * + * Devres automatically decrements the refcount when consumer device gets + * destroyed and frees the ICE instance when the last consumer goes away. * * Return: ICE pointer on success, NULL if there is no ICE data provided by the * consumer or ERR_PTR() on error. @@ -714,41 +720,5 @@ struct qcom_ice *devm_of_qcom_ice_get(struct device *dev) } EXPORT_SYMBOL_GPL(devm_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.51.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/4] soc: qcom: ice: Remove platform_driver support and expose as a pure library 2026-02-10 6:56 ` [PATCH v2 1/4] " Manivannan Sadhasivam via B4 Relay @ 2026-02-10 9:39 ` Konrad Dybcio 2026-02-10 12:08 ` Abel Vesa 2026-02-10 12:19 ` Manivannan Sadhasivam 2026-02-13 1:02 ` Eric Biggers 1 sibling, 2 replies; 10+ messages in thread From: Konrad Dybcio @ 2026-02-10 9:39 UTC (permalink / raw) To: manivannan.sadhasivam, Bjorn Andersson, Konrad Dybcio, Abel Vesa, Adrian Hunter, Ulf Hansson, Manivannan Sadhasivam, James E.J. Bottomley, Martin K. Petersen Cc: linux-arm-msm, linux-kernel, linux-mmc, linux-scsi, Sumit Garg, stable, Abel Vesa On 2/10/26 7:56 AM, Manivannan Sadhasivam via B4 Relay wrote: > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com> > > The current platform driver design causes probe ordering races with > consumers (UFS, eMMC) due to ICE's dependency on SCM firmware calls. If ICE > probe fails (missing ICE SCM or DT registers), devm_of_qcom_ice_get() loops > with -EPROBE_DEFER, leaving consumers non-functional even when ICE should > be gracefully disabled. devm_of_qcom_ice_get() cannot know if the ICE > driver probe has failed due to above reasons or it is waiting for the SCM > driver. [...] > -static void qcom_ice_put(const struct qcom_ice *ice) > +static void qcom_ice_put(struct kref *kref) > { > - struct platform_device *pdev = to_platform_device(ice->dev); > - > - if (!platform_get_resource_byname(pdev, IORESOURCE_MEM, "ice")) > - platform_device_put(pdev); > + platform_device_put(to_platform_device(ice_handle->dev)); > + ice_handle = NULL; > } > > static void devm_of_qcom_ice_put(struct device *dev, void *res) > { > - qcom_ice_put(*(struct qcom_ice **)res); > + const struct qcom_ice *ice = *(struct qcom_ice **)res; > + struct platform_device *pdev = to_platform_device(ice->dev); > + > + if (!platform_get_resource_byname(pdev, IORESOURCE_MEM, "ice")) > + kref_put(&ice_handle->refcount, qcom_ice_put); IIUC this makes the refcount go down only in the legacy DT case - why? Konrad ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/4] soc: qcom: ice: Remove platform_driver support and expose as a pure library 2026-02-10 9:39 ` Konrad Dybcio @ 2026-02-10 12:08 ` Abel Vesa 2026-02-10 12:19 ` Manivannan Sadhasivam 1 sibling, 0 replies; 10+ messages in thread From: Abel Vesa @ 2026-02-10 12:08 UTC (permalink / raw) To: Konrad Dybcio Cc: manivannan.sadhasivam, Bjorn Andersson, Konrad Dybcio, Abel Vesa, Adrian Hunter, Ulf Hansson, Manivannan Sadhasivam, James E.J. Bottomley, Martin K. Petersen, linux-arm-msm, linux-kernel, linux-mmc, linux-scsi, Sumit Garg, stable On 26-02-10 10:39:54, Konrad Dybcio wrote: > On 2/10/26 7:56 AM, Manivannan Sadhasivam via B4 Relay wrote: > > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com> > > > > The current platform driver design causes probe ordering races with > > consumers (UFS, eMMC) due to ICE's dependency on SCM firmware calls. If ICE > > probe fails (missing ICE SCM or DT registers), devm_of_qcom_ice_get() loops > > with -EPROBE_DEFER, leaving consumers non-functional even when ICE should > > be gracefully disabled. devm_of_qcom_ice_get() cannot know if the ICE > > driver probe has failed due to above reasons or it is waiting for the SCM > > driver. > > [...] > > > -static void qcom_ice_put(const struct qcom_ice *ice) > > +static void qcom_ice_put(struct kref *kref) > > { > > - struct platform_device *pdev = to_platform_device(ice->dev); > > - > > - if (!platform_get_resource_byname(pdev, IORESOURCE_MEM, "ice")) > > - platform_device_put(pdev); > > + platform_device_put(to_platform_device(ice_handle->dev)); > > + ice_handle = NULL; > > } > > > > static void devm_of_qcom_ice_put(struct device *dev, void *res) > > { > > - qcom_ice_put(*(struct qcom_ice **)res); > > + const struct qcom_ice *ice = *(struct qcom_ice **)res; > > + struct platform_device *pdev = to_platform_device(ice->dev); > > + > > + if (!platform_get_resource_byname(pdev, IORESOURCE_MEM, "ice")) > > + kref_put(&ice_handle->refcount, qcom_ice_put); > > IIUC this makes the refcount go down only in the legacy DT case - why? Good point. I think it needs to go down unconditionally here. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/4] soc: qcom: ice: Remove platform_driver support and expose as a pure library 2026-02-10 9:39 ` Konrad Dybcio 2026-02-10 12:08 ` Abel Vesa @ 2026-02-10 12:19 ` Manivannan Sadhasivam 2026-02-12 12:05 ` Konrad Dybcio 1 sibling, 1 reply; 10+ messages in thread From: Manivannan Sadhasivam @ 2026-02-10 12:19 UTC (permalink / raw) To: Konrad Dybcio Cc: manivannan.sadhasivam, Bjorn Andersson, Konrad Dybcio, Abel Vesa, Adrian Hunter, Ulf Hansson, James E.J. Bottomley, Martin K. Petersen, linux-arm-msm, linux-kernel, linux-mmc, linux-scsi, Sumit Garg, stable, Abel Vesa On Tue, Feb 10, 2026 at 10:39:54AM +0100, Konrad Dybcio wrote: > On 2/10/26 7:56 AM, Manivannan Sadhasivam via B4 Relay wrote: > > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com> > > > > The current platform driver design causes probe ordering races with > > consumers (UFS, eMMC) due to ICE's dependency on SCM firmware calls. If ICE > > probe fails (missing ICE SCM or DT registers), devm_of_qcom_ice_get() loops > > with -EPROBE_DEFER, leaving consumers non-functional even when ICE should > > be gracefully disabled. devm_of_qcom_ice_get() cannot know if the ICE > > driver probe has failed due to above reasons or it is waiting for the SCM > > driver. > > [...] > > > -static void qcom_ice_put(const struct qcom_ice *ice) > > +static void qcom_ice_put(struct kref *kref) > > { > > - struct platform_device *pdev = to_platform_device(ice->dev); > > - > > - if (!platform_get_resource_byname(pdev, IORESOURCE_MEM, "ice")) > > - platform_device_put(pdev); > > + platform_device_put(to_platform_device(ice_handle->dev)); > > + ice_handle = NULL; > > } > > > > static void devm_of_qcom_ice_put(struct device *dev, void *res) > > { > > - qcom_ice_put(*(struct qcom_ice **)res); > > + const struct qcom_ice *ice = *(struct qcom_ice **)res; > > + struct platform_device *pdev = to_platform_device(ice->dev); > > + > > + if (!platform_get_resource_byname(pdev, IORESOURCE_MEM, "ice")) > > + kref_put(&ice_handle->refcount, qcom_ice_put); > > IIUC this makes the refcount go down only in the legacy DT case - why? > It is the other way around, no? Absence of 'ice' reg range in the consumer node means it is using *new* binding. - Mani -- மணிவண்ணன் சதாசிவம் ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/4] soc: qcom: ice: Remove platform_driver support and expose as a pure library 2026-02-10 12:19 ` Manivannan Sadhasivam @ 2026-02-12 12:05 ` Konrad Dybcio 0 siblings, 0 replies; 10+ messages in thread From: Konrad Dybcio @ 2026-02-12 12:05 UTC (permalink / raw) To: Manivannan Sadhasivam Cc: manivannan.sadhasivam, Bjorn Andersson, Konrad Dybcio, Abel Vesa, Adrian Hunter, Ulf Hansson, James E.J. Bottomley, Martin K. Petersen, linux-arm-msm, linux-kernel, linux-mmc, linux-scsi, Sumit Garg, stable, Abel Vesa On 2/10/26 1:19 PM, Manivannan Sadhasivam wrote: > On Tue, Feb 10, 2026 at 10:39:54AM +0100, Konrad Dybcio wrote: >> On 2/10/26 7:56 AM, Manivannan Sadhasivam via B4 Relay wrote: >>> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com> >>> >>> The current platform driver design causes probe ordering races with >>> consumers (UFS, eMMC) due to ICE's dependency on SCM firmware calls. If ICE >>> probe fails (missing ICE SCM or DT registers), devm_of_qcom_ice_get() loops >>> with -EPROBE_DEFER, leaving consumers non-functional even when ICE should >>> be gracefully disabled. devm_of_qcom_ice_get() cannot know if the ICE >>> driver probe has failed due to above reasons or it is waiting for the SCM >>> driver. >> >> [...] >> >>> -static void qcom_ice_put(const struct qcom_ice *ice) >>> +static void qcom_ice_put(struct kref *kref) >>> { >>> - struct platform_device *pdev = to_platform_device(ice->dev); >>> - >>> - if (!platform_get_resource_byname(pdev, IORESOURCE_MEM, "ice")) >>> - platform_device_put(pdev); >>> + platform_device_put(to_platform_device(ice_handle->dev)); >>> + ice_handle = NULL; >>> } >>> >>> static void devm_of_qcom_ice_put(struct device *dev, void *res) >>> { >>> - qcom_ice_put(*(struct qcom_ice **)res); >>> + const struct qcom_ice *ice = *(struct qcom_ice **)res; >>> + struct platform_device *pdev = to_platform_device(ice->dev); >>> + >>> + if (!platform_get_resource_byname(pdev, IORESOURCE_MEM, "ice")) >>> + kref_put(&ice_handle->refcount, qcom_ice_put); >> >> IIUC this makes the refcount go down only in the legacy DT case - why? >> > > It is the other way around, no? Absence of 'ice' reg range in the consumer node > means it is using *new* binding. Yeah obviously you're right I suppose in the legacy case we don't need any refcounting, since there's only a single reference, from the storage controller device.. This assumption would break if someone had a funny idea to specify the same "ice" range for sdcc and ufs though (via the legacy binding), but let's hope no one does that.. Konrad ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/4] soc: qcom: ice: Remove platform_driver support and expose as a pure library 2026-02-10 6:56 ` [PATCH v2 1/4] " Manivannan Sadhasivam via B4 Relay 2026-02-10 9:39 ` Konrad Dybcio @ 2026-02-13 1:02 ` Eric Biggers 2026-02-13 2:17 ` Manivannan Sadhasivam 1 sibling, 1 reply; 10+ messages in thread From: Eric Biggers @ 2026-02-13 1:02 UTC (permalink / raw) To: manivannan.sadhasivam Cc: Bjorn Andersson, Konrad Dybcio, Abel Vesa, Adrian Hunter, Ulf Hansson, Manivannan Sadhasivam, James E.J. Bottomley, Martin K. Petersen, linux-arm-msm, linux-kernel, linux-mmc, linux-scsi, Sumit Garg, stable, Abel Vesa On Tue, Feb 10, 2026 at 12:26:50PM +0530, Manivannan Sadhasivam via B4 Relay wrote: > drivers/soc/qcom/ice.c | 118 ++++++++++++++++++------------------------------- > 1 file changed, 44 insertions(+), 74 deletions(-) I don't yet know enough to be confident that this is the correct fix, but there are a few things I noticed that look like bugs: > +static DEFINE_MUTEX(ice_mutex); > +struct qcom_ice *ice_handle; ice_handle is used only in this file, so it should be static > @@ -643,41 +645,42 @@ static struct qcom_ice *of_qcom_ice_get(struct device *dev) [...] > + ice = qcom_ice_create(&pdev->dev, base); > + if (IS_ERR(ice)) { > platform_device_put(pdev); > - ice = ERR_PTR(-EINVAL); > + return ice_handle; > } This error path returns NULL, where this patch seems to have been intended to remove NULL as a possible return value. > -static void qcom_ice_put(const struct qcom_ice *ice) > +static void qcom_ice_put(struct kref *kref) > { > - struct platform_device *pdev = to_platform_device(ice->dev); > - > - if (!platform_get_resource_byname(pdev, IORESOURCE_MEM, "ice")) > - platform_device_put(pdev); > + platform_device_put(to_platform_device(ice_handle->dev)); > + ice_handle = NULL; > } Elsewhere ice_handle is protected by ice_mutex, but this seems to modify it without holding the mutex. I'm also wondering what happens if all consumer devices are removed. platform_device_put() gets executed on the ICE platform_device for each one, but does that actually drop the last reference and cause the resources allocated with devm_*() to be freed? On do they stick around until/unless the ICE device is actually removed as well? > static void devm_of_qcom_ice_put(struct device *dev, void *res) > { > - qcom_ice_put(*(struct qcom_ice **)res); > + const struct qcom_ice *ice = *(struct qcom_ice **)res; > + struct platform_device *pdev = to_platform_device(ice->dev); > + > + if (!platform_get_resource_byname(pdev, IORESOURCE_MEM, "ice")) > + kref_put(&ice_handle->refcount, qcom_ice_put); > } Above probably should use the ice local variable, not ice_handle. - Eric ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/4] soc: qcom: ice: Remove platform_driver support and expose as a pure library 2026-02-13 1:02 ` Eric Biggers @ 2026-02-13 2:17 ` Manivannan Sadhasivam 0 siblings, 0 replies; 10+ messages in thread From: Manivannan Sadhasivam @ 2026-02-13 2:17 UTC (permalink / raw) To: Eric Biggers Cc: manivannan.sadhasivam, Bjorn Andersson, Konrad Dybcio, Abel Vesa, Adrian Hunter, Ulf Hansson, James E.J. Bottomley, Martin K. Petersen, linux-arm-msm, linux-kernel, linux-mmc, linux-scsi, Sumit Garg, stable, Abel Vesa On Thu, Feb 12, 2026 at 05:02:53PM -0800, Eric Biggers wrote: > On Tue, Feb 10, 2026 at 12:26:50PM +0530, Manivannan Sadhasivam via B4 Relay wrote: > > drivers/soc/qcom/ice.c | 118 ++++++++++++++++++------------------------------- > > 1 file changed, 44 insertions(+), 74 deletions(-) > > I don't yet know enough to be confident that this is the correct fix, > but there are a few things I noticed that look like bugs: > > > +static DEFINE_MUTEX(ice_mutex); > > +struct qcom_ice *ice_handle; > > ice_handle is used only in this file, so it should be static > > > @@ -643,41 +645,42 @@ static struct qcom_ice *of_qcom_ice_get(struct device *dev) > [...] > > + ice = qcom_ice_create(&pdev->dev, base); > > + if (IS_ERR(ice)) { > > platform_device_put(pdev); > > - ice = ERR_PTR(-EINVAL); > > + return ice_handle; > > } > > This error path returns NULL, where this patch seems to have been > intended to remove NULL as a possible return value. > Oops... I should return 'ice' here, not 'ice_handle'. > > -static void qcom_ice_put(const struct qcom_ice *ice) > > +static void qcom_ice_put(struct kref *kref) > > { > > - struct platform_device *pdev = to_platform_device(ice->dev); > > - > > - if (!platform_get_resource_byname(pdev, IORESOURCE_MEM, "ice")) > > - platform_device_put(pdev); > > + platform_device_put(to_platform_device(ice_handle->dev)); > > + ice_handle = NULL; > > } > > Elsewhere ice_handle is protected by ice_mutex, but this seems to modify > it without holding the mutex. > I'll add it. > I'm also wondering what happens if all consumer devices are removed. > platform_device_put() gets executed on the ICE platform_device for each > one, but does that actually drop the last reference and cause the > resources allocated with devm_*() to be freed? On do they stick around > until/unless the ICE device is actually removed as well? > No, they'll stick around because the platform device itself won't be removed. of_qcom_ice_get() increments the refcount due to of_find_device_by_node() and we keep the refcount as we expect the platform device to be available until all the consumers call devm_of_qcom_ice_put(). Then we'll finally drop our own refcount. But I get your concern that we are not freeing the ioremap. > > static void devm_of_qcom_ice_put(struct device *dev, void *res) > > { > > - qcom_ice_put(*(struct qcom_ice **)res); > > + const struct qcom_ice *ice = *(struct qcom_ice **)res; > > + struct platform_device *pdev = to_platform_device(ice->dev); > > + > > + if (!platform_get_resource_byname(pdev, IORESOURCE_MEM, "ice")) > > + kref_put(&ice_handle->refcount, qcom_ice_put); > > } > > Above probably should use the ice local variable, not ice_handle. > Ack. - Mani -- மணிவண்ணன் சதாசிவம் ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 0/4] soc: qcom: ice: Remove platform_driver support and expose as a pure library 2026-02-10 6:56 [PATCH v2 0/4] soc: qcom: ice: Remove platform_driver support and expose as a pure library Manivannan Sadhasivam via B4 Relay 2026-02-10 6:56 ` [PATCH v2 1/4] " Manivannan Sadhasivam via B4 Relay @ 2026-02-10 13:43 ` Ulf Hansson 2026-02-10 13:54 ` Manivannan Sadhasivam 1 sibling, 1 reply; 10+ messages in thread From: Ulf Hansson @ 2026-02-10 13:43 UTC (permalink / raw) To: manivannan.sadhasivam Cc: Bjorn Andersson, Konrad Dybcio, Abel Vesa, Adrian Hunter, Manivannan Sadhasivam, James E.J. Bottomley, Martin K. Petersen, linux-arm-msm, linux-kernel, linux-mmc, linux-scsi, Sumit Garg, stable, Abel Vesa On Tue, 10 Feb 2026 at 07:56, Manivannan Sadhasivam via B4 Relay <devnull+manivannan.sadhasivam.oss.qualcomm.com@kernel.org> wrote: > > Hi, > > This series removes the platform_driver support from Qcom ICE driver and > exposes it as a pure library to the clients to avoid race conditions with ICE > SCM call availability. > > Merge Strategy > ============== > > ICE patches (1,2) through Qcom tree and MMC/UFS patches (3,4) through respective > subsystem trees as there is no dependency. Just wanted to double check that this is really correct.... The propagated error codes (or NULL) are changed in patch1/patch2, so is it really okay to pick the mmc/ufs patches (patch3 and patch4) independently? Kind regards Uffe > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com> > --- > Changes in v2: > > * Added MODULE_* macros back > * Removed spurious platform_device_put() > * Added patches to remove NULL return > > --- > Manivannan Sadhasivam (4): > soc: qcom: ice: Remove platform_driver support and expose as a pure library > soc: qcom: ice: Return proper error codes from devm_of_qcom_ice_get() instead of NULL > mmc: sdhci-msm: Remove NULL check from devm_of_qcom_ice_get() > scsi: ufs: ufs-qcom: Remove NULL check from devm_of_qcom_ice_get() > > drivers/mmc/host/sdhci-msm.c | 10 ++-- > drivers/soc/qcom/ice.c | 127 ++++++++++++++++--------------------------- > drivers/ufs/host/ufs-qcom.c | 10 ++-- > 3 files changed, 58 insertions(+), 89 deletions(-) > --- > base-commit: 8f0b4cce4481fb22653697cced8d0d04027cb1e8 > change-id: 20260210-qcom-ice-fix-d2a3a045b32d > > Best regards, > -- > Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com> > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 0/4] soc: qcom: ice: Remove platform_driver support and expose as a pure library 2026-02-10 13:43 ` [PATCH v2 0/4] " Ulf Hansson @ 2026-02-10 13:54 ` Manivannan Sadhasivam 0 siblings, 0 replies; 10+ messages in thread From: Manivannan Sadhasivam @ 2026-02-10 13:54 UTC (permalink / raw) To: Ulf Hansson Cc: manivannan.sadhasivam, Bjorn Andersson, Konrad Dybcio, Abel Vesa, Adrian Hunter, James E.J. Bottomley, Martin K. Petersen, linux-arm-msm, linux-kernel, linux-mmc, linux-scsi, Sumit Garg, stable, Abel Vesa On Tue, Feb 10, 2026 at 02:43:53PM +0100, Ulf Hansson wrote: > On Tue, 10 Feb 2026 at 07:56, Manivannan Sadhasivam via B4 Relay > <devnull+manivannan.sadhasivam.oss.qualcomm.com@kernel.org> wrote: > > > > Hi, > > > > This series removes the platform_driver support from Qcom ICE driver and > > exposes it as a pure library to the clients to avoid race conditions with ICE > > SCM call availability. > > > > Merge Strategy > > ============== > > > > ICE patches (1,2) through Qcom tree and MMC/UFS patches (3,4) through respective > > subsystem trees as there is no dependency. > > Just wanted to double check that this is really correct.... > > The propagated error codes (or NULL) are changed in patch1/patch2, so > is it really okay to pick the mmc/ufs patches (patch3 and patch4) > independently? > Darn... No, I was being stupid here. Without patch 2, removing NULL check in patches 3 and 4 will break the respective drivers, but patch 1 is fine though. Thanks for spotting this. All patches should go at once through Qcom tree. - Mani > Kind regards > Uffe > > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com> > > --- > > Changes in v2: > > > > * Added MODULE_* macros back > > * Removed spurious platform_device_put() > > * Added patches to remove NULL return > > > > --- > > Manivannan Sadhasivam (4): > > soc: qcom: ice: Remove platform_driver support and expose as a pure library > > soc: qcom: ice: Return proper error codes from devm_of_qcom_ice_get() instead of NULL > > mmc: sdhci-msm: Remove NULL check from devm_of_qcom_ice_get() > > scsi: ufs: ufs-qcom: Remove NULL check from devm_of_qcom_ice_get() > > > > drivers/mmc/host/sdhci-msm.c | 10 ++-- > > drivers/soc/qcom/ice.c | 127 ++++++++++++++++--------------------------- > > drivers/ufs/host/ufs-qcom.c | 10 ++-- > > 3 files changed, 58 insertions(+), 89 deletions(-) > > --- > > base-commit: 8f0b4cce4481fb22653697cced8d0d04027cb1e8 > > change-id: 20260210-qcom-ice-fix-d2a3a045b32d > > > > Best regards, > > -- > > Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com> > > > > -- மணிவண்ணன் சதாசிவம் ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2026-02-13 2:17 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-02-10 6:56 [PATCH v2 0/4] soc: qcom: ice: Remove platform_driver support and expose as a pure library Manivannan Sadhasivam via B4 Relay 2026-02-10 6:56 ` [PATCH v2 1/4] " Manivannan Sadhasivam via B4 Relay 2026-02-10 9:39 ` Konrad Dybcio 2026-02-10 12:08 ` Abel Vesa 2026-02-10 12:19 ` Manivannan Sadhasivam 2026-02-12 12:05 ` Konrad Dybcio 2026-02-13 1:02 ` Eric Biggers 2026-02-13 2:17 ` Manivannan Sadhasivam 2026-02-10 13:43 ` [PATCH v2 0/4] " Ulf Hansson 2026-02-10 13:54 ` Manivannan Sadhasivam
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox