Return-Path: MIME-Version: 1.0 In-Reply-To: <7D190559-F7EA-4A3B-BF1F-EB53D1715049@holtmann.org> References: <1414607071-9801-1-git-send-email-jpawlowski@google.com> <7D190559-F7EA-4A3B-BF1F-EB53D1715049@holtmann.org> Date: Mon, 3 Nov 2014 09:52:15 -0800 Message-ID: Subject: Re: [PATCH v2 1/4] add restart_le_scan From: Jakub Pawlowski To: Marcel Holtmann Cc: BlueZ development Content-Type: multipart/alternative; boundary=001a113b39eaccdb560506f801cf List-ID: --001a113b39eaccdb560506f801cf Content-Type: text/plain; charset=UTF-8 On Sun, Nov 2, 2014 at 11:42 AM, Marcel Holtmann wrote: > Hi Jakub, > > the patch subject needs to start with Bluetooth: > I'll send fixed path today. > > > Currently there is no way to restart le scan. It's needed in > > preparation for new service scan method. The way it work: it disable, > > and then enable le scan on controller. During this restart special flag > > is set to make sure we won't remove disable scan work from workqueue. > > > > Signed-off-by: Jakub Pawlowski > > --- > > include/net/bluetooth/hci.h | 1 + > > include/net/bluetooth/hci_core.h | 2 ++ > > net/bluetooth/hci_core.c | 42 > ++++++++++++++++++++++++++++++++++++++++ > > net/bluetooth/hci_event.c | 9 ++++++--- > > 4 files changed, 51 insertions(+), 3 deletions(-) > > > > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h > > index 6e8f249..b15d240 100644 > > --- a/include/net/bluetooth/hci.h > > +++ b/include/net/bluetooth/hci.h > > @@ -194,6 +194,7 @@ enum { > > HCI_FAST_CONNECTABLE, > > HCI_BREDR_ENABLED, > > HCI_LE_SCAN_INTERRUPTED, > > + HCI_LE_SCAN_RESTARTING, > > }; > > > > /* A mask for the flags that are supposed to remain when a reset happens > > diff --git a/include/net/bluetooth/hci_core.h > b/include/net/bluetooth/hci_core.h > > index b8685a7..3c585885 100644 > > --- a/include/net/bluetooth/hci_core.h > > +++ b/include/net/bluetooth/hci_core.h > > @@ -334,6 +334,7 @@ struct hci_dev { > > unsigned long dev_flags; > > > > struct delayed_work le_scan_disable; > > + struct delayed_work le_scan_restart; > > So I wonder if we need le_scan_restart delayed_work here. We might be able > to just put that into le_scan_disable one using the HCI_LE_SCAN_RESTART > flag. > When you start regular scan or service scan, you schedule le_scan_disable to take place in around 5-10 seconds to finish scan (it depends on kind of scan, and it's DISCOV_LE_TIMEOUT, or hdev->discov_interleaved_timeout) During those 5-10 seconds we need to restart the scan multiple times. If we'll try to queue the same work again, that would not work because of the way queue_delayed_work works. We need to have a different delayed_work to achieve that, that's why I introduced le_scan_restart. Also it's possible to have situation when the scan is finishing, and we call le_scan_disable, but at the same time the HCI_LE_SCAN_RESTART was just set, and you get race condition that would cause the scan to be restarted instead of being disabled and run all the time. The way I wrote that right now you can't get that race condition. > That said, I like to get this working also with the regular discovery > procedure. It is just that there every found device will trigger the > restart, while in the service discovery it is only the ones we care about. > So we should expose an general mechanism here. > When I was initially implementing service scan, I was doing it inside regular scan, and le_scan_restart was working fine there (later I just extracted different opcode for the scan method). There's nothing special now in regular, or service scan that would make le_scan_restart not work. > > I do not have figured every single detail our here, but that is where I > like to go with this. > queue_delayed_work > > Regards > > Marcel > > --001a113b39eaccdb560506f801cf Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On Sun, Nov 2, 2014 at 11:42 AM, Marcel Holtmann <= marcel@holtmann.or= g> wrote:
Hi Jakub,

the patch subject needs to start with Bluetooth:
I'= ;ll send fixed path today.=C2=A0

> Currently there is no way to restart le scan. It's needed in
> preparation for new service scan method. The way it work: it disable,<= br> > and then enable le scan on controller. During this restart special fla= g
> is set to make sure we won't remove disable scan work from workque= ue.
>
> Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
> ---
> include/net/bluetooth/hci.h=C2=A0 =C2=A0 =C2=A0 |=C2=A0 1 +
> include/net/bluetooth/hci_core.h |=C2=A0 2 ++
> net/bluetooth/hci_core.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| 42 +++++++= +++++++++++++++++++++++++++++++++
> net/bluetooth/hci_event.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2=A0 9 ++++++-= --
> 4 files changed, 51 insertions(+), 3 deletions(-)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h=
> index 6e8f249..b15d240 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -194,6 +194,7 @@ enum {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0HCI_FAST_CONNECTABLE,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0HCI_BREDR_ENABLED,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0HCI_LE_SCAN_INTERRUPTED,
> +=C2=A0 =C2=A0 =C2=A0HCI_LE_SCAN_RESTARTING,
> };
>
> /* A mask for the flags that are supposed to remain when a reset happe= ns
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/= hci_core.h
> index b8685a7..3c585885 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -334,6 +334,7 @@ struct hci_dev {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0unsigned long=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0dev_flags;
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0struct delayed_work=C2=A0 =C2=A0 =C2=A0le_sc= an_disable;
> +=C2=A0 =C2=A0 =C2=A0struct delayed_work=C2=A0 =C2=A0 =C2=A0le_scan_re= start;

So I wonder if we need le_scan_restart delayed_work here. We mi= ght be able to just put that into le_scan_disable one using the HCI_LE_SCAN= _RESTART flag.

When you start regular s= can or service scan, you schedule le_scan_disable to take place in around 5= -10 seconds to finish scan (it depends on kind of scan, and it's=C2=A0D= ISCOV_LE_TIMEOUT, or=C2=A0hdev->discov_interleaved_timeout)
Du= ring those 5-10 seconds we need to restart the scan multiple times. If we&#= 39;ll try to queue the same work again, that would not work because of the = way=C2=A0queue_delayed_work works. We need to have a different delayed_work= to achieve that, that's why I introduced le_scan_restart.
Also it's possible to have situation when the scan is fini= shing, and we call le_scan_disable, but at the same time the HCI_LE_SCAN_RE= START was just set, and you get race condition that would cause the scan to= be restarted instead of being disabled and run all the time.
The= way I wrote that right now you can't get that race condition.
=C2=A0
That said, I like to get this working also with the regular discovery proce= dure. It is just that there every found device will trigger the restart, wh= ile in the service discovery it is only the ones we care about. So we shoul= d expose an general mechanism here.

Whe= n I was initially implementing service scan, I was doing it inside regular = scan, and=C2=A0le_scan_restart was working fine there (later I just extract= ed different opcode for the scan method). There's nothing special now i= n regular, or service scan that would make le_scan_restart not work.
<= div>=C2=A0

I do not have figured every single detail our here, but that is where I lik= e to go with this.

queue_delayed_work
=C2=A0

Regards

Marcel


--001a113b39eaccdb560506f801cf--