You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/07/23 22:28:17 UTC
[GitHub] [incubator-nuttx] onegray opened a new pull request, #6695: bluetooth: fixing BT buffer addref/release balance
onegray opened a new pull request, #6695:
URL: https://github.com/apache/incubator-nuttx/pull/6695
## Summary
This PR fixes the issue when the BT command buffer gets released more times than its `ref` number.
The issue is always reproducing when called `hci_initialize` function, and `DEBUG_ASSERT` is triggered.
## Impact
Stability fix
## Testing
Tested by monitoring logs and verifying allocated buffers to be released.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] onegray commented on a diff in pull request #6695: bluetooth: fixing BT buffer addref/release balance
Posted by GitBox <gi...@apache.org>.
onegray commented on code in PR #6695:
URL: https://github.com/apache/incubator-nuttx/pull/6695#discussion_r928952897
##########
wireless/bluetooth/bt_hcicore.c:
##########
@@ -1861,7 +1848,6 @@ int bt_hci_cmd_send_sync(uint16_t opcode, FAR struct bt_buf_s *buf,
bt_buf_release(buf->u.hci.sync);
}
- bt_buf_release(buf);
Review Comment:
This `release` was in the correct place for the 'taking ownership' function. But there is also a missing `addref` when we make a strong ref pointer when passing the buffer to `g_btdev.tx_queue`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] onegray commented on a diff in pull request #6695: bluetooth: fixing BT buffer addref/release balance
Posted by GitBox <gi...@apache.org>.
onegray commented on code in PR #6695:
URL: https://github.com/apache/incubator-nuttx/pull/6695#discussion_r928927049
##########
wireless/bluetooth/bt_hcicore.c:
##########
@@ -1741,15 +1741,6 @@ int bt_hci_cmd_send(uint16_t opcode, FAR struct bt_buf_s *buf)
return -ENOBUFS;
}
}
- else
- {
- /* We manage the refcount the same for supplied and created
- * buffers so increment the supplied count so we can manage
- * it as-if we created it.
- */
-
- bt_buf_addref(buf);
- }
Review Comment:
The explanation in comment would be correct, if the function is a `neutral` function. When we pass a created buffer to `neutral` function, we also responsible to release the buffer in calling code. But accordingly to its usage, this is a 'taking ownership' function. The buffer comes with positive `ref` and here we are responsible to `release` it after usage. No additional `addref` is needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #6695: bluetooth: fixing BT buffer addref/release balance
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged PR #6695:
URL: https://github.com/apache/incubator-nuttx/pull/6695
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] onegray commented on a diff in pull request #6695: bluetooth: fixing BT buffer addref/release balance
Posted by GitBox <gi...@apache.org>.
onegray commented on code in PR #6695:
URL: https://github.com/apache/incubator-nuttx/pull/6695#discussion_r928952897
##########
wireless/bluetooth/bt_hcicore.c:
##########
@@ -1861,7 +1848,6 @@ int bt_hci_cmd_send_sync(uint16_t opcode, FAR struct bt_buf_s *buf,
bt_buf_release(buf->u.hci.sync);
}
- bt_buf_release(buf);
Review Comment:
This `release` was in the correct place for a 'taking ownership' function. But there is also a missing `addref` when we make a strong reference when passing the buffer pointer to `g_btdev.tx_queue`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] onegray commented on a diff in pull request #6695: bluetooth: fixing BT buffer addref/release balance
Posted by GitBox <gi...@apache.org>.
onegray commented on code in PR #6695:
URL: https://github.com/apache/incubator-nuttx/pull/6695#discussion_r928927049
##########
wireless/bluetooth/bt_hcicore.c:
##########
@@ -1741,15 +1741,6 @@ int bt_hci_cmd_send(uint16_t opcode, FAR struct bt_buf_s *buf)
return -ENOBUFS;
}
}
- else
- {
- /* We manage the refcount the same for supplied and created
- * buffers so increment the supplied count so we can manage
- * it as-if we created it.
- */
-
- bt_buf_addref(buf);
- }
Review Comment:
The explanation in comment would be correct, if the function is a neutral' function. When we pass a created buffer to 'neutral' function, we also responsible to release the buffer in calling code. But accordingly to its usage, this is a 'taking ownership' function. The buffer comes with positive `ref` and here we are responsible to `release` it after usage. No additional `addref` is needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6695: bluetooth: fixing BT buffer addref/release balance
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6695:
URL: https://github.com/apache/incubator-nuttx/pull/6695#issuecomment-1193272473
@btashton could you review the change is correct?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6695: bluetooth: fixing BT buffer addref/release balance
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6695:
URL: https://github.com/apache/incubator-nuttx/pull/6695#issuecomment-1193495966
@onegray here is @btashton change for your reference: https://github.com/apache/incubator-nuttx/pull/2571
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] onegray commented on a diff in pull request #6695: bluetooth: fixing BT buffer addref/release balance
Posted by GitBox <gi...@apache.org>.
onegray commented on code in PR #6695:
URL: https://github.com/apache/incubator-nuttx/pull/6695#discussion_r928952897
##########
wireless/bluetooth/bt_hcicore.c:
##########
@@ -1861,7 +1848,6 @@ int bt_hci_cmd_send_sync(uint16_t opcode, FAR struct bt_buf_s *buf,
bt_buf_release(buf->u.hci.sync);
}
- bt_buf_release(buf);
Review Comment:
This `release` was in correct place for the 'taking ownership' function. But there is also a missing `addref` when we make a strong ref pointer when passing the buffer to `g_btdev.tx_queue`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] onegray commented on pull request #6695: bluetooth: fixing BT buffer addref/release balance
Posted by GitBox <gi...@apache.org>.
onegray commented on PR #6695:
URL: https://github.com/apache/incubator-nuttx/pull/6695#issuecomment-1194084166
The proposed fix is the one with minimal code. Others are possible, but we need to choose rules to follow.
It can be difficult to manage lifecycle of each buffer over many functions where it is passed. There is a concept we can borrow from other languages (e.g. Objective C in early days).
The function which creates and returns buffer with `ref == 1` is a 'giving ownership' function. Calling code is responsible to add a balancing `release` call (or pass it to 'taking ownership' function).
The function which takes a buffer with `ref > 0` and takes a responsibility to call an additional (unbalancing) `release` is a 'taking ownership' function.
Other functions are 'neutral', and from calling code it is expected they do not change the `addref`/`release` balance (even it's not always true - 'neutral' functions can change the balance but also they are responsible to restore it).
The most functions should be 'neutral', and others are considered as a special case, and if so, they should loud announce it. The 'neutral' functions can pass buffer to other 'neutral' functions without care and that is the trick! We do not have to worry about the whole buffer lifecycle, just need to control the balance inside the function.
JFI: in ObjC its even possible to have a 'neutral' function which creates and returns buffer. They use deferred release feature (autorelease).
One more thing to take into account is a global pointer to buffer (or a pointer which is available out of the function).
Lets consider such pointers as a strong reference (lets ignore ability to have weak references for now). Then each time the pointer is assigned to a buffer we call `addref`, and call `release` when the pointer is not longer points to the buffer.
Such concept helps to manage ref-counting. I do not insist we should follow it, but keeping it in mind is helpful.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] onegray commented on a diff in pull request #6695: bluetooth: fixing BT buffer addref/release balance
Posted by GitBox <gi...@apache.org>.
onegray commented on code in PR #6695:
URL: https://github.com/apache/incubator-nuttx/pull/6695#discussion_r928927049
##########
wireless/bluetooth/bt_hcicore.c:
##########
@@ -1741,15 +1741,6 @@ int bt_hci_cmd_send(uint16_t opcode, FAR struct bt_buf_s *buf)
return -ENOBUFS;
}
}
- else
- {
- /* We manage the refcount the same for supplied and created
- * buffers so increment the supplied count so we can manage
- * it as-if we created it.
- */
-
- bt_buf_addref(buf);
- }
Review Comment:
The explanation in comment would be correct, if the function is a 'neutral' function. When we pass a created buffer to 'neutral' function, we also responsible to release the buffer in calling code. But accordingly to its usage, this is a 'taking ownership' function. The buffer comes with positive `ref` and here we are responsible to `release` it after usage. No additional `addref` is needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] onegray commented on a diff in pull request #6695: bluetooth: fixing BT buffer addref/release balance
Posted by GitBox <gi...@apache.org>.
onegray commented on code in PR #6695:
URL: https://github.com/apache/incubator-nuttx/pull/6695#discussion_r928952897
##########
wireless/bluetooth/bt_hcicore.c:
##########
@@ -1861,7 +1848,6 @@ int bt_hci_cmd_send_sync(uint16_t opcode, FAR struct bt_buf_s *buf,
bt_buf_release(buf->u.hci.sync);
}
- bt_buf_release(buf);
Review Comment:
This `release` was in the correct place for a 'taking ownership' function. But there is also a missing `addref` when we make a strong ref pointer when passing the buffer to `g_btdev.tx_queue`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org