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: <20220720233601.3648471-1-jiangzp@google.com> <20220720163548.kernel.v1.1.I4058a198aa4979ee74a219fe6e315fad1184d78d@changeid>
 <CABBYNZ+ao6r4ZHaVNGRe-Wp6077R2xmkYufOK7tN=B8rEJL1yg@mail.gmail.com>
In-Reply-To: <CABBYNZ+ao6r4ZHaVNGRe-Wp6077R2xmkYufOK7tN=B8rEJL1yg@mail.gmail.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Thu, 21 Jul 2022 15:40:56 -0700
Message-ID: <CABBYNZKhB82i12981ediTXgtOoepE9OjwK0UJQFY2wyDELKUbg@mail.gmail.com>
Subject: Re: [kernel PATCH v1 1/1] Bluetooth: Fix get clock info
To:     Zhengping Jiang <jiangzp@google.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        Marcel Holtmann <marcel@holtmann.org>,
        "Gix, Brian" <brian.gix@intel.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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