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;
Message-ID: <71ac3f7a801192980142beb34d618770802d9a4c.camel@iki.fi>
Subject: Re: [PATCH] Bluetooth: hci_conn: Fix CIS connection dst_type
 handling
From:   Pauli Virtanen <pav@iki.fi>
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc:     linux-bluetooth@vger.kernel.org
Date:   Mon, 10 Oct 2022 19:08:23 +0000
In-Reply-To: <CABBYNZ+KzN_4w2x4en+fXP+SqwA5-WF_SNL_Q7dRmiCN1JQLNw@mail.gmail.com>
References: <20221009174216.17533-1-pav@iki.fi>
         <CABBYNZ+KzN_4w2x4en+fXP+SqwA5-WF_SNL_Q7dRmiCN1JQLNw@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
Precedence: bulk

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.

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