Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] mmc: meson-gx: remove IRQF_ONESHOT
To:     Thomas Gleixner <tglx@linutronix.de>,
        Ulf Hansson <ulf.hansson@linaro.org>
Cc:     Jerome Brunet <jbrunet@baylibre.com>,
        Kevin Hilman <khilman@baylibre.com>,
        "open list:ARM/Amlogic Meson..." <linux-amlogic@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
        Sebastian Andrzej Siewior <bigeasy@linutronix.de>
References: <20201002164915.938217-1-jbrunet@baylibre.com>
 <CAPDyKFo6T_P+TQQZSzFgHwLeE08f146KxKBpAutv209MXq0mjA@mail.gmail.com>
 <87wo052grp.fsf@nanos.tec.linutronix.de>
 <87v9fn7ce2.fsf@nanos.tec.linutronix.de>
From:   Brad Harper <bjharper@gmail.com>
Message-ID: <e4e6cc45-bc18-40ec-035e-fdb45b9a8f46@gmail.com>
Date:   Wed, 7 Oct 2020 00:45:20 +1100
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.3.1
MIME-Version: 1.0
In-Reply-To: <87v9fn7ce2.fsf@nanos.tec.linutronix.de>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

I'm happy to test anything on a range of amlogic hardware with standard 
/ rt and  multiple mmc devices.  Ill test Jerome's patch in next 24 
hours to report the results.

On 6/10/2020 11:43 pm, Thomas Gleixner wrote:
> On Mon, Oct 05 2020 at 10:55, Thomas Gleixner wrote:
>> On Mon, Oct 05 2020 at 10:22, Ulf Hansson wrote:
>>> On Fri, 2 Oct 2020 at 18:49, Jerome Brunet <jbrunet@baylibre.com> wrote:
>>>> IRQF_ONESHOT was added to this driver to make sure the irq was not enabled
>>>> again until the thread part of the irq had finished doing its job.
>>>>
>>>> Doing so upsets RT because, under RT, the hardirq part of the irq handler
>>>> is not migrated to a thread if the irq is claimed with IRQF_ONESHOT.
>>>> In this case, it has been reported to eventually trigger a deadlock with
>>>> the led subsystem.
>>>>
>>>> Preventing RT from doing this migration was certainly not the intent, the
>>>> description of IRQF_ONESHOT does not really reflect this constraint:
>>>>
>>>>   > IRQF_ONESHOT - Interrupt is not reenabled after the hardirq handler finished.
>>>>   >              Used by threaded interrupts which need to keep the
>>>>   >              irq line disabled until the threaded handler has been run.
>>>>
>>>> This is exactly what this driver was trying to acheive so I'm still a bit
>>>> confused whether this is a driver or an RT issue.
>>>>
>>>> Anyway, this can be solved driver side by manually disabling the IRQs
>>>> instead of the relying on the IRQF_ONESHOT. IRQF_ONESHOT may then be removed
>>>> while still making sure the irq won't trigger until the threaded part of
>>>> the handler is done.
>>> Thomas, may I have your opinion on this one.
>>>
>>> I have no problem to apply $subject patch, but as Jerome also
>>> highlights above - this kind of makes me wonder if this is an RT
>>> issue, that perhaps deserves to be solved in a generic way.
>>>
>>> What do you think?
>> Let me stare at the core code. Something smells fishy.
> The point is that for threaded interrupts (without a primary handler)
> the core needs to be told that the interrupt line should be masked until
> the threaded handler finished. That's what IRQF_ONESHOT is for.
>
> For interrupts which have both a primary and a threaded handler that's a
> different story. The primary handler decides whether the thread should
> be woken and it decides whether to block further interrupt delivery in
> the device or keep it enabled.
>
> When forced interrupt threading is enabled (even independent of RT) then
> we have the following cases:
>
>    1) Regular device interrupt (primary handler only)
>
>       The primary handler is replaced with the default 'wake up thread'
>       handler and the original primary handler becomes the threaded
>       handler. This enforces IRQF_ONESHOT so that the interupt line (for
>       level interrupts) stays masked until the thread completed handling.
>
>    2) Threaded interrupts
>
>       Interrupts which have been requested as threaded handler (no
>       primary handler) are not changed obvioulsy
>
>    3) Interrupts which have both a primary and a thread handler
>
>       Here IRQF_ONESHOT decides whether the primary handler will be
>       forced threaded or not.
>
>       That's a bit unfortunate and ill defined and was not intended to be
>       used that way.
>
>       We rather should make interrupts which need to have their primary
>       handler in hard interrupt context to set IRQF_NO_THREAD. That
>       should at the same time confirm that the primary handler is RT
>       safe.
>
>       Let me stare at the core code and the actual usage sites some more.
>
> Thanks,
>
>          tglx
>
>
>
>
>
>