diff mbox series

[v4,2/7] soc: qcom: qmi: add a way to remove running service

Message ID 20240311-qcom-pd-mapper-v4-2-24679cca5c24@linaro.org
State Superseded
Headers show
Series soc: qcom: add in-kernel pd-mapper implementation | expand

Commit Message

Dmitry Baryshkov March 11, 2024, 3:34 p.m. UTC
Add qmi_del_server(), a pair to qmi_add_server(), a way to remove
running server from the QMI socket. This is e.g. necessary for
pd-mapper, which needs to readd a server each time the DSP is started or
stopped.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/soc/qcom/qmi_interface.c | 67 ++++++++++++++++++++++++++++++++++++++++
 include/linux/soc/qcom/qmi.h     |  2 ++
 2 files changed, 69 insertions(+)

Comments

Konrad Dybcio March 12, 2024, 12:53 a.m. UTC | #1
On 3/11/24 16:34, Dmitry Baryshkov wrote:
> Add qmi_del_server(), a pair to qmi_add_server(), a way to remove
> running server from the QMI socket. This is e.g. necessary for
> pd-mapper, which needs to readd a server each time the DSP is started or
> stopped.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/soc/qcom/qmi_interface.c | 67 ++++++++++++++++++++++++++++++++++++++++
>   include/linux/soc/qcom/qmi.h     |  2 ++
>   2 files changed, 69 insertions(+)
> 
> diff --git a/drivers/soc/qcom/qmi_interface.c b/drivers/soc/qcom/qmi_interface.c
> index bb98b06e87f8..18ff2015c682 100644
> --- a/drivers/soc/qcom/qmi_interface.c
> +++ b/drivers/soc/qcom/qmi_interface.c
> @@ -289,6 +289,73 @@ int qmi_add_server(struct qmi_handle *qmi, unsigned int service,
>   }
>   EXPORT_SYMBOL_GPL(qmi_add_server);
>   
> +static void qmi_send_del_server(struct qmi_handle *qmi, struct qmi_service *svc)
> +{
> +	struct qrtr_ctrl_pkt pkt;
> +	struct sockaddr_qrtr sq;
> +	struct msghdr msg = { };
> +	struct kvec iv = { &pkt, sizeof(pkt) };
> +	int ret;
> +
> +	memset(&pkt, 0, sizeof(pkt));

0-init instead?

> +	pkt.cmd = cpu_to_le32(QRTR_TYPE_DEL_SERVER);
> +	pkt.server.service = cpu_to_le32(svc->service);
> +	pkt.server.instance = cpu_to_le32(svc->version | svc->instance << 8);
> +	pkt.server.node = cpu_to_le32(qmi->sq.sq_node);
> +	pkt.server.port = cpu_to_le32(qmi->sq.sq_port);

Or perhaps C99-init?

Konrad
Dmitry Baryshkov March 12, 2024, 1:03 a.m. UTC | #2
On Tue, 12 Mar 2024 at 02:53, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
>
>
> On 3/11/24 16:34, Dmitry Baryshkov wrote:
> > Add qmi_del_server(), a pair to qmi_add_server(), a way to remove
> > running server from the QMI socket. This is e.g. necessary for
> > pd-mapper, which needs to readd a server each time the DSP is started or
> > stopped.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/soc/qcom/qmi_interface.c | 67 ++++++++++++++++++++++++++++++++++++++++
> >   include/linux/soc/qcom/qmi.h     |  2 ++
> >   2 files changed, 69 insertions(+)
> >
> > diff --git a/drivers/soc/qcom/qmi_interface.c b/drivers/soc/qcom/qmi_interface.c
> > index bb98b06e87f8..18ff2015c682 100644
> > --- a/drivers/soc/qcom/qmi_interface.c
> > +++ b/drivers/soc/qcom/qmi_interface.c
> > @@ -289,6 +289,73 @@ int qmi_add_server(struct qmi_handle *qmi, unsigned int service,
> >   }
> >   EXPORT_SYMBOL_GPL(qmi_add_server);
> >
> > +static void qmi_send_del_server(struct qmi_handle *qmi, struct qmi_service *svc)
> > +{
> > +     struct qrtr_ctrl_pkt pkt;
> > +     struct sockaddr_qrtr sq;
> > +     struct msghdr msg = { };
> > +     struct kvec iv = { &pkt, sizeof(pkt) };
> > +     int ret;
> > +
> > +     memset(&pkt, 0, sizeof(pkt));
>
> 0-init instead?
>
> > +     pkt.cmd = cpu_to_le32(QRTR_TYPE_DEL_SERVER);
> > +     pkt.server.service = cpu_to_le32(svc->service);
> > +     pkt.server.instance = cpu_to_le32(svc->version | svc->instance << 8);
> > +     pkt.server.node = cpu_to_le32(qmi->sq.sq_node);
> > +     pkt.server.port = cpu_to_le32(qmi->sq.sq_port);
>
> Or perhaps C99-init?

This follows 1:1 qcom_send_add_server(). I don't think we should use
new style just for this function.

>
> Konrad
Chris Lew March 14, 2024, 12:03 a.m. UTC | #3
On 3/11/2024 8:34 AM, Dmitry Baryshkov wrote:
> +/**
> + * qmi_del_server() - register a service with the name service

s/register/deregister/g

> + * @qmi:	qmi handle
> + * @service:	type of the service
> + * @instance:	instance of the service
> + * @version:	version of the service
> + *
> + * Remove registration of the service with the name service. This notifies
> + * clients that they should no longer send messages to the client associated
> + * with @qmi.
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
> +int qmi_del_server(struct qmi_handle *qmi, unsigned int service,
> +		   unsigned int version, unsigned int instance)
> +{
> +	struct qmi_service *svc;
> +	struct qmi_service *tmp;
> +
> +	list_for_each_entry_safe(svc, tmp, &qmi->services, list_node) {
> +		if (svc->service != service ||
> +		    svc->version != version ||
> +		    svc->instance != instance)
> +			continue;
> +
> +		qmi_send_del_server(qmi, svc);
> +		list_del(&svc->list_node);
> +		kfree(svc);
> +
> +		return 0;
> +	}
> +

is list_for_each_entry_safe required if we stop iterating and return
after we find the first instance of the service?

> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(qmi_del_server);
Chris Lew March 14, 2024, 5:15 p.m. UTC | #4
On 3/13/2024 5:09 PM, Dmitry Baryshkov wrote:
> On Thu, 14 Mar 2024 at 02:03, Chris Lew <quic_clew@quicinc.com> wrote:
>>
>>
>>
>> On 3/11/2024 8:34 AM, Dmitry Baryshkov wrote:
>>> +/**
>>> + * qmi_del_server() - register a service with the name service
>>
>> s/register/deregister/g
> 
> ack
> 
>>
>>> + * @qmi:     qmi handle
>>> + * @service: type of the service
>>> + * @instance:        instance of the service
>>> + * @version: version of the service
>>> + *
>>> + * Remove registration of the service with the name service. This notifies
>>> + * clients that they should no longer send messages to the client associated
>>> + * with @qmi.
>>> + *
>>> + * Return: 0 on success, negative errno on failure.
>>> + */
>>> +int qmi_del_server(struct qmi_handle *qmi, unsigned int service,
>>> +                unsigned int version, unsigned int instance)
>>> +{
>>> +     struct qmi_service *svc;
>>> +     struct qmi_service *tmp;
>>> +
>>> +     list_for_each_entry_safe(svc, tmp, &qmi->services, list_node) {
>>> +             if (svc->service != service ||
>>> +                 svc->version != version ||
>>> +                 svc->instance != instance)
>>> +                     continue;
>>> +
>>> +             qmi_send_del_server(qmi, svc);
>>> +             list_del(&svc->list_node);
>>> +             kfree(svc);
>>> +
>>> +             return 0;
>>> +     }
>>> +
>>
>> is list_for_each_entry_safe required if we stop iterating and return
>> after we find the first instance of the service?
> 
> Yes, it just adds a temp variable here.
> 

Ok, I was thinking that tmp wasn't necessary because we don't continue
iterating through the list after we free the svc. I guess it never hurts
to be safe though.

>>
>>> +     return -EINVAL;
>>> +}
>>> +EXPORT_SYMBOL_GPL(qmi_del_server);
> 
> 
>
diff mbox series

Patch

diff --git a/drivers/soc/qcom/qmi_interface.c b/drivers/soc/qcom/qmi_interface.c
index bb98b06e87f8..18ff2015c682 100644
--- a/drivers/soc/qcom/qmi_interface.c
+++ b/drivers/soc/qcom/qmi_interface.c
@@ -289,6 +289,73 @@  int qmi_add_server(struct qmi_handle *qmi, unsigned int service,
 }
 EXPORT_SYMBOL_GPL(qmi_add_server);
 
+static void qmi_send_del_server(struct qmi_handle *qmi, struct qmi_service *svc)
+{
+	struct qrtr_ctrl_pkt pkt;
+	struct sockaddr_qrtr sq;
+	struct msghdr msg = { };
+	struct kvec iv = { &pkt, sizeof(pkt) };
+	int ret;
+
+	memset(&pkt, 0, sizeof(pkt));
+	pkt.cmd = cpu_to_le32(QRTR_TYPE_DEL_SERVER);
+	pkt.server.service = cpu_to_le32(svc->service);
+	pkt.server.instance = cpu_to_le32(svc->version | svc->instance << 8);
+	pkt.server.node = cpu_to_le32(qmi->sq.sq_node);
+	pkt.server.port = cpu_to_le32(qmi->sq.sq_port);
+
+	sq.sq_family = qmi->sq.sq_family;
+	sq.sq_node = qmi->sq.sq_node;
+	sq.sq_port = QRTR_PORT_CTRL;
+
+	msg.msg_name = &sq;
+	msg.msg_namelen = sizeof(sq);
+
+	mutex_lock(&qmi->sock_lock);
+	if (qmi->sock) {
+		ret = kernel_sendmsg(qmi->sock, &msg, &iv, 1, sizeof(pkt));
+		if (ret < 0)
+			pr_err("send service deregistration failed: %d\n", ret);
+	}
+	mutex_unlock(&qmi->sock_lock);
+}
+
+/**
+ * qmi_del_server() - register a service with the name service
+ * @qmi:	qmi handle
+ * @service:	type of the service
+ * @instance:	instance of the service
+ * @version:	version of the service
+ *
+ * Remove registration of the service with the name service. This notifies
+ * clients that they should no longer send messages to the client associated
+ * with @qmi.
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+int qmi_del_server(struct qmi_handle *qmi, unsigned int service,
+		   unsigned int version, unsigned int instance)
+{
+	struct qmi_service *svc;
+	struct qmi_service *tmp;
+
+	list_for_each_entry_safe(svc, tmp, &qmi->services, list_node) {
+		if (svc->service != service ||
+		    svc->version != version ||
+		    svc->instance != instance)
+			continue;
+
+		qmi_send_del_server(qmi, svc);
+		list_del(&svc->list_node);
+		kfree(svc);
+
+		return 0;
+	}
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(qmi_del_server);
+
 /**
  * qmi_txn_init() - allocate transaction id within the given QMI handle
  * @qmi:	QMI handle
diff --git a/include/linux/soc/qcom/qmi.h b/include/linux/soc/qcom/qmi.h
index 469e02d2aa0d..5039c30e4bdc 100644
--- a/include/linux/soc/qcom/qmi.h
+++ b/include/linux/soc/qcom/qmi.h
@@ -241,6 +241,8 @@  int qmi_add_lookup(struct qmi_handle *qmi, unsigned int service,
 		   unsigned int version, unsigned int instance);
 int qmi_add_server(struct qmi_handle *qmi, unsigned int service,
 		   unsigned int version, unsigned int instance);
+int qmi_del_server(struct qmi_handle *qmi, unsigned int service,
+		   unsigned int version, unsigned int instance);
 
 int qmi_handle_init(struct qmi_handle *qmi, size_t max_msg_len,
 		    const struct qmi_ops *ops,