2002-03-01 16:40:37

by Frode Isaksen

[permalink] [raw]
Subject: [PATCH] spinlock not locked when unlocking in atm_dev_register

If you compile the kernel with SMP and spinlock debugging, BUG() will be
called when registering your atm driver, since the "atm_dev_lock" spinlock is
not locked when unlocking it.

kernel 2.4.18

Regards,
Frode

--- resources.c.orig Fri Mar 1 18:34:02 2002
+++ resources.c Fri Mar 1 18:34:17 2002
@@ -110,12 +110,10 @@
if (atm_proc_dev_register(dev) < 0) {
printk(KERN_ERR "atm_dev_register: "
"atm_proc_dev_register failed for dev %s\n",type);
- spin_unlock (&atm_dev_lock);
free_atm_dev(dev);
return NULL;
}
#endif
- spin_unlock (&atm_dev_lock);
return dev;
}


2002-03-01 22:16:13

by Robert Love

[permalink] [raw]
Subject: Re: [PATCH] spinlock not locked when unlocking in atm_dev_register

On Fri, 2002-03-01 at 12:46, Frode Isaksen wrote:
> If you compile the kernel with SMP and spinlock debugging, BUG() will be
> called when registering your atm driver, since the "atm_dev_lock" spinlock is
> not locked when unlocking it.

I don't have any knowledge of the source in question, but wouldn't a
possibility (perhaps even more likely) be that you should _add_ the
spin_lock instead of remove the spin_unlocks ?

Robert Love

2002-03-01 22:33:23

by Max Krasnyansky

[permalink] [raw]
Subject: Re: [PATCH] spinlock not locked when unlocking in atm_dev_register


> > If you compile the kernel with SMP and spinlock debugging, BUG() will be
> > called when registering your atm driver, since the "atm_dev_lock"
> spinlock is
> > not locked when unlocking it.
>
>I don't have any knowledge of the source in question, but wouldn't a
>possibility (perhaps even more likely) be that you should _add_ the
>spin_lock instead of remove the spin_unlocks ?
Absolutely correct :)
I've got a patch for that, tested on SMP. I'll send it today or tomorrow.

btw ATM locking seems to be messed up. Is anybody working on that ?

Max

2002-03-01 22:35:33

by Robert Love

[permalink] [raw]
Subject: Re: [PATCH] spinlock not locked when unlocking in atm_dev_register

On Fri, 2002-03-01 at 17:32, Maksim Krasnyanskiy wrote:

> > I don't have any knowledge of the source in question, but wouldn't a
> > possibility (perhaps even more likely) be that you should _add_ the
> > spin_lock instead of remove the spin_unlocks ?
>
> Absolutely correct :)
> I've got a patch for that, tested on SMP. I'll send it today or tomorrow.

Yah, I just looked at the code - again, I don't profess to know the atm
driver - but it seems to need the lock. Thanks.

> btw ATM locking seems to be messed up. Is anybody working on that ?

Not that I know of. Volunteer? :)

Robert Love

2002-03-01 22:40:53

by David Miller

[permalink] [raw]
Subject: Re: [PATCH] spinlock not locked when unlocking in atm_dev_register

From: Robert Love <[email protected]>
Date: 01 Mar 2002 17:35:08 -0500

> btw ATM locking seems to be messed up. Is anybody working on that ?

Not that I know of. Volunteer? :)

I consider it pretty much unmaintained. Feel free to take it
up :)

2002-03-01 22:48:23

by Lei Wang

[permalink] [raw]
Subject: How to get kernel data using /proc system?

Hello, everyone, could anybody help me out of this? Any help would be
highly appreciated. Thanks!

I want to read out some kernel data. But I searched the web a lot and
all the methods I found in using /proc filesystem seem to be out of
date. Does anybody know how to do this in Linux 2.4 or 2.2?

Also, are other ways to get access to kernel data than using
/proc?

Thanks a lot!


2002-03-01 23:07:05

by Mitchell Blank Jr

[permalink] [raw]
Subject: Re: [PATCH] spinlock not locked when unlocking in atm_dev_register

Maksim Krasnyanskiy wrote:
> btw ATM locking seems to be messed up. Is anybody working on that ?

Yes, pretty badly. What we (the atm folks) really need to do for 2.5 is
rewrite a lot of code. The current driver model predates SMP in the kernel
and it's pretty much impossible to make a driver 100% SMP correct (although,
in practice, you can get pretty close and keep the races pretty tiny)
Really a lot of the driver model needs to be rethought.

As for maintainership I readily admit I haven't spent nearly as much time
as I would have liked on the ATM stuff in the last year or so. Too much
other work to do, etc. I'm really hoping to get some free time to play
with stuff in the next couple months... There are others actively working
on stuff. Particularly Paul Schroeder at IBM did a lot of good work on
the (long-suffering) userland tools.

Anyway I have been thinking that we should start kicking around ideas for
the next-generation ATM code. I'd recommend interested parties subscribe
to the linux-atm-general mailing list:
http://lists.sourceforge.net/lists/listinfo/linux-atm-general

-Mitch

2002-03-01 23:23:09

by Chen, Kenneth W

[permalink] [raw]
Subject: RE: How to get kernel data using /proc system?

Lookup the document directory in the latest kernel tree.
linux/Documentation/DocBook/procfs*


-----Original Message-----
From: Lei Wang [mailto:[email protected]]
Sent: Friday, March 01, 2002 2:48 PM
To: [email protected]
Subject: How to get kernel data using /proc system?


Hello, everyone, could anybody help me out of this? Any help would be
highly appreciated. Thanks!

I want to read out some kernel data. But I searched the web a lot and
all the methods I found in using /proc filesystem seem to be out of
date. Does anybody know how to do this in Linux 2.4 or 2.2?

Also, are other ways to get access to kernel data than using
/proc?

Thanks a lot!


2002-03-01 23:46:45

by Thomas Hood

[permalink] [raw]
Subject: RE: How to get kernel data using /proc system?

I wrote the following comment block explaining how
to write a proc_file_read function. This is the
function you register with procfs using
create_proc_read_entry(). Hope this helps.
(This patch went into 2.5.x-djy)

[...]
+ * How to be a proc read function
+ * ------------------------------
+ * Prototype:
+ * int f(char *buffer, char **start, off_t offset,
+ * int count, int *peof, void *dat)
+ *
+ * Assume that the buffer is "count" bytes in size.
+ *
+ * If you know you have supplied all the data you
+ * have, set *peof.
+ *
+ * You have three ways to return data:
+ * 0) Leave *start = NULL. (This is the default.)
+ * Put the data of the requested offset at that
+ * offset within the buffer. Return the number (n)
+ * of bytes there are from the beginning of the
+ * buffer up to the last byte of data. If the
+ * number of supplied bytes (= n - offset) is
+ * greater than zero and you didn't signal eof
+ * and the reader is prepared to take more data
+ * you will be called again with the requested
+ * offset advanced by the number of bytes
+ * absorbed. This interface is useful for files
+ * no larger than the buffer.
+ * 1) Set *start = an unsigned long value less than
+ * the buffer address but greater than zero.
+ * Put the data of the requested offset at the
+ * beginning of the buffer. Return the number of
+ * bytes of data placed there. If this number is
+ * greater than zero and you didn't signal eof
+ * and the reader is prepared to take more data
+ * you will be called again with the requested
+ * offset advanced by *start. This interface is
+ * useful when you have a large file consisting
+ * of a series of blocks which you want to count
+ * and return as wholes.
+ * (Hack by [email protected])
+ * 2) Set *start = an address within the buffer.
+ * Put the data of the requested offset at *start.
+ * Return the number of bytes of data placed there.
+ * If this number is greater than zero and you
+ * didn't signal eof and the reader is prepared to
+ * take more data you will be called again with the
+ * requested offset advanced by the number of bytes
+ * absorbed.

2002-03-02 00:29:56

by Max Krasnyansky

[permalink] [raw]
Subject: Re: [PATCH] spinlock not locked when unlocking in atm_dev_register


> > btw ATM locking seems to be messed up. Is anybody working on that ?
>
> Not that I know of. Volunteer? :)
>
>I consider it pretty much unmaintained. Feel free to take it up :)
I'd rather work on my Bluetooth stuff for now :)

The reason I looked at the ATM stuff is that I got this DSL thing from
PacBell that
came with Alcatel ADSL USB modem which talks to the ATM layer. The guy who
wrote that USB driver had a comment that it doesn't work on SMP for
_unknown reason_ :)
Quick glance at the ATM code, revealed that one of reasons is messed up
looking.
So I fixed device registration and now it works. But ATM locking in general
has to be fixed.

Max

2002-03-02 01:55:19

by Alexander Viro

[permalink] [raw]
Subject: RE: How to get kernel data using /proc system?



On 1 Mar 2002, Thomas Hood wrote:

> I wrote the following comment block explaining how
> to write a proc_file_read function. This is the
> function you register with procfs using
> create_proc_read_entry(). Hope this helps.

[snip ~50 lines of comments]

You know, that's a wonderful explanation... of reasons for _not_ using
->proc_read(). If interface takes that much to document - it's crap.

2002-03-04 08:29:47

by Frode Isaksen

[permalink] [raw]
Subject: Re: [PATCH] spinlock not locked when unlocking in atm_dev_register

> On Fri, 2002-03-01 at 12:46, Frode Isaksen wrote:
>> If you compile the kernel with SMP and spinlock debugging, BUG() will be
>> called when registering your atm driver, since the "atm_dev_lock" spinlock is
>> not locked when unlocking it.
>
> I don't have any knowledge of the source in question, but wouldn't a
> possibility (perhaps even more likely) be that you should _add_ the
> spin_lock instead of remove the spin_unlocks ?
The atm_dev_register function is calling functions that are using the same
spinlock, so you cannot just lock the spinlock when entering the function..

Hilsen,
Frode

2002-03-04 18:59:00

by Robert Love

[permalink] [raw]
Subject: Re: [PATCH] spinlock not locked when unlocking in atm_dev_register

On Mon, 2002-03-04 at 03:29, Frode Isaksen wrote:

> The atm_dev_register function is calling functions that are using the same
> spinlock, so you cannot just lock the spinlock when entering the function..

Ah, OK -- my apologies. Then, a few things need to be checked:

- atm_dev_register (ignoring its callees) does not need a lock
- every callee of atm_dev_register that does need the lock
acquires and releases it itself (this is good design, too)

I suspect the second point may be missing the releases in cases, since
those spin_unlocks were in the code.

Robert Love