Re: [PATCHv2 1/1] uio: add automata sercos3 pci card support

[John Ogness <john.ogness@linutronix.de>]

On 2008-09-17, Hans J. Koch <hjk@linutronix.de> wrote:
>> +/* this function assumes ier0_cache_lock is locked! */
>> +static void sercos3_disable_interrupts(struct uio_info *info,
>> +				       struct sercos3_priv *priv)
>> +{
>> +	void __iomem *ier0 = info->mem[3].internal_addr + IER0_OFFSET;
>> +	u32 tmp = ioread32(ier0);
>> +
>> +	if (tmp) {
>
> Is that needed?

I check the value here for 2 reasons:

1. If the interrupts are already disabled, there is no need to perform
   an iowrite32().

2. If the interrupts are already disabled, we do not want to overwrite
   the ier0_cache with a 0.

Be aware that sercos3_disable_interrupts() can also be called via
sercos3_irqcontrol(). The official driver would never do this, but
maybe someone else would? (Your review of PATCHv1 insisted that
sercos3_irqcontrol() supports disabling the interrupt.) I don't want
to risk enabled interrupts getting lost (i.e. not being re-enabled
later).

>> +		/* store previously enabled interrupts */
>> +		priv->ier0_cache |= tmp;
>
> This doesn't match the comment. Why |= and not a simple assignment?

Here I was concerned that interrupts are being disabled but the
ier0_cache isn't empty. This situation would occur if the
userspace-driver manually enabled some interrupts before handling
pending interrupts.

Normally this would not happen, but since UIO-drivers deal with
context switches to userspace, I decided that it could be possible. So
I decided to |= the bitmask instead of overwriting it to prevent
enabled interrupts from getting lost.

I can change the comment to read:
        /* add enabled interrupts to cache */

>> +
>> +		/* disable interrupts */
>> +		iowrite32(0, ier0);
>> +	}
>> +}

[...]

>> +static irqreturn_t sercos3_handler(int irq, struct uio_info *info)
>> +{
>> +	struct sercos3_priv *priv = info->priv;
>> +	void __iomem *isr0 = info->mem[3].internal_addr + ISR0_OFFSET;
>> +
>> +	if (!ioread32(isr0))
>> +		return IRQ_NONE;
>
> If that card has an irq status and an irq enable register, you have
> to check both. The status bit could be set, but the irq is not
> enabled, in which case you have to return IRQ_NONE. Should look
> something like this:
>
> 	if (!(ier & isr))
> 		return IRQ_NONE;
>
> This assumes the enable and corresponding status bits are in the
> same bit position in the registers.

They are in the same bit position. For PATCHv3 I will change to code
to be:

        if (!(ioread32(isr0) & ioread32(ier0)))
                return IRQ_NONE;

>> +	spin_lock(&priv->ier0_cache_lock);
>> +	sercos3_disable_interrupts(info, priv);
>> +	spin_unlock(&priv->ier0_cache_lock);
>> +
>> +	return IRQ_HANDLED;
>> +}