Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling

[Boris Brezillon]

On Wed,  7 Jun 2017 20:52:19 +0900
Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx> wrote:


> -/*
> - * This is the interrupt service routine. It handles all interrupts
> - * sent to this device. Note that on CE4100, this is a shared interrupt.
> - */
> -static irqreturn_t denali_isr(int irq, void *dev_id)
> +static uint32_t denali_wait_for_irq(struct denali_nand_info *denali,
> +				    uint32_t irq_mask)
>  {
> -	struct denali_nand_info *denali = dev_id;
> +	unsigned long time_left, flags;
>  	uint32_t irq_status;
> -	irqreturn_t result = IRQ_NONE;
>  
> -	spin_lock(&denali->irq_lock);
> +	spin_lock_irqsave(&denali->irq_lock, flags);
>  
> -	/* check to see if a valid NAND chip has been selected. */
> -	if (is_flash_bank_valid(denali->flash_bank)) {
> -		/*
> -		 * check to see if controller generated the interrupt,
> -		 * since this is a shared interrupt
> -		 */
> -		irq_status = denali_irq_detected(denali);
> -		if (irq_status != 0) {
> -			/* handle interrupt */
> -			/* first acknowledge it */
> -			clear_interrupt(denali, irq_status);
> -			/*
> -			 * store the status in the device context for someone
> -			 * to read
> -			 */
> -			denali->irq_status |= irq_status;
> -			/* notify anyone who cares that it happened */
> -			complete(&denali->complete);
> -			/* tell the OS that we've handled this */
> -			result = IRQ_HANDLED;
> -		}
> +	irq_status = denali->irq_status;
> +
> +	if (irq_mask & irq_status) {
> +		spin_unlock_irqrestore(&denali->irq_lock, flags);
> +		return irq_status;
>  	}
> -	spin_unlock(&denali->irq_lock);
> -	return result;
> +
> +	denali->irq_mask = irq_mask;
> +	reinit_completion(&denali->complete);

These 2 instructions should be done before calling
denali_wait_for_irq() (for example in denali_reset_irq()), otherwise
you might loose events if they happen between your irq_status read and
the reinit_completion() call. You should also clear existing interrupts
before launching your operation, otherwise you might wakeup on previous
events.

> +	spin_unlock_irqrestore(&denali->irq_lock, flags);
> +
> +	time_left = wait_for_completion_timeout(&denali->complete,
> +						msecs_to_jiffies(1000));
> +	if (!time_left) {
> +		dev_err(denali->dev, "timeout while waiting for irq 0x%x\n",
> +			denali->irq_mask);
> +		return 0;
> +	}
> +
> +	return denali->irq_status;
>  }
>