Re: [PATCH v8 1/5] soc: qcom: pdr: protect locator_addr with the main mutex

[Chris Lew]

Hi Dmitry,

On 5/11/2024 2:56 PM, Dmitry Baryshkov wrote:
...
@@ -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 = {
@@ -365,6 +365,7 @@ static int pdr_get_domain_list(struct 
servreg_get_domain_list_req *req,
        if (ret < 0)
                return ret;
  
+	mutex_lock(&pdr->lock);
        ret = qmi_send_request(&pdr->locator_hdl,
                               &pdr->locator_addr,
                               &txn, SERVREG_GET_DOMAIN_LIST_REQ,
@@ -373,15 +374,16 @@ static int pdr_get_domain_list(struct 
servreg_get_domain_list_req *req,
                               req);
        if (ret < 0) {
                qmi_txn_cancel(&txn);
-               return ret;
+               goto err_unlock;
        }
  
  	ret = qmi_txn_wait(&txn, 5 * HZ);
        if (ret < 0) {
                pr_err("PDR: %s get domain list txn wait failed: %d\n",
                       req->service_name, ret);
-               return ret;
+               goto err_unlock;
        }
+       mutex_unlock(&pdr->lock);

I'm not sure it is necessary to hold the the mutex during the 
qmi_txn_wait() since the only variable we are trying to protect is 
locator_addr.

Wouldn't this delay other work like new/del server notifications if this 
qmi service is delayed or non-responsive?

Thanks,
Chris

  
  	if (resp->resp.result != QMI_RESULT_SUCCESS_V01) {
                pr_err("PDR: %s get domain list failed: 0x%x\n",
@@ -390,6 +392,11 @@ static int pdr_get_domain_list(struct 
servreg_get_domain_list_req *req,
        }
  
  	return 0;
+
+err_unlock:
+       mutex_unlock(&pdr->lock);
+
+       return ret;
  }