Introduce Protection Domain Restart (PDR) Helpers
Subsystems (4)
v6 cover Introduce Protection Domain Restart (PDR) Helpers
Qualcomm SoCs (starting with MSM8998) allow for multiple protection domains (PDs) to run on the same Q6 sub-system. This allows for services like AVS AUDIO to have their own separate address space and crash/recover without disrupting the other PDs running on the same Q6 ADSP. This patch series introduces pdr helper library and adds PD tracking functionality for "avs/audio" allowing apr services to register themselves asynchronously once the dependent PDs are up.
V6:
* Fix minor compilation error introduced in V5
* Pick up Rob's R-b
V5:
* Pick up Bjorn's R-b
* Service all pending lookup, notifier_register and notifier_remove requests.
* Propagate error from pdr_register_listener
* Remove notifier_init_complete flag workaround with the introduction of in-kernel ns.
* Remove depends-on in Kconfig and add module license.
V4:
* Drop handling db_rev_count error handling since it remains unused in upstream/downstream pd-mapper
* Fixup dt bindings and examples. [Rob]
* Dropping r-b from Bjorn/Srini for the dt bindings.
* Privatize pdr_service/pdr_handle. [Srini/Bjorn]
* Introduce notifier_init_complete to deal with cases where qmi_handle_net_reset is not enough to reset the port. This is to deal with cases where qrtr-ns starts after the adsp.
* Introduce per addr per pds to deal with multiple service_paths per pdr_handle.
* Uniformly rename servreg -> notifier, servloc -> locator. [Narendra]
* Drop pdr_servreg_link_create tracking the service_path tracks all pds associated with it. [Bjorn]
* Remove safe traversal for all cases where list is left unmodified. [Bjorn]
* Address review comments in the apr driver and add comments. [Bjorn]
* Other misc fixes. [Narendra/Bjorn]
V3:
* patches 2 and 3 remain unchanged.
* reset servloc_addr/servreg_addr.
* fixup the helpers to handle servloc_work/servreg_work asynchronously.
* fixup useage of list_lock across traversals, insertions and deletions.
* fixup the helpers to use a single lookup list.
* skip waiting for response on ind_ack send.
* introduce pdr_servreg_link_create to re-use existing qmi connection to servreg instances. This helps tracking PDs running on the same remote processor.
* have a per node curr_state in pdr_list_node to preserve all state updates during indack_cb.
* introduce additional servreg_service_state values to help the client distinguish between a fatal and non-fatal pdr_lookup errors.
* re-order pdr_handle_release sequence.
* fixup "!ind_msg->service_path returns true always" warning.
* fixup comments.
V2:
* fixup pd_status callback to return void.
* return 0 from pdr_get_domain_list on success.
* introduce status_lock to linearize the pd_status reported back to the clients.
* use the correct service name length across various string operations performed.
* service locator will now schedule the pending lookups registered when it comes up.
* other minor cleanups that Bjorn suggested.
* use pr_warn to indicate that the wait for service locator timed out.
* add Bjorn/Srini's "Reviewed-by" for the dt-bindings.
Depends on in-kernel ns fixes:
https://patchwork.kernel.org/cover/11414841/
Sibi Sankar (3):
soc: qcom: Introduce Protection Domain Restart helpers
dt-bindings: soc: qcom: apr: Add protection domain bindings
soc: qcom: apr: Add avs/audio tracking functionality
.../devicetree/bindings/soc/qcom/qcom,apr.txt | 50 ++ drivers/soc/qcom/Kconfig | 5 + drivers/soc/qcom/Makefile | 1 + drivers/soc/qcom/apr.c | 123 ++- drivers/soc/qcom/pdr_interface.c | 754 ++++++++++++++++++ drivers/soc/qcom/pdr_internal.h | 379 +++++++++ include/linux/soc/qcom/apr.h | 1 + include/linux/soc/qcom/pdr.h | 30 + include/linux/soc/qcom/qmi.h | 1 + 9 files changed, 1335 insertions(+), 9 deletions(-) create mode 100644 drivers/soc/qcom/pdr_interface.c create mode 100644 drivers/soc/qcom/pdr_internal.h create mode 100644 include/linux/soc/qcom/pdr.h
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH v6 1/3 soc: qcom: Introduce Protection Domain Restart helpers
Qualcomm SoCs (starting with MSM8998) allow for multiple protection domains to run on the same Q6 sub-system. This allows for services like ATH10K WLAN FW to have their own separate address space and crash/recover without disrupting the modem and other PDs running on the same sub-system. The PDR helpers introduces an abstraction that allows for tracking/controlling the life cycle of protection domains running on various Q6 sub-systems.
---
V6:
* Fix minor compilation error introduceed in v5 while propagating error from pdr_register_listener
V5:
* Service all pending lookup, notifier_register and notifier_remove requests. [Bjorn]
* Propagate error from pdr_register_listener
* Remove notifier_init_complete flag workaround with the introduction of in-kernel ns.
* Remove depends-on in Kconfig and add module license.
drivers/soc/qcom/Kconfig | 4 + drivers/soc/qcom/Makefile | 1 + drivers/soc/qcom/pdr_interface.c | 754 +++++++++++++++++++++++++++++++ drivers/soc/qcom/pdr_internal.h | 379 ++++++++++++++++ include/linux/soc/qcom/pdr.h | 30 ++ include/linux/soc/qcom/qmi.h | 1 + 6 files changed, 1169 insertions(+) create mode 100644 drivers/soc/qcom/pdr_interface.c create mode 100644 drivers/soc/qcom/pdr_internal.h create mode 100644 include/linux/soc/qcom/pdr.h
drivers/soc/qcom/Kconfig +4 -0
@@ -76,6 +76,10 @@ config QCOM_OCMEM requirements. This is typically used by the GPU, camera/video, and audio components on some Snapdragon SoCs. +config QCOM_PDR_HELPERS + tristate + select QCOM_QMI_HELPERS + config QCOM_PM bool "Qualcomm Power Management" depends on ARCH_QCOM && !ARM64
drivers/soc/qcom/Makefile +1 -0
@@ -7,6 +7,7 @@ obj-$(CONFIG_QCOM_GLINK_SSR) += glink_ssr.o obj-$(CONFIG_QCOM_GSBI) += qcom_gsbi.o obj-$(CONFIG_QCOM_MDT_LOADER) += mdt_loader.o obj-$(CONFIG_QCOM_OCMEM) += ocmem.o +obj-$(CONFIG_QCOM_PDR_HELPERS) += pdr_interface.o obj-$(CONFIG_QCOM_PM) += spm.o obj-$(CONFIG_QCOM_QMI_HELPERS) += qmi_helpers.o qmi_helpers-y += qmi_encdec.o qmi_interface.o
drivers/soc/qcom/pdr_interface.c new file +754 -0
754 lines not on this page · raw message
drivers/soc/qcom/pdr_internal.h new file +379 -0
379 lines not on this page · raw message
include/linux/soc/qcom/pdr.h new file +30 -0
@@ -0,0 +1,30 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __QCOM_PDR_HELPER__ +#define __QCOM_PDR_HELPER__ + +#include <linux/soc/qcom/qmi.h> + +#define SERVREG_NAME_LENGTH 64 + +struct pdr_service ; +struct pdr_handle ; + +enum servreg_service_state { +SERVREG_LOCATOR_ERR =0x1 , +SERVREG_LOCATOR_UNKNOWN_SERVICE =0x2 , +SERVREG_SERVICE_STATE_DOWN =0x0FFFFFFF , +SERVREG_SERVICE_STATE_UP =0x1FFFFFFF , +SERVREG_SERVICE_STATE_EARLY_DOWN =0x2FFFFFFF , +SERVREG_SERVICE_STATE_UNINIT =0x7FFFFFFF , +}; + +struct pdr_handle *pdr_handle_alloc (void (*status)(int state, +char *service_path, +void *priv),void *priv); +struct pdr_service *pdr_add_lookup (struct pdr_handle *pdr, +const char *service_name, +const char *service_path); +int pdr_restart_pd (struct pdr_handle *pdr,struct pdr_service *pds); +void pdr_handle_release (struct pdr_handle *pdr); + +#endif
include/linux/soc/qcom/qmi.h +1 -0
@@ -88,6 +88,7 @@ struct qmi_elem_info {#define QMI_ERR_CLIENT_IDS_EXHAUSTED_V01 5#define QMI_ERR_INVALID_ID_V01 41#define QMI_ERR_ENCODING_V01 58 +#define QMI_ERR_DISABLED_V01 69#define QMI_ERR_INCOMPATIBLE_STATE_V01 90#define QMI_ERR_NOT_SUPPORTED_V01 94
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v6 1/3] soc: qcom: Introduce Protection Domain Restart helpers
Quoted 1 line · Sibi Sankar
drivers/soc/qcom/pdr_interface.c +0
-0
[..]
Quoted 59 lines
drivers/soc/qcom/pdr_interface.c +59
-0+static int pdr_locate_service (struct pdr_handle *pdr,struct pdr_service *pds) +{ +struct servreg_get_domain_list_resp *resp; +struct servreg_get_domain_list_req req; +struct servreg_location_entry *entry; +int domains_read =0 ; +int ret, i; + + resp =kzalloc (sizeof (*resp),GFP_KERNEL ); +if (!resp) +return -ENOMEM ; + +/* Prepare req message */ +strcpy (req.service_name, pds->service_name); + req.domain_offset_valid = true; + req.domain_offset =0 ; + +do { + req.domain_offset = domains_read; + ret =pdr_get_domain_list (&req, resp, pdr); +if (ret <0 ) + goto out; + +for (i = domains_read; i < resp->domain_list_len; i++) { + entry = &resp->domain_list[i]; + +if (strnlen (entry->name,sizeof (entry->name)) ==sizeof (entry->name)) +continue ; + +if (!strcmp (entry->name, pds->service_path)) { + pds->service_data_valid = entry->service_data_valid; + pds->service_data = entry->service_data; + pds->instance = entry->instance; + goto out; + } + } + +/* Update ret to indicate that the service is not yet found */ + ret = -ENXIO ; + +/* Always read total_domains from the response msg */ +if (resp->domain_list_len > resp->total_domains) + resp->domain_list_len = resp->total_domains; + + domains_read += resp->domain_list_len; + }while (domains_read < resp->total_domains); +out: +kfree (resp); +return ret; +} + +static void pdr_notify_lookup_failure (struct pdr_handle *pdr, +struct pdr_service *pds, +int err) +{ +list_del (&pds->node); + +if (err == -ENXIO ) + pds->state =SERVREG_LOCATOR_UNKNOWN_SERVICE ;
drivers/soc/qcom/pdr_interface.c line 465
Quoted 10 lines
drivers/soc/qcom/pdr_interface.c +10
-0+else + pds->state =SERVREG_LOCATOR_ERR ; + +pr_err ("PDR: service lookup for %s failed: %d\n" , + pds->service_name, err); + +mutex_lock (&pdr->status_lock); + pdr->status (pds->state, pds->service_path, pdr->priv); +mutex_unlock (&pdr->status_lock); +kfree (pds);
So this implies that we didn't find the service and we will never find it? How are the client drivers expected to react to above two states?
Quoted 17 lines
drivers/soc/qcom/pdr_interface.c +17
-0+} + +static void pdr_locator_work (struct work_struct *work) +{ + struct pdr_handle *pdr = container_of(work, struct pdr_handle, + locator_work); +struct pdr_service *pds, *tmp; +int ret =0 ; + +/* Bail out early if the SERVREG LOCATOR QMI service is not up */ +mutex_lock (&pdr->lock); +if (!pdr->locator_init_complete) { +mutex_unlock (&pdr->lock); +pr_debug ("PDR: SERVICE LOCATOR service not available\n" ); +return ; + } +mutex_unlock (&pdr->lock);
drivers/soc/qcom/pdr_interface.c line 492
Quoted 10 lines
drivers/soc/qcom/pdr_interface.c +10
-0+ +mutex_lock (&pdr->list_lock); +list_for_each_entry_safe (pds, tmp, &pdr->lookups, node) { +if (!pds->need_locator_lookup) +continue ; + + pds->need_locator_lookup = false; + ret =pdr_locate_service (pdr, pds); +if (ret <0 ) +pdr_notify_lookup_failure (pdr, pds, ret);
If I read this correctly, pdr_locate_service() returning an error seems to mean that pd->instance won't be filled out, as such I don't think you want to proceed.
Further more, pdr_notify_lookup_failure() ends up freeing the pds, so below lookup would be a use after free, not unlikely followed by a double free of pds.
How about a "continue" here and only clear need_locator_lookup if both of these checks pass?
drivers/soc/qcom/pdr_interface.c line 500
Quoted 8 lines
drivers/soc/qcom/pdr_interface.c +8
-0+ + ret =qmi_add_lookup (&pdr->notifier_hdl, pds->service,1 , + pds->instance); +if (ret <0 ) +pdr_notify_lookup_failure (pdr, pds, ret); + } +mutex_unlock (&pdr->list_lock); +}
Apart from that I think the patches look good now.
Regards,
Bjorn
PATCH v6 2/3 dt-bindings: soc: qcom: apr: Add protection domain bindings
In reply to Sibi SankarQualcomm SoCs (starting with MSM8998) allow for multiple protection domains (PDs) to run on the same Q6 sub-system. This allows for services like AVS AUDIO to have their own separate address space and crash/recover without disrupting the other PDs running on the same Q6 ADSP. Add "qcom,protection-domain" bindings to capture the dependencies between the APR service and the PD on which the apr service runs.
---
V6:
* Picked up Rob's R-b
V5:
* Picked up Bjorn's R-b
.../devicetree/bindings/soc/qcom/qcom,apr.txt | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+)
Documentation/devicetree/bindings/soc/qcom/qcom,apr.txt +50 -0
@@ -45,6 +45,18 @@ by the individual bindings for the specific service 12 - Ultrasound stream manager. 13 - Listen stream manager. +- qcom,protection-domain + Usage: optional + Value type: <stringlist> + Definition: Must list the protection domain service name and path + that the particular apr service has a dependency on. + Possible values are : + "avs/audio", "msm/adsp/audio_pd". + "kernel/elf_loader", "msm/modem/wlan_pd". + "tms/servreg", "msm/adsp/audio_pd". + "tms/servreg", "msm/modem/wlan_pd". + "tms/servreg", "msm/slpi/sensor_pd". + = EXAMPLE The following example represents a QDSP based sound card on a MSM8996 device which uses apr as communication between Apps and QDSP.
@@ -82,3 +94,41 @@ which uses apr as communication between Apps and QDSP. ... }; }; + += EXAMPLE 2 +The following example represents a QDSP based sound card with protection domain +dependencies specified. Here some of the apr services are dependent on services +running on protection domain hosted on ADSP/SLPI remote processors while others +have no such dependency. + + apr { + compatible = "qcom,apr-v2"; + qcom,glink-channels = "apr_audio_svc"; + qcom,apr-domain = <APR_DOMAIN_ADSP>; + + q6core { + compatible = "qcom,q6core"; + reg = <APR_SVC_ADSP_CORE>; + }; + + q6afe: q6afe { + compatible = "qcom,q6afe"; + reg = <APR_SVC_AFE>; + qcom,protection-domain = "avs/audio", "msm/adsp/audio_pd"; + ... + }; + + q6asm: q6asm { + compatible = "qcom,q6asm"; + reg = <APR_SVC_ASM>; + qcom,protection-domain = "tms/servreg", "msm/slpi/sensor_pd"; + ... + }; + + q6adm: q6adm { + compatible = "qcom,q6adm"; + reg = <APR_SVC_ADM>; + qcom,protection-domain = "avs/audio", "msm/adsp/audio_pd"; + ... + }; + };
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH v6 3/3 soc: qcom: apr: Add avs/audio tracking functionality
In reply to Sibi SankarUse PDR helper functions to track the protection domains that the apr services are dependent upon on SDM845 SoC, specifically the "avs/audio" service running on ADSP Q6.
---
V6:
* No change
V5:
* Picked up Bjorn's R-b
drivers/soc/qcom/Kconfig | 1 + drivers/soc/qcom/apr.c | 123 ++++++++++++++++++++++++++++++++--- include/linux/soc/qcom/apr.h | 1 + 3 files changed, 116 insertions(+), 9 deletions(-)
drivers/soc/qcom/Kconfig +1 -0
@@ -201,6 +201,7 @@ config QCOM_APR tristate "Qualcomm APR Bus (Asynchronous Packet Router)" depends on ARCH_QCOM || COMPILE_TEST depends on RPMSG + select QCOM_PDR_HELPERS help Enable APR IPC protocol support between application processor and QDSP6. APR is
drivers/soc/qcom/apr.c +114 -9
180 lines not on this page · raw message
include/linux/soc/qcom/apr.h +1 -0
@@ -85,6 +85,7 @@ struct apr_device {uint16_t domain_id;uint32_t version;char name[APR_NAME_SIZE ]; +const char *service_path;spinlock_t lock;struct list_head node; };
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project