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);
...
> >
> > > 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
