| Message ID | 20220928195633.2348848-14-quic_eberman@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Drivers for gunyah hypervisor | expand |
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
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
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. thanks, greg k-h
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
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 --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));
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(+)