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

From: Oliver Hartkopp <[email-masked]>
Date: Tue, 10 Jan 2012 15:50:12 +0100

On 10.01.2012 15:43, Wolfgang Grandegger wrote:

> 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.

Maybe due to old single core hardware that has SJA1000 hw with indirect
addressing? Or this old hardware has not been 'kernel-upgraded' so far ...

In any case you are right with the missing spinlocks.

Regards,
Oliver