This series adds new devm_work_autocancel() helper.
Note:
"The beef" of this series is the new devm-helper. This means that
normally it would be picked-up by Hans. In this case Hans asked if this
series could be taken in extconn tree:
https://lore.kernel.org/lkml/[email protected]/
Many drivers which use work-queues must ensure the work is not queued when
driver is detached. Often this is done by ensuring new work is not added and
then calling cancel_work_sync() at remove(). In many cases this also requires
cleanup at probe error path - which is easy to forget (or get wrong).
Also the "by ensuring new work is not added" has a gotcha.
It is not strange to see devm managed IRQs scheduling work.
Mixing this with manual wq clean-up is hard to do correctly because the
devm is likely to free the IRQ only after the remove() is ran. So manual
wq cancellation and devm-based IRQ management do not mix well - there is
a short(?) time-window after the wq clean-up when IRQs are still not
freed and may schedule new work.
When both WQs and IRQs are managed by devm things are likely to just
work. WQs should be initialized before IRQs (when IRQs need to schedule
work) and devm unwinds things in "FILO" order.
This series implements wq cancellation on top of devm and replaces
the obvious cases where only thing remove call-back in a driver does is
cancelling the work. There might be other cases where we could switch
more than just work cancellation to use managed version and thus get rid
of remove or mixed (manual and devm) resource management.
Changelog v2:
- rebased on v5.13-rc2
- split the extcon-max8997 change into two. First a simple,
back-portable fix for omitting IRQ freeing at error path, second
being the devm-simpification which does not need backporting.
---
Matti Vaittinen (5):
devm-helpers: Add resource managed version of work init
extcon: extcon-max14577: Fix potential work-queue cancellation race
extcon: extcon-max77693.c: Fix potential work-queue cancellation race
extcon: extcon-max8997: Fix IRQ freeing at error path
extcon: extcon-max8997: Simplify driver using devm
drivers/extcon/extcon-max14577.c | 16 ++++--------
drivers/extcon/extcon-max77693.c | 17 ++++--------
drivers/extcon/extcon-max8997.c | 45 +++++++++++---------------------
include/linux/devm-helpers.h | 25 ++++++++++++++++++
4 files changed, 50 insertions(+), 53 deletions(-)
base-commit: d07f6ca923ea0927a1024dfccafc5b53b61cfecc
--
2.25.4
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
Hi All,
On 6/8/21 12:09 PM, Matti Vaittinen wrote:
> This series adds new devm_work_autocancel() helper.
>
> Note:
> "The beef" of this series is the new devm-helper. This means that
> normally it would be picked-up by Hans. In this case Hans asked if this
> series could be taken in extconn tree:
> https://lore.kernel.org/lkml/[email protected]/
Yes, and given that most of the changes are in the extcon code I still
believe this is best.
Alternatively I can create an immutable branch with these 5 patches on
top of 5.13-rc1 and then send a pull-req to Chanwoo and MyongJoo.
Chanwoo and/or MyongJoo can you please let us know how you want to proceed
with this series?
Regards,
Hans
>
> Many drivers which use work-queues must ensure the work is not queued when
> driver is detached. Often this is done by ensuring new work is not added and
> then calling cancel_work_sync() at remove(). In many cases this also requires
> cleanup at probe error path - which is easy to forget (or get wrong).
>
> Also the "by ensuring new work is not added" has a gotcha.
>
> It is not strange to see devm managed IRQs scheduling work.
> Mixing this with manual wq clean-up is hard to do correctly because the
> devm is likely to free the IRQ only after the remove() is ran. So manual
> wq cancellation and devm-based IRQ management do not mix well - there is
> a short(?) time-window after the wq clean-up when IRQs are still not
> freed and may schedule new work.
>
> When both WQs and IRQs are managed by devm things are likely to just
> work. WQs should be initialized before IRQs (when IRQs need to schedule
> work) and devm unwinds things in "FILO" order.
>
> This series implements wq cancellation on top of devm and replaces
> the obvious cases where only thing remove call-back in a driver does is
> cancelling the work. There might be other cases where we could switch
> more than just work cancellation to use managed version and thus get rid
> of remove or mixed (manual and devm) resource management.
>
> Changelog v2:
> - rebased on v5.13-rc2
> - split the extcon-max8997 change into two. First a simple,
> back-portable fix for omitting IRQ freeing at error path, second
> being the devm-simpification which does not need backporting.
>
> ---
>
> Matti Vaittinen (5):
> devm-helpers: Add resource managed version of work init
> extcon: extcon-max14577: Fix potential work-queue cancellation race
> extcon: extcon-max77693.c: Fix potential work-queue cancellation race
> extcon: extcon-max8997: Fix IRQ freeing at error path
> extcon: extcon-max8997: Simplify driver using devm
>
> drivers/extcon/extcon-max14577.c | 16 ++++--------
> drivers/extcon/extcon-max77693.c | 17 ++++--------
> drivers/extcon/extcon-max8997.c | 45 +++++++++++---------------------
> include/linux/devm-helpers.h | 25 ++++++++++++++++++
> 4 files changed, 50 insertions(+), 53 deletions(-)
>
>
> base-commit: d07f6ca923ea0927a1024dfccafc5b53b61cfecc
>
Hi Hans,
On 6/10/21 12:23 AM, Hans de Goede wrote:
> Hi All,
>
> On 6/8/21 12:09 PM, Matti Vaittinen wrote:
>> This series adds new devm_work_autocancel() helper.
>>
>> Note:
>> "The beef" of this series is the new devm-helper. This means that
>> normally it would be picked-up by Hans. In this case Hans asked if this
>> series could be taken in extconn tree:
>> https://lore.kernel.org/lkml/[email protected]/
>
> Yes, and given that most of the changes are in the extcon code I still
> believe this is best.
>
> Alternatively I can create an immutable branch with these 5 patches on
> top of 5.13-rc1 and then send a pull-req to Chanwoo and MyongJoo.
Right. After creating the immutable branch, please send pull-request to me.
I'll merge them. Thanks.
>
> Chanwoo and/or MyongJoo can you please let us know how you want to proceed
> with this series?
>
> Regards,
>
> Hans
>
>
>
>>
>> Many drivers which use work-queues must ensure the work is not queued when
>> driver is detached. Often this is done by ensuring new work is not added and
>> then calling cancel_work_sync() at remove(). In many cases this also requires
>> cleanup at probe error path - which is easy to forget (or get wrong).
>>
>> Also the "by ensuring new work is not added" has a gotcha.
>>
>> It is not strange to see devm managed IRQs scheduling work.
>> Mixing this with manual wq clean-up is hard to do correctly because the
>> devm is likely to free the IRQ only after the remove() is ran. So manual
>> wq cancellation and devm-based IRQ management do not mix well - there is
>> a short(?) time-window after the wq clean-up when IRQs are still not
>> freed and may schedule new work.
>>
>> When both WQs and IRQs are managed by devm things are likely to just
>> work. WQs should be initialized before IRQs (when IRQs need to schedule
>> work) and devm unwinds things in "FILO" order.
>>
>> This series implements wq cancellation on top of devm and replaces
>> the obvious cases where only thing remove call-back in a driver does is
>> cancelling the work. There might be other cases where we could switch
>> more than just work cancellation to use managed version and thus get rid
>> of remove or mixed (manual and devm) resource management.
>>
>> Changelog v2:
>> - rebased on v5.13-rc2
>> - split the extcon-max8997 change into two. First a simple,
>> back-portable fix for omitting IRQ freeing at error path, second
>> being the devm-simpification which does not need backporting.
>>
>> ---
>>
>> Matti Vaittinen (5):
>> devm-helpers: Add resource managed version of work init
>> extcon: extcon-max14577: Fix potential work-queue cancellation race
>> extcon: extcon-max77693.c: Fix potential work-queue cancellation race
>> extcon: extcon-max8997: Fix IRQ freeing at error path
>> extcon: extcon-max8997: Simplify driver using devm
>>
>> drivers/extcon/extcon-max14577.c | 16 ++++--------
>> drivers/extcon/extcon-max77693.c | 17 ++++--------
>> drivers/extcon/extcon-max8997.c | 45 +++++++++++---------------------
>> include/linux/devm-helpers.h | 25 ++++++++++++++++++
>> 4 files changed, 50 insertions(+), 53 deletions(-)
>>
>>
>> base-commit: d07f6ca923ea0927a1024dfccafc5b53b61cfecc
>>
>
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
> Hi Hans,
>
> On 6/10/21 12:23 AM, Hans de Goede wrote:
> > Hi All,
> >
> > On 6/8/21 12:09 PM, Matti Vaittinen wrote:
> > > This series adds new devm_work_autocancel() helper.
> > >
> > > Note:
> > > "The beef" of this series is the new devm-helper. This means that
> > > normally it would be picked-up by Hans. In this case Hans asked
> > > if this
> > > series could be taken in extconn tree:
> > > https://lore.kernel.org/lkml/[email protected]/
> >
> > Yes, and given that most of the changes are in the extcon code I
> > still
> > believe this is best.
> >
> > Alternatively I can create an immutable branch with these 5 patches
> > on
> > top of 5.13-rc1 and then send a pull-req to Chanwoo and MyongJoo.
>
> Right. After creating the immutable branch, please send pull-request
> to me.
> I'll merge them. Thanks.
Chanwoo, I think the series has no formal ack from you... Can this be
treated as such?
Best Regards
Matti Vaittinen
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland
SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
"non cogito me" dixit Rene Descarte, deinde evanescavit
(Thanks for the translation Simon)