diff mbox series

[v5,1/5] sysfs: Add attribute info for /sys/devices/.../coredump_disabled

Message ID 20220810085753.v5.1.I5622b2a92dca4d2703a0f747e24f3ef19303e6df@changeid (mailing list archive)
State New, archived
Headers show
Series [v5,1/5] sysfs: Add attribute info for /sys/devices/.../coredump_disabled | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/checkpatch success Checkpatch PASS
tedd_an/gitlint success Gitlint PASS
tedd_an/subjectprefix fail "Bluetooth: " is not specified in the subject
tedd_an/buildkernel success Build Kernel PASS
tedd_an/buildkernel32 success Build Kernel32 PASS
tedd_an/incremental_build success Pass
tedd_an/testrunnersetup success Test Runner Setup PASS
tedd_an/testrunnerl2cap-tester success Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnerbnep-tester success Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnermgmt-tester success Total: 494, Passed: 494 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnerrfcomm-tester success Total: 10, Passed: 10 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnersco-tester success Total: 12, Passed: 12 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnersmp-tester success Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunneruserchan-tester success Total: 4, Passed: 4 (100.0%), Failed: 0, Not Run: 0

Commit Message

Manish Mandlik Aug. 10, 2022, 4 p.m. UTC 
This patch adds the specification for /sys/devices/.../coredump_disabled
attribute which allows the userspace to enable/disable devcoredump for a
particular device and drivers can use it to enable/disable functionality
accordingly. It is available when the CONFIG_DEV_COREDUMP is enabled and
driver has implemented the .coredump() callback.

Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Signed-off-by: Manish Mandlik <mmandlik@google.com>
---
Hi Arend, Greg,

The existing /sys/class/devcoredump/disabled provides only a one-way
disable functionality for devcoredump. It also disables the devcoredump
for everyone who is using it.

This and the next patch provides a way to selectively enable/disable the
devcoredump by creating a /sys/devices/.../coredump_disabled sysfs entry.
The userspace can write 0/1 to it to enable/disable devcoredump for that
particular device and drivers can use it accordingly. It will only be
available along with the /sys/devices/.../coredump sysfs entry when the
CONFIG_DEV_COREDUMP is enabled and the driver has implemented the
.coredump() callback.

Please let me know what you think about this.

Thanks,
Manish.

(no changes since v4)

Changes in v4:
- New patch in the series

 Documentation/ABI/testing/sysfs-devices-coredump | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Johannes Berg Aug. 10, 2022, 4:03 p.m. UTC | #1 
On Wed, 2022-08-10 at 09:00 -0700, Manish Mandlik wrote:
> This patch adds the specification for /sys/devices/.../coredump_disabled
> attribute which allows the userspace to enable/disable devcoredump for a
> particular device and drivers can use it to enable/disable functionality
> accordingly. It is available when the CONFIG_DEV_COREDUMP is enabled and
> driver has implemented the .coredump() callback.
> 

It would be nice to say _why_? What problem does this solve? You could
just create the dump and discard it, instead, for example?

johannes
Greg KH Aug. 10, 2022, 4:21 p.m. UTC | #2 
On Wed, Aug 10, 2022 at 06:03:37PM +0200, Johannes Berg wrote:
> On Wed, 2022-08-10 at 09:00 -0700, Manish Mandlik wrote:
> > This patch adds the specification for /sys/devices/.../coredump_disabled
> > attribute which allows the userspace to enable/disable devcoredump for a
> > particular device and drivers can use it to enable/disable functionality
> > accordingly. It is available when the CONFIG_DEV_COREDUMP is enabled and
> > driver has implemented the .coredump() callback.
> > 
> 
> It would be nice to say _why_? What problem does this solve? You could
> just create the dump and discard it, instead, for example?

Agreed, I do not understand the need for this at all.

thanks,

greg k-h
Greg KH Aug. 10, 2022, 4:22 p.m. UTC | #3 
On Wed, Aug 10, 2022 at 09:00:34AM -0700, Manish Mandlik wrote:
> +Date:		July 2022

You wrote this patch in August...
bluez.test.bot@gmail.com Aug. 10, 2022, 5:25 p.m. UTC | #4 
This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=666640

---Test result---

Test Summary:
CheckPatch                    FAIL      6.70 seconds
GitLint                       PASS      5.02 seconds
SubjectPrefix                 FAIL      1.74 seconds
BuildKernel                   PASS      34.25 seconds
BuildKernel32                 PASS      30.05 seconds
Incremental Build with patchesPASS      115.07 seconds
TestRunner: Setup             PASS      489.71 seconds
TestRunner: l2cap-tester      PASS      17.60 seconds
TestRunner: bnep-tester       PASS      6.69 seconds
TestRunner: mgmt-tester       PASS      104.89 seconds
TestRunner: rfcomm-tester     PASS      10.08 seconds
TestRunner: sco-tester        PASS      10.01 seconds
TestRunner: smp-tester        PASS      10.15 seconds
TestRunner: userchan-tester   PASS      7.00 seconds

Details
##############################
Test: CheckPatch - FAIL - 6.70 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
[v5,3/5] Bluetooth: Add support for hci devcoredump\Traceback (most recent call last):
  File "scripts/spdxcheck.py", line 6, in <module>
    from ply import lex, yacc
ModuleNotFoundError: No module named 'ply'
WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does MAINTAINERS need updating?
#152: 
new file mode 100644

WARNING:SPDX_LICENSE_TAG: Improper SPDX comment style for 'include/net/bluetooth/coredump.h', please use '/*' instead
#157: FILE: include/net/bluetooth/coredump.h:1:
+// SPDX-License-Identifier: GPL-2.0-only

WARNING:SPDX_LICENSE_TAG: Missing or malformed SPDX-License-Identifier tag in line 1
#157: FILE: include/net/bluetooth/coredump.h:1:
+// SPDX-License-Identifier: GPL-2.0-only

WARNING:SPLIT_STRING: quoted string split across lines
#617: FILE: net/bluetooth/coredump.c:300:
+				    "Devcoredump complete with size %u "
+				    "(expect %u)",

WARNING:SPLIT_STRING: quoted string split across lines
#636: FILE: net/bluetooth/coredump.c:319:
+				    "Devcoredump aborted with size %u "
+				    "(expect %u)",

WARNING:OOM_MESSAGE: Possible unnecessary 'out of memory' message
#739: FILE: net/bluetooth/coredump.c:422:
+	if (!skb) {
+		bt_dev_err(hdev, "Failed to allocate devcoredump init");

WARNING:OOM_MESSAGE: Possible unnecessary 'out of memory' message
#782: FILE: net/bluetooth/coredump.c:465:
+	if (!skb) {
+		bt_dev_err(hdev, "Failed to allocate devcoredump pattern");

WARNING:OOM_MESSAGE: Possible unnecessary 'out of memory' message
#808: FILE: net/bluetooth/coredump.c:491:
+	if (!skb) {
+		bt_dev_err(hdev, "Failed to allocate devcoredump complete");

WARNING:OOM_MESSAGE: Possible unnecessary 'out of memory' message
#830: FILE: net/bluetooth/coredump.c:513:
+	if (!skb) {
+		bt_dev_err(hdev, "Failed to allocate devcoredump abort");

total: 0 errors, 9 warnings, 0 checks, 699 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/12940740.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: SubjectPrefix - FAIL - 1.74 seconds
Check subject contains "Bluetooth" prefix
"Bluetooth: " is not specified in the subject
"Bluetooth: " is not specified in the subject



---
Regards,
Linux Bluetooth
Greg KH Aug. 12, 2022, 6:09 a.m. UTC | #5 
On Thu, Aug 11, 2022 at 04:21:54PM -0700, Manish Mandlik wrote:
> On Wed, Aug 10, 2022 at 9:21 AM Greg Kroah-Hartman <
> gregkh@linuxfoundation.org> wrote:
> 
> > On Wed, Aug 10, 2022 at 06:03:37PM +0200, Johannes Berg wrote:
> > > On Wed, 2022-08-10 at 09:00 -0700, Manish Mandlik wrote:
> > > > This patch adds the specification for
> > /sys/devices/.../coredump_disabled
> > > > attribute which allows the userspace to enable/disable devcoredump for
> > a
> > > > particular device and drivers can use it to enable/disable
> > functionality
> > > > accordingly. It is available when the CONFIG_DEV_COREDUMP is enabled
> > and
> > > > driver has implemented the .coredump() callback.
> > > >
> > >
> > > It would be nice to say _why_? What problem does this solve? You could
> > > just create the dump and discard it, instead, for example?
> >
> > Agreed, I do not understand the need for this at all.
> >
> 
> The existing /sys/class/devcoredump/disabled (devcd) switch has two
> limitations - it disables dev_coredump for everyone who's using it;

Which is good and is the design of the thing.

> and
> drivers don't have visibility if devcd is disabled or not, so, the
> dev_coredump API simply lets drivers collect the coredump from a device but
> then later discards it if devcd is disabled.

Why would a driver care?

> Now that there are more subsystems using the base dev_coredump API, having
> a granular control will make it easier to selectively disable dev_coredump
> only for a particular device. For ChromeOS, this is useful to allow drivers
> to develop coredump functionality and deploy it without affecting other
> drivers with stable devcoredump implementations (example, we've had some
> devcoredumps that take 12s to run and we only want to enable it on test
> builds because it has lots of PII). The drivers can use this flag to
> refrain from collecting or triggering coredump when undesirable.

This feels odd.  You have various out-of-tree drivers that take too long
when they crash to make a dump and it causes some unknown issue
elsewhere?

I don't really understand, sorry.

If you need something for development of a system, that's one thing, but
this feels like you are adding fine-grained tweaks that no one in a real
system would ever use.

What is broken with the current system of "on/off" that does not work
for you now?  Why would a normal user only want to turn this on for one
driver and not another?

thanks,

greg k-h
Arend Van Spriel Aug. 13, 2022, 9:39 a.m. UTC | #6 
On August 12, 2022 8:09:59 AM Greg Kroah-Hartman 
<gregkh@linuxfoundation.org> wrote:

> On Thu, Aug 11, 2022 at 04:21:54PM -0700, Manish Mandlik wrote:
>> On Wed, Aug 10, 2022 at 9:21 AM Greg Kroah-Hartman <
>> gregkh@linuxfoundation.org> wrote:
>>
>>> On Wed, Aug 10, 2022 at 06:03:37PM +0200, Johannes Berg wrote:
>>>> On Wed, 2022-08-10 at 09:00 -0700, Manish Mandlik wrote:
>>>>> This patch adds the specification for
>>> /sys/devices/.../coredump_disabled
>>>>> attribute which allows the userspace to enable/disable devcoredump for
>>> a
>>>>> particular device and drivers can use it to enable/disable
>>> functionality
>>>>> accordingly. It is available when the CONFIG_DEV_COREDUMP is enabled
>>> and
>>>>> driver has implemented the .coredump() callback.
>>>>
>>>> It would be nice to say _why_? What problem does this solve? You could
>>>> just create the dump and discard it, instead, for example?
>>>
>>> Agreed, I do not understand the need for this at all.
>>
>> The existing /sys/class/devcoredump/disabled (devcd) switch has two
>> limitations - it disables dev_coredump for everyone who's using it;
>
> Which is good and is the design of the thing.
>
>> and
>> drivers don't have visibility if devcd is disabled or not, so, the
>> dev_coredump API simply lets drivers collect the coredump from a device but
>> then later discards it if devcd is disabled.
>
> Why would a driver care?
>
>> Now that there are more subsystems using the base dev_coredump API, having
>> a granular control will make it easier to selectively disable dev_coredump
>> only for a particular device. For ChromeOS, this is useful to allow drivers
>> to develop coredump functionality and deploy it without affecting other
>> drivers with stable devcoredump implementations (example, we've had some
>> devcoredumps that take 12s to run and we only want to enable it on test
>> builds because it has lots of PII). The drivers can use this flag to
>> refrain from collecting or triggering coredump when undesirable.
>
> This feels odd.  You have various out-of-tree drivers that take too long
> when they crash to make a dump and it causes some unknown issue
> elsewhere?

If you have drivers taking 12s for coredump you could/should consider doing 
it asynchronous, eg. schedule a worker for it. The coredump callback has 
void return type so it would be fairly easy.

Regards,
Arend
diff mbox series

Patch

diff --git a/Documentation/ABI/testing/sysfs-devices-coredump b/Documentation/ABI/testing/sysfs-devices-coredump
index e459368533a4..4bcfc7dbad67 100644
--- a/Documentation/ABI/testing/sysfs-devices-coredump
+++ b/Documentation/ABI/testing/sysfs-devices-coredump
@@ -8,3 +8,17 @@  Description:
 		file will trigger the .coredump() callback.
 
 		Available when CONFIG_DEV_COREDUMP is enabled.
+
+What:		/sys/devices/.../coredump_disabled
+Date:		July 2022
+Contact:	Manish Mandlik <mmandlik@google.com>
+Description:
+		The /sys/devices/.../coredump_disabled attribute can be used by
+		drivers to selectively enable/disable devcoredump functionality
+		for a device. The userspace can write 0/1 to it to control
+		enabling/disabling of devcoredump for that particular device.
+		This attribute is present only when the device is bound to a
+		driver which implements the .coredump() callback. The attribute
+		is readable and writeable.
+
+		Available when CONFIG_DEV_COREDUMP is enabled.