Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20220113141533.Bluez.1.Iad485a29772515142eefb1b120d5eb5e101d82f6@changeid>
 <2413B80E-E93D-49F1-849D-BAA899BAF832@holtmann.org>
In-Reply-To: <2413B80E-E93D-49F1-849D-BAA899BAF832@holtmann.org>
From:   Archie Pusaka <apusaka@google.com>
Date:   Thu, 13 Jan 2022 15:51:20 +0800
Message-ID: <CAJQfnxHEG0pmsucfs+VOp67RvzVO9g1i1oHjdePGaqmcwo+9nA@mail.gmail.com>
Subject: Re: [Bluez PATCH] device: Fix device can't be scanned for 5 mins
 after reboot
To:     Marcel Holtmann <marcel@holtmann.org>
Cc:     linux-bluetooth <linux-bluetooth@vger.kernel.org>,
        Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
        CrosBT Upstreaming <chromeos-bluetooth-upstreaming@chromium.org>,
        Shuo-Peng Liao <deanliao@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
>