On 2/24/21 12:35 AM, Mathieu Poirier wrote: > If it is possible to detach the remote processor, keep an untouched > copy of the resource table. That way we can start from the same > resource table without having to worry about original values or what > elements the startup code has changed when re-attaching to the remote > processor. > > Signed-off-by: Mathieu Poirier <[email protected]> > --- > New for V6: > - Double free of the cached table has been fixed. > - rproc_reset_loaded_rsc_table() has seen a complete re-write. > - rproc_stop() now calls rproc_reset_loaded_rsc_table() rather than > dealing with the cached. This allows to properly shutdown a > remote processor that was attached to. > --- > > drivers/remoteproc/remoteproc_core.c | 86 +++++++++++++++++++++++++++- > include/linux/remoteproc.h | 3 + > 2 files changed, 88 insertions(+), 1 deletion(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c > b/drivers/remoteproc/remoteproc_core.c > index fc01b29290a6..3a4692cc5220 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -1556,6 +1556,21 @@ static int rproc_set_loaded_rsc_table(struct rproc > *rproc) > return ret; > } > > + /* > + * If it is possible to detach the remote processor, keep an untouched > + * copy of the resource table. That way we can start fresh again when > + * the remote processor is re-attached, that is: > + * > + * DETACHED -> ATTACHED -> DETACHED -> ATTACHED > + * > + * Free'd in rproc_reset_loaded_rsc_table(). > + */ > + if (rproc->ops->detach) { > + rproc->clean_table = kmemdup(table_ptr, table_sz, GFP_KERNEL); > + if (!rproc->clean_table) > + return -ENOMEM; > + } > + > rproc->cached_table = NULL; > rproc->table_ptr = table_ptr; > rproc->table_sz = table_sz; > @@ -1563,6 +1578,65 @@ static int rproc_set_loaded_rsc_table(struct rproc > *rproc) > return 0; > } > > +static int rproc_reset_loaded_rsc_table(struct rproc *rproc) I spent some time to review this function that handles both the resource table for both the stop and detach. To make it easier to read, I would divide it into two functions. I added a proposal at the end of this mail. Regards, Arnaud > +{ > + struct resource_table *table_ptr; > + > + /* > + * The cached table is already set if the remote processor was started > + * by the remoteproc core. > + */ > + if (rproc->state == RPROC_RUNNING) { > + rproc->table_ptr = rproc->cached_table; > + return 0; > + } > + > + /* A resource table was never retrieved, nothing to do here */ > + if (!rproc->table_ptr) > + return 0; > + > + /* > + * If we made it to this point a cached_table _must_ have been > + * allocated in rproc_set_loaded_rsc_table(). If one isn't present > + * something went really wrong and we must complain. > + */ > + if (WARN_ON(!rproc->clean_table)) > + return -EINVAL; > + > + /* Remember where the external entity installed the resource table */ > + table_ptr = rproc->table_ptr; > + > + /* > + * Make a copy of the resource table currently used by the remote > + * processor. Free'd in rproc_detach() or rproc_shutdown(). > + */ > + rproc->cached_table = kmemdup(rproc->table_ptr, > + rproc->table_sz, GFP_KERNEL); > + if (!rproc->cached_table) > + return -ENOMEM; > + > + /* > + * Use a copy of the resource table for the remainder of the > + * shutdown process. > + */ > + rproc->table_ptr = rproc->cached_table; > + > + /* > + * Reset the memory area where the firmware loaded the resource table > + * to its original value. That way when we re-attach the remote > + * processor the resource table is clean and ready to be used again. > + */ > + memcpy(table_ptr, rproc->clean_table, rproc->table_sz); > + > + /* > + * The clean resource table is no longer needed. Allocated in > + * rproc_set_loaded_rsc_table(). > + */ > + kfree(rproc->clean_table); > + > + return 0; > +} > + > /* > * Attach to remote processor - similar to rproc_fw_boot() but without > * the steps that deal with the firmware image. > @@ -1688,7 +1762,11 @@ static int rproc_stop(struct rproc *rproc, bool > crashed) > rproc_stop_subdevices(rproc, crashed); > > /* the installed resource table is no longer accessible */ > - rproc->table_ptr = rproc->cached_table; > + ret = rproc_reset_loaded_rsc_table(rproc); > + if (ret) { > + dev_err(dev, "can't reset resource table: %d\n", ret); > + return ret; > + } > > /* power off the remote processor */ > ret = rproc->ops->stop(rproc); > @@ -1721,6 +1799,9 @@ static int __rproc_detach(struct rproc *rproc) > /* Stop any subdevices for the remote processor */ > rproc_stop_subdevices(rproc, false); > > + /* the installed resource table is no longer accessible */ > + ret = rproc_reset_loaded_rsc_table(rproc); > + > /* Tell the remote processor the core isn't available anymore */ > ret = rproc->ops->detach(rproc); > if (ret) { > @@ -1997,6 +2078,9 @@ int rproc_detach(struct rproc *rproc) > > rproc_disable_iommu(rproc); > > + /* Free the copy of the resource table */ > + kfree(rproc->cached_table); > + rproc->cached_table = NULL; > rproc->table_ptr = NULL; > out: > mutex_unlock(&rproc->lock); > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > index e1c843c19cc6..e5f52a12a650 100644 > --- a/include/linux/remoteproc.h > +++ b/include/linux/remoteproc.h > @@ -514,6 +514,8 @@ struct rproc_dump_segment { > * @recovery_disabled: flag that state if recovery was disabled > * @max_notifyid: largest allocated notify id. > * @table_ptr: pointer to the resource table in effect > + * @clean_table: copy of the resource table without modifications. Used > + * when a remote processor is attached or detached from the core > * @cached_table: copy of the resource table > * @table_sz: size of @cached_table > * @has_iommu: flag to indicate if remote processor is behind an MMU > @@ -550,6 +552,7 @@ struct rproc { > bool recovery_disabled; > int max_notifyid; > struct resource_table *table_ptr; > + struct resource_table *clean_table; > struct resource_table *cached_table; > size_t table_sz; > bool has_iommu; > --- drivers/remoteproc/remoteproc_core.c | 101 ++++++++++++++++++++++++++- include/linux/remoteproc.h | 3 + 2 files changed, 103 insertions(+), 1 deletion(-) diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index fc01b29290a6..cc24c360eb8b 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -1556,6 +1556,21 @@ static int rproc_set_loaded_rsc_table(struct rproc *rproc) return ret; } + /* + * If it is possible to detach the remote processor, keep an untouched + * copy of the resource table. That way we can start fresh again when + * the remote processor is re-attached, that is: + * + * DETACHED -> ATTACHED -> DETACHED -> ATTACHED + * + * Free'd in rproc_reset_loaded_rsc_table(). + */ + if (rproc->ops->detach) { + rproc->clean_table = kmemdup(table_ptr, table_sz, GFP_KERNEL); + if (!rproc->clean_table) + return -ENOMEM; + } + rproc->cached_table = NULL; rproc->table_ptr = table_ptr; rproc->table_sz = table_sz; @@ -1563,6 +1578,79 @@ static int rproc_set_loaded_rsc_table(struct rproc *rproc) return 0; } +static int rproc_set_to_cached_rsc_table(struct rproc *rproc) +{ + /* A resource table was never retrieved, nothing to do here */ + if (!rproc->table_ptr) + return 0; + + /* + * The cached table is already set if the remote processor was started + * by the remoteproc core. + */ + if (!rproc->cached_table) { + rproc->table_ptr = rproc->cached_table; + return 0; + } + + /* + * Make a copy of the resource table currently used by the remote + * processor. Free'd in rproc_detach() or rproc_shutdown(). + */ + rproc->cached_table = kmemdup(rproc->table_ptr, + rproc->table_sz, GFP_KERNEL); + if (!rproc->cached_table) + return -ENOMEM; + + /* + * Use a copy of the resource table for the remainder of the + * shutdown process. + */ + rproc->table_ptr = rproc->cached_table; + + return 0; +} + +static int rproc_reset_loaded_rsc_table(struct rproc *rproc) +{ + struct resource_table *table_ptr; + int ret; + + /* A resource table was never retrieved, nothing to do here */ + if (!rproc->table_ptr) + return 0; + + /* + * If we made it to this point a cached_table _mst_ have been + * allocated in rproc_set_loaded_rsc_table(). If one isn't present + * something went really wrong and we must complain. + */ + if (WARN_ON(!rproc->clean_table)) + return -EINVAL; + + /* Remember where the external entity installed the resource table */ + table_ptr = rproc->table_ptr; + + ret = rproc_set_to_cached_rsc_table(rproc); + if (ret) + return ret; + + /* + * Reset the memory area where the firmware loaded the resource table + * to its original value. That way when we re-attach the remote + * processor the resource table is clean and ready to be used again. + */ + memcpy(table_ptr, rproc->clean_table, rproc->table_sz); + + /* + * The clean resource table is no longer needed. Allocated in + * rproc_set_loaded_rsc_table(). + */ + kfree(rproc->clean_table); + + return 0; +} + /* * Attach to remote processor - similar to rproc_fw_boot() but without * the steps that deal with the firmware image. @@ -1688,7 +1776,11 @@ static int rproc_stop(struct rproc *rproc, bool crashed) rproc_stop_subdevices(rproc, crashed); /* the installed resource table is no longer accessible */ - rproc->table_ptr = rproc->cached_table; + ret = rproc_set_to_cached_rsc_table(rproc); + if (ret) { + dev_err(dev, "can't reset resource table: %d\n", ret); + return ret; + } /* power off the remote processor */ ret = rproc->ops->stop(rproc); @@ -1721,6 +1813,9 @@ static int __rproc_detach(struct rproc *rproc) /* Stop any subdevices for the remote processor */ rproc_stop_subdevices(rproc, false); + /* The installed resource table need to be reset and no longer accessible */ + ret = rproc_reset_loaded_rsc_table(rproc); + /* Tell the remote processor the core isn't available anymore */ ret = rproc->ops->detach(rproc); if (ret) { @@ -1941,6 +2036,7 @@ void rproc_shutdown(struct rproc *rproc) /* Free the copy of the resource table */ kfree(rproc->cached_table); + kfree(rproc->clean_table); rproc->cached_table = NULL; rproc->table_ptr = NULL; out: @@ -1997,6 +2093,9 @@ int rproc_detach(struct rproc *rproc) rproc_disable_iommu(rproc); + /* Free the copy of the resource table */ + kfree(rproc->cached_table); + rproc->cached_table = NULL; rproc->table_ptr = NULL; out: mutex_unlock(&rproc->lock); diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h index e1c843c19cc6..e5f52a12a650 100644 --- a/include/linux/remoteproc.h +++ b/include/linux/remoteproc.h @@ -514,6 +514,8 @@ struct rproc_dump_segment { * @recovery_disabled: flag that state if recovery was disabled * @max_notifyid: largest allocated notify id. * @table_ptr: pointer to the resource table in effect + * @clean_table: copy of the resource table without modifications. Used + * when a remote processor is attached or detached from the core * @cached_table: copy of the resource table * @table_sz: size of @cached_table * @has_iommu: flag to indicate if remote processor is behind an MMU @@ -550,6 +552,7 @@ struct rproc { bool recovery_disabled; int max_notifyid; struct resource_table *table_ptr; + struct resource_table *clean_table; struct resource_table *cached_table; size_t table_sz; bool has_iommu; --
