From 1ac98d6484c4a53e9b3ed1f59d03a2dd2d287b70 Mon Sep 17 00:00:00 2001 From: Krzysztof Kozlowski Date: Sat, 11 Jan 2025 18:23:26 +0100 Subject: [PATCH 1/4] soc: qcom: pmic_glink: Drop redundant pg assignment before taking lock Commit e9f826b0459f ("soc: qcom: pmic_glink: simplify locking with guard()") was on top of a fix [1] which was moving the 'pg = __pmic_glink' assignment into the critical section. Unfortunately the actual fix was not applied and instead rebased version of the next patch got in. The resulting code is in general correct, but now there is a duplicated assignment 'pg = __pmic_glink'. [1] https://lore.kernel.org/all/20240822164815.230167-1-krzysztof.kozlowski@linaro.org/ Fixes: e9f826b0459f ("soc: qcom: pmic_glink: simplify locking with guard()") Signed-off-by: Krzysztof Kozlowski Link: https://lore.kernel.org/r/20250111172326.101779-1-krzysztof.kozlowski@linaro.org Signed-off-by: Bjorn Andersson --- drivers/soc/qcom/pmic_glink.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/soc/qcom/pmic_glink.c b/drivers/soc/qcom/pmic_glink.c index 052c292eeda6..cde19cdfd3c7 100644 --- a/drivers/soc/qcom/pmic_glink.c +++ b/drivers/soc/qcom/pmic_glink.c @@ -233,7 +233,7 @@ static void pmic_glink_pdr_callback(int state, char *svc_path, void *priv) static int pmic_glink_rpmsg_probe(struct rpmsg_device *rpdev) { - struct pmic_glink *pg = __pmic_glink; + struct pmic_glink *pg; guard(mutex)(&__pmic_glink_lock); pg = __pmic_glink; From 7f048b202333b967782a98aa21bb3354dc379bbf Mon Sep 17 00:00:00 2001 From: Dan Carpenter Date: Wed, 15 Jan 2025 09:52:53 +0300 Subject: [PATCH 2/4] firmware: qcom: scm: Fix error code in probe() Set the error code if devm_qcom_tzmem_pool_new() fails. Don't return success. Fixes: 1e76b546e6fc ("firmware: qcom: scm: Cleanup global '__scm' on probe failures") Signed-off-by: Dan Carpenter Reviewed-by: Krzysztof Kozlowski Link: https://lore.kernel.org/r/a0845467-4f83-4070-ab1e-ff7e6764609f@stanley.mountain Signed-off-by: Bjorn Andersson --- drivers/firmware/qcom/qcom_scm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c index f0569bb9411f..fc4d67e4c4a6 100644 --- a/drivers/firmware/qcom/qcom_scm.c +++ b/drivers/firmware/qcom/qcom_scm.c @@ -2301,8 +2301,8 @@ static int qcom_scm_probe(struct platform_device *pdev) __scm->mempool = devm_qcom_tzmem_pool_new(__scm->dev, &pool_config); if (IS_ERR(__scm->mempool)) { - dev_err_probe(__scm->dev, PTR_ERR(__scm->mempool), - "Failed to create the SCM memory pool\n"); + ret = dev_err_probe(__scm->dev, PTR_ERR(__scm->mempool), + "Failed to create the SCM memory pool\n"); goto err; } From da8d493a80993972c427002684d0742560f3be4a Mon Sep 17 00:00:00 2001 From: Johan Hovold Date: Mon, 20 Jan 2025 16:10:00 +0100 Subject: [PATCH 3/4] firmware: qcom: uefisecapp: fix efivars registration race Since the conversion to using the TZ allocator, the efivars service is registered before the memory pool has been allocated, something which can lead to a NULL-pointer dereference in case of a racing EFI variable access. Make sure that all resources have been set up before registering the efivars. Fixes: 6612103ec35a ("firmware: qcom: qseecom: convert to using the TZ allocator") Cc: stable@vger.kernel.org # 6.11 Cc: Bartosz Golaszewski Signed-off-by: Johan Hovold Reviewed-by: Konrad Dybcio Reviewed-by: Maximilian Luz Reviewed-by: Bartosz Golaszewski Link: https://lore.kernel.org/r/20250120151000.13870-1-johan+linaro@kernel.org Signed-off-by: Bjorn Andersson --- .../firmware/qcom/qcom_qseecom_uefisecapp.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c b/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c index 447246bd04be..98a463e9774b 100644 --- a/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c +++ b/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c @@ -814,15 +814,6 @@ static int qcom_uefisecapp_probe(struct auxiliary_device *aux_dev, qcuefi->client = container_of(aux_dev, struct qseecom_client, aux_dev); - auxiliary_set_drvdata(aux_dev, qcuefi); - status = qcuefi_set_reference(qcuefi); - if (status) - return status; - - status = efivars_register(&qcuefi->efivars, &qcom_efivar_ops); - if (status) - qcuefi_set_reference(NULL); - memset(&pool_config, 0, sizeof(pool_config)); pool_config.initial_size = SZ_4K; pool_config.policy = QCOM_TZMEM_POLICY_MULTIPLIER; @@ -833,6 +824,15 @@ static int qcom_uefisecapp_probe(struct auxiliary_device *aux_dev, if (IS_ERR(qcuefi->mempool)) return PTR_ERR(qcuefi->mempool); + auxiliary_set_drvdata(aux_dev, qcuefi); + status = qcuefi_set_reference(qcuefi); + if (status) + return status; + + status = efivars_register(&qcuefi->efivars, &qcom_efivar_ops); + if (status) + qcuefi_set_reference(NULL); + return status; } From 2eeb03ad9f42dfece63051be2400af487ddb96d2 Mon Sep 17 00:00:00 2001 From: Saranya R Date: Wed, 12 Feb 2025 22:07:20 +0530 Subject: [PATCH 4/4] soc: qcom: pdr: Fix the potential deadlock When some client process A call pdr_add_lookup() to add the look up for the service and does schedule locator work, later a process B got a new server packet indicating locator is up and call pdr_locator_new_server() which eventually sets pdr->locator_init_complete to true which process A sees and takes list lock and queries domain list but it will timeout due to deadlock as the response will queued to the same qmi->wq and it is ordered workqueue and process B is not able to complete new server request work due to deadlock on list lock. Fix it by removing the unnecessary list iteration as the list iteration is already being done inside locator work, so avoid it here and just call schedule_work() here. Process A Process B process_scheduled_works() pdr_add_lookup() qmi_data_ready_work() process_scheduled_works() pdr_locator_new_server() pdr->locator_init_complete=true; pdr_locator_work() mutex_lock(&pdr->list_lock); pdr_locate_service() mutex_lock(&pdr->list_lock); pdr_get_domain_list() pr_err("PDR: %s get domain list txn wait failed: %d\n", req->service_name, ret); Timeout error log due to deadlock: " PDR: tms/servreg get domain list txn wait failed: -110 PDR: service lookup for msm/adsp/sensor_pd:tms/servreg failed: -110 " Thanks to Bjorn and Johan for letting me know that this commit also fixes an audio regression when using the in-kernel pd-mapper as that makes it easier to hit this race. [1] Link: https://lore.kernel.org/lkml/Zqet8iInnDhnxkT9@hovoldconsulting.com/ # [1] Fixes: fbe639b44a82 ("soc: qcom: Introduce Protection Domain Restart helpers") CC: stable@vger.kernel.org Reviewed-by: Bjorn Andersson Tested-by: Bjorn Andersson Tested-by: Johan Hovold Signed-off-by: Saranya R Co-developed-by: Mukesh Ojha Signed-off-by: Mukesh Ojha Link: https://lore.kernel.org/r/20250212163720.1577876-1-mukesh.ojha@oss.qualcomm.com Signed-off-by: Bjorn Andersson --- drivers/soc/qcom/pdr_interface.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/drivers/soc/qcom/pdr_interface.c b/drivers/soc/qcom/pdr_interface.c index 328b6153b2be..71be378d2e43 100644 --- a/drivers/soc/qcom/pdr_interface.c +++ b/drivers/soc/qcom/pdr_interface.c @@ -75,7 +75,6 @@ static int pdr_locator_new_server(struct qmi_handle *qmi, { struct pdr_handle *pdr = container_of(qmi, struct pdr_handle, locator_hdl); - struct pdr_service *pds; mutex_lock(&pdr->lock); /* Create a local client port for QMI communication */ @@ -87,12 +86,7 @@ static int pdr_locator_new_server(struct qmi_handle *qmi, mutex_unlock(&pdr->lock); /* Service pending lookup requests */ - mutex_lock(&pdr->list_lock); - list_for_each_entry(pds, &pdr->lookups, node) { - if (pds->need_locator_lookup) - schedule_work(&pdr->locator_work); - } - mutex_unlock(&pdr->list_lock); + schedule_work(&pdr->locator_work); return 0; }