From: Johannes Berg <johannes@sipsolutions.net> Date: 2020-05-18 19:26:41
On Mon, 2020-05-18 at 19:09 +0000, Luis Chamberlain wrote:
quoted
Unfortunately a "taint" is interpreted by many users as: "your kernel
is really F#*D up, you better do something about it right now."
Assuming they're paying attention at all in the first place of course.
Taint historically has been used and still is today to help rule out
whether or not you get support, or how you get support.
For instance, a staging driver is not supported by some upstream
developers, but it will be by those who help staging and Greg. TAINT_CRAP
cannot be even more clear.
So, no, it is not just about "hey your kernel is messed up", there are
clear support boundaries being drawn.
Err, no. Those two are most definitely related. Have you looked at (most
or some or whatever) staging drivers recently? Those contain all kinds
of garbage that might do whatever with your kernel.
Of course that's not a completely clear boundary, maybe you can find a
driver in staging that's perfect code just not written to kernel style?
But I find that hard to believe, in most cases.
So no, it's really not about "[a] staging driver is not supported" vs.
"your kernel is messed up". The very fact that you loaded one of those
things might very well have messed up your kernel entirely.
These days though, I think we all admit, that firmware crashes can use
a better generic infrastructure for ensuring that clearly affecting-user
experience issues. This patch is about that *when and if these happen*,
we annotate it in the kernel for support pursposes.
That's all fine, I just don't think it's appropriate to pretend that
your kernel is now 'tainted' (think about the meaning of that word) when
the firmware of some random device crashed. Heck, that could have been a
USB device that was since unplugged. Unless the driver is complete
garbage (hello staging again?) that really should have no lasting effect
on the system itself.
Recovery without affecting user experience would be great, the taint is
*not* for those cases. The taint definition has:
+ 18) ``Q`` used by device drivers to annotate that the device driver's firmware
+ has crashed and the device's operation has been severely affected. The
+ device may be left in a crippled state, requiring full driver removal /
+ addition, system reboot, or it is unclear how long recovery will take.
Let me know if this is not clear.
It's pretty clear, but even then, first of all I doubt this is the case
for many of the places that you've sprinkled the annotation on, and
secondly it actually hides useful information.
Regardless of the support issue, I think this hiding of information is
also problematic.
I really think we'd all be better off if you just made a sysfs file (I
mistyped debugfs in some other email, sorry, apparently you didn't see
the correction in time) that listed which device(s) crashed and how many
times. That would actually be useful. Because honestly, if a random
device crashed for some random reason, that's pretty much a non-event.
If it keeps happening, then we might even want to know about it.
You can obviously save the contents of this file into your bug reports
automatically and act accordingly, but I think you'll find that this is
far more useful than saying "TAINT_FIRMWARE_CRASHED" so I'll ignore this
report. Yeah, that might be reasonable thing if the bug report is about
slow wifi *and* you see that ath10k firmware crashed every 10 seconds,
but if it just crashed once a few days earlier it's of no importance to
the system anymore ... And certainly a reasonable driver (which I
believe ath10k to be) would _not_ randomly start corrupting memory
because its firmware crashed. Which really is what tainting the kernel
is about.
So no, even with all that, I still really believe you're solving the
wrong problem. Having information about firmware crashes, preferably
with some kind of frequency information attached, and *clearly* with
information about which device attached would be _great_. Munging it all
into one bit is actively harmful, IMO.
johannes
From: Luis Chamberlain <mcgrof@kernel.org> Date: 2020-05-18 19:59:55
On Mon, May 18, 2020 at 09:25:09PM +0200, Johannes Berg wrote:
On Mon, 2020-05-18 at 19:09 +0000, Luis Chamberlain wrote:
quoted
quoted
Unfortunately a "taint" is interpreted by many users as: "your kernel
is really F#*D up, you better do something about it right now."
Assuming they're paying attention at all in the first place of course.
Taint historically has been used and still is today to help rule out
whether or not you get support, or how you get support.
For instance, a staging driver is not supported by some upstream
developers, but it will be by those who help staging and Greg. TAINT_CRAP
cannot be even more clear.
So, no, it is not just about "hey your kernel is messed up", there are
clear support boundaries being drawn.
Err, no. Those two are most definitely related. Have you looked at (most
or some or whatever) staging drivers recently? Those contain all kinds
of garbage that might do whatever with your kernel.
No, I stay away :)
Of course that's not a completely clear boundary, maybe you can find a
driver in staging that's perfect code just not written to kernel style?
But I find that hard to believe, in most cases.
So no, it's really not about "[a] staging driver is not supported" vs.
"your kernel is messed up". The very fact that you loaded one of those
things might very well have messed up your kernel entirely.
quoted
These days though, I think we all admit, that firmware crashes can use
a better generic infrastructure for ensuring that clearly affecting-user
experience issues. This patch is about that *when and if these happen*,
we annotate it in the kernel for support pursposes.
That's all fine, I just don't think it's appropriate to pretend that
your kernel is now 'tainted' (think about the meaning of that word) when
the firmware of some random device crashed.
If the firmware crash *does* require driver remove / addition again,
or a reboot, would you think that this is a situation that merits a taint?
quoted
Recovery without affecting user experience would be great, the taint is
*not* for those cases. The taint definition has:
+ 18) ``Q`` used by device drivers to annotate that the device driver's firmware
+ has crashed and the device's operation has been severely affected. The
+ device may be left in a crippled state, requiring full driver removal /
+ addition, system reboot, or it is unclear how long recovery will take.
Let me know if this is not clear.
It's pretty clear, but even then, first of all I doubt this is the case
for many of the places that you've sprinkled the annotation on,
We can remove it, for this driver I can vouch for its location as it did
reach a state where I required a reboot. And its not the first time this
has happened. This got me thinking about the bigger picture of the lack
of proper way to address these cases in the kernel, and how the user is
left dumbfounded.
and secondly it actually hides useful information.
What is it hiding?
Regardless of the support issue, I think this hiding of information is
also problematic.
I really think we'd all be better off if you just made a sysfs file (I
mistyped debugfs in some other email, sorry, apparently you didn't see
the correction in time) that listed which device(s) crashed and how many
times.
Ah yes, count. The taint does not address count.
That would actually be useful. Because honestly, if a random
device crashed for some random reason, that's pretty much a non-event.
If it keeps happening, then we might even want to know about it.
True.
You can obviously save the contents of this file into your bug reports
automatically and act accordingly, but I think you'll find that this is
far more useful than saying "TAINT_FIRMWARE_CRASHED" so I'll ignore this
report.
Absolutely.
Yeah, that might be reasonable thing if the bug report is about
slow wifi *and* you see that ath10k firmware crashed every 10 seconds,
but if it just crashed once a few days earlier it's of no importance to
the system anymore ... And certainly a reasonable driver (which I
believe ath10k to be) would _not_ randomly start corrupting memory
because its firmware crashed. Which really is what tainting the kernel
is about.
I still see it as a support thing too. But discussing this further is
pointless as I agree that taint does not cover count and that it is
important.
Luis
From: Jakub Kicinski <kuba@kernel.org> Date: 2020-05-18 20:28:35
On Mon, 18 May 2020 21:25:09 +0200 Johannes Berg wrote:
It's pretty clear, but even then, first of all I doubt this is the case
for many of the places that you've sprinkled the annotation on, and
secondly it actually hides useful information.
Regardless of the support issue, I think this hiding of information is
also problematic.
I really think we'd all be better off if you just made a sysfs file (I
mistyped debugfs in some other email, sorry, apparently you didn't see
the correction in time) that listed which device(s) crashed and how many
times. That would actually be useful. Because honestly, if a random
device crashed for some random reason, that's pretty much a non-event.
If it keeps happening, then we might even want to know about it.
Johannes - have you seen devlink health? I think we should just use
that interface, since it supports all the things you're requesting,
rather than duplicate it in sysfs.
From: Johannes Berg <johannes@sipsolutions.net> Date: 2020-05-18 20:31:04
On Mon, 2020-05-18 at 13:28 -0700, Jakub Kicinski wrote:
On Mon, 18 May 2020 21:25:09 +0200 Johannes Berg wrote:
quoted
It's pretty clear, but even then, first of all I doubt this is the case
for many of the places that you've sprinkled the annotation on, and
secondly it actually hides useful information.
Regardless of the support issue, I think this hiding of information is
also problematic.
I really think we'd all be better off if you just made a sysfs file (I
mistyped debugfs in some other email, sorry, apparently you didn't see
the correction in time) that listed which device(s) crashed and how many
times. That would actually be useful. Because honestly, if a random
device crashed for some random reason, that's pretty much a non-event.
If it keeps happening, then we might even want to know about it.
Johannes - have you seen devlink health? I think we should just use
that interface, since it supports all the things you're requesting,
rather than duplicate it in sysfs.
I haven't, and I'm glad to hear that's there, sounds good!
I suspect that Luis wants something more generic though, that isn't just
applicable to netdevices, unless devlink grew some kind of non-netdev
stuff while I wasn't looking? :)
johannes
From: Jakub Kicinski <kuba@kernel.org> Date: 2020-05-18 20:35:27
On Mon, 18 May 2020 22:29:53 +0200 Johannes Berg wrote:
On Mon, 2020-05-18 at 13:28 -0700, Jakub Kicinski wrote:
quoted
On Mon, 18 May 2020 21:25:09 +0200 Johannes Berg wrote:
quoted
It's pretty clear, but even then, first of all I doubt this is the case
for many of the places that you've sprinkled the annotation on, and
secondly it actually hides useful information.
Regardless of the support issue, I think this hiding of information is
also problematic.
I really think we'd all be better off if you just made a sysfs file (I
mistyped debugfs in some other email, sorry, apparently you didn't see
the correction in time) that listed which device(s) crashed and how many
times. That would actually be useful. Because honestly, if a random
device crashed for some random reason, that's pretty much a non-event.
If it keeps happening, then we might even want to know about it.
Johannes - have you seen devlink health? I think we should just use
that interface, since it supports all the things you're requesting,
rather than duplicate it in sysfs.
I haven't, and I'm glad to hear that's there, sounds good!
I suspect that Luis wants something more generic though, that isn't just
applicable to netdevices, unless devlink grew some kind of non-netdev
stuff while I wasn't looking? :)
It's intended to be a generic netlink channel for configuring devices.
All the firmware-related interfaces have no dependencies on netdevs,
in fact that's one of the reasons we moved to devlink - we don't want
to hold rtnl lock just for talking to device firmware.
From: Johannes Berg <johannes@sipsolutions.net> Date: 2020-05-18 20:42:24
On Mon, 2020-05-18 at 13:35 -0700, Jakub Kicinski wrote:
It's intended to be a generic netlink channel for configuring devices.
All the firmware-related interfaces have no dependencies on netdevs,
in fact that's one of the reasons we moved to devlink - we don't want
to hold rtnl lock just for talking to device firmware.
Sounds good :)
So I guess Luis just has to add some way in devlink to hook up devlink
health in a simple way to drivers, perhaps? I mean, many drivers won't
really want to use devlink for anything else, so I guess it should be as
simple as the API that Luis proposed ("firmware crashed for this struct
device"), if nothing more interesting is done with devlink?
Dunno. But anyway sounds like it should somehow integrate there rather
than the way this patchset proposed?
johannes
From: Jakub Kicinski <kuba@kernel.org> Date: 2020-05-18 20:46:48
On Mon, 18 May 2020 22:41:48 +0200 Johannes Berg wrote:
On Mon, 2020-05-18 at 13:35 -0700, Jakub Kicinski wrote:
quoted
It's intended to be a generic netlink channel for configuring devices.
All the firmware-related interfaces have no dependencies on netdevs,
in fact that's one of the reasons we moved to devlink - we don't want
to hold rtnl lock just for talking to device firmware.
Sounds good :)
So I guess Luis just has to add some way in devlink to hook up devlink
health in a simple way to drivers, perhaps? I mean, many drivers won't
really want to use devlink for anything else, so I guess it should be as
simple as the API that Luis proposed ("firmware crashed for this struct
device"), if nothing more interesting is done with devlink?
Dunno. But anyway sounds like it should somehow integrate there rather
than the way this patchset proposed?
Right, that'd be great. Simple API to register a devlink instance with
whatever number of reporters the device would need. I'm happy to help.
From: Luis Chamberlain <mcgrof@kernel.org> Date: 2020-05-18 21:22:06
On Mon, May 18, 2020 at 01:46:43PM -0700, Jakub Kicinski wrote:
On Mon, 18 May 2020 22:41:48 +0200 Johannes Berg wrote:
quoted
On Mon, 2020-05-18 at 13:35 -0700, Jakub Kicinski wrote:
quoted
It's intended to be a generic netlink channel for configuring devices.
All the firmware-related interfaces have no dependencies on netdevs,
in fact that's one of the reasons we moved to devlink - we don't want
to hold rtnl lock just for talking to device firmware.
Sounds good :)
So I guess Luis just has to add some way in devlink to hook up devlink
health in a simple way to drivers, perhaps? I mean, many drivers won't
really want to use devlink for anything else, so I guess it should be as
simple as the API that Luis proposed ("firmware crashed for this struct
device"), if nothing more interesting is done with devlink?
Dunno. But anyway sounds like it should somehow integrate there rather
than the way this patchset proposed?
Right, that'd be great. Simple API to register a devlink instance with
whatever number of reporters the device would need. I'm happy to help.
Indeed my issue with devlink is that it did not seem generic enough for
all devices which use firmware and for which firmware can crash. Support
should not have to be "add devlink support" + "now use this new hook",
but rather a very lighweight devlink_crash(device) call we can sprinkly
with only the device as a functional requirement.
Luis
From: Jakub Kicinski <kuba@kernel.org> Date: 2020-05-18 22:16:51
On Mon, 18 May 2020 21:22:02 +0000 Luis Chamberlain wrote:
Indeed my issue with devlink is that it did not seem generic enough for
all devices which use firmware and for which firmware can crash. Support
should not have to be "add devlink support" + "now use this new hook",
but rather a very lighweight devlink_crash(device) call we can sprinkly
with only the device as a functional requirement.
We can provide a lightweight devlink_crash(device) which only generates
the notification, without the need to register the health reporter or a
devlink instance upfront. But then we loose the ability to control the
recovery, count errors, etc. So I'd think that's not the direction we
want to go in.
From: Luis Chamberlain <mcgrof@kernel.org> Date: 2020-05-19 01:05:35
On Mon, May 18, 2020 at 03:16:45PM -0700, Jakub Kicinski wrote:
On Mon, 18 May 2020 21:22:02 +0000 Luis Chamberlain wrote:
quoted
Indeed my issue with devlink is that it did not seem generic enough for
all devices which use firmware and for which firmware can crash. Support
should not have to be "add devlink support" + "now use this new hook",
but rather a very lighweight devlink_crash(device) call we can sprinkly
with only the device as a functional requirement.
We can provide a lightweight devlink_crash(device) which only generates
the notification, without the need to register the health reporter or a
devlink instance upfront. But then we loose the ability to control the
recovery, count errors, etc. So I'd think that's not the direction we
want to go in.
Care to show me what the diff stat for a non devlink driver would look
like? Just keep in mind this taint is 1 line addition. Granted, if we
can use SmPL grammar to automate addition of an initial framework to a
driver that'd be great, but since the device addition is subsystem
specific (device_add() and friends), I don't suspect this will be easily
automated.
Luis
From: Jakub Kicinski <kuba@kernel.org> Date: 2020-05-19 21:15:55
Add infra for creating devlink instances for a device to report
fw crashes. This patch expects the devlink instance to be registered
at probe time. I belive to be the cleanest. We can also add a devm
version of the helpers, so that we don't have to do the clean up.
Or we can go even further and register the devlink instance only
once error has happened (for the first time, then we can just
find out if already registered by traversing the list like we
do here).
With the patch applied and a sample driver converted we get:
$ devlink dev
pci/0000:07:00.0
Then monitor for errors:
$ devlink mon health
[health,status] pci/0000:07:00.0:
reporter fw
state error error 1 recover 0
[health,status] pci/0000:07:00.0:
reporter fw
state error error 2 recover 0
These are the events I triggered on purpose. One can also inspect
the health of all devices capable of reporting fw errors:
$ devlink health
pci/0000:07:00.0:
reporter fw
state error error 7 recover 0
Obviously drivers may upgrade to the full devlink health API
which includes state dump, state dump auto-collect and automatic
error recovery control.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
include/linux/devlink.h | 11 +++
net/core/Makefile | 2 +-
net/core/devlink_simple_fw_reporter.c | 101 ++++++++++++++++++++++++++
3 files changed, 113 insertions(+), 1 deletion(-)
create mode 100644 include/linux/devlink.h
create mode 100644 net/core/devlink_simple_fw_reporter.c
@@ -423,6 +424,8 @@ int i2400mu_probe(struct usb_interface *iface,if(usb_dev->speed!=USB_SPEED_HIGH)dev_err(dev,"device not connected as high speed\n");+devlink_simple_fw_reporter_prepare(dev);+/* Allocate instance [calls i2400m_netdev_setup() on it]. */result=-ENOMEM;net_dev=alloc_netdev(sizeof(*i2400mu),"wmx%d",NET_NAME_UNKNOWN,
@@ -506,6 +509,7 @@ int i2400mu_probe(struct usb_interface *iface,usb_put_dev(i2400mu->usb_dev);free_netdev(net_dev);error_alloc_netdev:+devlink_simple_fw_reporter_cleanup(dev);returnresult;}
From: Luis Chamberlain <mcgrof@kernel.org> Date: 2020-05-22 05:20:51
On Tue, May 19, 2020 at 02:15:30PM -0700, Jakub Kicinski wrote:
Add infra for creating devlink instances for a device to report
Thanks for doing this series as a PoC, counter to the module_firmware_crash()
which I proposed to taint the kernel with a firmware crash flag to the kernel
and module.
For those not famliar about devlink:
https://lwn.net/Articles/677967/https://www.kernel.org/doc/html/latest/networking/devlink/index.html
The github page also is now 404 as Jiri merged that stuff into iproute2:
git://git.kernel.org/pub/scm/network/iproute2/iproute2.git
quoted hunk
fw crashes. This patch expects the devlink instance to be registered
at probe time. I belive to be the cleanest. We can also add a devm
version of the helpers, so that we don't have to do the clean up.
Or we can go even further and register the devlink instance only
once error has happened (for the first time, then we can just
find out if already registered by traversing the list like we
do here).
With the patch applied and a sample driver converted we get:
$ devlink dev
pci/0000:07:00.0
Then monitor for errors:
$ devlink mon health
[health,status] pci/0000:07:00.0:
reporter fw
state error error 1 recover 0
[health,status] pci/0000:07:00.0:
reporter fw
state error error 2 recover 0
These are the events I triggered on purpose. One can also inspect
the health of all devices capable of reporting fw errors:
$ devlink health
pci/0000:07:00.0:
reporter fw
state error error 7 recover 0
Obviously drivers may upgrade to the full devlink health API
which includes state dump, state dump auto-collect and automatic
error recovery control.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
include/linux/devlink.h | 11 +++
net/core/Makefile | 2 +-
net/core/devlink_simple_fw_reporter.c | 101 ++++++++++++++++++++++++++
3 files changed, 113 insertions(+), 1 deletion(-)
create mode 100644 include/linux/devlink.h
create mode 100644 net/core/devlink_simple_fw_reporter.c
This was looking super sexy up to here. This is networking specific.
We want something generic for *anything* that requests firmware.
I'm afraid this won't work for something generic. I don't think its
throw-away work though, the idea to provide a generic interface to
dump firmware through netlink might be nice for networking, or other
things.
But I have a feeling we'll want something still more generic than this.
So networking may want to be aware that a firmware crash happened as
part of this network device health thing, but firmware crashing is a
generic thing.
I have now extended my patch set to include uvents and I am more set on
that we need the taint now more than ever.
Luis
This was looking super sexy up to here. This is networking specific.
We want something generic for *anything* that requests firmware.
You can't be serious. It's network specific because of how the Kconfig
is named?
Working for a company operating large data centers I would strongly
prefer if we didn't have ten different ways of reporting firmware
problems in the fleet.
I'm afraid this won't work for something generic. I don't think its
throw-away work though, the idea to provide a generic interface to
dump firmware through netlink might be nice for networking, or other
things.
But I have a feeling we'll want something still more generic than this.
Please be specific. Saying generic a lot is not helpful. The code (as
you can see in this patch) is in no way network specific. Or are you
saying there are machines out there running without netlink sockets?
So networking may want to be aware that a firmware crash happened as
part of this network device health thing, but firmware crashing is a
generic thing.
I have now extended my patch set to include uvents and I am more set on
that we need the taint now more than ever.
Please expect my nack if you're trying to add this to networking
drivers.
The irony is you have a problem with a networking device and all the
devices your initial set touched are networking. Two of the drivers
you touched either have or will soon have devlink health reporters
implemented.
This was looking super sexy up to here. This is networking specific.
We want something generic for *anything* that requests firmware.
You can't be serious. It's network specific because of how the Kconfig
is named?
Wait, yeah, what?
Working for a company operating large data centers I would strongly
prefer if we didn't have ten different ways of reporting firmware
problems in the fleet.
Agree. I don't actually operate anything, but still ...
Thinking about this - maybe there's a way to still combine devcoredump
and devlink somehow?
Or (optionally) make devlink trigger devcoredump while userspace
migrates?
quoted
So networking may want to be aware that a firmware crash happened as
part of this network device health thing, but firmware crashing is a
generic thing.
I have now extended my patch set to include uvents and I am more set on
that we need the taint now more than ever.
FWIW, I still completely disagree on that taint. You (Luis) obviously
have been running into a bug in that driver, I doubt the firmware
actually managed to wedge the hardware.
But even if it did, that's still not really a kernel taint. The kernel
itself isn't in any way affected by this.
Yes, the system is in a weird state now. But that's *not* equivalent to
"kernel tainted".
The irony is you have a problem with a networking device and all the
devices your initial set touched are networking. Two of the drivers
you touched either have or will soon have devlink health reporters
implemented.
Like I said above, do you think it'd be feasible to make a devcoredump
out of devlink health reports? And can the report be in a way that we
control the file format, or are there limits? I guess I should read the
code to find out, but I figure you probably just know. But feel free to
tell me to read it :)
The reason I'm asking is that it's starting to sound like we really
ought to be implementing devlink, but we've got a bunch of
infrastructure that uses the devcoredump, and it'll take time
(significantly so) to change all that...
johannes
This was looking super sexy up to here. This is networking specific.
We want something generic for *anything* that requests firmware.
You can't be serious. It's network specific because of how the Kconfig
is named?
Kconfig? What has that to do with anything? The issue I have is that the
solution I am looking for is for it to be agnostic to the subsystem. I
have found similar firmware crashes on gpu, media, scsci.
Working for a company operating large data centers I would strongly
prefer if we didn't have ten different ways of reporting firmware
problems in the fleet.
Indeed.
quoted
I'm afraid this won't work for something generic. I don't think its
throw-away work though, the idea to provide a generic interface to
dump firmware through netlink might be nice for networking, or other
things.
But I have a feeling we'll want something still more generic than this.
Please be specific. Saying generic a lot is not helpful. The code (as
you can see in this patch) is in no way network specific. Or are you
saying there are machines out there running without netlink sockets?
No, I am saying I want something to work with any struct device.
quoted
So networking may want to be aware that a firmware crash happened as
part of this network device health thing, but firmware crashing is a
generic thing.
I have now extended my patch set to include uvents and I am more set on
that we need the taint now more than ever.
Please expect my nack if you're trying to add this to networking
drivers.
The uevent mechanism is not for networking.
The taint however is, and I'd like to undertand how it is you do not see
that an undesirable requirement for a reboot is a clear case for a taint.
The irony is you have a problem with a networking device and all the
devices your initial set touched are networking. Two of the drivers
you touched either have or will soon have devlink health reporters
implemented.
That is all great, and I don't think its a bad idea to add
infrastructure / extend it to get more information about a firmware
crash dump. However, suggesting that devlink is the only solution we
need in the kernel without considering other subsystems is what I am
suggesting doesn't suit my needs. Networking was just the first
subsystem I am taclking now but I have patches where similar situations
happen across the kernel.
Luis
From: Luis Chamberlain <mcgrof@kernel.org> Date: 2020-05-22 21:51:50
On Fri, May 22, 2020 at 10:46:07PM +0200, Johannes Berg wrote:
FWIW, I still completely disagree on that taint. You (Luis) obviously
have been running into a bug in that driver, I doubt the firmware
actually managed to wedge the hardware.
This hasn't happened just once, its happed many times sporadically now,
once a week or two weeks I'd say. And the system isn't being moved
around.
But even if it did, that's still not really a kernel taint. The kernel
itself isn't in any way affected by this.
Of course it is, a full reboot is required.
Yes, the system is in a weird state now. But that's *not* equivalent to
"kernel tainted".
Requiring a full reboot is a dire situation to be in, and loosing
connectivity to the point this is not recoverable likewise.
You guys are making out a taint to be the end of the world. We have a
taint even for a kernel warning, and as others have mentioned mac80211
already produces these.
What exactly is the opposition to a taint to clarify that a device
firmware has crashed and your system requires a full reboot?
Luis
From: Steve deRosier <hidden> Date: 2020-05-22 23:24:35
On Fri, May 22, 2020 at 2:51 PM Luis Chamberlain [off-list ref] wrote:
On Fri, May 22, 2020 at 10:46:07PM +0200, Johannes Berg wrote:
quoted
FWIW, I still completely disagree on that taint. You (Luis) obviously
have been running into a bug in that driver, I doubt the firmware
actually managed to wedge the hardware.
This hasn't happened just once, its happed many times sporadically now,
once a week or two weeks I'd say. And the system isn't being moved
around.
quoted
But even if it did, that's still not really a kernel taint. The kernel
itself isn't in any way affected by this.
Of course it is, a full reboot is required.
quoted
Yes, the system is in a weird state now. But that's *not* equivalent to
"kernel tainted".
Requiring a full reboot is a dire situation to be in, and loosing
connectivity to the point this is not recoverable likewise.
You guys are making out a taint to be the end of the world. We have a
taint even for a kernel warning, and as others have mentioned mac80211
already produces these.
I had to go RTFM re: kernel taints because it has been a very long
time since I looked at them. It had always seemed to me that most were
caused by "kernel-unfriendly" user actions. The most famous of course
is loading proprietary modules, out-of-tree modules, forced module
loads, etc... Honestly, I had forgotten the large variety of uses of
the taint flags. For anyone who hasn't looked at taints recently, I
recommend: https://www.kernel.org/doc/html/latest/admin-guide/tainted-kernels.html
In light of this I don't object to setting a taint on this anymore.
I'm a little uneasy, but I've softened on it now, and now I feel it
depends on implementation.
Specifically, I don't think we should set a taint flag when a driver
easily handles a routine firmware crash and is confident that things
have come up just fine again. In other words, triggering the taint in
every driver module where it spits out a log comment that it had a
firmware crash and had to recover seems too much. Sure, firmware
shouldn't crash, sure it should be open source so we can fix it,
whatever... those sort of wishful comments simply ignore reality and
our ability to affect effective change. A lot of WiFi firmware crashes
and for well-known cases the drivers handle them well. And in some
cases, not so well and that should be a place the driver should detect
and thus raise a red flag. If a WiFi firmware crash can bring down
the kernel, there's either a major driver bug or some very funky
hardware crap going on. That sort of thing we should be able to
detect, mark with a taint (or something), and fix if within our sphere
of influence. I guess what it comes down to me is how aggressive we
are about setting the flag.
I would like there to be a single solution, or a minimized set
depending on what makes sense for the requirements. I haven't had time
to look into the alternatives mentioned here so I don't have an
informed opinion about the solution. I do think Luis is trying to
solve a real problem though. Can we look at this from the point of
view of what are the requirements? What is it we're trying to solve?
I _think_ that the goal of Luis's original proposal is to report up to
the user, at some future point when the user is interested (because
something super drastic just occured, but long after the fw crash),
that there was a firmware crash without the user having to grep
through all logs on the machine. And then if the user sees that flag
and suspects it, then they can bother to find it in the logs or do
more drastic debugging steps like finding the fw crash in the log and
pulling firmware crash dumps, etc.
I think the various alternate solutions are great but perhaps solving
a superset of features (like adding in user-space notifications etc)?
Perhaps different people on these related threads are trying to solve
different problems?
- Steve
From: Luis Chamberlain <mcgrof@kernel.org> Date: 2020-05-22 23:44:14
On Fri, May 22, 2020 at 04:23:55PM -0700, Steve deRosier wrote:
Specifically, I don't think we should set a taint flag when a driver
easily handles a routine firmware crash and is confident that things
have come up just fine again. In other words, triggering the taint in
every driver module where it spits out a log comment that it had a
firmware crash and had to recover seems too much. Sure, firmware
shouldn't crash, sure it should be open source so we can fix it,
whatever... those sort of wishful comments simply ignore reality and
our ability to affect effective change. A lot of WiFi firmware crashes
and for well-known cases the drivers handle them well. And in some
cases, not so well and that should be a place the driver should detect
and thus raise a red flag. If a WiFi firmware crash can bring down
the kernel, there's either a major driver bug or some very funky
hardware crap going on. That sort of thing we should be able to
detect, mark with a taint (or something), and fix if within our sphere
of influence. I guess what it comes down to me is how aggressive we
are about setting the flag.
Exactly the crux of the issue.
I hope that by now we should all be in agreement that at least a
firmware crash requiring a reboot is something we should record and
inform the user of. A taint seems like a reasonable standard practice
for these sorts of things.
I would like there to be a single solution, or a minimized set
depending on what makes sense for the requirements. I haven't had time
to look into the alternatives mentioned here so I don't have an
informed opinion about the solution. I do think Luis is trying to
solve a real problem though. Can we look at this from the point of
view of what are the requirements? What is it we're trying to solve?
I _think_ that the goal of Luis's original proposal is to report up to
the user, at some future point when the user is interested (because
something super drastic just occured, but long after the fw crash),
that there was a firmware crash without the user having to grep
through all logs on the machine. And then if the user sees that flag
and suspects it, then they can bother to find it in the logs or do
more drastic debugging steps like finding the fw crash in the log and
pulling firmware crash dumps, etc.
Yes, that's exactly it. Not all users are clueful to inspect logs. I now
have a generic uevent mechanism drafted which sends a uevent for *any*
taint. So that is, it does not even depend on this series. But it
accomplishes the goal of informing the user of taints.
I think the various alternate solutions are great but perhaps solving
a superset of features (like adding in user-space notifications etc)?
Perhaps different people on these related threads are trying to solve
different problems?
The uevent mechanism I implemented (but not yet posted for review) at
least sends out a smoke signal. I think that if each subsystem wants
to expand on this with dumping facilities that is great too!
Luis
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2020-05-25 09:07:57
On Fri, May 22, 2020 at 04:23:55PM -0700, Steve deRosier wrote:
On Fri, May 22, 2020 at 2:51 PM Luis Chamberlain [off-list ref] wrote:
I had to go RTFM re: kernel taints because it has been a very long
time since I looked at them. It had always seemed to me that most were
caused by "kernel-unfriendly" user actions. The most famous of course
is loading proprietary modules, out-of-tree modules, forced module
loads, etc... Honestly, I had forgotten the large variety of uses of
the taint flags. For anyone who hasn't looked at taints recently, I
recommend: https://www.kernel.org/doc/html/latest/admin-guide/tainted-kernels.html
In light of this I don't object to setting a taint on this anymore.
I'm a little uneasy, but I've softened on it now, and now I feel it
depends on implementation.
Specifically, I don't think we should set a taint flag when a driver
easily handles a routine firmware crash and is confident that things
have come up just fine again. In other words, triggering the taint in
every driver module where it spits out a log comment that it had a
firmware crash and had to recover seems too much. Sure, firmware
shouldn't crash, sure it should be open source so we can fix it,
whatever...
While it may sound idealistic the firmware for the end-user, and even for mere
kernel developer like me, is a complete blackbox which has more access than
root user in the kernel. We have tons of firmwares and each of them potentially
dangerous beast. As a user I really care about my data and privacy (hacker can
oops a firmware in order to set a specific vector attack). So, tainting kernel
is _a least_ we can do there, the strict rules would be to reboot immediately.
those sort of wishful comments simply ignore reality and
our ability to affect effective change.
We can encourage users not to buy cheap crap for the starter.
A lot of WiFi firmware crashes
and for well-known cases the drivers handle them well. And in some
cases, not so well and that should be a place the driver should detect
and thus raise a red flag. If a WiFi firmware crash can bring down
the kernel, there's either a major driver bug or some very funky
hardware crap going on. That sort of thing we should be able to
detect, mark with a taint (or something), and fix if within our sphere
of influence. I guess what it comes down to me is how aggressive we
are about setting the flag.
From: Ben Greear <hidden> Date: 2020-05-25 17:09:15
On 05/25/2020 02:07 AM, Andy Shevchenko wrote:
On Fri, May 22, 2020 at 04:23:55PM -0700, Steve deRosier wrote:
quoted
On Fri, May 22, 2020 at 2:51 PM Luis Chamberlain [off-list ref] wrote:
quoted
I had to go RTFM re: kernel taints because it has been a very long
time since I looked at them. It had always seemed to me that most were
caused by "kernel-unfriendly" user actions. The most famous of course
is loading proprietary modules, out-of-tree modules, forced module
loads, etc... Honestly, I had forgotten the large variety of uses of
the taint flags. For anyone who hasn't looked at taints recently, I
recommend: https://www.kernel.org/doc/html/latest/admin-guide/tainted-kernels.html
In light of this I don't object to setting a taint on this anymore.
I'm a little uneasy, but I've softened on it now, and now I feel it
depends on implementation.
Specifically, I don't think we should set a taint flag when a driver
easily handles a routine firmware crash and is confident that things
have come up just fine again. In other words, triggering the taint in
every driver module where it spits out a log comment that it had a
firmware crash and had to recover seems too much. Sure, firmware
shouldn't crash, sure it should be open source so we can fix it,
whatever...
While it may sound idealistic the firmware for the end-user, and even for mere
kernel developer like me, is a complete blackbox which has more access than
root user in the kernel. We have tons of firmwares and each of them potentially
dangerous beast. As a user I really care about my data and privacy (hacker can
oops a firmware in order to set a specific vector attack). So, tainting kernel
is _a least_ we can do there, the strict rules would be to reboot immediately.
quoted
those sort of wishful comments simply ignore reality and
our ability to affect effective change.
We can encourage users not to buy cheap crap for the starter.
There is no stable wifi firmware for any price.
There is also no obvious feedback from even name-brand NICs like ath10k or AX200
when you report a crash.
That said, at least in my experience with ath10k-ct, the OS normally recovers fine
from firmware crashes. ath10k already reports full crash reports on udev, so
easy for user-space to notice and report bug reports upstream if it cares to. Probably
other NICs do the same, and if not, they certainly could.
Thanks,
Ben
--
Ben Greear [off-list ref]
Candela Technologies Inc http://www.candelatech.com
From: Jakub Kicinski <kuba@kernel.org> Date: 2020-05-25 20:57:50
On Fri, 22 May 2020 22:46:07 +0200 Johannes Berg wrote:
quoted
The irony is you have a problem with a networking device and all the
devices your initial set touched are networking. Two of the drivers
you touched either have or will soon have devlink health reporters
implemented.
Like I said above, do you think it'd be feasible to make a devcoredump
out of devlink health reports? And can the report be in a way that we
control the file format, or are there limits? I guess I should read the
code to find out, but I figure you probably just know. But feel free to
tell me to read it :)
The reason I'm asking is that it's starting to sound like we really
ought to be implementing devlink, but we've got a bunch of
infrastructure that uses the devcoredump, and it'll take time
(significantly so) to change all that...
In devlink world pure FW core dumps are exposed by devlink regions.
An API allowing reading device memory, registers, etc., but also
creating dumps of memory regions when things go wrong. It should be
a fairly straightforward migration.
Devlink health is more targeted, the dump is supposed to contain only
relevant information, selected and formatted by the driver. When device
misbehaves driver reads the relevant registers and FW state and creates
a formatted state dump. I believe each element of the dump must fit
into a netlink message (but there may be multiple elements, see
devlink_fmsg_prepare_skb()).
We should be able to convert dl-regions dumps and dl-health dumps into
devcoredumps, but since health reporters are supposed to be more
targeted there's usually multiple of them per device.
Conversely devcoredumps can be trivially exposed as dl-region dumps,
but I believe dl-health would require driver changes to format the
information appropriately.
From: Johannes Berg <johannes@sipsolutions.net> Date: 2020-07-30 13:57:49
Hi,
Sorry ... long delay.
quoted
The reason I'm asking is that it's starting to sound like we really
ought to be implementing devlink, but we've got a bunch of
infrastructure that uses the devcoredump, and it'll take time
(significantly so) to change all that...
In devlink world pure FW core dumps are exposed by devlink regions.
An API allowing reading device memory, registers, etc., but also
creating dumps of memory regions when things go wrong. It should be
a fairly straightforward migration.
Right. We also have regions (various memory pieces, registers, ...).
One issue might be that for devlink we wouldn't want to expose these as
a single blob, I guess, but for devcoredump we already have a custom
format to glue all the things together. Since it seems unlikely that
anyone else would want to use the *iwlwifi* format to glue things
together, we'd have to do something there.
But perhaps that could be a matter of providing a "glue things into a
devcoredump" function that would have a reasonable default but could be
overridden by the driver for those migration cases.
Devlink health is more targeted, the dump is supposed to contain only
relevant information, selected and formatted by the driver. When device
misbehaves driver reads the relevant registers and FW state and creates
a formatted state dump. I believe each element of the dump must fit
into a netlink message (but there may be multiple elements, see
devlink_fmsg_prepare_skb()).
That wouldn't help for our big memory dumps, but OK.
We should be able to convert dl-regions dumps and dl-health dumps into
devcoredumps, but since health reporters are supposed to be more
targeted there's usually multiple of them per device.
Right.
Conversely devcoredumps can be trivially exposed as dl-region dumps,
but I believe dl-health would require driver changes to format the
information appropriately.
Agree.
Anyway, thanks. I'll put it on my list of things to look at ... not too
hopeful that will be soon, given how long it even took me to get back to
this email :)
johannes