Hi Luiz,
On Tue, 30 Jan 2024 at 02:57, Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
>
> Hi Archie,
>
> On Sun, Jan 28, 2024 at 7:18 AM Archie Pusaka <apusaka@xxxxxxxxxx> wrote:
> >
> > Hi Luiz,
> >
> > On Sat, 27 Jan 2024 at 03:01, Luiz Augusto von Dentz
> > <luiz.dentz@xxxxxxxxx> wrote:
> > >
> > > Hi Archie,
> > >
> > > On Fri, Jan 26, 2024 at 1:42 AM Archie Pusaka <apusaka@xxxxxxxxxx> wrote:
> > > >
> > > > From: Archie Pusaka <apusaka@xxxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxx>
> > > >
> > > > ---
> > > >
> > > > 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
