linux-kernel - Re: [PATCH] serial: imx: drop workaround for forced irq threading

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20210323090413.ogeweygw3iejtbsv@linutronix.de>
Date:   Tue, 23 Mar 2021 10:04:13 +0100
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     Johan Hovold <johan@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org,
        Sam Nobs <samuel.nobs@...tradio.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>, kernel@...gutronix.de
Subject: Re: [PATCH] serial: imx: drop workaround for forced irq threading

On 2021-03-23 08:34:47 [+0100], Uwe Kleine-König wrote:
> Hello Sebastian,
Hi Uwe,

> On Mon, Mar 22, 2021 at 09:48:36PM +0100, Sebastian Andrzej Siewior wrote:
> > On 2021-03-22 14:40:32 [+0100], Uwe Kleine-König wrote:
> > > From a strictly logically point of view you indeed cannot. But if you go
> > > to the street and say to people there that they can park their car in
> > > this street free of charge between Monday and Friday, I expect that most
> > > of them will assume that they have to pay for parking on weekends.
> > 
> > If I hear that parking is free on weekdays and on paid on weekends, I
> > expect it to be a scam.
> 
> I don't feel taken seriously with this reply.

I'm sorry.

> > Uwe, the patch reverts a change which was needed for !RT + threadirqs.
> 
> This would be a useful information for the commit log.
> 
> > The commit message claims that since the referenced commit "… interrupt
> > handlers always run with interrupts disabled on non-RT… ". This has
> > nothing to do with _this_ change. It argues why the workaround is not
> > needed.
> 
> It argues why the work around is not needed on non-RT. It might be
> obvious for someone who is firm in the RT concepts, but IMHO commit logs
> should be understandable by and make sense for a wider audience than the
> deep experts. From what I know about RT "Force-threaded interrupt
> handlers used to run with interrupts enabled" still applies there.

Yes. The commit Johan referenced explains it in more detail.

> > If the referenced commit breaks RT then this is another story.
> 
> I'm surprised to hear that from you. With the goal to get RT into
> mainline I would expect you to be happy if people consider the effects
> on RT in their reviews.

Correct, I do and I am glad if people consider other aspects of the
kernel in their review including RT.

> > > So when you said that on on-RT the reason why it used to need a
> > > workaround is gone made me wonder what that implies for RT.
> > 
> > There was never reason (or a lockdep splat) for it on RT. If so you
> > should have seen it, right?
> 
> No, I don't consider myself to be an RT expert who is aware of all the
> problems. So I admit that for me the effect on RT of the patch under
> discussion isn't obvious. I just wonder that the change is justified
> with being OK on non-RT. So it's either bad that it breaks RT *or*
> improving the commit log would be great.
> 
> And even if I had reason to believe that there is no problem with the
> commit on RT, I'd still wish that the commit log wouldn't suggest to the
> casual reader that there might be a problem.

Okay. I added a sentence. What about this rewording:

  Force-threaded interrupt handlers used to run with interrupts enabled,
  something which could lead to deadlocks in case a threaded handler
  shared a lock with code running in hard interrupt context (e.g. timer
  callbacks) and did not explicitly disable interrupts.  
  
  This was specifically the case for serial drivers that take the port
  lock in their console write path as printk can be called from hard
  interrupt context also with forced threading ("threadirqs").
  
  Since commit 81e2073c175b ("genirq: Disable interrupts for force
  threaded handlers") interrupt handlers always run with interrupts
  disabled on non-RT so that drivers no longer need to do handle this.
  RT is not affected by the referenced commit and the workaround, that is
  reverted, was not required because spinlock_t must not be acquired on
  RT in hardirq context.
  
  Drop the now obsolete workaround added by commit 33f16855dcb9 ("tty:
  serial: imx: fix potential deadlock").

> Best regards
> Uwe
> 

Sebastian