2003-11-10 12:28:05

by Stefan Talpalaru

[permalink] [raw]
Subject: Re: PATCH: CMD640 IDE chipset

Hello Alexander!

> Hello,
> On Wed, 29 Oct 2003 04:12:18 -0800 (PST)
> Stefan Talpalaru <[email protected]> wrote:
> > and also some useless code (the wrapers: __put_cmd640_reg() and
> > __get_cmd640_reg() - which I removed and placed the locks where
> > needed; the pci_conf1() and pci_conf2() functions).
> These wrappers were added to correct the locking in
> driver. There are places that you must access serveral registers

where?

> holding the lock and that's why the wrappers came, so they
> are not useless. The locking in your patch is wrong -

They are ugly.

> the problem is that you call get_cmd640_reg or put_cmd640_reg, which
> try to take ide_lock, while already holding it and it's a deadlock

I find it hard to believe (I'm running a patched kernel right now!),
but
i'm only a newbie at kernel hacking, so please show me the light (not
joking).
Thanks in advance.

> - thats what the wrappers solved. So, please , drop that change.
> setup_count |= __get_cmd640_reg(arttim_regs[index]) & 0x3f;
> - __put_cmd640_reg(arttim_regs[index], setup_count);
> - __put_cmd640_reg(drwtim_regs[index],
pack_nibbles(active_count, recovery
> _count));
> + setup_count |= get_cmd640_reg(arttim_regs[index]) & 0x3f;
> + put_cmd640_reg(arttim_regs[index], setup_count);
> + put_cmd640_reg(drwtim_regs[index], pack_nibbles(active_count,
recovery_c
> ount));
> spin_unlock_irqrestore(&ide_lock, flags);
> }
> here
> - __put_cmd640_reg(reg, b);
> + put_cmd640_reg(reg, b);
> spin_unlock_irqrestore(&ide_lock, flags);
> and here for example.
> --
> have fun,
> alex

later,


=====
Stefan Talpalaru

__________________________________
Do you Yahoo!?
Protect your identity with Yahoo! Mail AddressGuard
http://antispam.yahoo.com/whatsnewfree