diff mbox series

[v4,13/14] gunyah: rsc_mgr: Add auxiliary devices for console

Message ID 20220928195633.2348848-14-quic_eberman@quicinc.com
State New
Headers show
Series Drivers for gunyah hypervisor | expand

Commit Message

Elliot Berman Sept. 28, 2022, 7:56 p.m. UTC
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/Kconfig   |  1 +
 drivers/virt/gunyah/rsc_mgr.c | 21 +++++++++++++++++++++
 2 files changed, 22 insertions(+)

Comments

Greg KH Sept. 30, 2022, 12:19 p.m. UTC | #1
On Wed, Sep 28, 2022 at 12:56:32PM -0700, Elliot Berman wrote:
> Gunyah resource manager exposes a concrete functionalities which
> complicate a single resource manager driver.

I am sorry, but I do not understand this sentance.  What is so
complicated about individual devices being created?  Where are they
created?  What bus?

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/Kconfig   |  1 +
>  drivers/virt/gunyah/rsc_mgr.c | 21 +++++++++++++++++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/drivers/virt/gunyah/Kconfig b/drivers/virt/gunyah/Kconfig
> index 78deed3c4562..610c8586005b 100644
> --- a/drivers/virt/gunyah/Kconfig
> +++ b/drivers/virt/gunyah/Kconfig
> @@ -17,6 +17,7 @@ config GUNYAH_RESORUCE_MANAGER
>  	tristate "Gunyah Resource Manager"
>  	select MAILBOX
>  	select GUNYAH_MESSAGE_QUEUES
> +	select AUXILIARY_BUS
>  	default y
>  	help
>  	  The resource manager (RM) is a privileged application VM supporting
> diff --git a/drivers/virt/gunyah/rsc_mgr.c b/drivers/virt/gunyah/rsc_mgr.c
> index 7f7e89a6436b..435fe0149915 100644
> --- a/drivers/virt/gunyah/rsc_mgr.c
> +++ b/drivers/virt/gunyah/rsc_mgr.c
> @@ -16,6 +16,7 @@
>  #include <linux/notifier.h>
>  #include <linux/workqueue.h>
>  #include <linux/completion.h>
> +#include <linux/auxiliary_bus.h>
>  #include <linux/gunyah_rsc_mgr.h>
>  #include <linux/platform_device.h>
>  
> @@ -98,6 +99,8 @@ struct gh_rsc_mgr {
>  	struct mutex send_lock;
>  
>  	struct work_struct recv_work;
> +
> +	struct auxiliary_device console_adev;
>  };
>  
>  static struct gh_rsc_mgr *__rsc_mgr;
> @@ -573,13 +576,31 @@ static int gh_rm_drv_probe(struct platform_device *pdev)
>  
>  	__rsc_mgr = rsc_mgr;
>  
> +	rsc_mgr->console_adev.dev.parent = &pdev->dev;
> +	rsc_mgr->console_adev.name = "console";
> +	ret = auxiliary_device_init(&rsc_mgr->console_adev);
> +	if (ret)
> +		goto err_msgq;
> +	ret = auxiliary_device_add(&rsc_mgr->console_adev);
> +	if (ret)
> +		goto err_console_adev_uninit;
> +
>  	return 0;
> +
> +err_console_adev_uninit:
> +	auxiliary_device_uninit(&rsc_mgr->console_adev);
> +err_msgq:
> +	gunyah_msgq_remove(&rsc_mgr->msgq);
> +	return ret;
>  }

Why can't you just have individual platform devices for the individual
devices your hypervisor exposes?

You control the platform devices, why force them to be shared like this?

thanks,

greg k-h
Elliot Berman Oct. 4, 2022, 11:49 p.m. UTC | #2
On 9/30/2022 5:19 AM, Greg Kroah-Hartman wrote:
> On Wed, Sep 28, 2022 at 12:56:32PM -0700, Elliot Berman wrote:
>> Gunyah resource manager exposes a concrete functionalities which
>> complicate a single resource manager driver.
> 
> I am sorry, but I do not understand this sentance.  What is so
> complicated about individual devices being created?  Where are they
> created?  What bus?

There's no complexity here with using individual devices, that's why I 
wanted to create secondary (auxiliary devices).

IOW -- "I have a platform device that does a lot of different things. 
Split up the different functionalities of that device into sub devices 
using the auxiliary bus."

> 
> 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/Kconfig   |  1 +
>>   drivers/virt/gunyah/rsc_mgr.c | 21 +++++++++++++++++++++
>>   2 files changed, 22 insertions(+)
>>
>> diff --git a/drivers/virt/gunyah/Kconfig b/drivers/virt/gunyah/Kconfig
>> index 78deed3c4562..610c8586005b 100644
>> --- a/drivers/virt/gunyah/Kconfig
>> +++ b/drivers/virt/gunyah/Kconfig
>> @@ -17,6 +17,7 @@ config GUNYAH_RESORUCE_MANAGER
>>   	tristate "Gunyah Resource Manager"
>>   	select MAILBOX
>>   	select GUNYAH_MESSAGE_QUEUES
>> +	select AUXILIARY_BUS
>>   	default y
>>   	help
>>   	  The resource manager (RM) is a privileged application VM supporting
>> diff --git a/drivers/virt/gunyah/rsc_mgr.c b/drivers/virt/gunyah/rsc_mgr.c
>> index 7f7e89a6436b..435fe0149915 100644
>> --- a/drivers/virt/gunyah/rsc_mgr.c
>> +++ b/drivers/virt/gunyah/rsc_mgr.c
>> @@ -16,6 +16,7 @@
>>   #include <linux/notifier.h>
>>   #include <linux/workqueue.h>
>>   #include <linux/completion.h>
>> +#include <linux/auxiliary_bus.h>
>>   #include <linux/gunyah_rsc_mgr.h>
>>   #include <linux/platform_device.h>
>>   
>> @@ -98,6 +99,8 @@ struct gh_rsc_mgr {
>>   	struct mutex send_lock;
>>   
>>   	struct work_struct recv_work;
>> +
>> +	struct auxiliary_device console_adev;
>>   };
>>   
>>   static struct gh_rsc_mgr *__rsc_mgr;
>> @@ -573,13 +576,31 @@ static int gh_rm_drv_probe(struct platform_device *pdev)
>>   
>>   	__rsc_mgr = rsc_mgr;
>>   
>> +	rsc_mgr->console_adev.dev.parent = &pdev->dev;
>> +	rsc_mgr->console_adev.name = "console";
>> +	ret = auxiliary_device_init(&rsc_mgr->console_adev);
>> +	if (ret)
>> +		goto err_msgq;
>> +	ret = auxiliary_device_add(&rsc_mgr->console_adev);
>> +	if (ret)
>> +		goto err_console_adev_uninit;
>> +
>>   	return 0;
>> +
>> +err_console_adev_uninit:
>> +	auxiliary_device_uninit(&rsc_mgr->console_adev);
>> +err_msgq:
>> +	gunyah_msgq_remove(&rsc_mgr->msgq);
>> +	return ret;
>>   }
> 
> Why can't you just have individual platform devices for the individual
> devices your hypervisor exposes?
> 
> You control the platform devices, why force them to be shared like this?
> 

The resource manager exposes quite a bit of functionality, and I think 
it makes sense to expose them as auxiliary devices. From 
Documentation/driver-api/auxiliary_bus.rst:

A key requirement for utilizing the auxiliary bus is that there is no
dependency on a physical bus, device, register accesses or regmap support.
These individual devices split from the core cannot live on the platform 
bus as
they are not physical devices that are controlled by DT/ACPI.

> thanks,
> 
> greg k-h
Elliot Berman Oct. 5, 2022, 9:47 p.m. UTC | #3
On 10/4/2022 11:21 PM, Greg Kroah-Hartman wrote:
> On Tue, Oct 04, 2022 at 04:49:27PM -0700, Elliot Berman wrote:
>> On 9/30/2022 5:19 AM, Greg Kroah-Hartman wrote:
>>> On Wed, Sep 28, 2022 at 12:56:32PM -0700, Elliot Berman wrote:
>>>> Gunyah resource manager exposes a concrete functionalities which
>>>> complicate a single resource manager driver.
>>>
>>> I am sorry, but I do not understand this sentance.  What is so
>>> complicated about individual devices being created?  Where are they
>>> created?  What bus?
>>
>> There's no complexity here with using individual devices, that's why I
>> wanted to create secondary (auxiliary devices).
>>
>> IOW -- "I have a platform device that does a lot of different things. Split
>> up the different functionalities of that device into sub devices using the
>> auxiliary bus."
> 
> Why not just have multiple platform devices?  You control them, don't
> make it more complex than it should be.
> 
> And why are these platform devices at all?
> 
> As you say:
> 
>> A key requirement for utilizing the auxiliary bus is that there is no
>> dependency on a physical bus, device, register accesses or regmap support.
>> These individual devices split from the core cannot live on the platform bus
>> as they are not physical devices that are controlled by DT/ACPI.
> 
> These are not in the DT.  So just make your own bus for them instead of
> using a platform device.  Don't abuse a platform device please.
> 

I'll avoid creating platform devices. Are there any concerns with 
creating auxiliary device under the platform device? There will only be 
2 auxiliary devices under this Resource Manager device: one for console, 
and one for a VM loader.

> thanks,
> 
> greg k-h
Greg KH Oct. 6, 2022, 6:28 a.m. UTC | #4
On Wed, Oct 05, 2022 at 02:47:46PM -0700, Elliot Berman wrote:
> 
> 
> On 10/4/2022 11:21 PM, Greg Kroah-Hartman wrote:
> > On Tue, Oct 04, 2022 at 04:49:27PM -0700, Elliot Berman wrote:
> > > On 9/30/2022 5:19 AM, Greg Kroah-Hartman wrote:
> > > > On Wed, Sep 28, 2022 at 12:56:32PM -0700, Elliot Berman wrote:
> > > > > Gunyah resource manager exposes a concrete functionalities which
> > > > > complicate a single resource manager driver.
> > > > 
> > > > I am sorry, but I do not understand this sentance.  What is so
> > > > complicated about individual devices being created?  Where are they
> > > > created?  What bus?
> > > 
> > > There's no complexity here with using individual devices, that's why I
> > > wanted to create secondary (auxiliary devices).
> > > 
> > > IOW -- "I have a platform device that does a lot of different things. Split
> > > up the different functionalities of that device into sub devices using the
> > > auxiliary bus."
> > 
> > Why not just have multiple platform devices?  You control them, don't
> > make it more complex than it should be.
> > 
> > And why are these platform devices at all?
> > 
> > As you say:
> > 
> > > A key requirement for utilizing the auxiliary bus is that there is no
> > > dependency on a physical bus, device, register accesses or regmap support.
> > > These individual devices split from the core cannot live on the platform bus
> > > as they are not physical devices that are controlled by DT/ACPI.
> > 
> > These are not in the DT.  So just make your own bus for them instead of
> > using a platform device.  Don't abuse a platform device please.
> > 
> 
> I'll avoid creating platform devices. Are there any concerns with creating
> auxiliary device under the platform device?

Yes, don't do it if you do not have to, auxiliary devices are there only
if you have no other choice.

Just make 2 real devices on your own virtual bus please.

thanks,

greg k-h
diff mbox series

Patch

diff --git a/drivers/virt/gunyah/Kconfig b/drivers/virt/gunyah/Kconfig
index 78deed3c4562..610c8586005b 100644
--- a/drivers/virt/gunyah/Kconfig
+++ b/drivers/virt/gunyah/Kconfig
@@ -17,6 +17,7 @@  config GUNYAH_RESORUCE_MANAGER
 	tristate "Gunyah Resource Manager"
 	select MAILBOX
 	select GUNYAH_MESSAGE_QUEUES
+	select AUXILIARY_BUS
 	default y
 	help
 	  The resource manager (RM) is a privileged application VM supporting
diff --git a/drivers/virt/gunyah/rsc_mgr.c b/drivers/virt/gunyah/rsc_mgr.c
index 7f7e89a6436b..435fe0149915 100644
--- a/drivers/virt/gunyah/rsc_mgr.c
+++ b/drivers/virt/gunyah/rsc_mgr.c
@@ -16,6 +16,7 @@ 
 #include <linux/notifier.h>
 #include <linux/workqueue.h>
 #include <linux/completion.h>
+#include <linux/auxiliary_bus.h>
 #include <linux/gunyah_rsc_mgr.h>
 #include <linux/platform_device.h>
 
@@ -98,6 +99,8 @@  struct gh_rsc_mgr {
 	struct mutex send_lock;
 
 	struct work_struct recv_work;
+
+	struct auxiliary_device console_adev;
 };
 
 static struct gh_rsc_mgr *__rsc_mgr;
@@ -573,13 +576,31 @@  static int gh_rm_drv_probe(struct platform_device *pdev)
 
 	__rsc_mgr = rsc_mgr;
 
+	rsc_mgr->console_adev.dev.parent = &pdev->dev;
+	rsc_mgr->console_adev.name = "console";
+	ret = auxiliary_device_init(&rsc_mgr->console_adev);
+	if (ret)
+		goto err_msgq;
+	ret = auxiliary_device_add(&rsc_mgr->console_adev);
+	if (ret)
+		goto err_console_adev_uninit;
+
 	return 0;
+
+err_console_adev_uninit:
+	auxiliary_device_uninit(&rsc_mgr->console_adev);
+err_msgq:
+	gunyah_msgq_remove(&rsc_mgr->msgq);
+	return ret;
 }
 
 static int gh_rm_drv_remove(struct platform_device *pdev)
 {
 	struct gh_rsc_mgr *rsc_mgr = platform_get_drvdata(pdev);
 
+	auxiliary_device_delete(&rsc_mgr->console_adev);
+	auxiliary_device_uninit(&rsc_mgr->console_adev);
+
 	__rsc_mgr = NULL;
 
 	mbox_free_channel(gunyah_msgq_chan(&rsc_mgr->msgq));