| Message ID | 1519210220-22437-3-git-send-email-arend.vanspriel@broadcom.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
On Wed, Feb 21, 2018 at 11:50:19AM +0100, Arend van Spriel wrote: > Since commit 3c47d19ff4dc ("drivers: base: add coredump driver ops") > it is possible to initiate a device coredump from user-space. This > patch adds support for it adding the .coredump() driver callback. > As there is no longer a need to initiate it through debugfs remove > that code. > > Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com> > --- > drivers/net/wireless/marvell/mwifiex/debugfs.c | 31 +------------------------- > drivers/net/wireless/marvell/mwifiex/pcie.c | 19 ++++++++++++++-- > drivers/net/wireless/marvell/mwifiex/sdio.c | 13 +++++++++++ > drivers/net/wireless/marvell/mwifiex/usb.c | 14 ++++++++++++ > 4 files changed, 45 insertions(+), 32 deletions(-) The documentation doesn't really say [1], but is the coredump supposed to happen synchronously? Because the mwifiex implementation is asynchronous, whereas it looks like the brcmfmac one is synchronous. Brian [1] In fact, the ABI documentation really just describes kernel internals, rather than documenting any user-facing details, from what I can tell.
On 2/21/2018 11:59 PM, Brian Norris wrote: > On Wed, Feb 21, 2018 at 11:50:19AM +0100, Arend van Spriel wrote: >> Since commit 3c47d19ff4dc ("drivers: base: add coredump driver ops") >> it is possible to initiate a device coredump from user-space. This >> patch adds support for it adding the .coredump() driver callback. >> As there is no longer a need to initiate it through debugfs remove >> that code. >> >> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com> >> --- >> drivers/net/wireless/marvell/mwifiex/debugfs.c | 31 +------------------------- >> drivers/net/wireless/marvell/mwifiex/pcie.c | 19 ++++++++++++++-- >> drivers/net/wireless/marvell/mwifiex/sdio.c | 13 +++++++++++ >> drivers/net/wireless/marvell/mwifiex/usb.c | 14 ++++++++++++ >> 4 files changed, 45 insertions(+), 32 deletions(-) > > The documentation doesn't really say [1], but is the coredump supposed > to happen synchronously? Because the mwifiex implementation is > asynchronous, whereas it looks like the brcmfmac one is synchronous. 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. > Brian > > [1] In fact, the ABI documentation really just describes kernel > internals, rather than documenting any user-facing details, from what I > can tell. 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. Regards, Arend
Hi Arend, On Thu, Feb 22, 2018 at 01:17:56PM +0100, Arend van Spriel wrote: > On 2/21/2018 11:59 PM, Brian Norris wrote: > > On Wed, Feb 21, 2018 at 11:50:19AM +0100, Arend van Spriel wrote: > > > Since commit 3c47d19ff4dc ("drivers: base: add coredump driver ops") > > > it is possible to initiate a device coredump from user-space. This > > > patch adds support for it adding the .coredump() driver callback. > > > As there is no longer a need to initiate it through debugfs remove > > > that code. > > > > > > Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com> > > > --- > > > drivers/net/wireless/marvell/mwifiex/debugfs.c | 31 +------------------------- > > > drivers/net/wireless/marvell/mwifiex/pcie.c | 19 ++++++++++++++-- > > > drivers/net/wireless/marvell/mwifiex/sdio.c | 13 +++++++++++ > > > drivers/net/wireless/marvell/mwifiex/usb.c | 14 ++++++++++++ > > > 4 files changed, 45 insertions(+), 32 deletions(-) > > > > The documentation doesn't really say [1], but is the coredump supposed > > to happen synchronously? Because the mwifiex implementation is > > asynchronous, whereas it looks like the brcmfmac one is synchronous. > > 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. Anyway, *I'm* already personally used to these dumps being asynchronous, and writing tooling to listen for the uevent instead. But that doesn't mean everyone will be. Also, due to the differences in async/sync, mwifiex doesn't really provide you much chance for error handling, because most errors would be asynchronous. So brcmfmac's "coredump" has more chance for user programs to error-check than mwifiex's (due to the asynchronous nature) [1]. BTW, I push on this mostly because this is migrating from a debugfs feature (that is easy to hand-wave off as not really providing a consistent/stable API, etc., etc.) to a documented sysfs feature. If it were left to rot in debugfs, I probably wouldn't be as bothered ;) > > Brian > > > > [1] In fact, the ABI documentation really just describes kernel > > internals, rather than documenting any user-facing details, from what I > > can tell. > > 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. Brian [1] Oh wait, but I see that while ->coredump() has an integer return code...the caller ignores it: 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?
+ Johannes (for devcoredump documentation question). On 2/22/2018 8:35 PM, Brian Norris wrote: > Hi Arend, > > On Thu, Feb 22, 2018 at 01:17:56PM +0100, Arend van Spriel wrote: >> On 2/21/2018 11:59 PM, Brian Norris wrote: >>> On Wed, Feb 21, 2018 at 11:50:19AM +0100, Arend van Spriel wrote: >>>> Since commit 3c47d19ff4dc ("drivers: base: add coredump driver ops") >>>> it is possible to initiate a device coredump from user-space. This >>>> patch adds support for it adding the .coredump() driver callback. >>>> As there is no longer a need to initiate it through debugfs remove >>>> that code. >>>> >>>> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com> >>>> --- >>>> drivers/net/wireless/marvell/mwifiex/debugfs.c | 31 +------------------------- >>>> drivers/net/wireless/marvell/mwifiex/pcie.c | 19 ++++++++++++++-- >>>> drivers/net/wireless/marvell/mwifiex/sdio.c | 13 +++++++++++ >>>> drivers/net/wireless/marvell/mwifiex/usb.c | 14 ++++++++++++ >>>> 4 files changed, 45 insertions(+), 32 deletions(-) >>> >>> The documentation doesn't really say [1], but is the coredump supposed >>> to happen synchronously? Because the mwifiex implementation is >>> asynchronous, whereas it looks like the brcmfmac one is synchronous. >> >> 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. Ok. I already agreed that the uevent behavior should be documented. The error handling you are bringing up below was something I realized as well. I am not familiar with mwifiex to determine what it can say about the coredump succeeding before scheduling the work. > Anyway, *I'm* already personally used to these dumps being asynchronous, > and writing tooling to listen for the uevent instead. But that doesn't > mean everyone will be. > > Also, due to the differences in async/sync, mwifiex doesn't really > provide you much chance for error handling, because most errors would be > asynchronous. So brcmfmac's "coredump" has more chance for user programs > to error-check than mwifiex's (due to the asynchronous nature) [1]. > > BTW, I push on this mostly because this is migrating from a debugfs > feature (that is easy to hand-wave off as not really providing a > consistent/stable API, etc., etc.) to a documented sysfs feature. If it > were left to rot in debugfs, I probably wouldn't be as bothered ;) I appreciate it. The documentation is not in the stable ABI folder yet so I welcome any and all improvements. Might learn a thing or two from it. >>> Brian >>> >>> [1] In fact, the ABI documentation really just describes kernel >>> internals, rather than documenting any user-facing details, from what I >>> can tell. >> >> 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. > Brian > > [1] Oh wait, but I see that while ->coredump() has an integer return > code...the caller ignores it: > > 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. Regards, Arend
Hey, Hadn't really followed this discussion much due to other fires elsewhere :-) 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. > > > 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. > > 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. johannes
Hi, On Fri, Feb 23, 2018 at 2:51 AM, Johannes Berg <johannes@sipsolutions.net> 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." > > > > 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. > > > 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. Brian
On 2/26/2018 11:06 PM, Brian Norris wrote: > Hi, > > On Fri, Feb 23, 2018 at 2:51 AM, Johannes Berg > <johannes@sipsolutions.net> 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
Arend Van Spriel <arend.vanspriel@broadcom.com> wrote: > Since commit 3c47d19ff4dc ("drivers: base: add coredump driver ops") > it is possible to initiate a device coredump from user-space. This > patch adds support for it adding the .coredump() driver callback. > As there is no longer a need to initiate it through debugfs remove > that code. > > Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com> Based on the discussion I assume this is ok to take to w-d-next. If that's not the case, please let me know ASAP.
On 3/12/2018 10:41 AM, Kalle Valo wrote: > Arend Van Spriel <arend.vanspriel@broadcom.com> wrote: > >> Since commit 3c47d19ff4dc ("drivers: base: add coredump driver ops") >> it is possible to initiate a device coredump from user-space. This >> patch adds support for it adding the .coredump() driver callback. >> As there is no longer a need to initiate it through debugfs remove >> that code. >> >> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com> > > Based on the discussion I assume this is ok to take to w-d-next. If that's not > the case, please let me know ASAP. It is up to the mwifiex maintainers to decide, I guess. The ABI documentation need to be revised and change the callback to void return type. I am not sure what the best approach is. 1) apply this and fix return type later, or 2) fix return type and resubmit this. What is your opinion? Regards, Arend
Arend van Spriel <arend.vanspriel@broadcom.com> writes: > On 3/12/2018 10:41 AM, Kalle Valo wrote: >> Arend Van Spriel <arend.vanspriel@broadcom.com> wrote: >> >>> Since commit 3c47d19ff4dc ("drivers: base: add coredump driver ops") >>> it is possible to initiate a device coredump from user-space. This >>> patch adds support for it adding the .coredump() driver callback. >>> As there is no longer a need to initiate it through debugfs remove >>> that code. >>> >>> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com> >> >> Based on the discussion I assume this is ok to take to w-d-next. If that's not >> the case, please let me know ASAP. > > It is up to the mwifiex maintainers to decide, I guess. The ABI > documentation need to be revised and change the callback to void > return type. I am not sure what the best approach is. 1) apply this > and fix return type later, or 2) fix return type and resubmit this. > What is your opinion? I guess the callback change will go through Greg's tree? Then I suspect it's easier that you submit the callback change to Greg first and wait for it to trickle down to wireless-drivers-next (after the next merge window) and then I can apply the driver patches. Otherwise there might be a conflict between my and Greg's tree.
On 3/13/2018 2:10 PM, Kalle Valo wrote: > Arend van Spriel <arend.vanspriel@broadcom.com> writes: > >> On 3/12/2018 10:41 AM, Kalle Valo wrote: >>> Arend Van Spriel <arend.vanspriel@broadcom.com> wrote: >>> >>>> Since commit 3c47d19ff4dc ("drivers: base: add coredump driver ops") >>>> it is possible to initiate a device coredump from user-space. This >>>> patch adds support for it adding the .coredump() driver callback. >>>> As there is no longer a need to initiate it through debugfs remove >>>> that code. >>>> >>>> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com> >>> >>> Based on the discussion I assume this is ok to take to w-d-next. If that's not >>> the case, please let me know ASAP. >> >> It is up to the mwifiex maintainers to decide, I guess. The ABI >> documentation need to be revised and change the callback to void >> return type. I am not sure what the best approach is. 1) apply this >> and fix return type later, or 2) fix return type and resubmit this. >> What is your opinion? > > I guess the callback change will go through Greg's tree? Then I suspect > it's easier that you submit the callback change to Greg first and wait > for it to trickle down to wireless-drivers-next (after the next merge > window) and then I can apply the driver patches. Otherwise there might > be a conflict between my and Greg's tree. That was my assessment, but unfortunately Marcel already applied the btmrvl patch before I could reply. So how do I move from here? Option 1) revert brmrvl and fix callback return type, or 2) apply mwifiex patch and fix callback return type later for both drivers. Regards, Arend
Hi Arend, >>>>> Since commit 3c47d19ff4dc ("drivers: base: add coredump driver ops") >>>>> it is possible to initiate a device coredump from user-space. This >>>>> patch adds support for it adding the .coredump() driver callback. >>>>> As there is no longer a need to initiate it through debugfs remove >>>>> that code. >>>>> >>>>> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com> >>>> >>>> Based on the discussion I assume this is ok to take to w-d-next. If that's not >>>> the case, please let me know ASAP. >>> >>> It is up to the mwifiex maintainers to decide, I guess. The ABI >>> documentation need to be revised and change the callback to void >>> return type. I am not sure what the best approach is. 1) apply this >>> and fix return type later, or 2) fix return type and resubmit this. >>> What is your opinion? >> >> I guess the callback change will go through Greg's tree? Then I suspect >> it's easier that you submit the callback change to Greg first and wait >> for it to trickle down to wireless-drivers-next (after the next merge >> window) and then I can apply the driver patches. Otherwise there might >> be a conflict between my and Greg's tree. > > That was my assessment, but unfortunately Marcel already applied the btmrvl patch before I could reply. So how do I move from here? Option 1) revert brmrvl and fix callback return type, or 2) apply mwifiex patch and fix callback return type later for both drivers. I can take the patch back out of bluetooth-next if needed. It is your call. Regards Marcel
On 3/13/2018 9:19 PM, Marcel Holtmann wrote: > Hi Arend, > >>>>>> Since commit 3c47d19ff4dc ("drivers: base: add coredump driver ops") >>>>>> it is possible to initiate a device coredump from user-space. This >>>>>> patch adds support for it adding the .coredump() driver callback. >>>>>> As there is no longer a need to initiate it through debugfs remove >>>>>> that code. >>>>>> >>>>>> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com> >>>>> >>>>> Based on the discussion I assume this is ok to take to w-d-next. If that's not >>>>> the case, please let me know ASAP. >>>> >>>> It is up to the mwifiex maintainers to decide, I guess. The ABI >>>> documentation need to be revised and change the callback to void >>>> return type. I am not sure what the best approach is. 1) apply this >>>> and fix return type later, or 2) fix return type and resubmit this. >>>> What is your opinion? >>> >>> I guess the callback change will go through Greg's tree? Then I suspect >>> it's easier that you submit the callback change to Greg first and wait >>> for it to trickle down to wireless-drivers-next (after the next merge >>> window) and then I can apply the driver patches. Otherwise there might >>> be a conflict between my and Greg's tree. >> >> That was my assessment, but unfortunately Marcel already applied the btmrvl patch before I could reply. So how do I move from here? Option 1) revert brmrvl and fix callback return type, or 2) apply mwifiex patch and fix callback return type later for both drivers. > > I can take the patch back out of bluetooth-next if needed. It is your call. Thanks, Marcel Let's go for that. Please revert/remove the patch. Regards, Arend
diff --git a/drivers/net/wireless/marvell/mwifiex/debugfs.c b/drivers/net/wireless/marvell/mwifiex/debugfs.c index db2872d..0745393 100644 --- a/drivers/net/wireless/marvell/mwifiex/debugfs.c +++ b/drivers/net/wireless/marvell/mwifiex/debugfs.c @@ -154,34 +154,6 @@ } /* - * Proc device dump read handler. - * - * This function is called when the 'device_dump' file is opened for - * reading. - * This function dumps driver information and firmware memory segments - * (ex. DTCM, ITCM, SQRAM etc.) for - * debugging. - */ -static ssize_t -mwifiex_device_dump_read(struct file *file, char __user *ubuf, - size_t count, loff_t *ppos) -{ - struct mwifiex_private *priv = file->private_data; - - /* For command timeouts, USB firmware will automatically emit - * firmware dump events, so we don't implement device_dump(). - * For user-initiated dumps, we trigger it ourselves. - */ - if (priv->adapter->iface_type == MWIFIEX_USB) - mwifiex_send_cmd(priv, HostCmd_CMD_FW_DUMP_EVENT, - HostCmd_ACT_GEN_SET, 0, NULL, true); - else - priv->adapter->if_ops.device_dump(priv->adapter); - - return 0; -} - -/* * Proc getlog file read handler. * * This function is called when the 'getlog' file is opened for reading @@ -980,7 +952,6 @@ MWIFIEX_DFS_FILE_READ_OPS(info); MWIFIEX_DFS_FILE_READ_OPS(debug); MWIFIEX_DFS_FILE_READ_OPS(getlog); -MWIFIEX_DFS_FILE_READ_OPS(device_dump); MWIFIEX_DFS_FILE_OPS(regrdwr); MWIFIEX_DFS_FILE_OPS(rdeeprom); MWIFIEX_DFS_FILE_OPS(memrw); @@ -1011,7 +982,7 @@ MWIFIEX_DFS_ADD_FILE(getlog); MWIFIEX_DFS_ADD_FILE(regrdwr); MWIFIEX_DFS_ADD_FILE(rdeeprom); - MWIFIEX_DFS_ADD_FILE(device_dump); + MWIFIEX_DFS_ADD_FILE(memrw); MWIFIEX_DFS_ADD_FILE(hscfg); MWIFIEX_DFS_ADD_FILE(histogram); diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c index 97a6199..0959526 100644 --- a/drivers/net/wireless/marvell/mwifiex/pcie.c +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c @@ -320,6 +320,20 @@ static void mwifiex_pcie_shutdown(struct pci_dev *pdev) return; } +static int mwifiex_pcie_coredump(struct device *dev) +{ + struct pci_dev *pdev; + struct pcie_service_card *card; + + pdev = container_of(dev, struct pci_dev, dev); + card = pci_get_drvdata(pdev); + + if (!test_and_set_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, + &card->work_flags)) + schedule_work(&card->work); + return 0; +} + static const struct pci_device_id mwifiex_ids[] = { { PCIE_VENDOR_ID_MARVELL, PCIE_DEVICE_ID_MARVELL_88W8766P, @@ -415,11 +429,12 @@ static SIMPLE_DEV_PM_OPS(mwifiex_pcie_pm_ops, mwifiex_pcie_suspend, .id_table = mwifiex_ids, .probe = mwifiex_pcie_probe, .remove = mwifiex_pcie_remove, -#ifdef CONFIG_PM_SLEEP .driver = { + .coredump = mwifiex_pcie_coredump, +#ifdef CONFIG_PM_SLEEP .pm = &mwifiex_pcie_pm_ops, - }, #endif + }, .shutdown = mwifiex_pcie_shutdown, .err_handler = &mwifiex_pcie_err_handler, }; diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c index a828801..24b9ff3 100644 --- a/drivers/net/wireless/marvell/mwifiex/sdio.c +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c @@ -466,6 +466,18 @@ static int mwifiex_sdio_suspend(struct device *dev) return ret; } +static int mwifiex_sdio_coredump(struct device *dev) +{ + struct sdio_func *func = dev_to_sdio_func(dev); + struct sdio_mmc_card *card; + + card = sdio_get_drvdata(func); + if (!test_and_set_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, + &card->work_flags)) + schedule_work(&card->work); + return 0; +} + /* Device ID for SD8786 */ #define SDIO_DEVICE_ID_MARVELL_8786 (0x9116) /* Device ID for SD8787 */ @@ -515,6 +527,7 @@ static int mwifiex_sdio_suspend(struct device *dev) .remove = mwifiex_sdio_remove, .drv = { .owner = THIS_MODULE, + .coredump = mwifiex_sdio_coredump, .pm = &mwifiex_sdio_pm_ops, } }; diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c index 4bc2448..83815f6 100644 --- a/drivers/net/wireless/marvell/mwifiex/usb.c +++ b/drivers/net/wireless/marvell/mwifiex/usb.c @@ -653,6 +653,17 @@ static void mwifiex_usb_disconnect(struct usb_interface *intf) usb_put_dev(interface_to_usbdev(intf)); } +static int mwifiex_usb_coredump(struct device *dev) +{ + struct usb_interface *intf = to_usb_interface(dev); + struct usb_card_rec *card = usb_get_intfdata(intf); + + mwifiex_send_cmd(mwifiex_get_priv(card->adapter, MWIFIEX_BSS_ROLE_ANY), + HostCmd_CMD_FW_DUMP_EVENT, HostCmd_ACT_GEN_SET, 0, + NULL, true); + return 0; +} + static struct usb_driver mwifiex_usb_driver = { .name = "mwifiex_usb", .probe = mwifiex_usb_probe, @@ -661,6 +672,9 @@ static void mwifiex_usb_disconnect(struct usb_interface *intf) .suspend = mwifiex_usb_suspend, .resume = mwifiex_usb_resume, .soft_unbind = 1, + .drvwrap.driver = { + .coredump = mwifiex_usb_coredump, + }, }; static int mwifiex_write_data_sync(struct mwifiex_adapter *adapter, u8 *pbuf,
Since commit 3c47d19ff4dc ("drivers: base: add coredump driver ops") it is possible to initiate a device coredump from user-space. This patch adds support for it adding the .coredump() driver callback. As there is no longer a need to initiate it through debugfs remove that code. Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com> --- drivers/net/wireless/marvell/mwifiex/debugfs.c | 31 +------------------------- drivers/net/wireless/marvell/mwifiex/pcie.c | 19 ++++++++++++++-- drivers/net/wireless/marvell/mwifiex/sdio.c | 13 +++++++++++ drivers/net/wireless/marvell/mwifiex/usb.c | 14 ++++++++++++ 4 files changed, 45 insertions(+), 32 deletions(-)