| Message ID | 20240311-qcom-pd-mapper-v4-1-24679cca5c24@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | soc: qcom: add in-kernel pd-mapper implementation | expand |
On 3/11/2024 8:34 AM, Dmitry Baryshkov wrote: > @@ -133,11 +133,13 @@ static int pdr_register_listener(struct pdr_handle *pdr, > req.enable = enable; > strscpy(req.service_path, pds->service_path, sizeof(req.service_path)); > > + mutex_lock(&pdr->lock); > ret = qmi_send_request(&pdr->notifier_hdl, &pds->addr, > &txn, SERVREG_REGISTER_LISTENER_REQ, > SERVREG_REGISTER_LISTENER_REQ_LEN, > servreg_register_listener_req_ei, > &req); > + mutex_unlock(&pdr->lock); > if (ret < 0) { > qmi_txn_cancel(&txn); > return ret; > Hi Dmitry, What is the reason for taking the pdr lock here? The addr struct passed into qmi_send_request is from the pdr_service. I think this is different from the pdr_handle we are protecting in the other parts of the patch. Thanks, Chris
On Thu, 14 Mar 2024 at 01:35, Chris Lew <quic_clew@quicinc.com> wrote: > > > > On 3/11/2024 8:34 AM, Dmitry Baryshkov wrote: > > @@ -133,11 +133,13 @@ static int pdr_register_listener(struct pdr_handle *pdr, > > req.enable = enable; > > strscpy(req.service_path, pds->service_path, sizeof(req.service_path)); > > > > + mutex_lock(&pdr->lock); > > ret = qmi_send_request(&pdr->notifier_hdl, &pds->addr, > > &txn, SERVREG_REGISTER_LISTENER_REQ, > > SERVREG_REGISTER_LISTENER_REQ_LEN, > > servreg_register_listener_req_ei, > > &req); > > + mutex_unlock(&pdr->lock); > > if (ret < 0) { > > qmi_txn_cancel(&txn); > > return ret; > > > > Hi Dmitry, > > What is the reason for taking the pdr lock here? The addr struct passed > into qmi_send_request is from the pdr_service. I think this is different > from the pdr_handle we are protecting in the other parts of the patch. Indeed, we should be taking pdr->list_lock.
diff --git a/drivers/soc/qcom/pdr_interface.c b/drivers/soc/qcom/pdr_interface.c index a1b6a4081dea..7b5fdf9fd3d7 100644 --- a/drivers/soc/qcom/pdr_interface.c +++ b/drivers/soc/qcom/pdr_interface.c @@ -76,12 +76,12 @@ static int pdr_locator_new_server(struct qmi_handle *qmi, locator_hdl); struct pdr_service *pds; + mutex_lock(&pdr->lock); /* Create a local client port for QMI communication */ pdr->locator_addr.sq_family = AF_QIPCRTR; pdr->locator_addr.sq_node = svc->node; pdr->locator_addr.sq_port = svc->port; - mutex_lock(&pdr->lock); pdr->locator_init_complete = true; mutex_unlock(&pdr->lock); @@ -104,10 +104,10 @@ static void pdr_locator_del_server(struct qmi_handle *qmi, mutex_lock(&pdr->lock); pdr->locator_init_complete = false; - mutex_unlock(&pdr->lock); pdr->locator_addr.sq_node = 0; pdr->locator_addr.sq_port = 0; + mutex_unlock(&pdr->lock); } static const struct qmi_ops pdr_locator_ops = { @@ -133,11 +133,13 @@ static int pdr_register_listener(struct pdr_handle *pdr, req.enable = enable; strscpy(req.service_path, pds->service_path, sizeof(req.service_path)); + mutex_lock(&pdr->lock); ret = qmi_send_request(&pdr->notifier_hdl, &pds->addr, &txn, SERVREG_REGISTER_LISTENER_REQ, SERVREG_REGISTER_LISTENER_REQ_LEN, servreg_register_listener_req_ei, &req); + mutex_unlock(&pdr->lock); if (ret < 0) { qmi_txn_cancel(&txn); return ret;
If the service locator server is restarted fast enough, the PDR can rewrite locator_addr fields concurrently. Protect them by placing modification of those fields under the main pdr->lock. Fixes: fbe639b44a82 ("soc: qcom: Introduce Protection Domain Restart helpers") Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/soc/qcom/pdr_interface.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)