Re: [PATCH v2 0/3] Expose recovery/coredump configuration from sysfs

[Bjorn Andersson]

On Tue 29 Sep 02:43 CDT 2020, Arnaud POULIQUEN wrote:

> Hi Bjorn,
> 
> On 9/26/20 5:31 AM, Bjorn Andersson wrote:
> > On Tue 15 Sep 02:51 PDT 2020, Arnaud POULIQUEN wrote:
> > 
> >> Hi Rishabh,
> >>
> >> On 8/27/20 9:48 PM, Rishabh Bhatnagar wrote:
> >>> From Android R onwards Google has restricted access to debugfs in user
> >>> and user-debug builds. This restricts access to most of the features
> >>> exposed through debugfs. This patch series adds a configurable option
> >>> to move the recovery/coredump interfaces to sysfs. If the feature
> >>> flag is selected it would move these interfaces to sysfs and remove
> >>> the equivalent debugfs interface. 'Coredump' and 'Recovery' are critical
> >>> interfaces that are required for remoteproc to work on Qualcomm Chipsets.
> >>> Coredump configuration needs to be set to "inline" in debug/test build
> >>> and "disabled" in production builds. Whereas recovery needs to be
> >>> "disabled" for debugging purposes and "enabled" on production builds.
> >>
> >> The remoteproc_cdev had been created to respond to some sysfs limitations.
> > 
> > The limitation here is in debugfs not being available on all systems,
> > sysfs is present and I really do like the idea of being able to change
> > these things without having to compile a tool to invoke the ioctl...
> 
> Right,
> 
> > 
> >> I wonder if this evolution should not also be implemented in the cdev.
> >> In this case an additional event could be addedd to inform the application
> >> that a crash occurred and that a core dump is available.
> >>
> > 
> > Specifically for userspace to know when a coredump is present there's
> > already uevents being sent when the devcoredump is ready. That said,
> > having some means to getting notified about remoteproc state changes
> > does sounds reasonable. If there is a use case we should discuss that.
> 
> The main use case i have in mind is to inform the userspace that the remote
> processor has crashed. This would allow applications to perform specific action
> to avoid getting stuck and/or resetting it's environement befor restarting the
> remote processor and associated IPC.
> If i well remember QCOM has this kind of mechanism for its modem but this is
> implemented in a platform driver.
> We would be interested to have something more generic relying on the remoteproc
> framework.
> 

I believe that there is such a notification mechanism implemented by
Qualcomm downstream. Upstream we've so far relied on the fact that the
interfaces exposed by the various rpmsg_devices would be torn down and
re-registered as the remoteproc is restarted.

Same goes with the few cases where we use rpmsg_char, as the channels
are going down the IO operations on the rpmsg endpoint fails to allow
userspace to detect the shutdown part. Then as the new channels appears
userspace will be notified about the newly available channels through
the standard uevents.

Regards,
Bjorn

> Thanks,
> Arnaud
> 
> > 
> >> Of course it's only a suggestion... As it would be a redesign.
> > 
> > A very valid suggestion. I don't think it's a redesign, but more of an
> > extension of what we have today.
> > 
> > Regards,
> > Bjorn
> > 
> >> I let Björn and Mathieu comment.
> >>
> >> Regards,
> >> Arnaud
> >>
> >>>
> >>> Changelog:
> >>>
> >>> v1 -> v2:
> >>> - Correct the contact name in the sysfs documentation.
> >>> - Remove the redundant write documentation for coredump/recovery sysfs
> >>> - Add a feature flag to make this interface switch configurable.
> >>>
> >>> Rishabh Bhatnagar (3):
> >>>   remoteproc: Expose remoteproc configuration through sysfs
> >>>   remoteproc: Add coredump configuration to sysfs
> >>>   remoteproc: Add recovery configuration to sysfs
> >>>
> >>>  Documentation/ABI/testing/sysfs-class-remoteproc |  44 ++++++++
> >>>  drivers/remoteproc/Kconfig                       |  12 +++
> >>>  drivers/remoteproc/remoteproc_debugfs.c          |  10 +-
> >>>  drivers/remoteproc/remoteproc_sysfs.c            | 126 +++++++++++++++++++++++
> >>>  4 files changed, 190 insertions(+), 2 deletions(-)
> >>>