| Message ID | 20220720163548.kernel.v1.1.I4058a198aa4979ee74a219fe6e315fad1184d78d@changeid |
|---|---|
| State | New |
| Headers | show |
| Series | Fix get clock info | expand |
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 <jiangzp@google.com> 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 <jiangzp@google.com> > --- > > 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 >
Hi Zhengping, On Thu, Jul 21, 2022 at 3:20 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Zhengping, > > On Wed, Jul 20, 2022 at 4:36 PM Zhengping Jiang <jiangzp@google.com> 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 <jiangzp@google.com> > > --- > > > > 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
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 <luiz.dentz@gmail.com> wrote: > > Hi Zhengping, > > On Thu, Jul 21, 2022 at 3:20 PM Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: > > > > Hi Zhengping, > > > > On Wed, Jul 20, 2022 at 4:36 PM Zhengping Jiang <jiangzp@google.com> 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 <jiangzp@google.com> > > > --- > > > > > > 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
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;
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 <jiangzp@google.com> --- Changes in v1: - Fix input connection data net/bluetooth/mgmt.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)