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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8663ovf439.fsf@johno-ibook.fn.ogness.net>
Date:	Wed, 17 Sep 2008 12:40:10 +0200
From:	John Ogness <john.ogness@...utronix.de>
To:	"Hans J. Koch" <hjk@...utronix.de>
Cc:	Greg Kroah-Hartman <gregkh@...e.de>, linux-kernel@...r.kernel.org
Subject: Re: [PATCHv2 1/1] uio: add automata sercos3 pci card support

On 2008-09-17, Hans J. Koch <hjk@...utronix.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;
>> +}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/