Skip to content

Conversation

@pabigot
Copy link
Contributor

@pabigot pabigot commented Jan 16, 2019

driver: gpio: remove documentation related to pin-based callback configuration

This reverts the documentation component of commit
eb6ea28.

The original change broke the API contract: drivers that use GPIOs need
to be able to configure callbacks without being aware of whether a
particular implementation expects to use a mask or a pin ordinal.

Revert the API documentation to its original format, and mark that the
added field should be removed when issue #11565 is resolved.

drivers: gpio: fix mis-use of slist API in callback processing

The iterator over registered callbacks failed to account for the
possibility that the callback would remove itself from the list. If
this occurred any remaining callbacks would no longer be reachable from
the node. Switch to the slist iterator that is safe for self-removal.

Note that the slist API remains unsafe for removal of subsequent nodes.
Even with the corrected code removal of the next callback registration
(cached in tmp) will result in it being called anyway, with the
remaining unremoved registrations not being called. If the next
callback were removed and re-registered on a different device, the
callbacks would be invoked for the wrong device.

Resolve this by a documentation change describing the conditions under
which a change to callback registration from within a callback are
permitted. Add a similar note regarding the effect of adding a
callback. The current event invocation behavior for callbacks added
within an event is explicitly left unspecified, though in the current
slist implementation newly added callbacks will not be invoked until the
next event.

Closes #10186

@pabigot
Copy link
Contributor Author

pabigot commented Jan 16, 2019

@dcpleung The primary issue addressed in this PR affects gpio_intel_apl.c as well: i.e. it needs to switch to SYS_SLIST_FOR_EACH_CONTAINER_SAFE in gpio_intel_apl_isr.

Because I was touching the header anyway I also reverted the documentation change related to #11565, so nobody starts developing code with the wrong idea.

@pabigot pabigot requested a review from pdunaj January 16, 2019 13:34
@codecov-io
Copy link

codecov-io commented Jan 16, 2019

Codecov Report

Merging #12515 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #12515   +/-   ##
=======================================
  Coverage   53.82%   53.82%           
=======================================
  Files         243      243           
  Lines       27725    27725           
  Branches     6735     6735           
=======================================
  Hits        14922    14922           
  Misses       9998     9998           
  Partials     2805     2805

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5017ec...d414c86. Read the comment docs.

@zephyrbot
Copy link

zephyrbot commented Jan 16, 2019

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

This reverts the documentation component of commit
eb6ea28.

The original change broke the API contract: drivers that use GPIOs need
to be able to configure callbacks without being aware of whether a
particular implementation expects to use a mask or a pin ordinal.

Revert the API documentation to its original format, and mark that the
added field should be removed when issue zephyrproject-rtos#11565 is resolved.

Signed-off-by: Peter A. Bigot <[email protected]>
The iterator over registered callbacks failed to account for the
possibility that the callback would remove itself from the list.  If
this occurred any remaining callbacks would no longer be reachable from
the node.  Switch to the slist iterator that is safe for self-removal.

Note that the slist API remains unsafe for removal of subsequent nodes.
Even with the corrected code removal of the next callback registration
(cached in tmp) will result in it being called anyway, with the
remaining unremoved registrations not being called.  If the next
callback were removed and re-registered on a different device, the
callbacks would be invoked for the wrong device.

Resolve this by a documentation change describing the conditions under
which a change to callback registration from within a callback are
permitted.  Add a similar note regarding the effect of adding a
callback.  The current event invocation behavior for callbacks added
within an event is explicitly left unspecified, though in the current
slist implementation newly added callbacks will not be invoked until the
next event.

Closes zephyrproject-rtos#10186

Signed-off-by: Peter A. Bigot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GPIO callback disable has no effect due to _gpio_fire_callbacks

5 participants