linux-kernel - Re: [PATCH 4.19 053/125] media: gpio-ir-tx: improve precision of transmitted signal due to scheduling

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] [day] [month] [year] [list]

Date:   Sun, 25 Oct 2020 17:54:35 -0700 (PDT)
From:   Norman Rasmussen <norman@...mussen.co.za>
To:     Sean Young <sean@...s.org>
cc:     Pavel Machek <pavel@...x.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org,
        Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
        Sasha Levin <sashal@...nel.org>
Subject: Re: [PATCH 4.19 053/125] media: gpio-ir-tx: improve precision of
 transmitted signal due to scheduling

Hi Sean,

On Wed, 2 Sep 2020, Sean Young wrote:

> Hi Pavel,
>
> On Wed, Sep 02, 2020 at 12:25:21PM +0200, Pavel Machek wrote:
>> Hi!
>>>
>>> [ Upstream commit ea8912b788f8144e7d32ee61e5ccba45424bef83 ]
>>>
>>> usleep_range() may take longer than the max argument due to scheduling,
>>> especially under load. This is causing random errors in the transmitted
>>> IR. Remove the usleep_range() in favour of busy-looping with udelay().
>>>
>>> Signed-off-by: Sean Young <sean@...s.org>
>>
>> I don't believe this should be in stable.
>>
>> Yes, it probably fixes someone's remote control.
>>
>> It also introduces > half a second (!) with interrupts disabled
>> (according to the code comments), which will break other devices on
>> the system.
>
> Yes, I've always been uncomfortable with this code, but nothing else I
> tried worked.
>
> BTW the delay has a maximum of half a second, but the point stands of
> course.
>
>> Less intrusive solutions should be explored, first. Like.. if that
>> part is time-critical, perhaps it should set itself at realtime
>> priority, so that scheduler has motivation to schedule it at the right
>> times?
>
> That is an interesting idea, I'll explore that.
>

Did you try anything around this? It looks like it might be required for 
devices like the raspbetty pi zero w (see more details below).

Is there a way to temporarily increase the priority of the existing 
thread? or is the preferred way to do readtime priority with a dedicated 
thread? Assuming the latter: What's the preferred way to transfer data & 
control from the user's thread to the dedicated thread and back? I'd 
guess some sort of queue or mailbox?

>> Perhaps usleep_range should be delta, delta+1?
>
> I'm pretty sure I tried that and it didn't work. I'll give it another go.
>

Shouldn't max actually be less than delta? Otherwise the code is 
indicating that it's okay to sleep for longer than delta + rescheduling 
overhead.

I tried your latest patch ("re-introduce sleeping for periods of > 50us") 
on my Pi Zero W and the post-usleep "remaining" delta is anywhere between 
-25,500us and -50us (i.e. usleep_range usually oversleeps!).

The upstream patch gives very stable results: post-udelay delta is 
typically <10us (i.e. it's underdelaying just a tiny bit).

I tried adding a module_param to tune the sleep threshold buffer but 
because typical delays are 500us and 1,500us and the worst usleep overruns 
are way larger than that, effectively I had to set it so that usleep never 
triggered

I even tried usleep_range(0, delta - buffer), but that produced the same 
behaviour. (I even saw a post-usleep delta of -125,000us once!)

I added a call to switch to the FIFO scheduler at priority 50 (the same 
as the recently proposed sched_set_fifo function would do), and (as long 
as ir-ctl is run as root) it brings the post-usleep delta to ~500us (or 
with usleep_range(0, ...) for large deltas the post-usleep delta was 
between ~500us and ~5000us - still undersleeping and very acceptable).

Note that pwm-ir-tx has the same issue and so should probably get the same 
fixes (when they're figured out what they'll be).

>> Perhaps udelay makes sense to use for more than 10usec?
>
> I don't follow -- what are you suggesting here?
>
> So any other ideas here would be very welcome. I'm happy to explore options,
> so far under load the output is can be total garbage if you're unlucky.
>
>
> Thanks,
>
> Sean
>
>
>>
>> Best regards,
>> 										Pavel
>>
>>> @@ -87,13 +87,8 @@ static int gpio_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
>>>  			// space
>>>  			edge = ktime_add_us(edge, txbuf[i]);
>>>  			delta = ktime_us_delta(edge, ktime_get());
>>> -			if (delta > 10) {
>>> -				spin_unlock_irqrestore(&gpio_ir->lock, flags);
>>> -				usleep_range(delta, delta + 10);
>>> -				spin_lock_irqsave(&gpio_ir->lock, flags);
>>> -			} else if (delta > 0) {
>>> +			if (delta > 0)
>>>  				udelay(delta);
>>> -			}
>>>  		} else {
>>>  			// pulse
>>>  			ktime_t last = ktime_add_us(edge, txbuf[i]);
>>> --
>>> 2.25.1
>>>
>>>
>>
>> --
>> (english) http://www.livejournal.com/~pavelmachek
>> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
>
>
>

-- 
- Norman Rasmussen
 - Email: norman@...mussen.co.za
 - Home page: http://norman.rasmussen.co.za/