| Message ID | 20240424-qcom-pd-mapper-v7-1-05f7fc646e0f@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | soc: qcom: add in-kernel pd-mapper implementation | expand |
On 4/24/2024 2:27 AM, Dmitry Baryshkov wrote: > 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") > Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8550-QRD > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/soc/qcom/pdr_interface.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/soc/qcom/pdr_interface.c b/drivers/soc/qcom/pdr_interface.c > index a1b6a4081dea..19cfe4b41235 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 = { > These two functions are provided as qmi_ops handlers in pdr_locator_ops. Aren't they serialized in the qmi handle's workqueue since it as an ordered_workqueue? Even in a fast pdr scenario I don't think we would see a race condition between these two functions. The other access these two functions do race against is in the pdr_notifier_work. I think you would need to protect locator_addr in pdr_get_domain_list since the qmi_send_request there uses 'pdr->locator_addr'. Thanks! Chris
On Thu, 25 Apr 2024 at 22:30, Chris Lew <quic_clew@quicinc.com> wrote: > > > On 4/24/2024 2:27 AM, Dmitry Baryshkov wrote: > > 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") > > Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8550-QRD > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/soc/qcom/pdr_interface.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/soc/qcom/pdr_interface.c b/drivers/soc/qcom/pdr_interface.c > > index a1b6a4081dea..19cfe4b41235 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 = { > > > > These two functions are provided as qmi_ops handlers in pdr_locator_ops. > Aren't they serialized in the qmi handle's workqueue since it as an > ordered_workqueue? Even in a fast pdr scenario I don't think we would > see a race condition between these two functions. > > The other access these two functions do race against is in the > pdr_notifier_work. I think you would need to protect locator_addr in > pdr_get_domain_list since the qmi_send_request there uses > 'pdr->locator_addr'. Thanks, I missed it initially. I think I'd keep the rest of the changes and expand the lock to cover pdr_get_domain_list(). > > Thanks! > Chris
diff --git a/drivers/soc/qcom/pdr_interface.c b/drivers/soc/qcom/pdr_interface.c index a1b6a4081dea..19cfe4b41235 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 = {
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") Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8550-QRD Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/soc/qcom/pdr_interface.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)