| Message ID | 20220520183713.2641513-1-luiz.dentz@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [PATCH-stable] Bluetooth: hci_conn: Fix hci_connect_le_sync | expand |
Hello: This patch was applied to bluetooth/bluetooth-next.git (master) by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>: On Fri, 20 May 2022 11:37:13 -0700 you wrote: > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > The handling of connection failures shall be handled by the request > completion callback as already done by hci_cs_le_create_conn, also make > sure to use hci_conn_failed instead of hci_le_conn_failed as the later > don't actually call hci_conn_del to cleanup. > > [...] Here is the summary with links: - [PATCH-stable] Bluetooth: hci_conn: Fix hci_connect_le_sync https://git.kernel.org/bluetooth/bluetooth-next/c/c9f73a2178c1 You are awesome, thank you!
On 20.05.22 20:37, Luiz Augusto von Dentz wrote: > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > The handling of connection failures shall be handled by the request > completion callback as already done by hci_cs_le_create_conn, also make > sure to use hci_conn_failed instead of hci_le_conn_failed as the later > don't actually call hci_conn_del to cleanup. > > Link: https://github.com/bluez/bluez/issues/340 > Fixes: 8e8b92ee60de5 ("Bluetooth: hci_sync: Add hci_le_create_conn_sync") > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> A bit late, as I am not subscribed to linux-bluetooth and didn't notice this patch, but FWIW: Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de> Bluetooth: hci0: Opcode 0x200d failed: -110 Bluetooth: hci0: request failed to create LE connection: err -110 now, whereas before it crashed the kernel. Cheers, Ahmad > --- > net/bluetooth/hci_conn.c | 5 +++-- > net/bluetooth/hci_event.c | 8 +++++--- > 2 files changed, 8 insertions(+), 5 deletions(-) > > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c > index 882a7df13005..ac06c9724c7f 100644 > --- a/net/bluetooth/hci_conn.c > +++ b/net/bluetooth/hci_conn.c > @@ -943,10 +943,11 @@ static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err) > > bt_dev_err(hdev, "request failed to create LE connection: err %d", err); > > - if (!conn) > + /* Check if connection is still pending */ > + if (conn != hci_lookup_le_connect(hdev)) > goto done; > > - hci_le_conn_failed(conn, err); > + hci_conn_failed(conn, err); > > done: > hci_dev_unlock(hdev); > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c > index 0270e597c285..af17dfb20e01 100644 > --- a/net/bluetooth/hci_event.c > +++ b/net/bluetooth/hci_event.c > @@ -5632,10 +5632,12 @@ static void le_conn_complete_evt(struct hci_dev *hdev, u8 status, > status = HCI_ERROR_INVALID_PARAMETERS; > } > > - if (status) { > - hci_conn_failed(conn, status); > + /* All connection failure handling is taken care of by the > + * hci_conn_failed function which is triggered by the HCI > + * request completion callbacks used for connecting. > + */ > + if (status) > goto unlock; > - } > > if (conn->dst_type == ADDR_LE_DEV_PUBLIC) > addr_type = BDADDR_LE_PUBLIC;
Hello Luiz, On 24.05.22 16:48, Ahmad Fatoum wrote: > On 20.05.22 20:37, Luiz Augusto von Dentz wrote: >> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> >> >> The handling of connection failures shall be handled by the request >> completion callback as already done by hci_cs_le_create_conn, also make >> sure to use hci_conn_failed instead of hci_le_conn_failed as the later >> don't actually call hci_conn_del to cleanup. >> >> Link: https://github.com/bluez/bluez/issues/340 >> Fixes: 8e8b92ee60de5 ("Bluetooth: hci_sync: Add hci_le_create_conn_sync") >> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > A bit late, as I am not subscribed to linux-bluetooth and didn't notice this > patch, but FWIW: Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de> > > Bluetooth: hci0: Opcode 0x200d failed: -110 > Bluetooth: hci0: request failed to create LE connection: err -110 > > now, whereas before it crashed the kernel. I see now that this fix doesn't build for v5.17 because hci_conn_failed was only introduced in v5.18. Can the hci_conn.c hunk be safely dropped? Thanks, Ahmad > > Cheers, > Ahmad > >> --- >> net/bluetooth/hci_conn.c | 5 +++-- >> net/bluetooth/hci_event.c | 8 +++++--- >> 2 files changed, 8 insertions(+), 5 deletions(-) >> >> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c >> index 882a7df13005..ac06c9724c7f 100644 >> --- a/net/bluetooth/hci_conn.c >> +++ b/net/bluetooth/hci_conn.c >> @@ -943,10 +943,11 @@ static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err) >> >> bt_dev_err(hdev, "request failed to create LE connection: err %d", err); >> >> - if (!conn) >> + /* Check if connection is still pending */ >> + if (conn != hci_lookup_le_connect(hdev)) >> goto done; >> >> - hci_le_conn_failed(conn, err); >> + hci_conn_failed(conn, err); >> >> done: >> hci_dev_unlock(hdev); >> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c >> index 0270e597c285..af17dfb20e01 100644 >> --- a/net/bluetooth/hci_event.c >> +++ b/net/bluetooth/hci_event.c >> @@ -5632,10 +5632,12 @@ static void le_conn_complete_evt(struct hci_dev *hdev, u8 status, >> status = HCI_ERROR_INVALID_PARAMETERS; >> } >> >> - if (status) { >> - hci_conn_failed(conn, status); >> + /* All connection failure handling is taken care of by the >> + * hci_conn_failed function which is triggered by the HCI >> + * request completion callbacks used for connecting. >> + */ >> + if (status) >> goto unlock; >> - } >> >> if (conn->dst_type == ADDR_LE_DEV_PUBLIC) >> addr_type = BDADDR_LE_PUBLIC; > >
Hi Ahmad, On Tue, May 24, 2022 at 8:55 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote: > > Hello Luiz, > > On 24.05.22 16:48, Ahmad Fatoum wrote: > > On 20.05.22 20:37, Luiz Augusto von Dentz wrote: > >> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > >> > >> The handling of connection failures shall be handled by the request > >> completion callback as already done by hci_cs_le_create_conn, also make > >> sure to use hci_conn_failed instead of hci_le_conn_failed as the later > >> don't actually call hci_conn_del to cleanup. > >> > >> Link: https://github.com/bluez/bluez/issues/340 > >> Fixes: 8e8b92ee60de5 ("Bluetooth: hci_sync: Add hci_le_create_conn_sync") > >> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > > > A bit late, as I am not subscribed to linux-bluetooth and didn't notice this > > patch, but FWIW: Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de> > > > > Bluetooth: hci0: Opcode 0x200d failed: -110 > > Bluetooth: hci0: request failed to create LE connection: err -110 > > > > now, whereas before it crashed the kernel. > > I see now that this fix doesn't build for v5.17 because hci_conn_failed > was only introduced in v5.18. Can the hci_conn.c hunk be safely dropped? Are you talking about: if (status) { - hci_le_conn_failed(conn, status); + hci_conn_failed(conn, status); goto unlock; } You just need to replace hci_conn_failed with hci_le_conn_failed or well in the code above the end result is the same since it is not supposed to cleanup in the event handler. > Thanks, > Ahmad > > > > > Cheers, > > Ahmad > > > >> --- > >> net/bluetooth/hci_conn.c | 5 +++-- > >> net/bluetooth/hci_event.c | 8 +++++--- > >> 2 files changed, 8 insertions(+), 5 deletions(-) > >> > >> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c > >> index 882a7df13005..ac06c9724c7f 100644 > >> --- a/net/bluetooth/hci_conn.c > >> +++ b/net/bluetooth/hci_conn.c > >> @@ -943,10 +943,11 @@ static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err) > >> > >> bt_dev_err(hdev, "request failed to create LE connection: err %d", err); > >> > >> - if (!conn) > >> + /* Check if connection is still pending */ > >> + if (conn != hci_lookup_le_connect(hdev)) > >> goto done; > >> > >> - hci_le_conn_failed(conn, err); > >> + hci_conn_failed(conn, err); > >> > >> done: > >> hci_dev_unlock(hdev); > >> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c > >> index 0270e597c285..af17dfb20e01 100644 > >> --- a/net/bluetooth/hci_event.c > >> +++ b/net/bluetooth/hci_event.c > >> @@ -5632,10 +5632,12 @@ static void le_conn_complete_evt(struct hci_dev *hdev, u8 status, > >> status = HCI_ERROR_INVALID_PARAMETERS; > >> } > >> > >> - if (status) { > >> - hci_conn_failed(conn, status); > >> + /* All connection failure handling is taken care of by the > >> + * hci_conn_failed function which is triggered by the HCI > >> + * request completion callbacks used for connecting. > >> + */ > >> + if (status) > >> goto unlock; > >> - } > >> > >> if (conn->dst_type == ADDR_LE_DEV_PUBLIC) > >> addr_type = BDADDR_LE_PUBLIC; > > > > > > > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hello Luiz, On 24.05.22 20:08, Luiz Augusto von Dentz wrote: > On Tue, May 24, 2022 at 8:55 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote: >> On 24.05.22 16:48, Ahmad Fatoum wrote: >> I see now that this fix doesn't build for v5.17 because hci_conn_failed >> was only introduced in v5.18. Can the hci_conn.c hunk be safely dropped? > > Are you talking about: > > if (status) { > - hci_le_conn_failed(conn, status); > + hci_conn_failed(conn, status); > goto unlock; > } > > You just need to replace hci_conn_failed with hci_le_conn_failed or > well in the code above the end result is the same since it is not > supposed to cleanup in the event handler. Yes, that cleanup in le_conn_complete_evt() needs to be removed. I am talking about the other hunk in hci_conn.c: - if (!conn) + /* Check if connection is still pending */ + if (conn != hci_lookup_le_connect(hdev)) goto done; - hci_le_conn_failed(conn, err); + hci_conn_failed(conn, err); done: hci_dev_unlock(hdev); Can this be dropped for v5.17? Cheers, Ahmad
Hi Ahmad, On Tue, May 24, 2022 at 3:01 PM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote: > > Hello Luiz, > > On 24.05.22 20:08, Luiz Augusto von Dentz wrote: > > On Tue, May 24, 2022 at 8:55 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote: > >> On 24.05.22 16:48, Ahmad Fatoum wrote: > >> I see now that this fix doesn't build for v5.17 because hci_conn_failed > >> was only introduced in v5.18. Can the hci_conn.c hunk be safely dropped? > > > > Are you talking about: > > > > if (status) { > > - hci_le_conn_failed(conn, status); > > + hci_conn_failed(conn, status); > > goto unlock; > > } > > > > You just need to replace hci_conn_failed with hci_le_conn_failed or > > well in the code above the end result is the same since it is not > > supposed to cleanup in the event handler. > > Yes, that cleanup in le_conn_complete_evt() needs to be removed. > I am talking about the other hunk in hci_conn.c: > > - if (!conn) > + /* Check if connection is still pending */ > + if (conn != hci_lookup_le_connect(hdev)) > goto done; > > - hci_le_conn_failed(conn, err); > + hci_conn_failed(conn, err); > > done: > hci_dev_unlock(hdev); > > > Can this be dropped for v5.17? I guess it should be alright but perhaps keep if (conn != hci_lookup_le_connect(hdev)) just in case. > Cheers, > Ahmad > > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c index 882a7df13005..ac06c9724c7f 100644 --- a/net/bluetooth/hci_conn.c +++ b/net/bluetooth/hci_conn.c @@ -943,10 +943,11 @@ static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err) bt_dev_err(hdev, "request failed to create LE connection: err %d", err); - if (!conn) + /* Check if connection is still pending */ + if (conn != hci_lookup_le_connect(hdev)) goto done; - hci_le_conn_failed(conn, err); + hci_conn_failed(conn, err); done: hci_dev_unlock(hdev); diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index 0270e597c285..af17dfb20e01 100644 --- a/net/bluetooth/hci_event.c +++ b/net/bluetooth/hci_event.c @@ -5632,10 +5632,12 @@ static void le_conn_complete_evt(struct hci_dev *hdev, u8 status, status = HCI_ERROR_INVALID_PARAMETERS; } - if (status) { - hci_conn_failed(conn, status); + /* All connection failure handling is taken care of by the + * hci_conn_failed function which is triggered by the HCI + * request completion callbacks used for connecting. + */ + if (status) goto unlock; - } if (conn->dst_type == ADDR_LE_DEV_PUBLIC) addr_type = BDADDR_LE_PUBLIC;