netdev - Re: [PATCH net-next v2 2/4] can: cc770: add legacy ISA bus driver for the CC770 and AN82527

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: <4F0C653C.3020902@pengutronix.de>
Date:	Tue, 10 Jan 2012 17:20:12 +0100
From:	Marc Kleine-Budde <mkl@...gutronix.de>
To:	info@...ax.com
CC:	Wolfgang Grandegger <wg@...ndegger.com>,
	David Laight <David.Laight@...LAB.COM>,
	Oliver Hartkopp <socketcan@...tkopp.net>, henrik@...conx.com,
	netdev@...r.kernel.org, linux-can@...r.kernel.org,
	socketcan-users@...ts.berlios.de, IreneV <boir1@...dex.ru>,
	Stanislav Yelenskiy <stanislavelensky@...oo.com>, oe@...t.de,
	henrik@...us-sw.com
Subject: Re: [PATCH net-next v2 2/4] can: cc770: add legacy ISA bus driver
 for the CC770 and AN82527

On 01/10/2012 05:13 PM, Wolfgang Zarre wrote:
> Hello Wolfgang,
> 
>> On 01/10/2012 01:41 PM, Wolfgang Zarre wrote:
>>> Hello David,
>>>>
>>>>> cc770_isa_port_write_reg_indirect(const struct cc770_priv *priv,
>>>>>                             int reg, u8 val)
>>>>>     {
>>>>>         unsigned long base = (unsigned long)priv->reg_base;
>>>>> +    unsigned long flags;
>>>>>
>>>>> +    spin_lock_irqsave(&outb_lock, flags);
>>>>>         outb(reg, base);
>>>>>         outb(val, base + 1);
>>>>> +    spin_unlock_irqrestore(&outb_lock, flags);
>>>>
>>>> Is there a 'read_reg_indirect' function??
>>>
>>> Yes, there is.
>>>
>>>> If so it also needs to use the same mutex.
>>>
>>> Actually, I don't think that we have a problem with mutex
>>> beside that it's using just one inb() statement but having
>>> for sure with an interrupt between both outb() statements which
>>> seems to be critical for the cc770.
>>
>> But the indirect read function also sets the address register before
>> reading the data using inb(). This sequence should also not be
>> interrupted and therefore we need to synchronize. For the indirect
>> access of the SJA1000 we also need to add spinlocks. Wonder why nobody
>> complained so far.
> 
> So, if I understand correct that means that inb() can be interrupted
> between
> setting the address and reading. If this is the case then yes, we need
> spinlock if this is not the case then IMHO we wouldn't need or am I wrong?

If I understand correct, this function is the problem:

> static u8 cc770_isa_port_read_reg_indirect(const struct cc770_priv *priv,
>                                              int reg)
> {
>         unsigned long base = (unsigned long)priv->reg_base;
> 
>         outb(reg, base);

an interrupt can happen here, which does another indirect read or write
access, leaving "reg" pointing to the wrong register.

>         return inb(base + 1);
> }

Mar
c
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


Download attachment "signature.asc" of type "application/pgp-signature" (263 bytes)