On 10/06/2018 01:13 AM, Bjorn Andersson wrote:
> On Fri 14 Sep 17:37 PDT 2018, Suman Anna wrote:
>
>> The remoteproc core performs automatic boot and shutdown of a
>> remote processor during rproc_add() and rproc_del() for remote
>> processors supporting 'auto-boot'. The remoteproc devices not using
>> 'auto-boot' require either a remoteproc client driver or a
>> userspace client to use the sysfs 'state' variable to perform the
>> boot and shutdown. The in-kernel client drivers hold the
>> corresponding remoteproc driver module's reference count when they
>> acquire a rproc handle through the rproc_get_by_phandle() API, but
>> there is no such support for userspace applications performing the
>> boot through sysfs interface.
>>
>> The shutdown of a remoteproc upon removing a remoteproc platform
>> driver is automatic only with 'auto-boot' and this can cause a
>> remoteproc with no auto-boot to stay powered on and never freed up
>> if booted using the sysfs interface without a matching stop, and
>> when the remoteproc driver module is removed or unbound from the
>> device. This will result in a memory leak as well as the
>> corresponding remoteproc ida being never deallocated. Fix this by
>> holding a module reference count for the remoteproc's driver during
>> a sysfs 'start' and releasing it during the sysfs 'stop'
>> operation.
>>
>
> This prevents you from rmmod'ing the remoteproc driver, but it does
> not prevent you from issuing an unbind of the driver - resulting in
> the same issue.
Well, the unbind is always a problem as it ignores the module usecounts,
and that's a generic issue. I have used suppress_bind_attrs in the
relevant remoteproc drivers downstream (need to actually add that for
wkup_m3) when they are being booted by other clients, otherwise more
often than not the client drivers create a kernel crash.
>
> I would prefer if we made sure that rproc_del() always cleaned up
> any resources (and stopped the remoteproc processor), but I'm
> uncertain of how to deal with remote processors that are supposed to
> survive Linux shutting down.
This creates unbalanced paths and we definitely do not want stop a
processor from underneath the client drivers that have booted them.
Hence this patch which creates parity with auto-booted processors and
still requested by some other clients.
regards
Suman
>
> But I'm also uncertain how we can make the remoteproc core ensure
> that no dynamic resources are leaked in such scenario.
>
> Regards, Bjorn
>
>> Signed-off-by: Suman Anna <s-anna@xxxxxx> ---
>> drivers/remoteproc/remoteproc_sysfs.c | 16 +++++++++++++++- 1 file
>> changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_sysfs.c
>> b/drivers/remoteproc/remoteproc_sysfs.c index
>> 47be411400e5..2142b3ea726e 100644 ---
>> a/drivers/remoteproc/remoteproc_sysfs.c +++
>> b/drivers/remoteproc/remoteproc_sysfs.c @@ -11,6 +11,7 @@ * GNU
>> General Public License for more details. */
>>
>> +#include <linux/module.h> #include <linux/remoteproc.h>
>>
>> #include "remoteproc_internal.h" @@ -100,14 +101,27 @@ static
>> ssize_t state_store(struct device *dev, if (rproc->state ==
>> RPROC_RUNNING) return -EBUSY;
>>
>> + /* + * prevent underlying implementation from being removed +
>> * when remoteproc does not support auto-boot + */ + if
>> (!rproc->auto_boot && +
>> !try_module_get(dev->parent->driver->owner)) + return -EINVAL; +
>> ret = rproc_boot(rproc); - if (ret) + if (ret) {
>> dev_err(&rproc->dev, "Boot failed: %d\n", ret); + if
>> (!rproc->auto_boot) + module_put(dev->parent->driver->owner); +
>> } } else if (sysfs_streq(buf, "stop")) { if (rproc->state !=
>> RPROC_RUNNING) return -EINVAL;
>>
>> rproc_shutdown(rproc); + if (!rproc->auto_boot) +
>> module_put(dev->parent->driver->owner); } else {
>> dev_err(&rproc->dev, "Unrecognised option: %s\n", buf); ret =
>> -EINVAL; -- 2.18.0
>>
