Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221009174216.17533-1-pav@iki.fi> <CABBYNZ+KzN_4w2x4en+fXP+SqwA5-WF_SNL_Q7dRmiCN1JQLNw@mail.gmail.com>
 <71ac3f7a801192980142beb34d618770802d9a4c.camel@iki.fi>
In-Reply-To: <71ac3f7a801192980142beb34d618770802d9a4c.camel@iki.fi>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 10 Oct 2022 12:25:59 -0700
Message-ID: <CABBYNZLCN+SCw4N6T=ytgk4w48PFDJot528o26D5feBm1tD+4Q@mail.gmail.com>
Subject: Re: [PATCH] Bluetooth: hci_conn: Fix CIS connection dst_type handling
To:     Pauli Virtanen <pav@iki.fi>
Cc:     linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Pauli,

On Mon, Oct 10, 2022 at 12:08 PM Pauli Virtanen <pav@iki.fi> wrote:
>
> Hi Luiz,
>
> su, 2022-10-09 kello 14:45 -0700, Luiz Augusto von Dentz kirjoitti:
> [clip]
> > > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > > index 9777e7b109ee..78d8b8b7fd72 100644
> > > --- a/net/bluetooth/hci_conn.c
> > > +++ b/net/bluetooth/hci_conn.c
> > > @@ -1691,12 +1691,19 @@ struct hci_conn *hci_bind_cis(struct hci_dev *hdev, bdaddr_t *dst,
> > >  {
> > >         struct hci_conn *cis;
> > >
> > > +       /* Convert from ISO socket address type to HCI address type  */
> > > +       if (dst_type == BDADDR_LE_PUBLIC)
> > > +               dst_type = ADDR_LE_DEV_PUBLIC;
> > > +       else
> > > +               dst_type = ADDR_LE_DEV_RANDOM;
> > > +
> > >         cis = hci_conn_hash_lookup_cis(hdev, dst, dst_type);
> > >         if (!cis) {
> > >                 cis = hci_conn_add(hdev, ISO_LINK, dst, HCI_ROLE_MASTER);
> > >                 if (!cis)
> > >                         return ERR_PTR(-ENOMEM);
> > >                 cis->cleanup = cis_cleanup;
> > > +               cis->dst_type = dst_type;
> > >         }
> > >
> > >         if (cis->state == BT_CONNECTED)
> > > @@ -2075,20 +2082,21 @@ struct hci_conn *hci_connect_cis(struct hci_dev *hdev, bdaddr_t *dst,
> > >  {
> > >         struct hci_conn *le;
> > >         struct hci_conn *cis;
> > > +       u8 hci_dst_type;
> > >
> > >         /* Convert from ISO socket address type to HCI address type  */
> > >         if (dst_type == BDADDR_LE_PUBLIC)
> > > -               dst_type = ADDR_LE_DEV_PUBLIC;
> > > +               hci_dst_type = ADDR_LE_DEV_PUBLIC;
> > >         else
> > > -               dst_type = ADDR_LE_DEV_RANDOM;
> > > +               hci_dst_type = ADDR_LE_DEV_RANDOM;
> >
> > Nice catch, though I think we should make sure these types are not
> > from hci_conn.c as the name suggest these should be dealing HCI
> > procedures so it doesn't make much sense to propagate types other than
> > HCI.
>
> Not sure I parse right: You want to move the conversions to the
> callsite in iso.c? Or change the variable name here? If the former,
> there are a few other instances of these in hci_conn.c.

Lets move to iso.c and make the conversion before the calls into hci_conn.c.

> > >         if (hci_dev_test_flag(hdev, HCI_ADVERTISING))
> > > -               le = hci_connect_le(hdev, dst, dst_type, false,
> > > +               le = hci_connect_le(hdev, dst, hci_dst_type, false,
> > >                                     BT_SECURITY_LOW,
> > >                                     HCI_LE_CONN_TIMEOUT,
> > >                                     HCI_ROLE_SLAVE);
> > >         else
> > > -               le = hci_connect_le_scan(hdev, dst, dst_type,
> > > +               le = hci_connect_le_scan(hdev, dst, hci_dst_type,
> > >                                          BT_SECURITY_LOW,
> > >                                          HCI_LE_CONN_TIMEOUT,
> > >                                          CONN_REASON_ISO_CONNECT);
> > > --
> > > 2.37.3
> > >
> >
> > While at it probably makes sense to introduce a test to iso-tester
> > that uses random address rather than always using public, that way we
> > can make sure we exercise this code with CI.


-- 
Luiz Augusto von Dentz