bluetooth: rfcomm: fix issue of sending buf invalid

[lylezhu2012]

There is a change (commit no.: 93d0eac) that if the ref of sending buf is not 1, the error code -EINVAL will be returned from bt_conn_send_cb.

It causes the RFCOMM functionality cannot work properly.

Remove the ref operation from the buf to be sent to fix the issue.

Due to the sending buf cannot be referred by sending layer.
It is unsafe to use buf identification for a transmission, because the buf may have been newly transmitted when the
sent callback is triggered.

Now instead, when the send completion callback is received, the upper layer is notified that a transfer is completed.
If multiple bufs are sent at the same time, there is no guarantee which buf is completed when the sent callback triggered. Therefore, it is recommended that the caller transfers the next data block after the previous transfer is completed.

HFP AG: Due to the sent callback of RFCOMM is changed, the sending buf need to be primed waiting for the previous one to be completed. Add a worker for this purpose.

HFP Unit: Due to the parameter buf has been removed by rfcomm, update the prototype of channel sent callback hfp_hf_sent.

[jhedberg]

@lylezhu2012 seems like the issue was a reference leak, i.e. bt_rfcomm_dlc_send() was always supposed to move the reference rather than taking a new one. I think ideally it should be taking a struct net_buf **buf as a parameter and setting the original pointer to NULL to make the reference move clear. Btw, this also relates to #72798.

At a bare minimum I think it'd be important to accurately describe the bug that you're fixing. Now it sounds like you're working around an issue introduced by commit 93d0eac, but that's actually not the case - the reference leak existed even before that commit.

[jhedberg]

@lylezhu2012 also, the documentation for `bt_rfcomm_dlc_send() is missing the information bout the API moving the reference. I think that's a potential source of future bugs, so please add the missing information to the documentation as well.

[lylezhu2012]

@lylezhu2012 seems like the issue was a reference leak, i.e. bt_rfcomm_dlc_send() was always supposed to move the reference rather than taking a new one. I think ideally it should be taking a struct net_buf **buf as a parameter and setting the original pointer to NULL to make the reference move clear. Btw, this also relates to #72798.

At a bare minimum I think it'd be important to accurately describe the bug that you're fixing. Now it sounds like you're working around an issue introduced by commit 93d0eac, but that's actually not the case - the reference leak existed even before that commit.

Since conn has a TX pending queue, I want to leverage this queue, so all sending bufs are pending in the TX pending queue of the conn layer, so that the upper layer does not need to implement such a queue by itself. To achieve this, I ref the buf that needs to be sent in rfcomm. When the sending is completed, notify the upper layer which buf is sent and then unref this buf in RFCOMM layer.

It seems inappropriate to do so at the moment. I cache all the bufs that need to be sent by the upper layer of RFCOMM and send them one by one. At the same time the sending buf need to be primed waiting for the previous one to be completed.

[lylezhu2012]

@lylezhu2012 also, the documentation for `bt_rfcomm_dlc_send() is missing the information bout the API moving the reference. I think that's a potential source of future bugs, so please add the missing information to the documentation as well.

OK. But I plan to revert it in this PR. Do not update the reference of buf.

[lylezhu2012]

@jhedberg , @sjanc , @alwa-nordic , Could you please take a look at it.

[dleach02]

@jhedberg can you review to determine if your change request is satisfied.

[lylezhu2012]

When testing #72090, there is an issue found.

The change in the previous commit is to put all data sending operations into the work queue context, and lock the current
AG before sending data.

And in change of #72090, the HCI TX thread is removed. All sending sequence are happened in work queue context.

There is a possible problem when AG creates a SCO connection by calling the function bt_conn_create_sco. Before the
function bt_conn_create_sco is called, AG will be locked to avoid creating repeated SCO connection.
And the execution of the function bt_conn_create_sco depends on the work queue. Because the HCI command of
function bt_conn_create_sco is sent in work queue context.

In the normal case, there is not any issue.

But there is a case that when the function bt_conn_create_sco is being executed, there is a pending AG TX waiting to be executed.

Once the work queue starts executing the handler, the AG TX handler is executed first. Since the lock has been acquired
by other threads, the AG TX handler cannot acquire the lock. As a result, the SCO connection creation fails.

Remove the AG lock from SCO creating. Instead, use a flag to mark whether a SCO connection is be created.

[lylezhu2012]

@dleach02 , @carlescufi , now there is only one reviewer approves the PR. Is there anyone else who can help review this PR? Currently, handsfree and handsfree_ag based on RFCOMM cannot work properly.