diff mbox series

[Bluez] device: Fix device can't be scanned for 5 mins after reboot

Message ID 20220113141533.Bluez.1.Iad485a29772515142eefb1b120d5eb5e101d82f6@changeid (mailing list archive)
State Superseded
Headers show
Series [Bluez] device: Fix device can't be scanned for 5 mins after reboot | expand

Checks

Context Check Description
tedd_an/checkpatch success Checkpatch PASS
tedd_an/gitlint success Gitlint PASS
tedd_an/setupell success Setup ELL PASS
tedd_an/buildprep success Build Prep PASS
tedd_an/build success Build Configuration PASS
tedd_an/makecheck success Make Check PASS
tedd_an/makecheckvalgrind success Make Check PASS
tedd_an/makedistcheck success Make Distcheck PASS
tedd_an/build_extell success Build External ELL PASS
tedd_an/build_extell_make success Build Make with External ELL PASS

Commit Message

Archie Pusaka Jan. 13, 2022, 6:16 a.m. UTC 
From: Archie Pusaka <apusaka@chromium.org>

After the patches which limit the attempts of doing remote name
resolving, there's an issue which prevents BlueZ to RNR new devices
for 5 minutes after reboot. It's caused by failed_time is init to 0,
and is then treated as the timestamp when the device failed RNR.
However, actually there is no failure yet.

This CL fixes it by always allowing RNR when failed_time = 0.

Reviewed-by: Shuo-Peng Liao <deanliao@chromium.org>
---

 src/device.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Comments

Marcel Holtmann Jan. 13, 2022, 7:05 a.m. UTC | #1 
Hi Archie,

> After the patches which limit the attempts of doing remote name
> resolving, there's an issue which prevents BlueZ to RNR new devices
> for 5 minutes after reboot. It's caused by failed_time is init to 0,
> and is then treated as the timestamp when the device failed RNR.
> However, actually there is no failure yet.
> 
> This CL fixes it by always allowing RNR when failed_time = 0.
> 
> Reviewed-by: Shuo-Peng Liao <deanliao@chromium.org>
> ---
> 
> src/device.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/src/device.c b/src/device.c
> index f2447c478e..16b1ed5bd3 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -4487,10 +4487,15 @@ bool device_is_name_resolve_allowed(struct btd_device *device)
> 
> 	clock_gettime(CLOCK_MONOTONIC, &now);
> 
> -	/* If now < failed_time, it means the clock has somehow turned back,
> -	 * possibly because of system restart. Allow name request in this case.
> +	/* Allow name request for any of these cases:
> +	 * (1) failed_time is empty (0). Meaning no prior failure.
> +	 * (2) now < failed_time. Meaning the clock has somehow turned back,
> +	 *     possibly because of system restart. Allow just to be safe.
> +	 * (3) now >= failed_time + name_request_retry_delay. Meaning the
> +	 *     period of not sending name request is over.
> 	 */
> -	return now.tv_sec < device->name_resolve_failed_time ||
> +	return device->name_resolve_failed_time == 0 ||
> +		now.tv_sec < device->name_resolve_failed_time ||
> 		now.tv_sec >= device->name_resolve_failed_time +
> 					btd_opts.name_request_retry_delay;
> }

I have the feeling this becomes complex and hard to follow. I appreciate the comment above, but just for thoughts, would it be better to do something like this:

	/* failed_time is empty (0). Meaning no prior failure. */
	if (device->name_..failed_time == 0)
		return true;

	..

	return false;

Regards

Marcel
bluez.test.bot@gmail.com Jan. 13, 2022, 7:31 a.m. UTC | #2 
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=605086

---Test result---

Test Summary:
CheckPatch                    PASS      1.37 seconds
GitLint                       PASS      1.00 seconds
Prep - Setup ELL              PASS      42.67 seconds
Build - Prep                  PASS      0.67 seconds
Build - Configure             PASS      8.57 seconds
Build - Make                  PASS      1416.24 seconds
Make Check                    PASS      12.01 seconds
Make Check w/Valgrind         PASS      446.97 seconds
Make Distcheck                PASS      219.77 seconds
Build w/ext ELL - Configure   PASS      8.66 seconds
Build w/ext ELL - Make        PASS      1400.11 seconds
Incremental Build with patchesPASS      0.00 seconds



---
Regards,
Linux Bluetooth
Archie Pusaka Jan. 13, 2022, 7:51 a.m. UTC | #3 
Hi Marcel,

On Thu, 13 Jan 2022 at 15:05, Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Archie,
>
> > After the patches which limit the attempts of doing remote name
> > resolving, there's an issue which prevents BlueZ to RNR new devices
> > for 5 minutes after reboot. It's caused by failed_time is init to 0,
> > and is then treated as the timestamp when the device failed RNR.
> > However, actually there is no failure yet.
> >
> > This CL fixes it by always allowing RNR when failed_time = 0.
> >
> > Reviewed-by: Shuo-Peng Liao <deanliao@chromium.org>
> > ---
> >
> > src/device.c | 11 ++++++++---
> > 1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/device.c b/src/device.c
> > index f2447c478e..16b1ed5bd3 100644
> > --- a/src/device.c
> > +++ b/src/device.c
> > @@ -4487,10 +4487,15 @@ bool device_is_name_resolve_allowed(struct btd_device *device)
> >
> >       clock_gettime(CLOCK_MONOTONIC, &now);
> >
> > -     /* If now < failed_time, it means the clock has somehow turned back,
> > -      * possibly because of system restart. Allow name request in this case.
> > +     /* Allow name request for any of these cases:
> > +      * (1) failed_time is empty (0). Meaning no prior failure.
> > +      * (2) now < failed_time. Meaning the clock has somehow turned back,
> > +      *     possibly because of system restart. Allow just to be safe.
> > +      * (3) now >= failed_time + name_request_retry_delay. Meaning the
> > +      *     period of not sending name request is over.
> >        */
> > -     return now.tv_sec < device->name_resolve_failed_time ||
> > +     return device->name_resolve_failed_time == 0 ||
> > +             now.tv_sec < device->name_resolve_failed_time ||
> >               now.tv_sec >= device->name_resolve_failed_time +
> >                                       btd_opts.name_request_retry_delay;
> > }
>
> I have the feeling this becomes complex and hard to follow. I appreciate the comment above, but just for thoughts, would it be better to do something like this:
>
>         /* failed_time is empty (0). Meaning no prior failure. */
>         if (device->name_..failed_time == 0)
>                 return true;
>
>         ..
>
>         return false;
>

I actually prefer your suggestion, this makes the code much easier to read.
However it's not a very common pattern I observe so I went with the
previous approach.

I have sent a v2 to separate the big if, please take a look.

Thanks,
Archie

> Regards
>
> Marcel
>
diff mbox series

Patch

diff --git a/src/device.c b/src/device.c
index f2447c478e..16b1ed5bd3 100644
--- a/src/device.c
+++ b/src/device.c
@@ -4487,10 +4487,15 @@  bool device_is_name_resolve_allowed(struct btd_device *device)
 
 	clock_gettime(CLOCK_MONOTONIC, &now);
 
-	/* If now < failed_time, it means the clock has somehow turned back,
-	 * possibly because of system restart. Allow name request in this case.
+	/* Allow name request for any of these cases:
+	 * (1) failed_time is empty (0). Meaning no prior failure.
+	 * (2) now < failed_time. Meaning the clock has somehow turned back,
+	 *     possibly because of system restart. Allow just to be safe.
+	 * (3) now >= failed_time + name_request_retry_delay. Meaning the
+	 *     period of not sending name request is over.
 	 */
-	return now.tv_sec < device->name_resolve_failed_time ||
+	return device->name_resolve_failed_time == 0 ||
+		now.tv_sec < device->name_resolve_failed_time ||
 		now.tv_sec >= device->name_resolve_failed_time +
 					btd_opts.name_request_retry_delay;
 }