On 2/26/2018 11:06 PM, Brian Norris wrote:
Hi,
On Fri, Feb 23, 2018 at 2:51 AM, Johannes Berg
<johannes@xxxxxxxxxxxxxxxx> wrote:
On Fri, 2018-02-23 at 11:39 +0100, Arend van Spriel wrote:
Well, that depends on the eye of the beholder I guess. From user-space
perspective it is asynchronous regardless. A write access to the coredump
sysfs file eventually results in a uevent when the devcoredump entry is
created, ie. after driver has made a dev_coredump API call. Whether the
driver does that synchronously or asynchronously is irrelevant as far as
user-space is concerned.
Is it really? The driver infrastructure seems to guarantee that the
entirety of a driver's ->coredump() will complete before returning from
the write. So it might be reasonable for some user to assume (based on
implementation details, e.g., of brcmfmac) that the devcoredump will be
ready by the time the write() syscall returns, absent documentation that
says otherwise. But then, that's not how mwifiex works right now, so
they might be surprised if they switch drivers.
I can see how you might want to have that kind of behaviour, but you'd
have to jump through some hoops to see if the coredump you saw is
actually the right one - you probably want an asynchronous coredump
"collector" and then wait for it to show up (with some reasonable
timeout) on the actual filesystem, not on sysfs?
Otherwise you have to trawl sysfs for the right coredump I guess, which
too is possible.
It's not that I want that interface. It's that I want the *lack* of
such an interface to be guaranteed in the documentation. When the
questions like "where? when?" are not answered in the doc, users are
totally allowed to speculate ;) Perhaps the "where" can be deferred to
other documentation (which should probably exist someday), but the
"when" should be listed as "eventually; or not at all; listen for a
uevent."
Agree. Will extend/improve the ABI documentation.
You are right. Clearly I did not reach the end my learning curve here. I
assumed referring to the existing dev_coredump facility was sufficient, but
maybe it is worth a patch to be more explicit and mention the uevent
behavior. Also dev_coredump facility may be disabled upon which the trigger
will have no effect in sysfs. In the kernel the data passed by the driver is
simply freed by dev_coredump facility.
Is there any other documentation for the coredump feature? I don't
really see much.
Any other than the code itself you mean? I am not sure. Maybe Johannes
knows.
There isn't really, it originally was really simple, but then somebody
(Kees perhaps?) requested a way to turn it off forever for security or
privacy concerns and it became more complicated.
Then I don't think when adding a new sysfs ABI, we should be deferring
to "existing dev_coredump facility [documentation]" (which doesn't
exist). And just a few words about the user-facing interface would be
nice for the documentation. There previously wasn't any official way
to trigger a dump from userspace -- only from random debugfs files, I
think, or from unspecified device failures.
That was my main motivation to have this. The debugfs method did not
feel quite right as there is no kconfig dependency between dev_coredump
and debugfs. Now I discussed with Johannes about adding code into the
dev_coredump facility, but that seemed to add a lot of complexity. So I
looking into the device driver core and found it to be the simpler solution.
static ssize_t coredump_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
device_lock(dev);
if (dev->driver->coredump)
dev->driver->coredump(dev);
device_unlock(dev);
return count;
}
static DEVICE_ATTR_WO(coredump);
Is that a bug or a feature?
Yeah. Let's call it a bug. Just not sure what to go for. Return the
error or change coredump callback to void return type.
I'm not sure it matters all that much - the underlying devcoredump
calls all have no return value (void), and given the above complexities
with the ability to turn off devcoredumping entirely you cannot rely on
this return value to tell you if a dump was created or not, at least
not without much more infrastructure work.
Then perhaps it makes sense to remove the return code before you
create users of it.
Yup. Will sent out a patch for that as well.
Thanks,
Arend
