Received-SPF: pass (google.com: domain of linux-kernel+bounces-340-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <a6db4a82-3d04-47c0-8f29-0e2e5932face@kylinos.cn>
Date: Fri, 15 Dec 2023 10:18:55 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] irq: Resolve that mask_irq/unmask_irq may not be called
 in pairs
Content-Language: en-US
To: Andy Shevchenko <andy@kernel.org>, Serge Semin <fancer.lancer@gmail.com>,
 Thomas Gleixner <tglx@linutronix.de>
Cc: jikos@kernel.org, benjamin.tissoires@redhat.com,
 linux-input@vger.kernel.org, stable@vger.kernel.org,
 Riwen Lu <luriwen@kylinos.cn>, hoan@os.amperecomputing.com,
 linus.walleij@linaro.org, brgl@bgdev.pl, linux-gpio@vger.kernel.org,
 linux-kernel@vger.kernel.org
References: <20231207014003.12919-1-xiongxin@kylinos.cn> <87ttosssxd.ffs@tglx>
 <e125491c-4cdb-4870-924a-baeb7453bf78@kylinos.cn> <874jgnqwlo.ffs@tglx>
 <bf4004bf-4868-4953-8d8e-0c0e03be673e@kylinos.cn> <875y12p2r0.ffs@tglx>
 <1844c927-2dd4-49b4-a6c4-c4c176b1f75d@kylinos.cn>
 <f5vtfoqylht5ijj6tdvxee3f3u6ywps2it7kv3fhmfsx75od2y@ftjlt4t4z4dk>
 <ZXspNGWszB9jAown@smile.fi.intel.com>
From: xiongxin <xiongxin@kylinos.cn>
In-Reply-To: <ZXspNGWszB9jAown@smile.fi.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: quoted-printable

=E5=9C=A8 2023/12/15 00:11, Andy Shevchenko =E5=86=99=E9=81=93:
> On Thu, Dec 14, 2023 at 01:06:23PM +0300, Serge Semin wrote:
>> On Thu, Dec 14, 2023 at 09:54:06AM +0800, xiongxin wrote:
>>> =E5=9C=A8 2023/12/13 22:59, Thomas Gleixner =E5=86=99=E9=81=93:
>>>> On Wed, Dec 13 2023 at 10:29, xiongxin wrote:
>>>>> =E5=9C=A8 2023/12/12 23:17, Thomas Gleixner =E5=86=99=E9=81=93:
>>>>> Sorry, the previous reply may not have clarified the BUG process. I
>>>>> re-debugged and confirmed it yesterday. The current BUG execution
>>>>> sequence is described as follows:
>>>>
>>>> It's the sequence how this works and it works correctly.
>>>>
>>>> Just because it does not work on your machine it does not mean that =
this
>>>> is incorrect and a BUG.
>>>>
>>>> You are trying to fix a symptom and thereby violating guarantees of =
the
>>>> core code.
>>>>
>>>>> That is, there is a time between the 1:handle_level_irq() and
>>>>> 3:irq_thread_fn() calls for the 2:disable_irq() call to acquire the=
 lock
>>>>> and then implement the irq_state_set_disabled() operation. When fin=
ally
>>>>> call irq_thread_fn()->irq_finalize_oneshot(), it cannot enter the
>>>>> unmask_thread_irq() process.
>>>>
>>>> Correct, because the interrupt has been DISABLED in the mean time.
>>>>
>>>>> In this case, the gpio irq_chip irq_mask()/irq_unmask() callback pa=
irs
>>>>> are not called in pairs, so I think this is a BUG, but not necessar=
ily
>>>>> fixed from the irq core code layer.
>>>>
>>>> No. It is _NOT_ a BUG. unmask() is not allowed to be invoked when th=
e
>>>> interrupt is DISABLED. That's the last time I'm going to tell you th=
at.
>>>> Only enable_irq() can undo the effect of disable_irq(), period.
>>>>
>>>>> Next, when the gpio controller driver calls the suspend/resume proc=
ess,
>>>>> it is as follows:
>>>>>
>>>>> suspend process:
>>>>> dwapb_gpio_suspend()
>>>>>        ctx->int_mask   =3D dwapb_read(gpio, GPIO_INTMASK);
>>>>>
>>>>> resume process:
>>>>> dwapb_gpio_resume()
>>>>>        dwapb_write(gpio, GPIO_INTMASK, ctx->int_mask);
>>>>
>>>> Did you actually look at the sequence I gave you?
>>>>
>>>>      Suspend:
>>>>
>>>> 	  i2c_hid_core_suspend()
>>>> 	     disable_irq();       <- Marks it disabled and eventually
>>>> 				     masks it.
>>>>
>>>> 	  gpio_irq_suspend()
>>>> 	     save_registers();    <- Saves masked interrupt
>>>>
>>>>      Resume:
>>>>
>>>> 	  gpio_irq_resume()
>>>> 	     restore_registers(); <- Restores masked interrupt
>>>>
>>>> 	  i2c_hid_core_resume()
>>>> 	     enable_irq();        <- Unmasks interrupt and removes the
>>>> 				     disabled marker
>>>>
>>>>
>>>> Have you verified that this order of invocations is what happens on
>>>> your machine?
>>>>
>>>> Thanks,
>>>>
>>>>           tglx
>>>
>>> As described earlier, in the current situation, the irq_mask() callba=
ck of
>>> gpio irq_chip is called in mask_irq(), followed by the disable_irq() =
in
>>> i2c_hid_core_suspend(), unmask_irq() will not be executed.
>>>
>>> Then call enable_irq() in i2c_hid_core_resume(). Since gpio irq_chip =
does
>>> not implement the irq_startup() callback, it ends up calling irq_enab=
le().
>>>
>>> The irq_enable() function is then implemented as follows:
>>>
>>> irq_state_clr_disabled(desc);
>>> if (desc->irq_data.chip->irq_enable) {
>>> 	desc->irq_data.chip->irq_enable(&desc->irq_data);
>>> 	irq_state_clr_masked(desc);
>>> } else {
>>> 	unmask_irq(desc);
>>> }
>>>
>>> Because gpio irq_chip implements irq_enable(), unmask_irq() is not ex=
ecuted,
>>> and gpio irq_chip's irq_unmask() callback is not called. Instead,
>>> irq_state_clr_masked() was called to clear the masked flag.
>>>
>>> The irq masked behavior is actually controlled by the
>>> irq_mask()/irq_unmask() callback function pairs in gpio irq_chip. Whe=
n the
>>> whole situation occurs, there is one more irq_mask() operation, or on=
e less
>>> irq_unmask() operation. This ends the i2c hid resume and the gpio
>>> corresponding i2c hid interrupt is also masked.
>>>
>>> Please help confirm whether the current situation is a BUG, or sugges=
t other
>>> solutions to fix it.
>>
>> I has just been Cc'ed to this thread from the very start of the thread
>> so not sure whether I fully understand the problem. But a while ago an
>> IRQ disable/undisable-mask/unmask-unpairing problem was reported for
>> DW APB GPIO driver implementation, but in a another context though:
>> https://lore.kernel.org/linux-gpio/1606728979-44259-1-git-send-email-l=
uojiaxing@huawei.com/
>> We didn't come to a final conclusion back then, so the patch got lost
>> in the emails archive. Xiong, is it related to the problem in your
>> case?
>=20
>  From what explained it sounds to me that GPIO driver has missing part =
and
> this seems the missing patch (in one or another form). Perhaps we can g=
et
> a second round of review,
>=20

Yes, the current case is exactly the situation described in the link,=20
and the specific implementation process is as described in my previous=20
email. After adding the patch for retest, the exception can be=20
effectively solved. And at present, can the patch be incorporated?

Thank you Thomas for your kind advice!

My previous focus has been locked until mask_irq()/unmask_irq() is not=20
called in pairs, in fact, it can turn on the corresponding irq masked in=20
enable_irq. Looking at the irq_enable() callback implementation of other=20
GPIO interrupt controllers, there are indeed operations on masked registe=
rs.

Thank you all!