Fix two issues with the way the pmic_glink driver handles its clients.
First issue is mostly theoretical, while the second issue can easily be
reproduced if the drivers are built as modules.
Signed-off-by: Dmitry Baryshkov <[email protected]>
---
Dmitry Baryshkov (2):
soc: qcom: pmic_glink: don't traverse clients list without a lock
soc: qcom: pmic_glink: notify clients about the current state
drivers/soc/qcom/pmic_glink.c | 7 +++++++
1 file changed, 7 insertions(+)
---
base-commit: a6bd6c9333397f5a0e2667d4d82fef8c970108f2
change-id: 20240402-pmic-glink-fix-clients-5df0bab3e871
Best regards,
--
Dmitry Baryshkov <[email protected]>
Take the client_lock before traversing the clients list at the
pmic_glink_state_notify_clients() function. This is required to keep the
list traversal safe from concurrent modification.
Fixes: 58ef4ece1e41 ("soc: qcom: pmic_glink: Introduce base PMIC GLINK driver")
Signed-off-by: Dmitry Baryshkov <[email protected]>
---
drivers/soc/qcom/pmic_glink.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/soc/qcom/pmic_glink.c b/drivers/soc/qcom/pmic_glink.c
index f913e9bd57ed..c999358771b3 100644
--- a/drivers/soc/qcom/pmic_glink.c
+++ b/drivers/soc/qcom/pmic_glink.c
@@ -168,8 +168,10 @@ static void pmic_glink_state_notify_clients(struct pmic_glink *pg)
}
if (new_state != pg->client_state) {
+ mutex_lock(&pg->client_lock);
list_for_each_entry(client, &pg->clients, node)
client->pdr_notify(client->priv, new_state);
+ mutex_unlock(&pg->client_lock);
pg->client_state = new_state;
}
}
--
2.39.2
In case the client is registered after the pmic-glink recived a response
from the Protection Domain mapper, it is going to miss the notification
about the state. Notify clients about the current state upon
registration.
Fixes: 58ef4ece1e41 ("soc: qcom: pmic_glink: Introduce base PMIC GLINK driver")
Signed-off-by: Dmitry Baryshkov <[email protected]>
---
drivers/soc/qcom/pmic_glink.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/soc/qcom/pmic_glink.c b/drivers/soc/qcom/pmic_glink.c
index c999358771b3..089cdfb1c9d1 100644
--- a/drivers/soc/qcom/pmic_glink.c
+++ b/drivers/soc/qcom/pmic_glink.c
@@ -83,9 +83,14 @@ struct pmic_glink_client *devm_pmic_glink_register_client(struct device *dev,
client->pdr_notify = pdr;
client->priv = priv;
+ mutex_lock(&pg->state_lock);
mutex_lock(&pg->client_lock);
+
list_add(&client->node, &pg->clients);
+ client->pdr_notify(client->priv, pg->client_state);
+
mutex_unlock(&pg->client_lock);
+ mutex_unlock(&pg->state_lock);
devres_add(dev, client);
--
2.39.2
On Tue, Apr 02, 2024 at 08:07:06PM +0300, Dmitry Baryshkov wrote:
> Take the client_lock before traversing the clients list at the
> pmic_glink_state_notify_clients() function. This is required to keep the
> list traversal safe from concurrent modification.
>
> Fixes: 58ef4ece1e41 ("soc: qcom: pmic_glink: Introduce base PMIC GLINK driver")
> Signed-off-by: Dmitry Baryshkov <[email protected]>
> ---
> drivers/soc/qcom/pmic_glink.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/soc/qcom/pmic_glink.c b/drivers/soc/qcom/pmic_glink.c
> index f913e9bd57ed..c999358771b3 100644
> --- a/drivers/soc/qcom/pmic_glink.c
> +++ b/drivers/soc/qcom/pmic_glink.c
> @@ -168,8 +168,10 @@ static void pmic_glink_state_notify_clients(struct pmic_glink *pg)
> }
Does pmic_glink_rpmsg_callback() deserve similar locking when traversing
the clients list?
>
> if (new_state != pg->client_state) {
> + mutex_lock(&pg->client_lock);
> list_for_each_entry(client, &pg->clients, node)
> client->pdr_notify(client->priv, new_state);
> + mutex_unlock(&pg->client_lock);
> pg->client_state = new_state;
> }
> }
>
> --
> 2.39.2
>
>
On Tue, Apr 02, 2024 at 08:07:07PM +0300, Dmitry Baryshkov wrote:
> In case the client is registered after the pmic-glink recived a response
> from the Protection Domain mapper, it is going to miss the notification
> about the state. Notify clients about the current state upon
> registration.
>
> Fixes: 58ef4ece1e41 ("soc: qcom: pmic_glink: Introduce base PMIC GLINK driver")
This looks and sounds reasonable to me.
Reviewed-by: Andrew Halaney <[email protected]>
> Signed-off-by: Dmitry Baryshkov <[email protected]>
> ---
> drivers/soc/qcom/pmic_glink.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/soc/qcom/pmic_glink.c b/drivers/soc/qcom/pmic_glink.c
> index c999358771b3..089cdfb1c9d1 100644
> --- a/drivers/soc/qcom/pmic_glink.c
> +++ b/drivers/soc/qcom/pmic_glink.c
> @@ -83,9 +83,14 @@ struct pmic_glink_client *devm_pmic_glink_register_client(struct device *dev,
> client->pdr_notify = pdr;
> client->priv = priv;
>
> + mutex_lock(&pg->state_lock);
> mutex_lock(&pg->client_lock);
> +
> list_add(&client->node, &pg->clients);
> + client->pdr_notify(client->priv, pg->client_state);
> +
> mutex_unlock(&pg->client_lock);
> + mutex_unlock(&pg->state_lock);
>
> devres_add(dev, client);
>
>
> --
> 2.39.2
>
>
On Tue, 2 Apr 2024 at 20:56, Andrew Halaney <[email protected]> wrote:
>
> On Tue, Apr 02, 2024 at 08:07:06PM +0300, Dmitry Baryshkov wrote:
> > Take the client_lock before traversing the clients list at the
> > pmic_glink_state_notify_clients() function. This is required to keep the
> > list traversal safe from concurrent modification.
> >
> > Fixes: 58ef4ece1e41 ("soc: qcom: pmic_glink: Introduce base PMIC GLINK driver")
> > Signed-off-by: Dmitry Baryshkov <[email protected]>
> > ---
> > drivers/soc/qcom/pmic_glink.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/soc/qcom/pmic_glink.c b/drivers/soc/qcom/pmic_glink.c
> > index f913e9bd57ed..c999358771b3 100644
> > --- a/drivers/soc/qcom/pmic_glink.c
> > +++ b/drivers/soc/qcom/pmic_glink.c
> > @@ -168,8 +168,10 @@ static void pmic_glink_state_notify_clients(struct pmic_glink *pg)
> > }
>
> Does pmic_glink_rpmsg_callback() deserve similar locking when traversing
> the clients list?
True. Will fix in v2.
>
> >
> > if (new_state != pg->client_state) {
> > + mutex_lock(&pg->client_lock);
> > list_for_each_entry(client, &pg->clients, node)
> > client->pdr_notify(client->priv, new_state);
> > + mutex_unlock(&pg->client_lock);
> > pg->client_state = new_state;
> > }
> > }
> >
> > --
> > 2.39.2
> >
> >
>
--
With best wishes
Dmitry