linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Remoteproc adaptations for ST-Ericsson modems
@ 2012-04-25 21:10 sjur.brandeland
  2012-05-06 21:00 ` Ohad Ben-Cohen
  0 siblings, 1 reply; 7+ messages in thread
From: sjur.brandeland @ 2012-04-25 21:10 UTC (permalink / raw)
  To: Ohad Ben-Cohen
  Cc: Loic PALLARDY, Ludovic BARRE, linux-kernel, Arnd Bergmann,
	Linus Walleij, Sjur Brændeland

Hi Ohad.

I have been thinking a bit more on the requirement we may have for
using remoteproc. I've also played around with remoteproc a bit,
and this is what I've found so far.

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. I have put together a patch below
doing this by ”overloading” the functions find_rsc_table() and load().

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. I'm not sure what is best approach here.


And a couple "nice to have":

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.

Skip the dependency on a driver:
Current solution assume that remoteproc is initiated from device
driver. 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.

Skip load of firmware during early boot:
I probably missed something, but this feature doesn't make sense
to me. Also it causes big warning from sysfs if I don't finish
the async loading before starting the blocking loading of firmware.
And I fail to see the need for it. I think it would be nice to be
able to turn this feature off.

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...

Regards,
Sjur


---
 drivers/remoteproc/remoteproc_core.c |   62 ++++++++++++++++++++++------------
 include/linux/remoteproc.h           |   12 +++++-
 2 files changed, 50 insertions(+), 24 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index ee15c68..f1b2fc9 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -45,6 +45,7 @@
 
 static void klist_rproc_get(struct klist_node *n);
 static void klist_rproc_put(struct klist_node *n);
+static void rproc_fw_config_virtio(const struct firmware *fw, void *context);
 
 /*
  * klist of the available remote processors.
@@ -215,7 +216,7 @@ static void *rproc_da_to_va(struct rproc *rproc, u64 da, int len)
  * supported, though.
  */
 static int
-rproc_load_segments(struct rproc *rproc, const u8 *elf_data, size_t len)
+default_rproc_load_segments(struct rproc *rproc, const u8 *elf_data, size_t len)
 {
 	struct device *dev = rproc->dev;
 	struct elf32_hdr *ehdr;
@@ -822,7 +823,7 @@ rproc_handle_virtio_rsc(struct rproc *rproc, struct resource_table *table, int l
 }
 
 /**
- * rproc_find_rsc_table() - find the resource table
+ * default_rproc_find_rsc_table() - find the resource table
  * @rproc: the rproc handle
  * @elf_data: the content of the ELF firmware image
  * @len: firmware size (in bytes)
@@ -838,8 +839,8 @@ rproc_handle_virtio_rsc(struct rproc *rproc, struct resource_table *table, int l
  * (and @tablesz isn't set).
  */
 static struct resource_table *
-rproc_find_rsc_table(struct rproc *rproc, const u8 *elf_data, size_t len,
-							int *tablesz)
+default_rproc_find_rsc_table(struct rproc *rproc, const u8 *elf_data,
+			     size_t len, int *tablesz)
 {
 	struct elf32_hdr *ehdr;
 	struct elf32_shdr *shdr;
@@ -1014,11 +1015,18 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	struct resource_table *table;
 	int ret, tablesz;
 
-	ret = rproc_fw_sanity_check(rproc, fw);
-	if (ret)
-		return ret;
-
-	ehdr = (struct elf32_hdr *)fw->data;
+	if (rproc->feature & RPROC_DO_USE_ELF) {
+		ret = rproc_fw_sanity_check(rproc, fw);
+		if (ret)
+			return ret;
+		/*
+		 * The ELF entry point is the rproc's boot addr (though this
+		 * is not a configurable property of all remote processors:
+		 * some will always boot at a specific hardcoded address).
+		 */
+		ehdr = (struct elf32_hdr *)fw->data;
+		rproc->bootaddr = ehdr->e_entry;
+	}
 
 	dev_info(dev, "Booting fw image %s, size %d\n", name, fw->size);
 
@@ -1032,18 +1040,14 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 		return ret;
 	}
 
-	/*
-	 * The ELF entry point is the rproc's boot addr (though this is not
-	 * a configurable property of all remote processors: some will always
-	 * boot at a specific hardcoded address).
-	 */
-	rproc->bootaddr = ehdr->e_entry;
-
 	/* look for the resource table */
-	table = rproc_find_rsc_table(rproc, fw->data, fw->size, &tablesz);
+	table = rproc->ops->find_rsc_table(rproc, fw->data, fw->size, &tablesz);
 	if (!table)
 		goto clean_up;
 
+	if (!(rproc->feature & RPROC_DO_EARLY_LOAD))
+		rproc_fw_config_virtio(fw, rproc);
+
 	/* handle fw resources which are required to boot rproc */
 	ret = rproc_handle_boot_rsc(rproc, table, tablesz);
 	if (ret) {
@@ -1052,7 +1056,7 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	}
 
 	/* load the ELF segments to memory */
-	ret = rproc_load_segments(rproc, fw->data, fw->size);
+	ret = rproc->ops->load(rproc, fw->data, fw->size);
 	if (ret) {
 		dev_err(dev, "Failed to load program segments: %d\n", ret);
 		goto clean_up;
@@ -1091,11 +1095,13 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
 	struct resource_table *table;
 	int ret, tablesz;
 
-	if (rproc_fw_sanity_check(rproc, fw) < 0)
+	if ((rproc->feature & RPROC_DO_USE_ELF) &&
+	    rproc_fw_sanity_check(rproc, fw) < 0)
 		goto out;
 
 	/* look for the resource table */
-	table = rproc_find_rsc_table(rproc, fw->data, fw->size, &tablesz);
+	table = rproc->ops->find_rsc_table(rproc, fw->data,
+					   fw->size, &tablesz);
 	if (!table)
 		goto out;
 
@@ -1422,6 +1428,10 @@ int rproc_register(struct rproc *rproc)
 	/* rproc_unregister() calls must wait until async loader completes */
 	init_completion(&rproc->firmware_loading_complete);
 
+	/* look for the resource table */
+	if (!(rproc->feature & RPROC_DO_EARLY_LOAD))
+		return ret;
+
 	/*
 	 * We must retrieve early virtio configuration info from
 	 * the firmware (e.g. whether to register a virtio device,
@@ -1467,7 +1477,7 @@ EXPORT_SYMBOL(rproc_register);
  * yet. Instead, if you just need to unroll rproc_alloc(), use rproc_free().
  */
 struct rproc *rproc_alloc(struct device *dev, const char *name,
-				const struct rproc_ops *ops,
+				struct rproc_ops *ops,
 				const char *firmware, int len)
 {
 	struct rproc *rproc;
@@ -1487,6 +1497,13 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 	rproc->firmware = firmware;
 	rproc->priv = &rproc[1];
 
+
+	if (!rproc->ops->find_rsc_table)
+		rproc->ops->find_rsc_table = default_rproc_find_rsc_table;
+
+	if (!rproc->ops->load)
+		rproc->ops->load = default_rproc_load_segments;
+
 	atomic_set(&rproc->power, 0);
 
 	kref_init(&rproc->refcount);
@@ -1501,7 +1518,8 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 	INIT_LIST_HEAD(&rproc->rvdevs);
 
 	rproc->state = RPROC_OFFLINE;
-
+	rproc->feature = RPROC_DO_EARLY_LOAD;
+	rproc->feature = RPROC_DO_USE_ELF;
 	return rproc;
 }
 EXPORT_SYMBOL(rproc_alloc);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index f1ffabb..2f88b5e 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -336,6 +336,10 @@ struct rproc_ops {
 	int (*start)(struct rproc *rproc);
 	int (*stop)(struct rproc *rproc);
 	void (*kick)(struct rproc *rproc, int vqid);
+	struct resource_table *(*find_rsc_table) (struct rproc *rproc,
+						  const u8 *data,
+						  size_t len, int *tablesz);
+	int (*load)(struct rproc *rproc, const u8 *data, size_t len);
 };
 
 /**
@@ -390,7 +394,7 @@ struct rproc {
 	const char *name;
 	const char *firmware;
 	void *priv;
-	const struct rproc_ops *ops;
+	struct rproc_ops *ops;
 	struct device *dev;
 	struct kref refcount;
 	atomic_t power;
@@ -405,8 +409,12 @@ struct rproc {
 	u32 bootaddr;
 	struct list_head rvdevs;
 	struct idr notifyids;
+	u32 feature;
 };
 
+#define RPROC_DO_EARLY_LOAD (1 << 1)
+#define RPROC_DO_USE_ELF (1 << 2)
+
 /* we currently support only two vrings per rvdev */
 #define RVDEV_NUM_VRINGS 2
 
@@ -454,7 +462,7 @@ struct rproc *rproc_get_by_name(const char *name);
 void rproc_put(struct rproc *rproc);
 
 struct rproc *rproc_alloc(struct device *dev, const char *name,
-				const struct rproc_ops *ops,
+				struct rproc_ops *ops,
 				const char *firmware, int len);
 void rproc_free(struct rproc *rproc);
 int rproc_register(struct rproc *rproc);
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: Remoteproc adaptations for ST-Ericsson modems
  2012-04-25 21:10 Remoteproc adaptations for ST-Ericsson modems sjur.brandeland
@ 2012-05-06 21:00 ` Ohad Ben-Cohen
  2012-05-08 13:26   ` Sjur BRENDELAND
  0 siblings, 1 reply; 7+ messages in thread
From: Ohad Ben-Cohen @ 2012-05-06 21:00 UTC (permalink / raw)
  To: sjur.brandeland
  Cc: Loic PALLARDY, Ludovic BARRE, linux-kernel, Arnd Bergmann,
	Linus Walleij, Sjur Brændeland

Hi Sjur,

On Thu, Apr 26, 2012 at 12:10 AM,  <sjur.brandeland@stericsson.com> 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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: Remoteproc adaptations for ST-Ericsson modems
  2012-05-06 21:00 ` Ohad Ben-Cohen
@ 2012-05-08 13:26   ` Sjur BRENDELAND
  2012-05-17 15:44     ` Ohad Ben-Cohen
  0 siblings, 1 reply; 7+ messages in thread
From: Sjur BRENDELAND @ 2012-05-08 13:26 UTC (permalink / raw)
  To: Ohad Ben-Cohen
  Cc: Loic PALLARDY, Ludovic BARRE, linux-kernel, Arnd Bergmann,
	Linus Walleij, Sjur Brændeland

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Remoteproc adaptations for ST-Ericsson modems
  2012-05-08 13:26   ` Sjur BRENDELAND
@ 2012-05-17 15:44     ` Ohad Ben-Cohen
  2012-05-21 11:38       ` Ohad Ben-Cohen
  0 siblings, 1 reply; 7+ messages in thread
From: Ohad Ben-Cohen @ 2012-05-17 15:44 UTC (permalink / raw)
  To: Sjur BRENDELAND
  Cc: Loic PALLARDY, Ludovic BARRE, linux-kernel, Arnd Bergmann,
	Linus Walleij, Sjur Brændeland

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
<sjur.brandeland@stericsson.com> 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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Remoteproc adaptations for ST-Ericsson modems
  2012-05-17 15:44     ` Ohad Ben-Cohen
@ 2012-05-21 11:38       ` Ohad Ben-Cohen
  2012-05-21 15:21         ` Sjur Brændeland
  0 siblings, 1 reply; 7+ messages in thread
From: Ohad Ben-Cohen @ 2012-05-21 11:38 UTC (permalink / raw)
  To: Sjur BRENDELAND
  Cc: Loic PALLARDY, Ludovic BARRE, linux-kernel, Arnd Bergmann,
	Linus Walleij, Sjur Brændeland

On Thu, May 17, 2012 at 6:44 PM, Ohad Ben-Cohen <ohad@wizery.com> 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);

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: Remoteproc adaptations for ST-Ericsson modems
  2012-05-21 11:38       ` Ohad Ben-Cohen
@ 2012-05-21 15:21         ` Sjur Brændeland
  2012-05-22  7:49           ` Ohad Ben-Cohen
  0 siblings, 1 reply; 7+ messages in thread
From: Sjur Brændeland @ 2012-05-21 15:21 UTC (permalink / raw)
  To: Ohad Ben-Cohen
  Cc: Loic PALLARDY, Ludovic BARRE, linux-kernel, Arnd Bergmann, Linus Walleij

> 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 <sjur.brandeland@stericsson.com>

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Remoteproc adaptations for ST-Ericsson modems
  2012-05-21 15:21         ` Sjur Brændeland
@ 2012-05-22  7:49           ` Ohad Ben-Cohen
  0 siblings, 0 replies; 7+ messages in thread
From: Ohad Ben-Cohen @ 2012-05-22  7:49 UTC (permalink / raw)
  To: Sjur Brændeland
  Cc: Loic PALLARDY, Ludovic BARRE, linux-kernel, Arnd Bergmann, Linus Walleij

On Mon, May 21, 2012 at 6:21 PM, Sjur Brændeland <sjurbren@gmail.com> wrote:
> Sure this works for me. If you like you can add:
> Tested-by: Sjur Brændeland <sjur.brandeland@stericsson.com>

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 <ohad@wizery.com>
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 <sjur.brandeland@stericsson.com>
Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
 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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2012-05-22  7:49 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-25 21:10 Remoteproc adaptations for ST-Ericsson modems sjur.brandeland
2012-05-06 21:00 ` Ohad Ben-Cohen
2012-05-08 13:26   ` Sjur BRENDELAND
2012-05-17 15:44     ` Ohad Ben-Cohen
2012-05-21 11:38       ` Ohad Ben-Cohen
2012-05-21 15:21         ` Sjur Brændeland
2012-05-22  7:49           ` Ohad Ben-Cohen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).