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

[Elliot Berman <quic_eberman@quicinc.com>]

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