Messages in this thread | | | From | "Zhao, Yuanyuan" <> | Subject | RE: [RESEND 1/1] gic: its: Make sure a LPI is discarded before free. | Date | Thu, 10 Jan 2019 09:42:28 +0000 |
| |
> -----Original Message----- > From: Marc Zyngier [mailto:marc.zyngier@arm.com] > Sent: 2019年1月9日 17:52 > To: Zhao, Yuanyuan <yuanyuan.zhao@hxt-semitech.com> > Cc: tglx@linutronix.de; jason@lakedaemon.net; linux- > kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Zheng, Joey > <yu.zheng@hxt-semitech.com>; Wang, Dongsheng <dongsheng.wang@hxt- > semitech.com> > Subject: Re: [RESEND 1/1] gic: its: Make sure a LPI is discarded before free. > > On 09/01/2019 09:29, Zhao, Yuanyuan wrote: > > Hi Marc: > > > > Thank you for your reply. > > > > As you said, APIs such as free_irq will deactivate irq before free it. > > But deactivation is not forced by every API, for example > > irq_dispose_mapping. So I think it's better to check that irq was > > deactivated as expected. > > In general, we should fix the problem at the core API level instead of hacking > individual drivers. > > But more to the point, irq_dispose_mapping is not supposed to do anything > with the an active irq, as it doesn't have the required information to safely > remove it. > > So calling irq_dispose_mapping on an interrupt that still has registered > actions is a bug, and I'm not convinced we want to cater for such a case. Do > you have a concrete example of some kernel code expecting this behaviour? > > Thanks, > > M. >
Most driver use free_irq after register actions, I found this problem by a test case. But if this problem happen and the same DeviceID & EventID are reused, the freed ITT will be visit which cause delayed kernel panic, the prev INTs are triggered unexpected, but the new INTs lost.
So I think this check spend less, but gains more.
BRs, Yuanyuan.
> > > > BRs, > > Yuanyuan > > > > > >> -----Original Message----- > >> From: Marc Zyngier [mailto:marc.zyngier@arm.com] > >> Sent: 2019年1月9日 15:43 > >> To: Zhao, Yuanyuan <yuanyuan.zhao@hxt-semitech.com> > >> Cc: tglx@linutronix.de; jason@lakedaemon.net; linux- > >> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Zheng, > >> Joey <yu.zheng@hxt-semitech.com>; Wang, Dongsheng > >> <dongsheng.wang@hxt- semitech.com> > >> Subject: Re: [RESEND 1/1] gic: its: Make sure a LPI is discarded before free. > >> > >> On Wed, 9 Jan 2019 11:53:27 +0800 > >> Zhao Yuanyuan <yuanyuan.zhao@hxt-semitech.com> wrote: > >> > >> Hi Zhao, > >> > >>> Its device will be removed after all events be freed. > >>> Undisarded events can lead to unpredictable behaviar. > >>> > >>> Signed-off-by: Zhao Yuanyuan <yuanyuan.zhao@hxt-semitech.com> > >>> --- > >>> drivers/irqchip/irq-gic-v3-its.c | 4 ++++ > >>> 1 file changed, 4 insertions(+) > >>> > >>> diff --git a/drivers/irqchip/irq-gic-v3-its.c > >>> b/drivers/irqchip/irq-gic-v3-its.c > >>> index db20e99..4fee008 100644 > >>> --- a/drivers/irqchip/irq-gic-v3-its.c > >>> +++ b/drivers/irqchip/irq-gic-v3-its.c > >>> @@ -2572,6 +2572,10 @@ static void its_irq_domain_free(struct > >> irq_domain *domain, unsigned int virq, > >>> virq + i); > >>> u32 event = its_get_event_id(data); > >>> > >>> + /* Discard irq before free */ > >>> + if (irqd_is_activated(d)) > >>> + its_send_discard(its_dev, event); > >>> + > >>> /* Mark interrupt index as unused */ > >>> clear_bit(event, its_dev->event_map.lpi_map); > >>> > >> > >> But we already do send a discard on deactivate, which logically > >> happens before we free the domain. So what are you fixing here? > >> > >> Thanks, > >> > >> M. > >> -- > >> Without deviation from the norm, progress is not possible. > > > -- > Jazz is not dead. It just smells funny...
| |