* [PATCH] gpiolib: Defer failed gpio requests by default @ 2012-05-02 11:49 Mark Brown 2012-05-18 4:32 ` Grant Likely 0 siblings, 1 reply; 9+ messages in thread From: Mark Brown @ 2012-05-02 11:49 UTC (permalink / raw) To: Grant Likely, Linus Walleij; +Cc: linux-kernel, Mark Brown Since users must be explicitly provided with a GPIO number in order to request one the overwhelmingly common case for failing to request will be that the required GPIO driver has not yet registered and we should therefore defer until it has registered. In order to avoid having to code this logic in individual drivers have gpio_request() return -EPROBE_DEFER when failing to look up the GPIO. Drivers which don't want this behaviour can override it if they desire. Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com> --- drivers/gpio/gpiolib.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 566d012..b244b0c 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1186,7 +1186,7 @@ int gpio_request(unsigned gpio, const char *label) { struct gpio_desc *desc; struct gpio_chip *chip; - int status = -EINVAL; + int status = -EPROBE_DEFER; unsigned long flags; spin_lock_irqsave(&gpio_lock, flags); -- 1.7.10 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] gpiolib: Defer failed gpio requests by default 2012-05-02 11:49 [PATCH] gpiolib: Defer failed gpio requests by default Mark Brown @ 2012-05-18 4:32 ` Grant Likely 2012-05-18 8:45 ` Mark Brown 2012-05-18 16:35 ` Alan Stern 0 siblings, 2 replies; 9+ messages in thread From: Grant Likely @ 2012-05-18 4:32 UTC (permalink / raw) To: Mark Brown, Linus Walleij Cc: linux-kernel, Rafael J. Wysocki, Greg Kroah-Hartman, Kevin Hilman, Alan Stern, Thomas Gleixner, Jesse Barnes On Wed, 2 May 2012 12:49:40 +0100, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > Since users must be explicitly provided with a GPIO number in order to > request one the overwhelmingly common case for failing to request will > be that the required GPIO driver has not yet registered and we should > therefore defer until it has registered. > > In order to avoid having to code this logic in individual drivers have > gpio_request() return -EPROBE_DEFER when failing to look up the GPIO. > Drivers which don't want this behaviour can override it if they desire. > > Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com> [cc'ing a bunch of people who I think have some knowledge about the PM issues... I know this is a long email, but I think this problem needs to be looked at by folks more familiar with suspend/resume than I] This one is problematic.... well, not the patch itself but the way the larger driver mode works with suspend/resume looks really wrong to me. dpm_list keeps track of all the 'active' driver that need to be suspended. However, it assumes that the list are in the order that they were probed and therefore it can can walk the list backwards when suspending. The assumption is already kind of bogus because the list is in the order that devices are added, not in the order that they are probed. Probe order has pretty much no relationship with registration order. Probe order effectively is based on driver initialization order and module load order. So the ordering when suspending may be completely different from the order devices were initialized. I think there are probably all kinds of implicit dependency bugs hiding in that whole scheme. There are the device_pm_move_* functions for manipulating the dpm_list and trying to 'fix' the ordering, but I'm not confident that they are effective in the way they are used. Mostly it seems to be used in the path of removing a device. /anyway/ the reason I'm saying this is that devices are *supposed* to be in the correct order for suspend/resume. With the implicit dependencies expected by deferred probe, it becomes really important that devices are correctly ordered in the list. The only way this can be done is to move the device to the end of the list. The agreement when this was last discussed on the ML was to make the drivers do an explicit move immediately after it has validated all its dependencies. That is why it doesn't work to return -EPROBE_DEFER by default from helpers; because it becomes really hard to audit if the drivers are doing the right thing. I see two solutions to this though; 1) add a new .preprobe hook to device drivers that will respect -EPROBE_DEFER and ignore -EPROBE_DEFER on the regular .probe. Probing a driver will call both, but if the .preprobe hook is populated, then it will also reorder dpm_list between the two calls. - This still requires modifying drivers, but at least it is auditable by mere-mortal reviewers. - It also means all bus_type code has to add a new hook. Blech. 2) simply force devices to the end of dpm_list before each probe attempt (provided that it doesn't have any children). - I've avoided this because it adds a claim of the dpm_list_mtx mutex on every single probe call and adds a lot more manipulation of the list.... - However, it should make your patch here actually safe. If a device gets deferred, it will be guaranteed to be at the end of the list because it gets moved there on every probe attempt. - also after reading through the code (see my notes below; I'm a bit frustrated with the whole thing) I think it is probably the right thing to do. Will you be at Connect? I could use some time sitting in a room with smart people to dig through this code and talk out what it should be doing. Grumpily yours, g. My notes on the issue: device_pm_add() - is only called by device_add() - so that is easy to audit device_pm_remove() - similarly is only called by device_remove() device_pm_move_{before,after,last}() - Only called by device_move() device_move() - Used to reparent a device - only 4 in-tree users: - drivers/media/video/pvrusb2 - uses it to *remove* any parents to a device - this is a weird driver; I don't know why it is unparenting it's devices - drivers/s390/cio/device.c - Used to change subchannel of a device. - net/bluetooth/hci_sysfs.c & net/bluetooth/rfcomm/tty.c - 2 callsites used to unparent a device before removing the parent. This actually looks rather wrong to me; but I might be missing something. - 1 call site is used to set a new parent for the rfcomm device after opening the tty. As for /using/ dpm_list, I see 2 locations: dpm_complete() - entries in dpm_prepared_list are spliced into dpm_list. dpm_prepare() - moves entries into dpm_prepared_list - Hmmm... So what happens when a device is moved but it is already in dpm_prepared_list? Will it accidentally put /back/ onto the dpm_list when it shouldn't be? I don't see any protections against this. - It *looks* like dev->power.is_{prepared,suspended} is intended to protect that list, but I don't see it being used by the core. There are other lists too that devices can be on instead of dpm_list: - dpm_suspended_list - dpm_late_early_list - dpm_noirq_list Blech! None of this is ******* documented. How is this all supposed to work and stay consistent?!? Misc: - to_device is the magic macro that goes from the device->power.entry list item. - searching for power.entry also finds the relevant modifications. > --- > drivers/gpio/gpiolib.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 566d012..b244b0c 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -1186,7 +1186,7 @@ int gpio_request(unsigned gpio, const char *label) > { > struct gpio_desc *desc; > struct gpio_chip *chip; > - int status = -EINVAL; > + int status = -EPROBE_DEFER; > unsigned long flags; > > spin_lock_irqsave(&gpio_lock, flags); > -- > 1.7.10 > -- Grant Likely, B.Sc, P.Eng. Secret Lab Technologies, Ltd. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gpiolib: Defer failed gpio requests by default 2012-05-18 4:32 ` Grant Likely @ 2012-05-18 8:45 ` Mark Brown 2012-05-18 16:35 ` Alan Stern 1 sibling, 0 replies; 9+ messages in thread From: Mark Brown @ 2012-05-18 8:45 UTC (permalink / raw) To: Grant Likely Cc: Linus Walleij, linux-kernel, Rafael J. Wysocki, Greg Kroah-Hartman, Kevin Hilman, Alan Stern, Thomas Gleixner, Jesse Barnes [-- Attachment #1: Type: text/plain, Size: 2869 bytes --] On Thu, May 17, 2012 at 10:32:19PM -0600, Grant Likely wrote: > be done is to move the device to the end of the list. The agreement > when this was last discussed on the ML was to make the drivers do an > explicit move immediately after it has validated all its dependencies. > That is why it doesn't work to return -EPROBE_DEFER by default from > helpers; because it becomes really hard to audit if the drivers are > doing the right thing. Ah, oh dear - I missed that bit of the discussion I'm afraid and had thought we'd gone with your option 2 below. This means that the ASoC usage is currently not helping anything (though it's no worse than what we had before), and the regulator framework usage won't do the right thing (though in practice I don't expect it to actually go off and there's fun and games associated with that if we actually need to suspend regulators from software anyway). > I see two solutions to this though; > 1) add a new .preprobe hook to device drivers that will respect > -EPROBE_DEFER and ignore -EPROBE_DEFER on the regular .probe. > Probing a driver will call both, but if the .preprobe hook is > populated, then it will also reorder dpm_list between the two > calls. > - This still requires modifying drivers, but at least it is > auditable by mere-mortal reviewers. > - It also means all bus_type code has to add a new hook. Blech. Yeah, this seems very painful, especially the bit where we have to change a good selection of buses and drivers to implement and use it. > 2) simply force devices to the end of dpm_list before each probe > attempt (provided that it doesn't have any children). > - I've avoided this because it adds a claim of the dpm_list_mtx > mutex on every single probe call and adds a lot more manipulation > of the list.... > - However, it should make your patch here actually safe. If a > device gets deferred, it will be guaranteed to be at the end of > the list because it gets moved there on every probe attempt. > - also after reading through the code (see my notes below; I'm a > bit frustrated with the whole thing) I think it is probably the > right thing to do. This does feel much more robust to me than manually reordering in drivers, we'll have to see if it causes a performance problem though and if it does perhaps we can handle it. I guess the other thing is that if we get a lot of drivers implementing deferral and we stop playing around with initcall stuff then we'll loose a reasonable proportion of the advantage of not taking the mutex and reordering. Either of these options would avoid the surprise that currently exists with the API. > Will you be at Connect? I could use some time sitting in a room with > smart people to dig through this code and talk out what it should be > doing. I won't be unfortunately. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gpiolib: Defer failed gpio requests by default 2012-05-18 4:32 ` Grant Likely 2012-05-18 8:45 ` Mark Brown @ 2012-05-18 16:35 ` Alan Stern 1 sibling, 0 replies; 9+ messages in thread From: Alan Stern @ 2012-05-18 16:35 UTC (permalink / raw) To: Grant Likely Cc: Mark Brown, Linus Walleij, linux-kernel, Rafael J. Wysocki, Greg Kroah-Hartman, Kevin Hilman, Thomas Gleixner, Jesse Barnes On Thu, 17 May 2012, Grant Likely wrote: > /anyway/ the reason I'm saying this is that devices are *supposed* to > be in the correct order for suspend/resume. With the implicit > dependencies expected by deferred probe, it becomes really important > that devices are correctly ordered in the list. The only way this can > be done is to move the device to the end of the list. The agreement > when this was last discussed on the ML was to make the drivers do an > explicit move immediately after it has validated all its dependencies. > That is why it doesn't work to return -EPROBE_DEFER by default from > helpers; because it becomes really hard to audit if the drivers are > doing the right thing. > > I see two solutions to this though; > 1) add a new .preprobe hook to device drivers that will respect > -EPROBE_DEFER and ignore -EPROBE_DEFER on the regular .probe. > Probing a driver will call both, but if the .preprobe hook is > populated, then it will also reorder dpm_list between the two > calls. > - This still requires modifying drivers, but at least it is > auditable by mere-mortal reviewers. > - It also means all bus_type code has to add a new hook. Blech. > 2) simply force devices to the end of dpm_list before each probe > attempt (provided that it doesn't have any children). > - I've avoided this because it adds a claim of the dpm_list_mtx > mutex on every single probe call and adds a lot more manipulation > of the list.... > - However, it should make your patch here actually safe. If a > device gets deferred, it will be guaranteed to be at the end of > the list because it gets moved there on every probe attempt. > - also after reading through the code (see my notes below; I'm a > bit frustrated with the whole thing) I think it is probably the > right thing to do. 2) seems like the best approach. A little extra list manipulation shouldn't hurt anything (the bits don't wear out...). > As for /using/ dpm_list, I see 2 locations: > dpm_complete() > - entries in dpm_prepared_list are spliced into dpm_list. > dpm_prepare() > - moves entries into dpm_prepared_list > - Hmmm... So what happens when a device is moved but it is already > in dpm_prepared_list? Will it accidentally put /back/ onto the > dpm_list when it shouldn't be? I don't see any protections > against this. It should never happen. A device gets put on the dpm_prepared_list after its .prepare method is called, and one of the things that method is supposed to do is guarantee that no more children will be registered beneath the device. No registrations means no probes -- or at least, it did until the deferred probing mechanism was created. It may turn out to be necessary to make the deferred probing threads freezable, so they will not run while the dpm lists are being used. > - It *looks* like dev->power.is_{prepared,suspended} is intended > to protect that list, but I don't see it being used by the > core. IIRC, those flags is used mainly for error detection and reporting (also for error recovery, when a suspend is aborted in the middle). They don't protect anything. > There are other lists too that devices can be on instead of dpm_list: > - dpm_suspended_list > - dpm_late_early_list > - dpm_noirq_list > > Blech! None of this is ******* documented. How is this all supposed to > work and stay consistent?!? That's not entirely fair. The overall process is documented in Documentation/power/devices.txt. The list headers themselves are supposed to be private to drivers/base/power/main.c, although they currently aren't declared static for some reason -- an oversight? Alan Stern ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] gpiolib: Defer failed gpio requests by default @ 2012-07-09 11:22 Mark Brown 2012-07-09 20:31 ` Linus Walleij 2012-07-17 18:20 ` Linus Walleij 0 siblings, 2 replies; 9+ messages in thread From: Mark Brown @ 2012-07-09 11:22 UTC (permalink / raw) To: Linus Walleij, Grant Likely; +Cc: linux-kernel, Mark Brown Since users must be explicitly provided with a GPIO number in order to request one the overwhelmingly common case for failing to request will be that the required GPIO driver has not yet registered and we should therefore defer until it has registered. In order to avoid having to code this logic in individual drivers have gpio_request() return -EPROBE_DEFER when failing to look up the GPIO. Drivers which don't want this behaviour can override it if they desire. Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com> --- drivers/gpio/gpiolib.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 120b2a0..de0213c 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1186,7 +1186,7 @@ int gpio_request(unsigned gpio, const char *label) { struct gpio_desc *desc; struct gpio_chip *chip; - int status = -EINVAL; + int status = -EPROBE_DEFER; unsigned long flags; spin_lock_irqsave(&gpio_lock, flags); -- 1.7.10 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] gpiolib: Defer failed gpio requests by default 2012-07-09 11:22 Mark Brown @ 2012-07-09 20:31 ` Linus Walleij 2012-07-09 21:54 ` Grant Likely 2012-07-17 18:20 ` Linus Walleij 1 sibling, 1 reply; 9+ messages in thread From: Linus Walleij @ 2012-07-09 20:31 UTC (permalink / raw) To: Mark Brown; +Cc: Linus Walleij, Grant Likely, linux-kernel On Mon, Jul 9, 2012 at 1:22 PM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > Since users must be explicitly provided with a GPIO number in order to > request one the overwhelmingly common case for failing to request will > be that the required GPIO driver has not yet registered and we should > therefore defer until it has registered. > > In order to avoid having to code this logic in individual drivers have > gpio_request() return -EPROBE_DEFER when failing to look up the GPIO. > Drivers which don't want this behaviour can override it if they desire. > > Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com> While this makes perfect sense to me I would *really* like to wait for Grants opinion on this one patch, him having devised the deferral and being GPIO maintainer. Is any deferral of this deferral mechanism causing you to defer important work right now? Yours, Linus Walleij ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gpiolib: Defer failed gpio requests by default 2012-07-09 20:31 ` Linus Walleij @ 2012-07-09 21:54 ` Grant Likely 2012-07-10 11:07 ` Mark Brown 0 siblings, 1 reply; 9+ messages in thread From: Grant Likely @ 2012-07-09 21:54 UTC (permalink / raw) To: Linus Walleij; +Cc: Mark Brown, Linus Walleij, linux-kernel On Mon, Jul 9, 2012 at 9:31 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Mon, Jul 9, 2012 at 1:22 PM, Mark Brown > <broonie@opensource.wolfsonmicro.com> wrote: > >> Since users must be explicitly provided with a GPIO number in order to >> request one the overwhelmingly common case for failing to request will >> be that the required GPIO driver has not yet registered and we should >> therefore defer until it has registered. >> >> In order to avoid having to code this logic in individual drivers have >> gpio_request() return -EPROBE_DEFER when failing to look up the GPIO. >> Drivers which don't want this behaviour can override it if they desire. >> >> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com> > > While this makes perfect sense to me I would *really* like to > wait for Grants opinion on this one patch, him having devised > the deferral and being GPIO maintainer. > > Is any deferral of this deferral mechanism causing you to > defer important work right now? I'm fine with this patch, but the patch that adds the twizzling of the dpm_list when probing needs some tweaking, and this patch must be applied after that one. I'll go and reply to that patch now (and cc you if you're not already). g. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gpiolib: Defer failed gpio requests by default 2012-07-09 21:54 ` Grant Likely @ 2012-07-10 11:07 ` Mark Brown 0 siblings, 0 replies; 9+ messages in thread From: Mark Brown @ 2012-07-10 11:07 UTC (permalink / raw) To: Grant Likely; +Cc: Linus Walleij, Linus Walleij, linux-kernel [-- Attachment #1: Type: text/plain, Size: 597 bytes --] On Mon, Jul 09, 2012 at 10:54:41PM +0100, Grant Likely wrote: > I'm fine with this patch, but the patch that adds the twizzling of the > dpm_list when probing needs some tweaking, and this patch must be > applied after that one. I'll go and reply to that patch now (and cc > you if you're not already). Given how many subsystems are already doing the -EPROBE_DEFER stuff it seems like we're already at the point where the core logic needs to be bugfixes I'm not sure it's worth holding off, any system that's hitting this is already buggy anyway so at worst we're just shifting the bug around. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gpiolib: Defer failed gpio requests by default 2012-07-09 11:22 Mark Brown 2012-07-09 20:31 ` Linus Walleij @ 2012-07-17 18:20 ` Linus Walleij 1 sibling, 0 replies; 9+ messages in thread From: Linus Walleij @ 2012-07-17 18:20 UTC (permalink / raw) To: Mark Brown; +Cc: Linus Walleij, Grant Likely, linux-kernel On Mon, Jul 9, 2012 at 1:22 PM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > Since users must be explicitly provided with a GPIO number in order to > request one the overwhelmingly common case for failing to request will > be that the required GPIO driver has not yet registered and we should > therefore defer until it has registered. > > In order to avoid having to code this logic in individual drivers have > gpio_request() return -EPROBE_DEFER when failing to look up the GPIO. > Drivers which don't want this behaviour can override it if they desire. > > Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com> Following Greg's application of the required prerequisite patch I have merged this patch. Grant, shout if you feel bad about it, ACK if you like it :-) Linus Walleij ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-07-17 18:20 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-05-02 11:49 [PATCH] gpiolib: Defer failed gpio requests by default Mark Brown 2012-05-18 4:32 ` Grant Likely 2012-05-18 8:45 ` Mark Brown 2012-05-18 16:35 ` Alan Stern 2012-07-09 11:22 Mark Brown 2012-07-09 20:31 ` Linus Walleij 2012-07-09 21:54 ` Grant Likely 2012-07-10 11:07 ` Mark Brown 2012-07-17 18:20 ` Linus Walleij
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.