If connection exists, set the connection data before calling
get_clock_info_sync, so it can be verified the connection is still
connected, before retrieving clock info.
Signed-off-by: Zhengping Jiang <[email protected]>
---
Changes in v1:
- Fix input connection data
net/bluetooth/mgmt.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index ef8371975c4eb..947d700574c54 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -6971,11 +6971,16 @@ static int get_clock_info(struct sock *sk, struct hci_dev *hdev, void *data,
}
cmd = mgmt_pending_new(sk, MGMT_OP_GET_CLOCK_INFO, hdev, data, len);
- if (!cmd)
+ if (!cmd) {
err = -ENOMEM;
- else
+ } else {
+ if (conn) {
+ hci_conn_hold(conn);
+ cmd->user_data = hci_conn_get(conn);
+ }
err = hci_cmd_sync_queue(hdev, get_clock_info_sync, cmd,
get_clock_info_complete);
+ }
if (err < 0) {
err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_GET_CLOCK_INFO,
@@ -6984,12 +6989,8 @@ static int get_clock_info(struct sock *sk, struct hci_dev *hdev, void *data,
if (cmd)
mgmt_pending_free(cmd);
- } else if (conn) {
- hci_conn_hold(conn);
- cmd->user_data = hci_conn_get(conn);
}
-
unlock:
hci_dev_unlock(hdev);
return err;
--
2.37.0.170.g444d1eabd0-goog
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=661636
---Test result---
Test Summary:
CheckPatch PASS 1.88 seconds
GitLint PASS 1.06 seconds
SubjectPrefix PASS 0.90 seconds
BuildKernel PASS 35.95 seconds
BuildKernel32 PASS 30.80 seconds
Incremental Build with patchesPASS 42.99 seconds
TestRunner: Setup PASS 517.95 seconds
TestRunner: l2cap-tester PASS 17.38 seconds
TestRunner: bnep-tester PASS 6.03 seconds
TestRunner: mgmt-tester PASS 98.29 seconds
TestRunner: rfcomm-tester PASS 9.52 seconds
TestRunner: sco-tester PASS 9.18 seconds
TestRunner: smp-tester PASS 9.26 seconds
TestRunner: userchan-tester PASS 6.15 seconds
---
Regards,
Linux Bluetooth
Hi Zhengping,
On Wed, Jul 20, 2022 at 4:36 PM Zhengping Jiang <[email protected]> wrote:
>
> If connection exists, set the connection data before calling
> get_clock_info_sync, so it can be verified the connection is still
> connected, before retrieving clock info.
>
> Signed-off-by: Zhengping Jiang <[email protected]>
> ---
>
> Changes in v1:
> - Fix input connection data
>
> net/bluetooth/mgmt.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index ef8371975c4eb..947d700574c54 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -6971,11 +6971,16 @@ static int get_clock_info(struct sock *sk, struct hci_dev *hdev, void *data,
> }
>
> cmd = mgmt_pending_new(sk, MGMT_OP_GET_CLOCK_INFO, hdev, data, len);
> - if (!cmd)
> + if (!cmd) {
> err = -ENOMEM;
> - else
> + } else {
> + if (conn) {
> + hci_conn_hold(conn);
> + cmd->user_data = hci_conn_get(conn);
> + }
> err = hci_cmd_sync_queue(hdev, get_clock_info_sync, cmd,
> get_clock_info_complete);
> + }
Having seconds though if this is actually the right thing to do,
hci_cmd_sync_queue will queue the command so get_clock_info_sync
shouldn't execute immediately if that happens that actually would be
quite a problem since we are holding the hci_dev_lock so if the
callback executes and blocks waiting a command that would likely cause
a deadlock because no command can be completed while hci_dev_lock is
being held, in fact I don't really like the idea of holding hci_conn
reference either since we are doing a lookup by address on
get_clock_info_sync Id probably just remove this code as it seem
unnecessary.
Btw, there are tests for this command in mgmt-tester so if this is
actually attempting to fix a problem Id like to have a test to
reproduce it.
> if (err < 0) {
> err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_GET_CLOCK_INFO,
> @@ -6984,12 +6989,8 @@ static int get_clock_info(struct sock *sk, struct hci_dev *hdev, void *data,
> if (cmd)
> mgmt_pending_free(cmd);
>
> - } else if (conn) {
> - hci_conn_hold(conn);
> - cmd->user_data = hci_conn_get(conn);
> }
>
> -
> unlock:
> hci_dev_unlock(hdev);
> return err;
> --
> 2.37.0.170.g444d1eabd0-goog
>
--
Luiz Augusto von Dentz
Hi Zhengping,
On Thu, Jul 21, 2022 at 3:20 PM Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Zhengping,
>
> On Wed, Jul 20, 2022 at 4:36 PM Zhengping Jiang <[email protected]> wrote:
> >
> > If connection exists, set the connection data before calling
> > get_clock_info_sync, so it can be verified the connection is still
> > connected, before retrieving clock info.
> >
> > Signed-off-by: Zhengping Jiang <[email protected]>
> > ---
> >
> > Changes in v1:
> > - Fix input connection data
> >
> > net/bluetooth/mgmt.c | 13 +++++++------
> > 1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> > index ef8371975c4eb..947d700574c54 100644
> > --- a/net/bluetooth/mgmt.c
> > +++ b/net/bluetooth/mgmt.c
> > @@ -6971,11 +6971,16 @@ static int get_clock_info(struct sock *sk, struct hci_dev *hdev, void *data,
> > }
> >
> > cmd = mgmt_pending_new(sk, MGMT_OP_GET_CLOCK_INFO, hdev, data, len);
> > - if (!cmd)
> > + if (!cmd) {
> > err = -ENOMEM;
> > - else
> > + } else {
> > + if (conn) {
> > + hci_conn_hold(conn);
> > + cmd->user_data = hci_conn_get(conn);
> > + }
> > err = hci_cmd_sync_queue(hdev, get_clock_info_sync, cmd,
> > get_clock_info_complete);
> > + }
>
> Having seconds though if this is actually the right thing to do,
> hci_cmd_sync_queue will queue the command so get_clock_info_sync
> shouldn't execute immediately if that happens that actually would be
> quite a problem since we are holding the hci_dev_lock so if the
> callback executes and blocks waiting a command that would likely cause
> a deadlock because no command can be completed while hci_dev_lock is
> being held, in fact I don't really like the idea of holding hci_conn
> reference either since we are doing a lookup by address on
> get_clock_info_sync Id probably just remove this code as it seem
> unnecessary.
>
> Btw, there are tests for this command in mgmt-tester so if this is
> actually attempting to fix a problem Id like to have a test to
> reproduce it.
Looks at the other change you submitted that has a similar code
pattern I did the following change:
https://gist.github.com/Vudentz/91cd57373d5b0116e2c34b6fb6adaa07
And mgmt-tester seems to run just fine with these changes.
>
> > if (err < 0) {
> > err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_GET_CLOCK_INFO,
> > @@ -6984,12 +6989,8 @@ static int get_clock_info(struct sock *sk, struct hci_dev *hdev, void *data,
> > if (cmd)
> > mgmt_pending_free(cmd);
> >
> > - } else if (conn) {
> > - hci_conn_hold(conn);
> > - cmd->user_data = hci_conn_get(conn);
> > }
> >
> > -
> > unlock:
> > hci_dev_unlock(hdev);
> > return err;
> > --
> > 2.37.0.170.g444d1eabd0-goog
> >
>
>
> --
> Luiz Augusto von Dentz
--
Luiz Augusto von Dentz
Hi Luiz,
Thanks for the rework. This patch looks good. The function to get
connection info was causing test regression in some hardware
platforms, but not always. We don't currently have a test for getting
clock info here. I was submitting the patch because it is using the
same pattern as getting conn info.
I tested the new patch and it is working well, so we can abandon my patch.
Best,
Zhengping
On Thu, Jul 21, 2022 at 3:41 PM Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Zhengping,
>
> On Thu, Jul 21, 2022 at 3:20 PM Luiz Augusto von Dentz
> <[email protected]> wrote:
> >
> > Hi Zhengping,
> >
> > On Wed, Jul 20, 2022 at 4:36 PM Zhengping Jiang <[email protected]> wrote:
> > >
> > > If connection exists, set the connection data before calling
> > > get_clock_info_sync, so it can be verified the connection is still
> > > connected, before retrieving clock info.
> > >
> > > Signed-off-by: Zhengping Jiang <[email protected]>
> > > ---
> > >
> > > Changes in v1:
> > > - Fix input connection data
> > >
> > > net/bluetooth/mgmt.c | 13 +++++++------
> > > 1 file changed, 7 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> > > index ef8371975c4eb..947d700574c54 100644
> > > --- a/net/bluetooth/mgmt.c
> > > +++ b/net/bluetooth/mgmt.c
> > > @@ -6971,11 +6971,16 @@ static int get_clock_info(struct sock *sk, struct hci_dev *hdev, void *data,
> > > }
> > >
> > > cmd = mgmt_pending_new(sk, MGMT_OP_GET_CLOCK_INFO, hdev, data, len);
> > > - if (!cmd)
> > > + if (!cmd) {
> > > err = -ENOMEM;
> > > - else
> > > + } else {
> > > + if (conn) {
> > > + hci_conn_hold(conn);
> > > + cmd->user_data = hci_conn_get(conn);
> > > + }
> > > err = hci_cmd_sync_queue(hdev, get_clock_info_sync, cmd,
> > > get_clock_info_complete);
> > > + }
> >
> > Having seconds though if this is actually the right thing to do,
> > hci_cmd_sync_queue will queue the command so get_clock_info_sync
> > shouldn't execute immediately if that happens that actually would be
> > quite a problem since we are holding the hci_dev_lock so if the
> > callback executes and blocks waiting a command that would likely cause
> > a deadlock because no command can be completed while hci_dev_lock is
> > being held, in fact I don't really like the idea of holding hci_conn
> > reference either since we are doing a lookup by address on
> > get_clock_info_sync Id probably just remove this code as it seem
> > unnecessary.
> >
> > Btw, there are tests for this command in mgmt-tester so if this is
> > actually attempting to fix a problem Id like to have a test to
> > reproduce it.
>
> Looks at the other change you submitted that has a similar code
> pattern I did the following change:
>
> https://gist.github.com/Vudentz/91cd57373d5b0116e2c34b6fb6adaa07
>
> And mgmt-tester seems to run just fine with these changes.
>
> >
> > > if (err < 0) {
> > > err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_GET_CLOCK_INFO,
> > > @@ -6984,12 +6989,8 @@ static int get_clock_info(struct sock *sk, struct hci_dev *hdev, void *data,
> > > if (cmd)
> > > mgmt_pending_free(cmd);
> > >
> > > - } else if (conn) {
> > > - hci_conn_hold(conn);
> > > - cmd->user_data = hci_conn_get(conn);
> > > }
> > >
> > > -
> > > unlock:
> > > hci_dev_unlock(hdev);
> > > return err;
> > > --
> > > 2.37.0.170.g444d1eabd0-goog
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz