Received-SPF: pass (google.com: domain of linux-kernel+bounces-194939-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <f91d2ce8-0909-4404-9db3-488a5c40137c@quicinc.com>
Date: Thu, 30 May 2024 10:15:40 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 4/8] misc: fastrpc: Add static PD restart support
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
CC: <srinivas.kandagatla@linaro.org>, <linux-arm-msm@vger.kernel.org>,
        <gregkh@linuxfoundation.org>, <quic_bkumar@quicinc.com>,
        <linux-kernel@vger.kernel.org>, <quic_chennak@quicinc.com>
References: <20240528112956.5979-1-quic_ekangupt@quicinc.com>
 <20240528112956.5979-5-quic_ekangupt@quicinc.com>
 <wovshpnle7jvl7yc7spxixb46o7p4l3y4hmehjjpcjwh4jma74@k6kjzjx5di6m>
 <51facf80-2490-4533-8ba1-584769e1ca79@quicinc.com>
 <f5dq3boqfbs4zl7d27j26sqj63jharn6ux7vbgsfgz5i75g4v5@nmela43ftlfq>
Content-Language: en-US
From: Ekansh Gupta <quic_ekangupt@quicinc.com>
In-Reply-To: <f5dq3boqfbs4zl7d27j26sqj63jharn6ux7vbgsfgz5i75g4v5@nmela43ftlfq>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 5/29/2024 6:13 PM, Dmitry Baryshkov wrote:
> On Wed, May 29, 2024 at 04:41:51PM +0530, Ekansh Gupta wrote:
>> On 5/28/2024 6:03 PM, Dmitry Baryshkov wrote:
>>> On Tue, May 28, 2024 at 04:59:50PM +0530, Ekansh Gupta wrote:
>>>> Static PDs on the audio and sensor domains are expected to support
>>>> PD restart. The kernel resource handling for the PDs are expected
>>>> to be handled by fastrpc driver. For this, there is a requirement
>>>> of PD service locator to get the event notifications for static PD
>>>> services. Also when events are received, the driver needs to handle
>>>> based on PD states. Added changes to add service locator for audio
>>>> and sensor domain static PDs and handle the PD restart sequence.
>>>>
>>>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>>>> ---
>>>>    drivers/misc/Kconfig   |   2 +
>>>>    drivers/misc/fastrpc.c | 205 ++++++++++++++++++++++++++++++++++++++---
>>>>    2 files changed, 195 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>>>> index faf983680040..e2d83cd085b5 100644
>>>> --- a/drivers/misc/Kconfig
>>>> +++ b/drivers/misc/Kconfig
>>>> @@ -280,8 +280,10 @@ config QCOM_FASTRPC
>>>>    	tristate "Qualcomm FastRPC"
>>>>    	depends on ARCH_QCOM || COMPILE_TEST
>>>>    	depends on RPMSG
>>>> +	depends on NET
>>>>    	select DMA_SHARED_BUFFER
>>>>    	select QCOM_SCM
>>>> +	select QCOM_PDR_HELPERS
>>>>    	help
>>>>    	  Provides a communication mechanism that allows for clients to
>>>>    	  make remote method invocations across processor boundary to
>>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>>> index 6556c63c4ad7..7796b743cc45 100644
>>>> --- a/drivers/misc/fastrpc.c
>>>> +++ b/drivers/misc/fastrpc.c
>>>> @@ -22,6 +22,7 @@
>>>>    #include <linux/firmware/qcom/qcom_scm.h>
>>>>    #include <uapi/misc/fastrpc.h>
>>>>    #include <linux/of_reserved_mem.h>
>>>> +#include <linux/soc/qcom/pdr.h>
>>>>    #define ADSP_DOMAIN_ID (0)
>>>>    #define MDSP_DOMAIN_ID (1)
>>>> @@ -29,6 +30,7 @@
>>>>    #define CDSP_DOMAIN_ID (3)
>>>>    #define FASTRPC_DEV_MAX		4 /* adsp, mdsp, slpi, cdsp*/
>>>>    #define FASTRPC_MAX_SESSIONS	14
>>>> +#define FASTRPC_MAX_SPD		4
>>>>    #define FASTRPC_MAX_VMIDS	16
>>>>    #define FASTRPC_ALIGN		128
>>>>    #define FASTRPC_MAX_FDLIST	16
>>>> @@ -105,6 +107,18 @@
>>>>    #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
>>>> +#define AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME   "audio_pdr_adsp"
>>>> +#define AUDIO_PDR_ADSP_SERVICE_NAME              "avs/audio"
>>>> +#define ADSP_AUDIOPD_NAME                        "msm/adsp/audio_pd"
>>>> +
>>>> +#define SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME   "sensors_pdr_adsp"
>>>> +#define SENSORS_PDR_ADSP_SERVICE_NAME              "tms/servreg"
>>>> +#define ADSP_SENSORPD_NAME                       "msm/adsp/sensor_pd"
>>>> +
>>>> +#define SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME "sensors_pdr_slpi"
>>>> +#define SENSORS_PDR_SLPI_SERVICE_NAME            SENSORS_PDR_ADSP_SERVICE_NAME
>>>> +#define SLPI_SENSORPD_NAME                       "msm/slpi/sensor_pd"
>>>> +
>>>>    static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
>>>>    						"sdsp", "cdsp"};
>>>>    struct fastrpc_phy_page {
>>>> @@ -258,6 +272,15 @@ struct fastrpc_session_ctx {
>>>>    	bool valid;
>>>>    };
>>>> +struct fastrpc_static_pd {
>>>> +	char *servloc_name;
>>>> +	char *spdname;
>>>> +	void *pdrhandle;
>>>> +	struct fastrpc_channel_ctx *cctx;
>>>> +	struct fastrpc_user *fl;
>>>> +	bool ispdup;
>>>> +};
>>>> +
>>>>    struct fastrpc_channel_ctx {
>>>>    	int domain_id;
>>>>    	int sesscount;
>>>> @@ -265,6 +288,7 @@ struct fastrpc_channel_ctx {
>>>>    	struct qcom_scm_vmperm vmperms[FASTRPC_MAX_VMIDS];
>>>>    	struct rpmsg_device *rpdev;
>>>>    	struct fastrpc_session_ctx session[FASTRPC_MAX_SESSIONS];
>>>> +	struct fastrpc_static_pd spd[FASTRPC_MAX_SPD];
>>>>    	spinlock_t lock;
>>>>    	struct idr ctx_idr;
>>>>    	struct list_head users;
>>>> @@ -296,10 +320,12 @@ struct fastrpc_user {
>>>>    	struct fastrpc_channel_ctx *cctx;
>>>>    	struct fastrpc_session_ctx *sctx;
>>>>    	struct fastrpc_buf *init_mem;
>>>> +	struct fastrpc_static_pd *spd;
>>>>    	int tgid;
>>>>    	int pd;
>>>>    	bool is_secure_dev;
>>>> +	char *servloc_name;
>>>>    	/* Lock for lists */
>>>>    	spinlock_t lock;
>>>>    	/* lock for allocations */
>>>> @@ -1257,12 +1283,33 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques
>>>>    	return false;
>>>>    }
>>>> +static struct fastrpc_static_pd *fastrpc_get_spd_session(
>>>> +				struct fastrpc_user *fl)
>>>> +{
>>>> +	int i;
>>>> +	struct fastrpc_static_pd *spd = NULL;
>>>> +	struct fastrpc_channel_ctx *cctx = fl->cctx;
>>>> +
>>>> +	for (i = 0; i < FASTRPC_MAX_SPD ; i++) {
>>>> +		if (!cctx->spd[i].servloc_name)
>>>> +			continue;
>>>> +		if (!strcmp(fl->servloc_name, cctx->spd[i].servloc_name)) {
>>>> +			spd = &cctx->spd[i];
>>>> +			spd->fl = fl;
>>>> +			break;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	return spd;
>>>> +}
>>>> +
>>>>    static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>>>>    					      char __user *argp)
>>>>    {
>>>>    	struct fastrpc_init_create_static init;
>>>>    	struct fastrpc_invoke_args *args;
>>>>    	struct fastrpc_phy_page pages[1];
>>>> +	struct fastrpc_static_pd *spd = NULL;
>>>>    	char *name;
>>>>    	int err;
>>>>    	struct {
>>>> @@ -1297,6 +1344,19 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>>>>    		goto err_name;
>>>>    	}
>>>> +	fl->servloc_name = AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME;
>>> Why are the audio and sensors sessions handled at different places?
>>> What about the MDSP or CDSP restarts?
>> Thanks for reviewing the patches, Dmitry. The remote methods for audio and
>> sensors PD attach are different and that is why both are handled in
>> different places.
> In which way are they different?

Both audio and sensors daemons are attaching to audio PD and sensors PD on DSP. Audio PD expects
some CMA memory for dynamic loading purpose which is allocated and sent to DSP in
fastrpc_init_create_static_process call.
Whereas for sensor daemon, the expectation is just to attach to sensors PD and take up any requests
made by the PD(like file operations etc.).

>
>> As for MDSP and CDSP, no static PDs are supported, hence,
> Should we still notify userspace if we get a PDR message for
> msm/cdsp/root_pd? Or for msm/modem/root_pd?

Static PDs are created on DSPs to support specific use cases like Audio and Sensors. The static PDs
allow the dynamic loading of shared objects with the help of a daemon running on the CPU. PDR handling
is required for static PD only. There are no static PD supported on MDSP or CDSP hence no PDR handling
is required. If root_pd is shutting down, that basically suggests that the remoteproc itself is shutting
down which is handled with rpmsg functionalities(probe and remove).
--Ekansh

>
>> there is no requirement to handle static PD restarts there. Please let me
>> know if you have any other queries. --Ekansh
>>
>>>> +
>>>> +	spd = fastrpc_get_spd_session(fl);
>>>> +	if (!spd) {
>>>> +		err = -EUSERS;
>>>> +		goto err_name;
>>>> +	}
>>>> +
>>>> +	if (!spd->ispdup) {
>>>> +		err = -ENOTCONN;
>>>> +		goto err_name;
>>>> +	}
>>>> +	fl->spd = spd;
>>>>    	if (!fl->cctx->remote_heap) {
>>>>    		err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
>>>>    						&fl->cctx->remote_heap);
>>>> @@ -1688,6 +1748,7 @@ static int fastrpc_dmabuf_alloc(struct fastrpc_user *fl, char __user *argp)
>>>>    static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
>>>>    {
>>>>    	struct fastrpc_invoke_args args[1];
>>>> +	struct fastrpc_static_pd *spd = NULL;
>>>>    	int tgid = fl->tgid;
>>>>    	u32 sc;
>>>> @@ -1697,6 +1758,22 @@ static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
>>>>    	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
>>>>    	fl->pd = pd;
>>>> +	if (pd == SENSORS_PD) {
>>>> +		if (fl->cctx->domain_id == ADSP_DOMAIN_ID)
>>>> +			fl->servloc_name = SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME;
>>>> +		else if (fl->cctx->domain_id == SDSP_DOMAIN_ID)
>>>> +			fl->servloc_name = SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME;
>>>> +
>>>> +		spd = fastrpc_get_spd_session(fl);
>>>> +		if (!spd)
>>>> +			return -EUSERS;
>>>> +
>>>> +		if (!spd->ispdup)
>>>> +			return -ENOTCONN;
>>>> +
>>>> +		fl->spd = spd;
>>>> +	}
>>>> +
>>>>    	return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
>>>>    				       sc, &args[0]);
>>>>    }
>>>> @@ -2172,6 +2249,64 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
>>>>    	return err;
>>>>    }
>>>> +static void fastrpc_notify_users(struct fastrpc_user *user)
>>>> +{
>>>> +	struct fastrpc_invoke_ctx *ctx;
>>>> +
>>>> +	spin_lock(&user->lock);
>>>> +	list_for_each_entry(ctx, &user->pending, node) {
>>>> +		ctx->retval = -EPIPE;
>>>> +		complete(&ctx->work);
>>>> +	}
>>>> +	spin_unlock(&user->lock);
>>>> +}
>>>> +
>>>> +static void fastrpc_notify_pdr_drivers(struct fastrpc_channel_ctx *cctx,
>>>> +		char *servloc_name)
>>>> +{
>>>> +	struct fastrpc_user *fl;
>>>> +	unsigned long flags;
>>>> +
>>>> +	spin_lock_irqsave(&cctx->lock, flags);
>>>> +	list_for_each_entry(fl, &cctx->users, user) {
>>>> +		if (fl->servloc_name && !strcmp(servloc_name, fl->servloc_name))
>>>> +			fastrpc_notify_users(fl);
>>>> +	}
>>>> +	spin_unlock_irqrestore(&cctx->lock, flags);
>>>> +}
>>>> +
>>>> +static void fastrpc_pdr_cb(int state, char *service_path, void *priv)
>>>> +{
>>>> +	struct fastrpc_static_pd *spd = (struct fastrpc_static_pd *)priv;
>>>> +	struct fastrpc_channel_ctx *cctx;
>>>> +
>>>> +	if (!spd)
>>>> +		return;
>>>> +
>>>> +	cctx = spd->cctx;
>>>> +	switch (state) {
>>>> +	case SERVREG_SERVICE_STATE_DOWN:
>>>> +		dev_info(&cctx->rpdev->dev,
>>>> +			"%s: %s (%s) is down for PDR on %s\n",
>>>> +			__func__, spd->spdname,
>>>> +			spd->servloc_name,
>>>> +			domains[cctx->domain_id]);
>>>> +		spd->ispdup = false;
>>>> +		fastrpc_notify_pdr_drivers(cctx, spd->servloc_name);
>>>> +		break;
>>>> +	case SERVREG_SERVICE_STATE_UP:
>>>> +		dev_info(&cctx->rpdev->dev,
>>>> +			"%s: %s (%s) is up for PDR on %s\n",
>>>> +			__func__, spd->spdname,
>>>> +			spd->servloc_name,
>>>> +			domains[cctx->domain_id]);
>>>> +		spd->ispdup = true;
>>>> +		break;
>>>> +	default:
>>>> +		break;
>>>> +	}
>>>> +}
>>>> +
>>>>    static const struct file_operations fastrpc_fops = {
>>>>    	.open = fastrpc_device_open,
>>>>    	.release = fastrpc_device_release,
>>>> @@ -2291,6 +2426,39 @@ static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ct
>>>>    	return err;
>>>>    }
>>>> +static int fastrpc_setup_service_locator(struct fastrpc_channel_ctx *cctx, char *client_name,
>>>> +			char *service_name, char *service_path, int domain, int spd_session)
>>>> +{
>>>> +	int err = 0;
>>>> +	struct pdr_handle *handle = NULL;
>>>> +	struct pdr_service *service = NULL;
>>>> +
>>>> +	/* Register the service locator's callback function */
>>>> +	handle = pdr_handle_alloc(fastrpc_pdr_cb, &cctx->spd[spd_session]);
>>>> +	if (IS_ERR(handle)) {
>>>> +		err = PTR_ERR(handle);
>>>> +		goto bail;
>>>> +	}
>>>> +	cctx->spd[spd_session].pdrhandle = handle;
>>>> +	cctx->spd[spd_session].servloc_name = client_name;
>>>> +	cctx->spd[spd_session].spdname = service_path;
>>>> +	cctx->spd[spd_session].cctx = cctx;
>>>> +	service = pdr_add_lookup(handle, service_name, service_path);
>>>> +	if (IS_ERR(service)) {
>>>> +		err = PTR_ERR(service);
>>>> +		goto bail;
>>>> +	}
>>>> +	pr_info("fastrpc: %s: pdr_add_lookup enabled for %s (%s, %s)\n",
>>>> +		__func__, service_name, client_name, service_path);
>>>> +
>>>> +bail:
>>>> +	if (err) {
>>>> +		pr_warn("fastrpc: %s: failed for %s (%s, %s)with err %d\n",
>>>> +				__func__, service_name, client_name, service_path, err);
>>>> +	}
>>>> +	return err;
>>>> +}
>>>> +
>>>>    static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>>>    {
>>>>    	struct device *rdev = &rpdev->dev;
>>>> @@ -2369,6 +2537,25 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>>>    		goto fdev_error;
>>>>    	}
>>>> +	if (domain_id == ADSP_DOMAIN_ID) {
>>>> +		err = fastrpc_setup_service_locator(data, AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME,
>>>> +			AUDIO_PDR_ADSP_SERVICE_NAME, ADSP_AUDIOPD_NAME, domain_id, 0);
>>>> +		if (err)
>>>> +			goto populate_error;
>>>> +
>>>> +		err = fastrpc_setup_service_locator(data,
>>>> +			SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME,
>>>> +			SENSORS_PDR_ADSP_SERVICE_NAME, ADSP_SENSORPD_NAME, domain_id, 1);
>>>> +		if (err)
>>>> +			goto populate_error;
>>>> +	} else if (domain_id == SDSP_DOMAIN_ID) {
>>>> +		err = fastrpc_setup_service_locator(data,
>>>> +			SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME,
>>>> +			SENSORS_PDR_SLPI_SERVICE_NAME, SLPI_SENSORPD_NAME, domain_id, 0);
>>>> +		if (err)
>>>> +			goto populate_error;
>>>> +	}
>>>> +
>>>>    	kref_init(&data->refcount);
>>>>    	dev_set_drvdata(&rpdev->dev, data);
>>>> @@ -2397,23 +2584,12 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>>>    	return err;
>>>>    }
>>>> -static void fastrpc_notify_users(struct fastrpc_user *user)
>>>> -{
>>>> -	struct fastrpc_invoke_ctx *ctx;
>>>> -
>>>> -	spin_lock(&user->lock);
>>>> -	list_for_each_entry(ctx, &user->pending, node) {
>>>> -		ctx->retval = -EPIPE;
>>>> -		complete(&ctx->work);
>>>> -	}
>>>> -	spin_unlock(&user->lock);
>>>> -}
>>>> -
>>>>    static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
>>>>    {
>>>>    	struct fastrpc_channel_ctx *cctx = dev_get_drvdata(&rpdev->dev);
>>>>    	struct fastrpc_user *user;
>>>>    	unsigned long flags;
>>>> +	int i;
>>>>    	/* No invocations past this point */
>>>>    	spin_lock_irqsave(&cctx->lock, flags);
>>>> @@ -2431,6 +2607,11 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
>>>>    	if (cctx->remote_heap)
>>>>    		fastrpc_buf_free(cctx->remote_heap);
>>>> +	for (i = 0; i < FASTRPC_MAX_SPD; i++) {
>>>> +		if (cctx->spd[i].pdrhandle)
>>>> +			pdr_handle_release(cctx->spd[i].pdrhandle);
>>>> +	}
>>>> +
>>>>    	of_platform_depopulate(&rpdev->dev);
>>>>    	fastrpc_channel_ctx_put(cctx);
>>>> -- 
>>>> 2.43.0
>>>>