Rework gpio cansleep (was Re: gpiolib and sleepinggpios)
in [Kernel]
|
Prev: 2.6.35-rc3 -- kernel BUG at mm/vmalloc.c:216! - invalid opcode: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
Next: [PATCH V3 3/5] omap_hsmmc: Add erase capability
From: Lars-Peter Clausen on 24 Jun 2010 04:30 Jon Povey wrote: > Ryan Mallon wrote: > > >> If we strip my patch back to just introducing gpio_request_cansleep, >> which would be used in any driver where all of the calls are >> gpio_(set/get)_cansleep, and make gpio_request only allow non-sleeping >> gpios then incorrect use of gpios would be caught at request time and >> returned to the caller as an error. >> > > It seems like a good idea to catch these at request time. There is support in the API for this already (gpio_cansleep), but driver writers are not steered towards checking and thinking in these ways by the current API or documentation. Perhaps a documentation change with some cut and paste boilerplate would be enough, but I think some API enforcement/encouragement would be helpful. > > I think this agrees with you, Ryan: > > gpio_request_cansleep would be the same as current gpio_request > gpio_request changes to error if this is a sleepy gpio. > > Imagine a situation where GPIOs are being assigned and passed around between drivers in some dynamic way, or some way unpredictable to the driver writer. In development only non-sleeping GPIOs have been seen and everything is fine. One day someone feeds it a GPIO on an I2C expander and the driver crashes. > If gpio_request had this built-in check the driver could gracefuly fail to load instead with an appropriate error message. > > > Hi Given that that most users will only access the gpios in context where sleeping is possible it would make more sense to add a special case for those who use it in contexts where sleeping is not possible. This wouldn't change the semantics of the current API and furthermore it is possible to implement it as a small helper function on top of the current API. Something like: int gpio_request_atomic(unsigned gpio, const char *label) { if (gpio_cansleep(gpio)) return -EINVAL; return gpio_request(gpio, label); } - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: David Brownell on 24 Jun 2010 06:40 --- On Wed, 6/23/10, Jon Povey <Jon.Povey(a)racelogic.co.uk> wrote: > > Date: Wednesday, June 23, 2010, 9:46 PM > Ryan Mallon wrote: > > > If we strip my patch back to just introducing > gpio_request_cansleep, > > which would be used in any driver where all of the > calls are > > gpio_(set/get)_cansleep, and make gpio_request only > allow non-sleeping > > gpios then incorrect use of gpios would be caught at > request time and > > returned to the caller as an error. > > It seems like a good idea to catch these at request time. > There is support in the API for this already > (gpio_cansleep), but driver writers are not steered towards > checking and thinking in these ways by the current API or > > gpio_request_cansleep would be the same as current > gpio_request I wonder if, by the time I catch up on this ever-extending email thread .... someone else will have noted that because gpio_request() can now poke the GPIO chip, that call might actually need to sleep. So there'd be a difference between the two calls:� one would *NEED* to be called in a sleepable thread context, vs. that just being well advised (e.g. as part of board setup in arch init code after tasking is working)... So that couldn't work quite that way. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Lars-Peter Clausen on 24 Jun 2010 06:40
Jani Nikula wrote: > > On Thu, 24 Jun 2010, ext Jon Povey wrote: > >> Ryan Mallon wrote: >> >>> If we strip my patch back to just introducing gpio_request_cansleep, >>> which would be used in any driver where all of the calls are >>> gpio_(set/get)_cansleep, and make gpio_request only allow >>> non-sleeping gpios then incorrect use of gpios would be caught at >>> request time and returned to the caller as an error. >> >> It seems like a good idea to catch these at request time. There is >> support in the API for this already (gpio_cansleep), but driver >> writers are not steered towards checking and thinking in these ways >> by the current API or documentation. Perhaps a documentation change >> with some cut and paste boilerplate would be enough, but I think some >> API enforcement/encouragement would be helpful. >> >> I think this agrees with you, Ryan: >> >> gpio_request_cansleep would be the same as current gpio_request >> gpio_request changes to error if this is a sleepy gpio. >> >> Imagine a situation where GPIOs are being assigned and passed around >> between drivers in some dynamic way, or some way unpredictable to the >> driver writer. In development only non-sleeping GPIOs have been seen >> and everything is fine. One day someone feeds it a GPIO on an I2C >> expander and the driver crashes. If gpio_request had this built-in >> check the driver could gracefuly fail to load instead with an >> appropriate error message. > > Hi - > > There's no need to imagine such situations. It's not at all uncommon > to request GPIOs in board files, and pass the already requested GPIO > numbers to drivers. Replacing gpio_request() with > gpio_request_cansleep() (or gpio_request_atomic() as suggested in > another mail) in the board files does *nothing* to help such drivers > use the correct gpio get/set calls. The driver will need to know what > it's doing, in what contexts. Some drivers might not work with > "sleepy" GPIOs, and that's fine - they can check using gpio_cansleep() > and fail gracefully. Is there a reason, why a gpio is requested in the board file and not in the driver? I would have considered that the later is far more common. Sure, drivers which do not request the gpios themselves would have to call gpio_cansleep, but for those which request the gpios themselves it would help to reduce code clutter to have a gpio_request_atomic. The only argument speaking against adding such a helper function would be that drivers accessing gpios in contexts where they can not sleep are far to rare to bother. - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |