linux-kernel - Re: [PATCH 1/8] staging: vchiq_arm: replace sleep() with usleep

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAK8P3a3HGm1cPo4sW9fOY4E8AN8yAq3tevXxU5m8bmtmsU8WKw@mail.gmail.com>
Date:   Wed, 15 Sep 2021 16:19:43 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Phil Elwell <phil@...pberrypi.com>
Cc:     Stefan Wahren <stefan.wahren@...e.com>,
        Gaston Gonzalez <gascoar@...il.com>,
        linux-staging@...ts.linux.dev, gregkh <gregkh@...uxfoundation.org>,
        Nicolas Saenz Julienne <nsaenz@...nel.org>,
        Arnd Bergmann <arnd@...db.de>,
        Dan Carpenter <dan.carpenter@...cle.com>, ojaswin98@...il.com,
        Amarjargal Gundjalam <amarjargal16@...il.com>,
        "moderated list:BROADCOM BCM2835 ARM ARCHITECTURE" 
        <linux-rpi-kernel@...ts.infradead.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        bcm-kernel-feedback-list <bcm-kernel-feedback-list@...adcom.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/8] staging: vchiq_arm: replace sleep() with usleep_range()

On Wed, Sep 15, 2021 at 10:06 AM Phil Elwell <phil@...pberrypi.com> wrote:
>
> Hi Stefan,
>
> On 15/09/2021 06:21, Stefan Wahren wrote:
> > Hi,
> >
> > Am 14.09.21 um 23:35 schrieb Gaston Gonzalez:
> >> usleep_range() should be used instead of sleep() when sleepings range
> >> from 10 us to 20 ms, [1].
> >>
> >> Reported by checkpatch.pl
> >>
> >> [1] Documentation/timers/timers-howto.txt
> >> ---
> >>   drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> >> index b25369a13452..0214ae37e01f 100644
> >> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> >> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> >> @@ -824,7 +824,7 @@ vchiq_bulk_transmit(unsigned int handle, const void *data, unsigned int size,
> >>              if (status != VCHIQ_RETRY)
> >>                      break;
> >>
> >> -            msleep(1);
> >> +            usleep_range(1000, 1100);
> >
> > from my understanding the usage of usleep_range() and hrtimers isn't
> > necessary here. The intention is to sleep a little bit and not "exactly"
> > 1 ms.
> >
> > @Phil Elwell: what is your opinion?
>
> Exactly - the aim is just to stop it spinning.

This is usually a sign for something else being wrong in the thing it's
waiting for. I can see multiple 'return VCHIQ_RETRY' statements in
vchiq_bulk_transfer(), however these all happen when the task
has received a signal and wait_for_completion_interruptible()
or mutex_lock_killable() has returned an error.

I don't see why one of them would return on any signal and the other
one only fatal signals, as you usually want those conditions to be the
same, but in either case, the loop is broken because as soon as
you get a signal, those interfaces will keep returning the same error
and you can never break out of the loop any more.

I don't know how to properly fix it, but it's clear that vchiq_bulk_transmit()
needs to propagate the -EINTR or -ERESTSTARTSYS back to user space
to let the calling task handle the signal before retrying.

        Arnd