Re: pcie: xilinx: kernel hang - ISR readl()

[Bjorn Helgaas]

On Tue, Jan 07, 2020 at 09:45:13PM +0530, Muni Sekhar wrote:
> Hi,
> 
> I have module with Xilinx FPGA. It implements UART(s), SPI(s),
> parallel I/O and interfaces them to the Host CPU via PCI Express bus.
> I see that my system freezes without capturing the crash dump for
> certain tests. I debugged this issue and it was tracked down to the
> below mentioned interrupt handler code.
> 
> 
> In ISR, first reads the Interrupt Status register using âreadl()â as
> given below.
>     status = readl(ctrl->reg + INT_STATUS);
> 
> 
> And then clears the pending interrupts using âwritel()â as given blow.
>         writel(status, ctrl->reg + INT_STATUS);
> 
> 
> I've noticed a kernel hang if INT_STATUS register read again after
> clearing the pending interrupts.
> 
> Can someone clarify me why the kernel hangs without crash dump incase
> if I read the INT_STATUS register using readl() after clearing the
> pending bits?
> 
> Can readl() block?

readl() should not block in software.  Obviously at the hardware CPU
instruction level, the read instruction has to wait for the result of
the read.  Since that data is provided by the device, i.e., your FPGA,
it's possible there's a problem there.

Can you tell whether the FPGA has received the Memory Read for
INT_STATUS and sent the completion?

On the architectures I'm familiar with, if a device doesn't respond,
something would eventually time out so the CPU doesn't wait forever.

> Snippet of the ISR code is given blow:
> 
> https://pastebin.com/WdnZJZF5
> 
> 
> 
> static irqreturn_t pcie_isr(int irq, void *dev_id)
> 
> {
> 
>         struct test_device *ctrl = data;
> 
>         u32 status;
> 
> â
> 
> 
> 
>         status = readl(ctrl->reg + INT_STATUS);
> 
>         /*
> 
>          * Check to see if it was our interrupt
> 
>          */
> 
>         if (!(status & 0x000C))
> 
>                 return IRQ_NONE;
> 
> 
> 
>         /* Clear the interrupt */
> 
>         writel(status, ctrl->reg + INT_STATUS);
> 
> 
> 
>         if (status & 0x0004) {
> 
>                 /*
> 
>                  * Tx interrupt pending.
> 
>                  */
> 
>                  ....
> 
>        }
> 
> 
> 
>         if (status & 0x0008) {
> 
>                 /* Rx interrupt Pending */
> 
>                 /* The system freezes if I read again the INT_STATUS
> register as given below */
> 
>                 status = readl(ctrl->reg + INT_STATUS);
> 
>                 ....
> 
>         }
> 
> ..
> 
>         return IRQ_HANDLED;
> }
> 
> 
> 
> -- 
> Thanks,
> Sekhar