| Message ID | 1601331456-20432-1-git-send-email-rishabhb@codeaurora.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Move recovery/coredump configuration to sysfs | expand |
On 9/29/20 12:17 AM, 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. '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 builds > and "disabled" in production builds. Whereas recovery needs to be > "disabled" for debugging purposes and "enabled" on production builds. > This patch series removes the recovery/coredump entries from debugfs > and moves them to sysfs. Also, this disables the coredump collection > by default as this is a requirement for production devices. > > Changelog: > > v6 -> v5: > - Disable coredump collection by default > - Rename the "default" configuration to "enabled" to avoid confusion > > v5 -> v4: > - Fix the cover-letter of tha patch series. > > v4 -> v3: > - Remove the feature flag to expose recovery/coredump > > v3 -> v2: > - Remove the coredump/recovery entries from debugfs Sorry i missed this and some associated discussion in V2... I have also some concerns about the ABI breaks. In ST and I suppose in several companies we have some test environments that use the debugfs to generate and/or get the core dump. Even if the stability of the debugfs is not guaranteed it would be nice to keep both interface. It seems that it is possible to create symbolic link in the debugfs thanks to the "debugfs_create_symlink" function. This seems allowing to keep files in both place without duplicating the code. To be honest i have never used this function so I'm not 100% sure that this would do the job... But if you think that this could be a good compromise, i can test it. Regards, Arnaud > - Expose recovery/coredump from sysfs under a feature flag > > 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: Move coredump configuration to sysfs > remoteproc: Move recovery configuration to sysfs > remoteproc: Change default dump configuration to "disabled" > > Documentation/ABI/testing/sysfs-class-remoteproc | 46 +++++++ > drivers/remoteproc/remoteproc_coredump.c | 6 +- > drivers/remoteproc/remoteproc_debugfs.c | 168 ----------------------- > drivers/remoteproc/remoteproc_sysfs.c | 120 ++++++++++++++++ > include/linux/remoteproc.h | 8 +- > 5 files changed, 173 insertions(+), 175 deletions(-) >
On Tue 29 Sep 03:44 CDT 2020, Arnaud POULIQUEN wrote: > > > On 9/29/20 12:17 AM, 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. '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 builds > > and "disabled" in production builds. Whereas recovery needs to be > > "disabled" for debugging purposes and "enabled" on production builds. > > This patch series removes the recovery/coredump entries from debugfs > > and moves them to sysfs. Also, this disables the coredump collection > > by default as this is a requirement for production devices. > > > > Changelog: > > > > v6 -> v5: > > - Disable coredump collection by default > > - Rename the "default" configuration to "enabled" to avoid confusion > > > > v5 -> v4: > > - Fix the cover-letter of tha patch series. > > > > v4 -> v3: > > - Remove the feature flag to expose recovery/coredump > > > > v3 -> v2: > > - Remove the coredump/recovery entries from debugfs > > Sorry i missed this and some associated discussion in V2... > > I have also some concerns about the ABI breaks. Debugfs is not an ABI... > In ST and I suppose in several companies we have some > test environments that use the debugfs to generate and/or get > the core dump. > I do however acknowledge the inconvenience you're facing... > Even if the stability of the debugfs is not guaranteed it would > be nice to keep both interface. > ...and I wouldn't mind keeping the debugfs interface around, at least for some time to allow people to transition their tools/muscle memory. > It seems that it is possible to create symbolic link in the debugfs > thanks to the "debugfs_create_symlink" function. > This seems allowing to keep files in both place without duplicating the code. > To be honest i have never used this function so I'm not 100% sure that this > would do the job... > But if you think that this could be a good compromise, i can test it. > The duplicated code is rather simple, so I don't mind the duplication - for now. So, how about we add the sysfs pieces of Rishabh's patches, leave out the debugfs and then in a while (e.g. one LTS) we remove the debugfs code? Regards, Bjorn > Regards, > Arnaud > > > - Expose recovery/coredump from sysfs under a feature flag > > > > 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: Move coredump configuration to sysfs > > remoteproc: Move recovery configuration to sysfs > > remoteproc: Change default dump configuration to "disabled" > > > > Documentation/ABI/testing/sysfs-class-remoteproc | 46 +++++++ > > drivers/remoteproc/remoteproc_coredump.c | 6 +- > > drivers/remoteproc/remoteproc_debugfs.c | 168 ----------------------- > > drivers/remoteproc/remoteproc_sysfs.c | 120 ++++++++++++++++ > > include/linux/remoteproc.h | 8 +- > > 5 files changed, 173 insertions(+), 175 deletions(-) > >
On 9/29/20 4:33 PM, Bjorn Andersson wrote: > On Tue 29 Sep 03:44 CDT 2020, Arnaud POULIQUEN wrote: > >> >> >> On 9/29/20 12:17 AM, 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. '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 builds >>> and "disabled" in production builds. Whereas recovery needs to be >>> "disabled" for debugging purposes and "enabled" on production builds. >>> This patch series removes the recovery/coredump entries from debugfs >>> and moves them to sysfs. Also, this disables the coredump collection >>> by default as this is a requirement for production devices. >>> >>> Changelog: >>> >>> v6 -> v5: >>> - Disable coredump collection by default >>> - Rename the "default" configuration to "enabled" to avoid confusion >>> >>> v5 -> v4: >>> - Fix the cover-letter of tha patch series. >>> >>> v4 -> v3: >>> - Remove the feature flag to expose recovery/coredump >>> >>> v3 -> v2: >>> - Remove the coredump/recovery entries from debugfs >> >> Sorry i missed this and some associated discussion in V2... >> >> I have also some concerns about the ABI breaks. > > Debugfs is not an ABI... > >> In ST and I suppose in several companies we have some >> test environments that use the debugfs to generate and/or get >> the core dump. >> > > I do however acknowledge the inconvenience you're facing... > >> Even if the stability of the debugfs is not guaranteed it would >> be nice to keep both interface. >> > > ...and I wouldn't mind keeping the debugfs interface around, at least > for some time to allow people to transition their tools/muscle memory. > >> It seems that it is possible to create symbolic link in the debugfs >> thanks to the "debugfs_create_symlink" function. >> This seems allowing to keep files in both place without duplicating the code. >> To be honest i have never used this function so I'm not 100% sure that this >> would do the job... >> But if you think that this could be a good compromise, i can test it. >> > > The duplicated code is rather simple, so I don't mind the duplication - > for now. > > > So, how about we add the sysfs pieces of Rishabh's patches, leave out > the debugfs and then in a while (e.g. one LTS) we remove the debugfs > code? This smooth transition seems to me a very good compromise. Thanks, Arnaud > > Regards, > Bjorn > >> Regards, >> Arnaud >> >>> - Expose recovery/coredump from sysfs under a feature flag >>> >>> 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: Move coredump configuration to sysfs >>> remoteproc: Move recovery configuration to sysfs >>> remoteproc: Change default dump configuration to "disabled" >>> >>> Documentation/ABI/testing/sysfs-class-remoteproc | 46 +++++++ >>> drivers/remoteproc/remoteproc_coredump.c | 6 +- >>> drivers/remoteproc/remoteproc_debugfs.c | 168 ----------------------- >>> drivers/remoteproc/remoteproc_sysfs.c | 120 ++++++++++++++++ >>> include/linux/remoteproc.h | 8 +- >>> 5 files changed, 173 insertions(+), 175 deletions(-) >>>