| Message ID | 20240311-qcom-pd-mapper-v4-2-24679cca5c24@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | soc: qcom: add in-kernel pd-mapper implementation | expand |
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
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
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);
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. > > > + return -EINVAL; > > +} > > +EXPORT_SYMBOL_GPL(qmi_del_server);
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 --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,
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(+)