* [Bluez PATCH] Monitor: Remove handle before assigning
@ 2024-01-26 6:41 Archie Pusaka
2024-01-26 8:05 ` [Bluez] " bluez.test.bot
2024-01-26 19:01 ` [Bluez PATCH] " Luiz Augusto von Dentz
0 siblings, 2 replies; 6+ messages in thread
From: Archie Pusaka @ 2024-01-26 6:41 UTC (permalink / raw)
To: linux-bluetooth, Luiz Augusto von Dentz, Johan Hedberg,
Marcel Holtmann
Cc: CrosBT Upstreaming, Archie Pusaka, Zhengping Jiang
From: Archie Pusaka <apusaka@chromium.org>
It is possible to have some handles not removed, for example the host
may decide not to wait for disconnection complete event when it is
suspending. In this case, when the peer device reconnected, we might
have two of the same handle assigned and create problem down the road.
This patch solves the issue by always removing any previous handles
when assigning a new handle if they are the same.
Reviewed-by: Zhengping Jiang <jiangzp@google.com>
---
monitor/packet.c | 50 +++++++++++++++++++++++++-----------------------
1 file changed, 26 insertions(+), 24 deletions(-)
diff --git a/monitor/packet.c b/monitor/packet.c
index 42e711cafa..b5dea6384a 100644
--- a/monitor/packet.c
+++ b/monitor/packet.c
@@ -189,11 +189,33 @@ static struct packet_conn_data *lookup_parent(uint16_t handle)
return NULL;
}
+static void release_handle(uint16_t handle)
+{
+ int i;
+
+ for (i = 0; i < MAX_CONN; i++) {
+ struct packet_conn_data *conn = &conn_list[i];
+
+ if (conn->handle == handle) {
+ if (conn->destroy)
+ conn->destroy(conn->data);
+
+ queue_destroy(conn->tx_q, free);
+ queue_destroy(conn->chan_q, free);
+ memset(conn, 0, sizeof(*conn));
+ conn->handle = 0xffff;
+ break;
+ }
+ }
+}
+
static void assign_handle(uint16_t index, uint16_t handle, uint8_t type,
uint8_t *dst, uint8_t dst_type)
{
int i;
+ release_handle(handle);
+
for (i = 0; i < MAX_CONN; i++) {
if (conn_list[i].handle == 0xffff) {
hci_devba(index, (bdaddr_t *)conn_list[i].src);
@@ -225,26 +247,6 @@ static void assign_handle(uint16_t index, uint16_t handle, uint8_t type,
}
}
-static void release_handle(uint16_t handle)
-{
- int i;
-
- for (i = 0; i < MAX_CONN; i++) {
- struct packet_conn_data *conn = &conn_list[i];
-
- if (conn->handle == handle) {
- if (conn->destroy)
- conn->destroy(conn->data);
-
- queue_destroy(conn->tx_q, free);
- queue_destroy(conn->chan_q, free);
- memset(conn, 0, sizeof(*conn));
- conn->handle = 0xffff;
- break;
- }
- }
-}
-
struct packet_conn_data *packet_get_conn_data(uint16_t handle)
{
int i;
@@ -10076,7 +10078,7 @@ static void conn_complete_evt(struct timeval *tv, uint16_t index,
const struct bt_hci_evt_conn_complete *evt = data;
print_status(evt->status);
- print_handle(evt->handle);
+ print_field("Handle: %d", le16_to_cpu(evt->handle));
print_bdaddr(evt->bdaddr);
print_link_type(evt->link_type);
print_enable("Encryption", evt->encr_mode);
@@ -10648,7 +10650,7 @@ static void sync_conn_complete_evt(struct timeval *tv, uint16_t index,
const struct bt_hci_evt_sync_conn_complete *evt = data;
print_status(evt->status);
- print_handle(evt->handle);
+ print_field("Handle: %d", le16_to_cpu(evt->handle));
print_bdaddr(evt->bdaddr);
print_link_type(evt->link_type);
print_field("Transmission interval: 0x%2.2x", evt->tx_interval);
@@ -11077,7 +11079,7 @@ static void le_conn_complete_evt(struct timeval *tv, uint16_t index,
const struct bt_hci_evt_le_conn_complete *evt = data;
print_status(evt->status);
- print_handle(evt->handle);
+ print_field("Handle: %d", le16_to_cpu(evt->handle));
print_role(evt->role);
print_peer_addr_type("Peer address type", evt->peer_addr_type);
print_addr("Peer address", evt->peer_addr, evt->peer_addr_type);
@@ -11206,7 +11208,7 @@ static void le_enhanced_conn_complete_evt(struct timeval *tv, uint16_t index,
const struct bt_hci_evt_le_enhanced_conn_complete *evt = data;
print_status(evt->status);
- print_handle(evt->handle);
+ print_field("Handle: %d", le16_to_cpu(evt->handle));
print_role(evt->role);
print_peer_addr_type("Peer address type", evt->peer_addr_type);
print_addr("Peer address", evt->peer_addr, evt->peer_addr_type);
--
2.43.0.429.g432eaa2c6b-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [Bluez] Monitor: Remove handle before assigning
2024-01-26 6:41 [Bluez PATCH] Monitor: Remove handle before assigning Archie Pusaka
@ 2024-01-26 8:05 ` bluez.test.bot
2024-01-26 19:01 ` [Bluez PATCH] " Luiz Augusto von Dentz
1 sibling, 0 replies; 6+ messages in thread
From: bluez.test.bot @ 2024-01-26 8:05 UTC (permalink / raw)
To: linux-bluetooth, apusaka
[-- Attachment #1: Type: text/plain, Size: 1385 bytes --]
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=820136
---Test result---
Test Summary:
CheckPatch PASS 0.49 seconds
GitLint PASS 0.35 seconds
BuildEll PASS 23.96 seconds
BluezMake PASS 733.31 seconds
MakeCheck PASS 11.41 seconds
MakeDistcheck PASS 166.43 seconds
CheckValgrind PASS 225.96 seconds
CheckSmatch WARNING 328.90 seconds
bluezmakeextell PASS 106.99 seconds
IncrementalBuild PASS 660.88 seconds
ScanBuild PASS 939.16 seconds
Details
##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
monitor/packet.c: note: in included file:monitor/display.h:82:26: warning: Variable length array is used.monitor/packet.c:1862:26: warning: Variable length array is used.monitor/packet.c: note: in included file:monitor/bt.h:3606:52: warning: array of flexible structuresmonitor/bt.h:3594:40: warning: array of flexible structures
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Bluez PATCH] Monitor: Remove handle before assigning
2024-01-26 6:41 [Bluez PATCH] Monitor: Remove handle before assigning Archie Pusaka
2024-01-26 8:05 ` [Bluez] " bluez.test.bot
@ 2024-01-26 19:01 ` Luiz Augusto von Dentz
2024-01-28 12:17 ` Archie Pusaka
1 sibling, 1 reply; 6+ messages in thread
From: Luiz Augusto von Dentz @ 2024-01-26 19:01 UTC (permalink / raw)
To: Archie Pusaka
Cc: linux-bluetooth, Johan Hedberg, Marcel Holtmann,
CrosBT Upstreaming, Archie Pusaka, Zhengping Jiang
Hi Archie,
On Fri, Jan 26, 2024 at 1:42 AM Archie Pusaka <apusaka@google.com> wrote:
>
> From: Archie Pusaka <apusaka@chromium.org>
>
> It is possible to have some handles not removed, for example the host
> may decide not to wait for disconnection complete event when it is
> suspending. In this case, when the peer device reconnected, we might
> have two of the same handle assigned and create problem down the road.
>
> This patch solves the issue by always removing any previous handles
> when assigning a new handle if they are the same.
>
> Reviewed-by: Zhengping Jiang <jiangzp@google.com>
>
> ---
>
> monitor/packet.c | 50 +++++++++++++++++++++++++-----------------------
> 1 file changed, 26 insertions(+), 24 deletions(-)
>
> diff --git a/monitor/packet.c b/monitor/packet.c
> index 42e711cafa..b5dea6384a 100644
> --- a/monitor/packet.c
> +++ b/monitor/packet.c
> @@ -189,11 +189,33 @@ static struct packet_conn_data *lookup_parent(uint16_t handle)
> return NULL;
> }
>
> +static void release_handle(uint16_t handle)
> +{
> + int i;
> +
> + for (i = 0; i < MAX_CONN; i++) {
> + struct packet_conn_data *conn = &conn_list[i];
> +
> + if (conn->handle == handle) {
> + if (conn->destroy)
> + conn->destroy(conn->data);
> +
> + queue_destroy(conn->tx_q, free);
> + queue_destroy(conn->chan_q, free);
> + memset(conn, 0, sizeof(*conn));
> + conn->handle = 0xffff;
> + break;
> + }
> + }
> +}
We might as well return if we find out there is a packet_conn_data,
that way we don't need to do 2 lookups in a row.
> static void assign_handle(uint16_t index, uint16_t handle, uint8_t type,
> uint8_t *dst, uint8_t dst_type)
> {
> int i;
>
> + release_handle(handle);
> +
> for (i = 0; i < MAX_CONN; i++) {
> if (conn_list[i].handle == 0xffff) {
> hci_devba(index, (bdaddr_t *)conn_list[i].src);
> @@ -225,26 +247,6 @@ static void assign_handle(uint16_t index, uint16_t handle, uint8_t type,
> }
> }
>
> -static void release_handle(uint16_t handle)
> -{
> - int i;
> -
> - for (i = 0; i < MAX_CONN; i++) {
> - struct packet_conn_data *conn = &conn_list[i];
> -
> - if (conn->handle == handle) {
> - if (conn->destroy)
> - conn->destroy(conn->data);
> -
> - queue_destroy(conn->tx_q, free);
> - queue_destroy(conn->chan_q, free);
> - memset(conn, 0, sizeof(*conn));
> - conn->handle = 0xffff;
> - break;
> - }
> - }
> -}
> -
> struct packet_conn_data *packet_get_conn_data(uint16_t handle)
> {
> int i;
> @@ -10076,7 +10078,7 @@ static void conn_complete_evt(struct timeval *tv, uint16_t index,
> const struct bt_hci_evt_conn_complete *evt = data;
>
> print_status(evt->status);
> - print_handle(evt->handle);
> + print_field("Handle: %d", le16_to_cpu(evt->handle));
> print_bdaddr(evt->bdaddr);
> print_link_type(evt->link_type);
> print_enable("Encryption", evt->encr_mode);
> @@ -10648,7 +10650,7 @@ static void sync_conn_complete_evt(struct timeval *tv, uint16_t index,
> const struct bt_hci_evt_sync_conn_complete *evt = data;
>
> print_status(evt->status);
> - print_handle(evt->handle);
> + print_field("Handle: %d", le16_to_cpu(evt->handle));
> print_bdaddr(evt->bdaddr);
> print_link_type(evt->link_type);
> print_field("Transmission interval: 0x%2.2x", evt->tx_interval);
> @@ -11077,7 +11079,7 @@ static void le_conn_complete_evt(struct timeval *tv, uint16_t index,
> const struct bt_hci_evt_le_conn_complete *evt = data;
>
> print_status(evt->status);
> - print_handle(evt->handle);
> + print_field("Handle: %d", le16_to_cpu(evt->handle));
> print_role(evt->role);
> print_peer_addr_type("Peer address type", evt->peer_addr_type);
> print_addr("Peer address", evt->peer_addr, evt->peer_addr_type);
> @@ -11206,7 +11208,7 @@ static void le_enhanced_conn_complete_evt(struct timeval *tv, uint16_t index,
> const struct bt_hci_evt_le_enhanced_conn_complete *evt = data;
>
> print_status(evt->status);
> - print_handle(evt->handle);
> + print_field("Handle: %d", le16_to_cpu(evt->handle));
> print_role(evt->role);
> print_peer_addr_type("Peer address type", evt->peer_addr_type);
> print_addr("Peer address", evt->peer_addr, evt->peer_addr_type);
Are these changes really intentional? Or perhaps we don't want to
attempt to resolve the address since these are the event that would
assign them in the first place? In that case I had these fixed
separately with a proper patch description.
> --
> 2.43.0.429.g432eaa2c6b-goog
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Bluez PATCH] Monitor: Remove handle before assigning
2024-01-26 19:01 ` [Bluez PATCH] " Luiz Augusto von Dentz
@ 2024-01-28 12:17 ` Archie Pusaka
2024-01-29 18:56 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 6+ messages in thread
From: Archie Pusaka @ 2024-01-28 12:17 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: linux-bluetooth, Johan Hedberg, Marcel Holtmann,
CrosBT Upstreaming, Archie Pusaka, Zhengping Jiang
Hi Luiz,
On Sat, 27 Jan 2024 at 03:01, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Archie,
>
> On Fri, Jan 26, 2024 at 1:42 AM Archie Pusaka <apusaka@google.com> wrote:
> >
> > From: Archie Pusaka <apusaka@chromium.org>
> >
> > It is possible to have some handles not removed, for example the host
> > may decide not to wait for disconnection complete event when it is
> > suspending. In this case, when the peer device reconnected, we might
> > have two of the same handle assigned and create problem down the road.
> >
> > This patch solves the issue by always removing any previous handles
> > when assigning a new handle if they are the same.
> >
> > Reviewed-by: Zhengping Jiang <jiangzp@google.com>
> >
> > ---
> >
> > monitor/packet.c | 50 +++++++++++++++++++++++++-----------------------
> > 1 file changed, 26 insertions(+), 24 deletions(-)
> >
> > diff --git a/monitor/packet.c b/monitor/packet.c
> > index 42e711cafa..b5dea6384a 100644
> > --- a/monitor/packet.c
> > +++ b/monitor/packet.c
> > @@ -189,11 +189,33 @@ static struct packet_conn_data *lookup_parent(uint16_t handle)
> > return NULL;
> > }
> >
> > +static void release_handle(uint16_t handle)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < MAX_CONN; i++) {
> > + struct packet_conn_data *conn = &conn_list[i];
> > +
> > + if (conn->handle == handle) {
> > + if (conn->destroy)
> > + conn->destroy(conn->data);
> > +
> > + queue_destroy(conn->tx_q, free);
> > + queue_destroy(conn->chan_q, free);
> > + memset(conn, 0, sizeof(*conn));
> > + conn->handle = 0xffff;
> > + break;
> > + }
> > + }
> > +}
>
> We might as well return if we find out there is a packet_conn_data,
> that way we don't need to do 2 lookups in a row.
Do you mean to return the index, so we can immediately use the index
in the assign_handle function?
>
> > static void assign_handle(uint16_t index, uint16_t handle, uint8_t type,
> > uint8_t *dst, uint8_t dst_type)
> > {
> > int i;
> >
> > + release_handle(handle);
> > +
> > for (i = 0; i < MAX_CONN; i++) {
> > if (conn_list[i].handle == 0xffff) {
> > hci_devba(index, (bdaddr_t *)conn_list[i].src);
> > @@ -225,26 +247,6 @@ static void assign_handle(uint16_t index, uint16_t handle, uint8_t type,
> > }
> > }
> >
> > -static void release_handle(uint16_t handle)
> > -{
> > - int i;
> > -
> > - for (i = 0; i < MAX_CONN; i++) {
> > - struct packet_conn_data *conn = &conn_list[i];
> > -
> > - if (conn->handle == handle) {
> > - if (conn->destroy)
> > - conn->destroy(conn->data);
> > -
> > - queue_destroy(conn->tx_q, free);
> > - queue_destroy(conn->chan_q, free);
> > - memset(conn, 0, sizeof(*conn));
> > - conn->handle = 0xffff;
> > - break;
> > - }
> > - }
> > -}
> > -
> > struct packet_conn_data *packet_get_conn_data(uint16_t handle)
> > {
> > int i;
> > @@ -10076,7 +10078,7 @@ static void conn_complete_evt(struct timeval *tv, uint16_t index,
> > const struct bt_hci_evt_conn_complete *evt = data;
> >
> > print_status(evt->status);
> > - print_handle(evt->handle);
> > + print_field("Handle: %d", le16_to_cpu(evt->handle));
> > print_bdaddr(evt->bdaddr);
> > print_link_type(evt->link_type);
> > print_enable("Encryption", evt->encr_mode);
> > @@ -10648,7 +10650,7 @@ static void sync_conn_complete_evt(struct timeval *tv, uint16_t index,
> > const struct bt_hci_evt_sync_conn_complete *evt = data;
> >
> > print_status(evt->status);
> > - print_handle(evt->handle);
> > + print_field("Handle: %d", le16_to_cpu(evt->handle));
> > print_bdaddr(evt->bdaddr);
> > print_link_type(evt->link_type);
> > print_field("Transmission interval: 0x%2.2x", evt->tx_interval);
> > @@ -11077,7 +11079,7 @@ static void le_conn_complete_evt(struct timeval *tv, uint16_t index,
> > const struct bt_hci_evt_le_conn_complete *evt = data;
> >
> > print_status(evt->status);
> > - print_handle(evt->handle);
> > + print_field("Handle: %d", le16_to_cpu(evt->handle));
> > print_role(evt->role);
> > print_peer_addr_type("Peer address type", evt->peer_addr_type);
> > print_addr("Peer address", evt->peer_addr, evt->peer_addr_type);
> > @@ -11206,7 +11208,7 @@ static void le_enhanced_conn_complete_evt(struct timeval *tv, uint16_t index,
> > const struct bt_hci_evt_le_enhanced_conn_complete *evt = data;
> >
> > print_status(evt->status);
> > - print_handle(evt->handle);
> > + print_field("Handle: %d", le16_to_cpu(evt->handle));
> > print_role(evt->role);
> > print_peer_addr_type("Peer address type", evt->peer_addr_type);
> > print_addr("Peer address", evt->peer_addr, evt->peer_addr_type);
>
> Are these changes really intentional? Or perhaps we don't want to
> attempt to resolve the address since these are the event that would
> assign them in the first place? In that case I had these fixed
> separately with a proper patch description.
Yes, these are intentional. Otherwise, we would still print the
previous (faulty) address as the printing happens before we release
the handle.
Also, we print the (correct) address anyway immediately on the next line.
Do you still want this to be in a separate patch?
>
> > --
> > 2.43.0.429.g432eaa2c6b-goog
> >
>
>
> --
> Luiz Augusto von Dentz
Thanks,
Archie
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Bluez PATCH] Monitor: Remove handle before assigning
2024-01-28 12:17 ` Archie Pusaka
@ 2024-01-29 18:56 ` Luiz Augusto von Dentz
2024-01-30 10:27 ` Archie Pusaka
0 siblings, 1 reply; 6+ messages in thread
From: Luiz Augusto von Dentz @ 2024-01-29 18:56 UTC (permalink / raw)
To: Archie Pusaka
Cc: linux-bluetooth, Johan Hedberg, Marcel Holtmann,
CrosBT Upstreaming, Archie Pusaka, Zhengping Jiang
Hi Archie,
On Sun, Jan 28, 2024 at 7:18 AM Archie Pusaka <apusaka@google.com> wrote:
>
> Hi Luiz,
>
> On Sat, 27 Jan 2024 at 03:01, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Archie,
> >
> > On Fri, Jan 26, 2024 at 1:42 AM Archie Pusaka <apusaka@google.com> wrote:
> > >
> > > From: Archie Pusaka <apusaka@chromium.org>
> > >
> > > It is possible to have some handles not removed, for example the host
> > > may decide not to wait for disconnection complete event when it is
> > > suspending. In this case, when the peer device reconnected, we might
> > > have two of the same handle assigned and create problem down the road.
> > >
> > > This patch solves the issue by always removing any previous handles
> > > when assigning a new handle if they are the same.
> > >
> > > Reviewed-by: Zhengping Jiang <jiangzp@google.com>
> > >
> > > ---
> > >
> > > monitor/packet.c | 50 +++++++++++++++++++++++++-----------------------
> > > 1 file changed, 26 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/monitor/packet.c b/monitor/packet.c
> > > index 42e711cafa..b5dea6384a 100644
> > > --- a/monitor/packet.c
> > > +++ b/monitor/packet.c
> > > @@ -189,11 +189,33 @@ static struct packet_conn_data *lookup_parent(uint16_t handle)
> > > return NULL;
> > > }
> > >
> > > +static void release_handle(uint16_t handle)
> > > +{
> > > + int i;
> > > +
> > > + for (i = 0; i < MAX_CONN; i++) {
> > > + struct packet_conn_data *conn = &conn_list[i];
> > > +
> > > + if (conn->handle == handle) {
> > > + if (conn->destroy)
> > > + conn->destroy(conn->data);
> > > +
> > > + queue_destroy(conn->tx_q, free);
> > > + queue_destroy(conn->chan_q, free);
> > > + memset(conn, 0, sizeof(*conn));
> > > + conn->handle = 0xffff;
> > > + break;
> > > + }
> > > + }
> > > +}
> >
> > We might as well return if we find out there is a packet_conn_data,
> > that way we don't need to do 2 lookups in a row.
>
> Do you mean to return the index, so we can immediately use the index
> in the assign_handle function?
I think you can return the pointer directly:
conn = release_handle(handle);
if (conn)
hci_devba(index, conn->src);
...
> >
> > > static void assign_handle(uint16_t index, uint16_t handle, uint8_t type,
> > > uint8_t *dst, uint8_t dst_type)
> > > {
> > > int i;
> > >
> > > + release_handle(handle);
> > > +
> > > for (i = 0; i < MAX_CONN; i++) {
> > > if (conn_list[i].handle == 0xffff) {
> > > hci_devba(index, (bdaddr_t *)conn_list[i].src);
> > > @@ -225,26 +247,6 @@ static void assign_handle(uint16_t index, uint16_t handle, uint8_t type,
> > > }
> > > }
> > >
> > > -static void release_handle(uint16_t handle)
> > > -{
> > > - int i;
> > > -
> > > - for (i = 0; i < MAX_CONN; i++) {
> > > - struct packet_conn_data *conn = &conn_list[i];
> > > -
> > > - if (conn->handle == handle) {
> > > - if (conn->destroy)
> > > - conn->destroy(conn->data);
> > > -
> > > - queue_destroy(conn->tx_q, free);
> > > - queue_destroy(conn->chan_q, free);
> > > - memset(conn, 0, sizeof(*conn));
> > > - conn->handle = 0xffff;
> > > - break;
> > > - }
> > > - }
> > > -}
> > > -
> > > struct packet_conn_data *packet_get_conn_data(uint16_t handle)
> > > {
> > > int i;
> > > @@ -10076,7 +10078,7 @@ static void conn_complete_evt(struct timeval *tv, uint16_t index,
> > > const struct bt_hci_evt_conn_complete *evt = data;
> > >
> > > print_status(evt->status);
> > > - print_handle(evt->handle);
> > > + print_field("Handle: %d", le16_to_cpu(evt->handle));
> > > print_bdaddr(evt->bdaddr);
> > > print_link_type(evt->link_type);
> > > print_enable("Encryption", evt->encr_mode);
> > > @@ -10648,7 +10650,7 @@ static void sync_conn_complete_evt(struct timeval *tv, uint16_t index,
> > > const struct bt_hci_evt_sync_conn_complete *evt = data;
> > >
> > > print_status(evt->status);
> > > - print_handle(evt->handle);
> > > + print_field("Handle: %d", le16_to_cpu(evt->handle));
> > > print_bdaddr(evt->bdaddr);
> > > print_link_type(evt->link_type);
> > > print_field("Transmission interval: 0x%2.2x", evt->tx_interval);
> > > @@ -11077,7 +11079,7 @@ static void le_conn_complete_evt(struct timeval *tv, uint16_t index,
> > > const struct bt_hci_evt_le_conn_complete *evt = data;
> > >
> > > print_status(evt->status);
> > > - print_handle(evt->handle);
> > > + print_field("Handle: %d", le16_to_cpu(evt->handle));
> > > print_role(evt->role);
> > > print_peer_addr_type("Peer address type", evt->peer_addr_type);
> > > print_addr("Peer address", evt->peer_addr, evt->peer_addr_type);
> > > @@ -11206,7 +11208,7 @@ static void le_enhanced_conn_complete_evt(struct timeval *tv, uint16_t index,
> > > const struct bt_hci_evt_le_enhanced_conn_complete *evt = data;
> > >
> > > print_status(evt->status);
> > > - print_handle(evt->handle);
> > > + print_field("Handle: %d", le16_to_cpu(evt->handle));
> > > print_role(evt->role);
> > > print_peer_addr_type("Peer address type", evt->peer_addr_type);
> > > print_addr("Peer address", evt->peer_addr, evt->peer_addr_type);
> >
> > Are these changes really intentional? Or perhaps we don't want to
> > attempt to resolve the address since these are the event that would
> > assign them in the first place? In that case I had these fixed
> > separately with a proper patch description.
>
> Yes, these are intentional. Otherwise, we would still print the
> previous (faulty) address as the printing happens before we release
> the handle.
> Also, we print the (correct) address anyway immediately on the next line.
> Do you still want this to be in a separate patch?
Yes, please split these changes since they should be considered a different fix.
> >
> > > --
> > > 2.43.0.429.g432eaa2c6b-goog
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz
>
> Thanks,
> Archie
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Bluez PATCH] Monitor: Remove handle before assigning
2024-01-29 18:56 ` Luiz Augusto von Dentz
@ 2024-01-30 10:27 ` Archie Pusaka
0 siblings, 0 replies; 6+ messages in thread
From: Archie Pusaka @ 2024-01-30 10:27 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: linux-bluetooth, Johan Hedberg, Marcel Holtmann,
CrosBT Upstreaming, Archie Pusaka, Zhengping Jiang
Hi Luiz,
On Tue, 30 Jan 2024 at 02:57, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Archie,
>
> On Sun, Jan 28, 2024 at 7:18 AM Archie Pusaka <apusaka@google.com> wrote:
> >
> > Hi Luiz,
> >
> > On Sat, 27 Jan 2024 at 03:01, Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > Hi Archie,
> > >
> > > On Fri, Jan 26, 2024 at 1:42 AM Archie Pusaka <apusaka@google.com> wrote:
> > > >
> > > > From: Archie Pusaka <apusaka@chromium.org>
> > > >
> > > > It is possible to have some handles not removed, for example the host
> > > > may decide not to wait for disconnection complete event when it is
> > > > suspending. In this case, when the peer device reconnected, we might
> > > > have two of the same handle assigned and create problem down the road.
> > > >
> > > > This patch solves the issue by always removing any previous handles
> > > > when assigning a new handle if they are the same.
> > > >
> > > > Reviewed-by: Zhengping Jiang <jiangzp@google.com>
> > > >
> > > > ---
> > > >
> > > > monitor/packet.c | 50 +++++++++++++++++++++++++-----------------------
> > > > 1 file changed, 26 insertions(+), 24 deletions(-)
> > > >
> > > > diff --git a/monitor/packet.c b/monitor/packet.c
> > > > index 42e711cafa..b5dea6384a 100644
> > > > --- a/monitor/packet.c
> > > > +++ b/monitor/packet.c
> > > > @@ -189,11 +189,33 @@ static struct packet_conn_data *lookup_parent(uint16_t handle)
> > > > return NULL;
> > > > }
> > > >
> > > > +static void release_handle(uint16_t handle)
> > > > +{
> > > > + int i;
> > > > +
> > > > + for (i = 0; i < MAX_CONN; i++) {
> > > > + struct packet_conn_data *conn = &conn_list[i];
> > > > +
> > > > + if (conn->handle == handle) {
> > > > + if (conn->destroy)
> > > > + conn->destroy(conn->data);
> > > > +
> > > > + queue_destroy(conn->tx_q, free);
> > > > + queue_destroy(conn->chan_q, free);
> > > > + memset(conn, 0, sizeof(*conn));
> > > > + conn->handle = 0xffff;
> > > > + break;
> > > > + }
> > > > + }
> > > > +}
> > >
> > > We might as well return if we find out there is a packet_conn_data,
> > > that way we don't need to do 2 lookups in a row.
> >
> > Do you mean to return the index, so we can immediately use the index
> > in the assign_handle function?
>
> I think you can return the pointer directly:
>
>
> conn = release_handle(handle);
> if (conn)
> hci_devba(index, conn->src);
> ...
>
Sure, let's implement it this way.
>
> > >
> > > > static void assign_handle(uint16_t index, uint16_t handle, uint8_t type,
> > > > uint8_t *dst, uint8_t dst_type)
> > > > {
> > > > int i;
> > > >
> > > > + release_handle(handle);
> > > > +
> > > > for (i = 0; i < MAX_CONN; i++) {
> > > > if (conn_list[i].handle == 0xffff) {
> > > > hci_devba(index, (bdaddr_t *)conn_list[i].src);
> > > > @@ -225,26 +247,6 @@ static void assign_handle(uint16_t index, uint16_t handle, uint8_t type,
> > > > }
> > > > }
> > > >
> > > > -static void release_handle(uint16_t handle)
> > > > -{
> > > > - int i;
> > > > -
> > > > - for (i = 0; i < MAX_CONN; i++) {
> > > > - struct packet_conn_data *conn = &conn_list[i];
> > > > -
> > > > - if (conn->handle == handle) {
> > > > - if (conn->destroy)
> > > > - conn->destroy(conn->data);
> > > > -
> > > > - queue_destroy(conn->tx_q, free);
> > > > - queue_destroy(conn->chan_q, free);
> > > > - memset(conn, 0, sizeof(*conn));
> > > > - conn->handle = 0xffff;
> > > > - break;
> > > > - }
> > > > - }
> > > > -}
> > > > -
> > > > struct packet_conn_data *packet_get_conn_data(uint16_t handle)
> > > > {
> > > > int i;
> > > > @@ -10076,7 +10078,7 @@ static void conn_complete_evt(struct timeval *tv, uint16_t index,
> > > > const struct bt_hci_evt_conn_complete *evt = data;
> > > >
> > > > print_status(evt->status);
> > > > - print_handle(evt->handle);
> > > > + print_field("Handle: %d", le16_to_cpu(evt->handle));
> > > > print_bdaddr(evt->bdaddr);
> > > > print_link_type(evt->link_type);
> > > > print_enable("Encryption", evt->encr_mode);
> > > > @@ -10648,7 +10650,7 @@ static void sync_conn_complete_evt(struct timeval *tv, uint16_t index,
> > > > const struct bt_hci_evt_sync_conn_complete *evt = data;
> > > >
> > > > print_status(evt->status);
> > > > - print_handle(evt->handle);
> > > > + print_field("Handle: %d", le16_to_cpu(evt->handle));
> > > > print_bdaddr(evt->bdaddr);
> > > > print_link_type(evt->link_type);
> > > > print_field("Transmission interval: 0x%2.2x", evt->tx_interval);
> > > > @@ -11077,7 +11079,7 @@ static void le_conn_complete_evt(struct timeval *tv, uint16_t index,
> > > > const struct bt_hci_evt_le_conn_complete *evt = data;
> > > >
> > > > print_status(evt->status);
> > > > - print_handle(evt->handle);
> > > > + print_field("Handle: %d", le16_to_cpu(evt->handle));
> > > > print_role(evt->role);
> > > > print_peer_addr_type("Peer address type", evt->peer_addr_type);
> > > > print_addr("Peer address", evt->peer_addr, evt->peer_addr_type);
> > > > @@ -11206,7 +11208,7 @@ static void le_enhanced_conn_complete_evt(struct timeval *tv, uint16_t index,
> > > > const struct bt_hci_evt_le_enhanced_conn_complete *evt = data;
> > > >
> > > > print_status(evt->status);
> > > > - print_handle(evt->handle);
> > > > + print_field("Handle: %d", le16_to_cpu(evt->handle));
> > > > print_role(evt->role);
> > > > print_peer_addr_type("Peer address type", evt->peer_addr_type);
> > > > print_addr("Peer address", evt->peer_addr, evt->peer_addr_type);
> > >
> > > Are these changes really intentional? Or perhaps we don't want to
> > > attempt to resolve the address since these are the event that would
> > > assign them in the first place? In that case I had these fixed
> > > separately with a proper patch description.
> >
> > Yes, these are intentional. Otherwise, we would still print the
> > previous (faulty) address as the printing happens before we release
> > the handle.
> > Also, we print the (correct) address anyway immediately on the next line.
> > Do you still want this to be in a separate patch?
>
> Yes, please split these changes since they should be considered a different fix.
>
OK, I split them into two patches for v2.
> > >
> > > > --
> > > > 2.43.0.429.g432eaa2c6b-goog
> > > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
> >
> > Thanks,
> > Archie
>
>
>
> --
> Luiz Augusto von Dentz
Cheers,
Archie
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-01-30 10:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-26 6:41 [Bluez PATCH] Monitor: Remove handle before assigning Archie Pusaka
2024-01-26 8:05 ` [Bluez] " bluez.test.bot
2024-01-26 19:01 ` [Bluez PATCH] " Luiz Augusto von Dentz
2024-01-28 12:17 ` Archie Pusaka
2024-01-29 18:56 ` Luiz Augusto von Dentz
2024-01-30 10:27 ` Archie Pusaka
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).