Hi, Trent > On Thu, 2019-07-18 at 03:08 +0000, Aisheng Dong wrote: > > > From: Anson Huang > > > Sent: Wednesday, July 17, 2019 9:58 PM> Hi, Aisheng > > > > > > > > From: Anson.Huang@xxxxxxx <Anson.Huang@xxxxxxx> > > > > > Sent: Tuesday, July 16, 2019 3:19 PM > > > > > > > > > > The RTC IRQ is requested before the struct rtc_device is > > > > > allocated, this may lead to a NULL pointer dereference in IRQ > > > > > handler. > > > > > > > > > > To fix this issue, allocating the rtc_device struct before > > > > > requesting the RTC IRQ using devm_rtc_allocate_device, and use > > > > > rtc_register_device to register the RTC device. > > > > > > > > > > > > > I saw other rtc drivers did the same way as us, so this looks like > > > > a common problem. > > > > My question is if we can clear interrupt status before register to > > > > avoid this issue as other rtc drivers? > > > > > > I think we can NOT predict when the IRQ will be pending, IRQ could > > > arrive at any time, the most safe way is to prepare everything > > > before requesting/enabling IRQ. > > > There is also patch to fix similar issue: > > I think one could attempt to disable all irq sources in the device via its > register space, then enable the interrupt. But this seems more specific to > each device than changing the pattern of device registration, so IMHO, it's > not really better. > > I do worry that handling the irq before the rtc device is registered could still > result in a crash. From what I saw, the irq path in snvs only uses driver state > members that are fully initialized for the most part, and the allocated but > unregistered data->rtc is only used in one call to rtc_update_irq(), which > appears to be ok with this. > > But it is not that hard to imagine that something could go into the rtc core > that assumes call like rtc_update_irq() are only made on registered devices. > > If there was a way to do it, I think allocating the irq in a masked state and > then unmasking it as part of the final registration call to make the device go > live would be a safer and more general pattern. It makes sense, I think we can just move the devm_request_irq() to after rtc_register_device(), It will make sure everything is ready before IRQ is enabled. Will send out a V2 patch. Thanks, Anson
