[PATCH] ipic: change ack operation that register is accessedonly when needed

[Li Yang]

> -----Original Message-----
> From: Benjamin Herrenschmidt [mailto:benh at kernel.crashing.org] 
> Sent: Tuesday, December 04, 2007 5:02 AM
> To: Li Yang
> Cc: galak at kernel.crashing.org; linuxppc-dev at ozlabs.org
> Subject: Re: [PATCH] ipic: change ack operation that register 
> is accessedonly when needed
> 
> 
> >  static void ipic_ack_irq(unsigned int virq)  {
> > -	struct ipic *ipic = ipic_from_irq(virq);
> >  	unsigned int src = ipic_irq_to_hw(virq);
> > -	unsigned long flags;
> > -	u32 temp;
> >  
> > -	spin_lock_irqsave(&ipic_lock, flags);
> > +	/* Only external interrupts in edge mode support ACK */
> > +	if (unlikely(ipic_info[src].ack &&
> > +			((get_irq_desc(virq)->status & 
> IRQ_TYPE_SENSE_MASK) ==
> > +			IRQ_TYPE_EDGE_FALLING))) {
> > +		struct ipic *ipic = ipic_from_irq(virq);
> > +		unsigned long flags;
> > +		u32 temp;
> >  
> > -	temp = ipic_read(ipic->regs, ipic_info[src].pend);
> > -	temp |= (1 << (31 - ipic_info[src].bit));
> > -	ipic_write(ipic->regs, ipic_info[src].pend, temp);
> > +		spin_lock_irqsave(&ipic_lock, flags);
> >  
> > -	spin_unlock_irqrestore(&ipic_lock, flags);
> > +		temp = ipic_read(ipic->regs, ipic_info[src].ack);
> > +		temp |= (1 << (31 - ipic_info[src].bit));
> > +		ipic_write(ipic->regs, ipic_info[src].ack, temp);
> > +
> > +		spin_unlock_irqrestore(&ipic_lock, flags);
> > +	}
> >  }
> 
> That doesn't look right... 
> 
> That should be handled by the higher level flow handler. The 
> generic edge one calls ack and the level one mask_and_ack. 
> Just make them do the right thing, no need to test for the 
> flow type in the low level function.

But actually ack is called by edge and per cpu handlers.  Mask_and_ack
is also called by edge handler when the same interrupt is already in
progress.  So I don't think that ack/mask_and_ack implicates flow type
by design.

- Leo