This patch series adds core functionality to the remoteproc framework.
Patch 1 adds a new API to the framework which allows kernel clients to update
the firmware name for the specified remoteproc.
Patch 2 intends to improve the user experience by preventing the system from
going to sleep while the remoteproc is recovering from a crash.
Siddharth Gupta (2):
remoteproc: core: Add an API for booting with firmware name
remoteproc: core: Prevent sleep when rproc crashes
drivers/remoteproc/remoteproc_core.c | 38 ++++++++++++++++++++++++++++++++++++
include/linux/remoteproc.h | 1 +
2 files changed, 39 insertions(+)
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Remoteproc recovery should be fast and any delay will have an impact on the
user-experience. Use power management APIs (pm_stay_awake and pm_relax) to
ensure that the system does not go to sleep.
Signed-off-by: Siddharth Gupta <[email protected]>
---
drivers/remoteproc/remoteproc_core.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 5ab65a4..52e318c 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1712,6 +1712,8 @@ static void rproc_crash_handler_work(struct work_struct *work)
if (!rproc->recovery_disabled)
rproc_trigger_recovery(rproc);
+
+ pm_relax(&rproc->dev);
}
/**
@@ -2242,6 +2244,8 @@ void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type)
return;
}
+ pm_stay_awake(&rproc->dev);
+
dev_err(&rproc->dev, "crash detected in %s: type %s\n",
rproc->name, rproc_crash_to_string(type));
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Add an API which allows to change the name of the firmware to be booted on
the specified rproc. This change gives us the flixibility to change the
firmware at run-time depending on the usecase. Some remoteprocs might use
a different firmware for testing, production and development purposes,
which may be selected based on the fuse settings during bootup.
Signed-off-by: Siddharth Gupta <[email protected]>
---
drivers/remoteproc/remoteproc_core.c | 34 ++++++++++++++++++++++++++++++++++
include/linux/remoteproc.h | 1 +
2 files changed, 35 insertions(+)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 097f33e..5ab65a4 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1779,6 +1779,40 @@ int rproc_boot(struct rproc *rproc)
EXPORT_SYMBOL(rproc_boot);
/**
+ * rproc_boot_with_fw() - boot a remote processor with the specified firmware
+ * @rproc: handle of a remote processor
+ * @firmware: name of the firmware to boot with
+ *
+ * Change the name of the firmware to be loaded to @firmware in the rproc
+ * structure, and call rproc_boot().
+ *
+ * Returns 0 on success, and an appropriate error value otherwise.
+ */
+int rproc_boot_with_fw(struct rproc *rproc, const char *firmware)
+{
+ char *p;
+
+ if (!rproc) {
+ pr_err("invalid rproc handle\n");
+ return -EINVAL;
+ }
+
+ if (firmware) {
+ p = kstrdup(firmware, GFP_KERNEL);
+ if (!p)
+ return -ENOMEM;
+
+ mutex_lock(&rproc->lock);
+ kfree(rproc->firmware);
+ rproc->firmware = p;
+ mutex_unlock(&rproc->lock);
+ }
+
+ return rproc_boot(rproc);
+}
+EXPORT_SYMBOL(rproc_boot_with_fw);
+
+/**
* rproc_shutdown() - power off the remote processor
* @rproc: the remote processor
*
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 16ad666..e2eaba9 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -609,6 +609,7 @@ rproc_of_resm_mem_entry_init(struct device *dev, u32 of_resm_idx, int len,
u32 da, const char *name, ...);
int rproc_boot(struct rproc *rproc);
+int rproc_boot_with_fw(struct rproc *rproc, const char *firmware);
void rproc_shutdown(struct rproc *rproc);
void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type);
int rproc_coredump_add_segment(struct rproc *rproc, dma_addr_t da, size_t size);
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Hi Siddharth,
On Wed, Feb 19, 2020 at 06:11:52PM -0800, Siddharth Gupta wrote:
> Add an API which allows to change the name of the firmware to be booted on
> the specified rproc. This change gives us the flixibility to change the
> firmware at run-time depending on the usecase. Some remoteprocs might use
> a different firmware for testing, production and development purposes,
> which may be selected based on the fuse settings during bootup.
>
> Signed-off-by: Siddharth Gupta <[email protected]>
> ---
> drivers/remoteproc/remoteproc_core.c | 34 ++++++++++++++++++++++++++++++++++
> include/linux/remoteproc.h | 1 +
> 2 files changed, 35 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 097f33e..5ab65a4 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1779,6 +1779,40 @@ int rproc_boot(struct rproc *rproc)
> EXPORT_SYMBOL(rproc_boot);
>
> /**
> + * rproc_boot_with_fw() - boot a remote processor with the specified firmware
> + * @rproc: handle of a remote processor
> + * @firmware: name of the firmware to boot with
> + *
> + * Change the name of the firmware to be loaded to @firmware in the rproc
> + * structure, and call rproc_boot().
> + *
> + * Returns 0 on success, and an appropriate error value otherwise.
> + */
> +int rproc_boot_with_fw(struct rproc *rproc, const char *firmware)
> +{
> + char *p;
> +
> + if (!rproc) {
> + pr_err("invalid rproc handle\n");
> + return -EINVAL;
> + }
if (!rproc || !firmware)
return -EINVAL;
There is no user involved here so no point in printing anything. If @rproc or
@firmware is NULL than callers should be smart enough to figure it out from the
error code.
> +
> + if (firmware) {
> + p = kstrdup(firmware, GFP_KERNEL);
> + if (!p)
> + return -ENOMEM;
As in firmware_store() I think it is a good idea to mandate the MCU be offline
before changing the firmware name. That way we avoid situations where what is
running on the MCU is not what gets reported in sysfs.
> +
> + mutex_lock(&rproc->lock);
> + kfree(rproc->firmware);
> + rproc->firmware = p;
> + mutex_unlock(&rproc->lock);
> + }
> +
> + return rproc_boot(rproc);
Function rproc_boot() is also an exported symbol and belongs in the caller -
please move it out of here. When that is done rproc_boot_with_fw() can become
rproc_set_firmware_name() and concentrate on doing just that.
> +}
> +EXPORT_SYMBOL(rproc_boot_with_fw);
Although choosing the firmware image to boot without user involvement seems like
a valid scenario to me, this can't be added until there is an actual user of
this API.
> +
> +/**
> * rproc_shutdown() - power off the remote processor
> * @rproc: the remote processor
> *
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 16ad666..e2eaba9 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -609,6 +609,7 @@ rproc_of_resm_mem_entry_init(struct device *dev, u32 of_resm_idx, int len,
> u32 da, const char *name, ...);
>
> int rproc_boot(struct rproc *rproc);
> +int rproc_boot_with_fw(struct rproc *rproc, const char *firmware);
> void rproc_shutdown(struct rproc *rproc);
> void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type);
> int rproc_coredump_add_segment(struct rproc *rproc, dma_addr_t da, size_t size);
> --
> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
On Wed, Feb 19, 2020 at 06:11:53PM -0800, Siddharth Gupta wrote:
> Remoteproc recovery should be fast and any delay will have an impact on the
> user-experience. Use power management APIs (pm_stay_awake and pm_relax) to
> ensure that the system does not go to sleep.
When you say "ensure the system does not go to sleep", you're referring to the
system going idle from the CPUidle subsystem?
>
> Signed-off-by: Siddharth Gupta <[email protected]>
> ---
> drivers/remoteproc/remoteproc_core.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 5ab65a4..52e318c 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1712,6 +1712,8 @@ static void rproc_crash_handler_work(struct work_struct *work)
>
> if (!rproc->recovery_disabled)
> rproc_trigger_recovery(rproc);
> +
> + pm_relax(&rproc->dev);
> }
>
> /**
> @@ -2242,6 +2244,8 @@ void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type)
> return;
> }
>
> + pm_stay_awake(&rproc->dev);
> +
I fail to understand how this can be useful since there is no HW associted to
rproc->dev... Is it possible for you to elaborate more on the problem you're
trying to fix?
Thanks,
Mathieu
> dev_err(&rproc->dev, "crash detected in %s: type %s\n",
> rproc->name, rproc_crash_to_string(type));
>
> --
> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
Hey Mathieu,
On 2/24/2020 10:30 AM, Mathieu Poirier wrote:
> Hi Siddharth,
>
> On Wed, Feb 19, 2020 at 06:11:52PM -0800, Siddharth Gupta wrote:
>> Add an API which allows to change the name of the firmware to be booted on
>> the specified rproc. This change gives us the flixibility to change the
>> firmware at run-time depending on the usecase. Some remoteprocs might use
>> a different firmware for testing, production and development purposes,
>> which may be selected based on the fuse settings during bootup.
>>
>> Signed-off-by: Siddharth Gupta <[email protected]>
>> ---
>> drivers/remoteproc/remoteproc_core.c | 34 ++++++++++++++++++++++++++++++++++
>> include/linux/remoteproc.h | 1 +
>> 2 files changed, 35 insertions(+)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index 097f33e..5ab65a4 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1779,6 +1779,40 @@ int rproc_boot(struct rproc *rproc)
>> EXPORT_SYMBOL(rproc_boot);
>>
>> /**
>> + * rproc_boot_with_fw() - boot a remote processor with the specified firmware
>> + * @rproc: handle of a remote processor
>> + * @firmware: name of the firmware to boot with
>> + *
>> + * Change the name of the firmware to be loaded to @firmware in the rproc
>> + * structure, and call rproc_boot().
>> + *
>> + * Returns 0 on success, and an appropriate error value otherwise.
>> + */
>> +int rproc_boot_with_fw(struct rproc *rproc, const char *firmware)
>> +{
>> + char *p;
>> +
>> + if (!rproc) {
>> + pr_err("invalid rproc handle\n");
>> + return -EINVAL;
>> + }
> if (!rproc || !firmware)
> return -EINVAL;
>
> There is no user involved here so no point in printing anything. If @rproc or
> @firmware is NULL than callers should be smart enough to figure it out from the
> error code.
I was trying to mimic the behaviour of rproc_boot here actually, since
we were trying to make this
an API for users to directly boot with firmware name.
>
>> +
>> + if (firmware) {
>> + p = kstrdup(firmware, GFP_KERNEL);
>> + if (!p)
>> + return -ENOMEM;
> As in firmware_store() I think it is a good idea to mandate the MCU be offline
> before changing the firmware name. That way we avoid situations where what is
> running on the MCU is not what gets reported in sysfs.
Sure, that makes sense.
>> +
>> + mutex_lock(&rproc->lock);
>> + kfree(rproc->firmware);
>> + rproc->firmware = p;
>> + mutex_unlock(&rproc->lock);
>> + }
>> +
>> + return rproc_boot(rproc);
> Function rproc_boot() is also an exported symbol and belongs in the caller -
> please move it out of here. When that is done rproc_boot_with_fw() can become
> rproc_set_firmware_name() and concentrate on doing just that.
Okay sounds good.
>
>> +}
>> +EXPORT_SYMBOL(rproc_boot_with_fw);
> Although choosing the firmware image to boot without user involvement seems like
> a valid scenario to me, this can't be added until there is an actual user of
> this API.
That's true. We have a few cases downstream where we need this
functionality. We were wondering
if anyone else might have use of such functionality, and create an
upstream API in that case. Your
suggestion of creating rproc_set_firmware_name() is a better approach
for sure though. We're looking
at creating a new remoteproc (platform) driver which will need this
functionality.
>> +
>> +/**
>> * rproc_shutdown() - power off the remote processor
>> * @rproc: the remote processor
>> *
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index 16ad666..e2eaba9 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -609,6 +609,7 @@ rproc_of_resm_mem_entry_init(struct device *dev, u32 of_resm_idx, int len,
>> u32 da, const char *name, ...);
>>
>> int rproc_boot(struct rproc *rproc);
>> +int rproc_boot_with_fw(struct rproc *rproc, const char *firmware);
>> void rproc_shutdown(struct rproc *rproc);
>> void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type);
>> int rproc_coredump_add_segment(struct rproc *rproc, dma_addr_t da, size_t size);
>> --
>> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
Hey Mathieu,
I will be sending a revised patchset soon. Will try to address your
comments there.
Thanks,
Siddharth
On 2/24/2020 10:53 AM, Mathieu Poirier wrote:
> On Wed, Feb 19, 2020 at 06:11:53PM -0800, Siddharth Gupta wrote:
>> Remoteproc recovery should be fast and any delay will have an impact on the
>> user-experience. Use power management APIs (pm_stay_awake and pm_relax) to
>> ensure that the system does not go to sleep.
> When you say "ensure the system does not go to sleep", you're referring to the
> system going idle from the CPUidle subsystem?
>
>> Signed-off-by: Siddharth Gupta <[email protected]>
>> ---
>> drivers/remoteproc/remoteproc_core.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index 5ab65a4..52e318c 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1712,6 +1712,8 @@ static void rproc_crash_handler_work(struct work_struct *work)
>>
>> if (!rproc->recovery_disabled)
>> rproc_trigger_recovery(rproc);
>> +
>> + pm_relax(&rproc->dev);
>> }
>>
>> /**
>> @@ -2242,6 +2244,8 @@ void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type)
>> return;
>> }
>>
>> + pm_stay_awake(&rproc->dev);
>> +
> I fail to understand how this can be useful since there is no HW associted to
> rproc->dev... Is it possible for you to elaborate more on the problem you're
> trying to fix?
>
> Thanks,
> Mathieu
>
>> dev_err(&rproc->dev, "crash detected in %s: type %s\n",
>> rproc->name, rproc_crash_to_string(type));
>>
>> --
>> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
On Wed 19 Feb 18:11 PST 2020, Siddharth Gupta wrote:
> Remoteproc recovery should be fast and any delay will have an impact on the
> user-experience. Use power management APIs (pm_stay_awake and pm_relax) to
> ensure that the system does not go to sleep.
>
> Signed-off-by: Siddharth Gupta <[email protected]>
> ---
> drivers/remoteproc/remoteproc_core.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 5ab65a4..52e318c 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1712,6 +1712,8 @@ static void rproc_crash_handler_work(struct work_struct *work)
>
> if (!rproc->recovery_disabled)
> rproc_trigger_recovery(rproc);
> +
> + pm_relax(&rproc->dev);
> }
>
> /**
> @@ -2242,6 +2244,8 @@ void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type)
> return;
> }
>
> + pm_stay_awake(&rproc->dev);
Following Mathieu's question I was expecting you to do this on
rproc->dev.parent.
But looking at the implementation of pm_stay_awake(), it ends up being a
nop if dev->power.wakeup isn't specified. This in turn seems to come
from device_wakeup_enable(), which will bail if dev->power.can_wakeup is
not set. But I don't see where this would be set for either the platform
driver or the remoteproc's struct device - and neither one of them have
a "wakeup" attribute in sysfs.
Is there some additional plumbing needed for this?
Regards,
Bjorn
> +
> dev_err(&rproc->dev, "crash detected in %s: type %s\n",
> rproc->name, rproc_crash_to_string(type));
>
> --
> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
On 2020-04-07 15:29, Bjorn Andersson wrote:
> On Wed 19 Feb 18:11 PST 2020, Siddharth Gupta wrote:
>
>> Remoteproc recovery should be fast and any delay will have an impact
>> on the
>> user-experience. Use power management APIs (pm_stay_awake and
>> pm_relax) to
>> ensure that the system does not go to sleep.
>>
>> Signed-off-by: Siddharth Gupta <[email protected]>
>> ---
>> drivers/remoteproc/remoteproc_core.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c
>> b/drivers/remoteproc/remoteproc_core.c
>> index 5ab65a4..52e318c 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1712,6 +1712,8 @@ static void rproc_crash_handler_work(struct
>> work_struct *work)
>>
>> if (!rproc->recovery_disabled)
>> rproc_trigger_recovery(rproc);
>> +
>> + pm_relax(&rproc->dev);
>> }
>>
>> /**
>> @@ -2242,6 +2244,8 @@ void rproc_report_crash(struct rproc *rproc,
>> enum rproc_crash_type type)
>> return;
>> }
>>
>> + pm_stay_awake(&rproc->dev);
>
> Following Mathieu's question I was expecting you to do this on
> rproc->dev.parent.
>
> But looking at the implementation of pm_stay_awake(), it ends up being
> a
> nop if dev->power.wakeup isn't specified. This in turn seems to come
> from device_wakeup_enable(), which will bail if dev->power.can_wakeup
> is
> not set. But I don't see where this would be set for either the
> platform
> driver or the remoteproc's struct device - and neither one of them have
> a "wakeup" attribute in sysfs.
>
> Is there some additional plumbing needed for this?
We should be able to create a standalone wakeup source using
wakeup_source_init.
Then we can use _pm_stay_awake and _pm_relax on it.
>
> Regards,
> Bjorn
>
>> +
>> dev_err(&rproc->dev, "crash detected in %s: type %s\n",
>> rproc->name, rproc_crash_to_string(type));
>>
>> --
>> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
On Tue 07 Apr 15:59 PDT 2020, [email protected] wrote:
> On 2020-04-07 15:29, Bjorn Andersson wrote:
> > On Wed 19 Feb 18:11 PST 2020, Siddharth Gupta wrote:
> >
> > > Remoteproc recovery should be fast and any delay will have an impact
> > > on the
> > > user-experience. Use power management APIs (pm_stay_awake and
> > > pm_relax) to
> > > ensure that the system does not go to sleep.
> > >
> > > Signed-off-by: Siddharth Gupta <[email protected]>
> > > ---
> > > drivers/remoteproc/remoteproc_core.c | 4 ++++
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/remoteproc/remoteproc_core.c
> > > b/drivers/remoteproc/remoteproc_core.c
> > > index 5ab65a4..52e318c 100644
> > > --- a/drivers/remoteproc/remoteproc_core.c
> > > +++ b/drivers/remoteproc/remoteproc_core.c
> > > @@ -1712,6 +1712,8 @@ static void rproc_crash_handler_work(struct
> > > work_struct *work)
> > >
> > > if (!rproc->recovery_disabled)
> > > rproc_trigger_recovery(rproc);
> > > +
> > > + pm_relax(&rproc->dev);
> > > }
> > >
> > > /**
> > > @@ -2242,6 +2244,8 @@ void rproc_report_crash(struct rproc *rproc,
> > > enum rproc_crash_type type)
> > > return;
> > > }
> > >
> > > + pm_stay_awake(&rproc->dev);
> >
> > Following Mathieu's question I was expecting you to do this on
> > rproc->dev.parent.
> >
> > But looking at the implementation of pm_stay_awake(), it ends up being a
> > nop if dev->power.wakeup isn't specified. This in turn seems to come
> > from device_wakeup_enable(), which will bail if dev->power.can_wakeup is
> > not set. But I don't see where this would be set for either the platform
> > driver or the remoteproc's struct device - and neither one of them have
> > a "wakeup" attribute in sysfs.
> >
> > Is there some additional plumbing needed for this?
> We should be able to create a standalone wakeup source using
> wakeup_source_init.
> Then we can use _pm_stay_awake and _pm_relax on it.
Afaict the way to do this would be to call device_wakeup_enable() on
either the remoteproc or platform driver's struct device.
Given that the resources related to waking up the system are associated
with the platform driver I think this should be done on the platform
driver's struct device and these calls should operate on the rproc's
parent.
Regards,
Bjorn
> >
> > Regards,
> > Bjorn
> >
> > > +
> > > dev_err(&rproc->dev, "crash detected in %s: type %s\n",
> > > rproc->name, rproc_crash_to_string(type));
> > >
> > > --
> > > Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> > > a Linux Foundation Collaborative Project