Hi Sjur,
On Thu, Apr 26, 2012 at 12:10 AM, <[email protected]> wrote:
> 1. Support for non-ELF binaries:
> I'd like to see a solution for finding the ?resource_table? in non-ELF
> binaries and loading a non-ELF file.
Yes, several different vendors already indicated they need this.
So far It seems though that the non-ELF folks are waiting for someone
else to step up and implement this :)
> I have put together a patch below
> doing this by ?overloading? the functions find_rsc_table() and load().
Nice start!
I think we might want a slightly different abstraction though: instead
of allowing drivers to override the default ELF loader, we probably
want to have a set of external loader implementations, which drivers
could then pick when they register with the core (something like i2c's
algos).
This would make it possible for drivers to easily share the same
loader implementation without having to duplicate any loader code. A
nice exercise is then going to be extracting the ELF handling code
from remoteproc_core into its own external loader implementation.
Drivers should still be able to implement their own proprietary loader
if they want to, but if there's a chance it's going to be shared with
others, then they should implement it as an external loader.
> 2. Physical addressing:
> Support for specifying physical memory locations in resource table.
> We need some way to specify physical memory locations instead of using
> carveouts. The physical address will be outside the allocatable Linux
> memory.
Can you please share more details about the use case ? is this a
hardware limitation or a policy (i.e. by reserving memory using the
boot argument mem=) ?
Can you make this memory allocatable via the DMA API (e.g. via CMA or
dma_declare_coherent_memory) ?
> User-space life-cycle interface:
> The modem life-cycle is managed from user-space. It would be nice to
> have e.g. sysfs entries for start and stop of the modem from
> remoteproc.
Sounds fair. Feel free to suggest an interface.
> Skip the dependency on a driver:
> Current solution assume that remoteproc is initiated from device
> driver.
What do you mean by "initiated" ? powered on ?
> In the current C2C driver implementation we have internally,
> the device and driver is hidden underneath a functional API.
> So it doesn't necessarily make sense for us that remoteproc requires
> a device and driver as input.
Not sure I'm following. Care to elaborate ?
> Skip load of firmware during early boot:
> I probably missed something, but this feature doesn't make sense
> to me.
This mechanism registers the virito devices that are supported by the firmware.
Relevant drivers (if any) will then get probed, and may then power up
the remote processor (if needed).
> Also it causes big warning from sysfs if I don't finish
> the async loading before starting the blocking loading of firmware.
Can you explain how you triggered this ? Are you using rproc_get_by_name() ?
> And I fail to see the need for it.
Without it, no virtio driver is going to be probed - we don't
statically register virtio devices, because we don't know what kind of
functionality the remote processor supports without loading the
firmware first.
> I think it would be nice to be able to turn this feature off.
Care to explain why ? what exactly do you want to do that you can't today ?
> Below is a stab at supporting non-ELF binaries and disabling the
> pre-loading of ELF. Please consider this as an idea - I'd be quite
> happy if you take over the initiative and come up with a competely
> different solution...
It's in the TODO list of remoteproc, but unfortunately not a high
priority for me...
Thanks,
Ohad.
Hi Ohad,
>> 2. Physical addressing:
>> Support for specifying physical memory locations in resource table.
>> We need some way to specify physical memory locations instead of using
>> carveouts. The physical address will be outside the allocatable Linux
>> memory.
>
> Can you please share more details about the use case ? is this a
> hardware limitation or a policy (i.e. by reserving memory using the
> boot argument mem=) ?
For the C2C memory we don't have a IOMMU between memory and modem.
(There is some black magic using a offset register etc, but this is
all setup in the Linux boot loader.) So seen from Linux kernel point
of view we're working with a fixed physical address for the shared
memory.
> Can you make this memory allocatable via the DMA API (e.g. via CMA or
> dma_declare_coherent_memory) ?
Yes, I might be able to do just that. I wasn't aware of this function.
Nice! This way I don't need any other tweaks for the memory allocation
in remoteproc either - Very nice!
>> Skip the dependency on a driver:
...
> Not sure I'm following. Care to elaborate ?
I was trying to point out that I might call remoteproc from a
module_init function and not from a device driver. But I'm starting
to realize that both firmware-loading and dma_declare_coherent_memory
requires to work on a device. So let's just forget about this point.
>> Skip load of firmware during early boot:
>> I probably missed something, but this feature doesn't make sense
>> to me.
>
> This mechanism registers the virito devices that are supported by the
> firmware.
>
> Relevant drivers (if any) will then get probed, and may then power up
> the remote processor (if needed).
>
> > Also it causes big warning from sysfs if I don't finish
> > the async loading before starting the blocking loading of firmware.
>
> Can you explain how you triggered this ? Are you using
> rproc_get_by_name() ?
No, I trigger this warning by calling rproc_register() and rproc_boot()
in sequence from a remoteproc client without using rpmsg. If you haven't
finished the asynchronous loading of firmware initiated by
rproc_register() and call rproc_boot() immediately after you will get
a warning when initiating synchronous firmware loading from rproc_boot().
This is caused by loading the firmware for the same device twice
simultaneously.
> > And I fail to see the need for it.
>
> Without it, no virtio driver is going to be probed - we don't
> statically register virtio devices, because we don't know what kind of
> functionality the remote processor supports without loading the
> firmware first.
Ok, I see. You have a chicken and egg problem. And solve this by loading
the firmware twice. If I understand correctly you do something like this:
1. rproc-client does rproc_alloc() and rproc_register()
2. This trigger loading of firmware asynchronously.
3. Resource table is scanned for virtio device and virtio device
are registered.
4. Registration cause probing of rpmsg virtio driver
5. rpmsg virtio driver calls rproc_boot() from probe function.
6. remote_proc loads downloads firmware, parses the resource table 2nd time.
7. rproc->ops->load() is called and DSP is loaded and started.
>> I think it would be nice to be able to turn this feature off.
>
> Care to explain why ? what exactly do you want to do that you can't
> today ?
The difference is that I am not planning to call rproc_boot()
from rpmsg, but trigger booting from user space using sysfs. In this
case I don't need to probe any virtio drivers in order to trigger
the call of rproc_boot(). Consequently I don't need to load firmware twice.
So I think I'd like to see the following sequence:
1. rproc-client calls rproc_alloc() and rproc_register()
2. User space initiates booting via sysfs, causing rproc_boot() to be
called.
3. remoteproc initiates synchronous firmware loading.
4. resource table is scanned once, handling all resource entries
including virtio device registration.
5. rproc->ops->load() is called and DSP is loaded and started.
It would be good to know if this approach makes sense for you. And if
you agree, I'd like to get your view on how you think we could support
this in remoteproc. ie. do you approve of the feature bit I proposed,
or do you have something else in mind?
>> Below is a stab at supporting non-ELF binaries and disabling the
>> pre-loading of ELF.
> It's in the TODO list of remoteproc, but unfortunately not a high
> priority for me...
OK, good to know.
Best regards,
Sjur
Hi Sjur,
Sorry for the late response. I wanted to play with a few of the stuff
you mentioned, but some internal schedule prevented me from doing
that, and eventually I just decided not to wait anymore so we could at
least continue our discussion. so sorry again and thanks for your
patience.
On Tue, May 8, 2012 at 4:26 PM, Sjur BRENDELAND
<[email protected]> wrote:
> No, I trigger this warning by calling rproc_register() and rproc_boot()
> in sequence from a remoteproc client without using rpmsg.
Is this just testing code or a real driver you expect to merge ?
It sounds like we may have a race there, but the usage you describe is
somewhat non-standard and I don't think we really want to consider it
valid: usually we have a low level driver, responsible for the
platform-specific remoteproc implementation, which only calls
rproc_register() and not rproc_boot(), and a different, high-level
driver which triggers an rproc_boot() invocation when required by the
use case.
> 1. rproc-client does rproc_alloc() and rproc_register()
> 2. This trigger loading of firmware asynchronously.
> 3. Resource table is scanned for virtio device and virtio device
> ? are registered.
> 4. Registration cause probing of rpmsg virtio driver
Yes, and it may also cause probing of any other virtio driver whose
device was just registered by remoteproc.
> >> I think it would be nice to be able to turn this feature off.
> >
> > Care to explain why ? what exactly do you want to do that you can't
> > today ?
>
> The difference is that I am not planning to call rproc_boot()
> from rpmsg, but trigger booting from user space using sysfs.
What's the bigger design picture you have in mind ? No kernel drivers at all ?
How do you plan to support recovery from crashes of the remote
processor (i.e. who notifies user space that something bad happened) ?
After we'll understand the bigger picture, we could surely come up
with the technical solution.
Thanks!
Ohad.
On Thu, May 17, 2012 at 6:44 PM, Ohad Ben-Cohen <[email protected]> wrote:
> It sounds like we may have a race there
Can you please check if the below helps ? this will also protects
against premature invocation of rproc_get_by_name().
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remotepro
index 40e2b2d..3d93d9c 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1139,6 +1139,9 @@ int rproc_boot(struct rproc *rproc)
return -EINVAL;
}
+ /* if asynchronoush fw loading is underway, wait */
+ wait_for_completion(&rproc->firmware_loading_complete);
+
dev = rproc->dev;
ret = mutex_lock_interruptible(&rproc->lock);
> Can you please check if the below helps ? this will also protects
> against premature invocation of rproc_get_by_name().
Sure this works for me. If you like you can add:
Tested-by: Sjur Brændeland <[email protected]>
But, are you sure you don't want to do use wait_for_completion_timeout()
so you don't risk to block the clients if firmware loading fails?
For reference the crach without this patch looks like this:
------------[ cut here ]------------
WARNING: at fs/sysfs/dir.c:508 sysfs_add_one+0xb9/0xd4()
sysfs: cannot create duplicate filename '/devices/platform/foo.1/firmware/foo'
Modules linked in: virtio_rpmsg_bus remoteproc virtio_ring virtio
Call Trace:
6316f928: [<6003536e>] warn_slowpath_common+0x86/0xb0
6316f958: [<60189e2a>] strcat+0x0/0x28
6316f978: [<600354d6>] warn_slowpath_fmt+0x94/0x96
6316f9a8: [<6018505a>] ida_get_new_above+0x119/0x1dc
6316f9c0: [<60035442>] warn_slowpath_fmt+0x0/0x96
6316f9d8: [<60102c49>] sysfs_pathname+0x4f/0x57
6316f9f8: [<60102c49>] sysfs_pathname+0x4f/0x57
6316fa18: [<60102c49>] sysfs_pathname+0x4f/0x57
6316fa38: [<60102c49>] sysfs_pathname+0x4f/0x57
6316fa58: [<60103407>] sysfs_add_one+0xb9/0xd4
6316fa88: [<6010366e>] create_dir+0x89/0xd9
6316fae8: [<60103af6>] sysfs_create_dir+0x141/0x15f
6316faf8: [<6018bc35>] vsnprintf+0x212/0x48b
6316fb08: [<60185a01>] kobject_get+0x0/0x3a
6316fb28: [<60185c87>] kobject_add_internal+0xe1/0x24b
6316fb68: [<60186332>] kobject_add+0x110/0x124
6316fba8: [<60186222>] kobject_add+0x0/0x124
6316fbc0: [<6002c0f7>] unblock_signals+0x0/0x82
6316fbe8: [<6002c2b7>] set_signals+0x29/0x3f
6316fbf0: [<6002c0f7>] unblock_signals+0x0/0x82
6316fc18: [<600a639b>] kmem_cache_alloc+0x9f/0xab
6316fc38: [<6019ff09>] get_device_parent+0x1e0/0x210
6316fc68: [<601a162d>] device_add+0x11e/0x6dd
6316fc90: [<60053f82>] up_read+0x0/0x18
6316fc98: [<600502f4>] prepare_to_wait+0x0/0x8d
6316fcd8: [<601a879f>] _request_firmware_load+0x40/0x239
6316fd18: [<601a93ae>] request_firmware+0xb5/0x107
6316fd48: [<6006954a>] try_module_get+0x0/0x20
6316fd58: [<6388ecab>] rproc_boot+0x141/0x3e2 [remoteproc]
Regards,
Sjur
On Mon, May 21, 2012 at 6:21 PM, Sjur Br?ndeland <[email protected]> wrote:
> Sure this works for me. If you like you can add:
> Tested-by: Sjur Br?ndeland <[email protected]>
Sure, thanks!
> But, are you sure you don't want to do use wait_for_completion_timeout()
> so you don't risk to block the clients if firmware loading fails?
Good point. And if we're at it, I'll also use the _interruptible_ variant, see:
>From 2f2994b0159c400af53f3ec74c79574bb58a84d0 Mon Sep 17 00:00:00 2001
From: Ohad Ben-Cohen <[email protected]>
Date: Mon, 21 May 2012 14:55:47 +0300
Subject: [PATCH 2/3] remoteproc: block premature rproc booting
When an rproc instance is registered, remoteproc asynchronously
loads its firmware in order to tell which vdevs it supports.
Later on those vdevs are registered, and when probed, their drivers
usually trigger powering on of the remote processor.
OTOH, non-standard scenarios might involve early invocation of
rproc_boot even before the asynchronous fw loading has completed.
We're not sure we really want to support those scenarios, but right
now we do (e.g. via rproc_get_by_name), so let's simply fix this race
by blocking those premature rproc_boot() flows until the async fw
loading is completed.
Reported-and-tested-by: Sjur Brandeland <[email protected]>
Signed-off-by: Ohad Ben-Cohen <[email protected]>
---
drivers/remoteproc/remoteproc_core.c | 12 ++++++++++++
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_core.c
b/drivers/remoteproc/remoteproc_core.c
index 40e2b2d..464ea4f 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1141,6 +1141,18 @@ int rproc_boot(struct rproc *rproc)
dev = rproc->dev;
+ /*
+ * if asynchronoush fw loading is underway, wait up to 65 secs
+ * (just a bit more than the firmware request's timeout)
+ */
+ ret = wait_for_completion_interruptible_timeout(
+ &rproc->firmware_loading_complete,
+ msecs_to_jiffies(65000));
+ if (ret <= 0) {
+ dev_err(dev, "async fw loading isn't complete: %d\n", ret);
+ return ret ? ret : -ETIMEDOUT;
+ }
+
ret = mutex_lock_interruptible(&rproc->lock);
if (ret) {
dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
--
1.7.5.4