Re: [PATCH v2 09/11] gunyah: rsc_mgr: Add auxiliary devices for console

From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Elliot Berman <quic_eberman@quicinc.com>,
	Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Murali Nalajala <quic_mnalajal@quicinc.com>,
	Trilok Soni <quic_tsoni@quicinc.com>,
	Srivatsa Vaddagiri <quic_svaddagi@quicinc.com>,
	Carl van Schaik <quic_cvanscha@quicinc.com>,
	Andy Gross <agross@kernel.org>,
	linux-arm-kernel@lists.infradead.org,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Sudeep Holla <sudeep.holla@arm.com>,
	Marc Zyngier <maz@kernel.org>, Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Jonathan Corbet <corbet@lwn.net>, Will Deacon <will@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	devicetree@vger.kernel.org, linux-doc@vger.kernel.org,
	linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH v2 09/11] gunyah: rsc_mgr: Add auxiliary devices for console
Date: Tue, 2 Aug 2022 11:38:50 +0300	[thread overview]
Message-ID: <869e9d41-7a21-c01a-ccbd-53bfb236938c@linaro.org> (raw)
In-Reply-To: <20220801211240.597859-10-quic_eberman@quicinc.com>

On 02/08/2022 00:12, Elliot Berman wrote:
> Gunyah resource manager exposes a concrete functionalities which
> complicate a single resource manager driver. Use auxiliary bus
> to help split high level functions for the resource manager and keep the
> primary resource manager driver focused on the RPC with RM itself.
> Delegate Resource Manager's console functionality to the auxiliary bus.
> 
> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> ---
>   drivers/virt/gunyah/rsc_mgr.c | 61 ++++++++++++++++++++++++++++++++++-
>   1 file changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/virt/gunyah/rsc_mgr.c b/drivers/virt/gunyah/rsc_mgr.c
> index b8268ee02fab..44b22cef7d44 100644
> --- a/drivers/virt/gunyah/rsc_mgr.c
> +++ b/drivers/virt/gunyah/rsc_mgr.c
> @@ -91,6 +91,11 @@ struct gh_rm_notif_complete {
>   	struct work_struct work;
>   };
>   
> +struct gh_rsc_mgr_adev {
> +	struct auxiliary_device adev;
> +	struct list_head list;
> +};
> +
>   struct gh_rsc_mgr {
>   	struct task_struct *recv_task;
>   	struct gunyah_device *msgq_tx, *msgq_rx;
> @@ -99,6 +104,13 @@ struct gh_rsc_mgr {
>   	struct mutex call_idr_lock;
>   
>   	struct mutex send_lock;
> +
> +	struct list_head adevs;
> +};
> +
> +/* List of auxiliary devices which resource manager creates */
> +static const char * const adev_names[] = {
> +	"console",
>   };

Which other auxilliary devices do you expect at this moment?

>   
>   static struct gh_rsc_mgr *__rsc_mgr;
> @@ -516,6 +528,14 @@ int gh_rm_unregister_notifier(struct notifier_block *nb)
>   }
>   EXPORT_SYMBOL_GPL(gh_rm_unregister_notifier);
>   
> +static void gh_rm_adev_release(struct device *dev)
> +{
> +	struct gh_rsc_mgr_adev *rm_adev = container_of(dev, struct gh_rsc_mgr_adev, adev.dev);
> +
> +	list_del(&rm_adev->list);

is there a race for the rsc_mgr->list? Can multiple release functions be 
called in parallel?
Rather than having a list, it would be easier to have an array of 
devices. Less race conditions, simpler code.

Or just add gh_rsc_msg->console_adev and use it directly without any 
additional bells and whistles.

> +	kfree(rm_adev);
> +}
> +
>   static struct gunyah_device *gh_msgq_platform_probe_direction(struct platform_device *pdev,
>   				u8 gh_type, int idx)
>   {
-- 
With best wishes
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel