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

[Wolfgang Zarre <info@essax.com>]

Hello Wolfgang,

-------- Original Message  --------
Subject: Re: [PATCH net-next v2 2/4] can: cc770: add legacy ISA bus driver for the CC770 and AN82527
From: Wolfgang Grandegger <wg@grandegger.com>
To: info@essax.com
Cc: David Laight <David.Laight@ACULAB.COM>, Oliver Hartkopp <socketcan@hartkopp.net>, henrik@proconx.com, netdev@vger.kernel.org, linux-can@vger.kernel.org, 
socketcan-users@lists.berlios.de, IreneV <boir1@yandex.ru>, Stanislav Yelenskiy <stanislavelensky@yahoo.com>, oe@port.de, henrik@focus-sw.com
Date: Tue Jan 10 2012 17:23:59 GMT+0100 (CET)

> Hi Wolfgang,
>
> 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?
>
> I think we speak about different things. inb() cannot be interrupted but
> outb() followed by inb(). For indirect accesses we need something like:
>
> /* Spinlock for cc770_isa_port_write_reg_indirect */
> static DEFINE_SPINLOCK(cc770_isa_port_lock);
>
> static u8 cc770_isa_port_read_reg_indirect(const struct cc770_priv *priv,
>                                               int reg)
> {
>          unsigned long base = (unsigned long)priv->reg_base;
> 	unsigned long flags;
> 	u8 val;
>
> 	spin_lock_irqsave(&cc770_isa_port_lock, flags);
>          outb(reg, base);
>          val = inb(base + 1);
> 	spin_unlock_irqrestore(&cc770_isa_port_lock, flags);
>
> 	return val;
> }
>
> static void 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(&cc770_isa_port_lock, flags);
>          outb(reg, base);
>          outb(val, base + 1);
> 	spin_unlock_irqrestore(&cc770_isa_port_lock, flags);
> }
>
> Hope we are in synch now.
>

Thanks a lot. Yes, now phase locked and in synch and sorry, by mistake
I looked at the wrong function (cc770_isa_port_read_reg) in the heat of
the moment.

Absolutely clear, there we need a spinlock definitely. I'll start another test run
just to confirm.


> Wolfgang.
>

Wolfgang