2004-09-16 10:59:03

by Nigel Cunningham

[permalink] [raw]
Subject: [PATCH] Suspend2 Merge: Driver model patches 2/2

Hi.

This simple helper adds support for finding a class given its name. I
use this to locate the frame buffer drivers and move them to the
keep-alive tree while suspending other drivers.

Regards,

Nigel

diff -ruN linux-2.6.9-rc1/drivers/base/class.c software-suspend-linux-2.6.9-rc1-rev3/drivers/base/class.c
--- linux-2.6.9-rc1/drivers/base/class.c 2004-09-07 21:58:30.000000000 +1000
+++ software-suspend-linux-2.6.9-rc1-rev3/drivers/base/class.c 2004-09-09 19:36:24.000000000 +1000
@@ -460,6 +460,20 @@
kobject_put(&class_dev->kobj);
}

+struct class * class_find(char * name)
+{
+ struct class * this_class;
+
+ if (!name)
+ return NULL;
+
+ list_for_each_entry(this_class, &class_subsys.kset.list, subsys.kset.kobj.entry) {
+ if (!(strcmp(this_class->name, name)))
+ return this_class;
+ }
+
+ return NULL;
+}

int class_interface_register(struct class_interface *class_intf)
{
@@ -542,3 +556,5 @@

EXPORT_SYMBOL(class_interface_register);
EXPORT_SYMBOL(class_interface_unregister);
+
+EXPORT_SYMBOL(class_find);
diff -ruN linux-2.6.9-rc1/include/linux/device.h software-suspend-linux-2.6.9-rc1-rev3/include/linux/device.h
--- linux-2.6.9-rc1/include/linux/device.h 2004-09-07 21:58:59.000000000 +1000
+++ software-suspend-linux-2.6.9-rc1-rev3/include/linux/device.h 2004-09-09 19:36:24.000000000 +1000
@@ -163,6 +163,7 @@

extern struct class * class_get(struct class *);
extern void class_put(struct class *);
+extern struct class * class_find(char * name);


struct class_attribute {

--
Nigel Cunningham
Pastoral Worker
Christian Reformed Church of Tuggeranong
PO Box 1004, Tuggeranong, ACT 2901

Many today claim to be tolerant. True tolerance, however, can cope with others
being intolerant.


2004-09-16 14:29:56

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH] Suspend2 Merge: Driver model patches 2/2

On Thu, Sep 16, 2004 at 08:58:51PM +1000, Nigel Cunningham wrote:
>
> This simple helper adds support for finding a class given its name. I
> use this to locate the frame buffer drivers and move them to the
> keep-alive tree while suspending other drivers.
>
> +struct class * class_find(char * name)
> +{
> + struct class * this_class;
> +
> + if (!name)
> + return NULL;
> +
> + list_for_each_entry(this_class, &class_subsys.kset.list, subsys.kset.kobj.entry) {
> + if (!(strcmp(this_class->name, name)))
> + return this_class;
> + }
> +
> + return NULL;
> +}

Ick, no. I've been over this before with the fb people, and am not going
to accept this patch (nevermind that it's broken...) See the lkml
archives for more info on why I don't like this.

thanks,

greg k-h

2004-09-16 22:21:21

by Nigel Cunningham

[permalink] [raw]
Subject: Re: [PATCH] Suspend2 Merge: Driver model patches 2/2

Hi.

On Fri, 2004-09-17 at 00:28, Greg KH wrote:
> On Thu, Sep 16, 2004 at 08:58:51PM +1000, Nigel Cunningham wrote:
> >
> > This simple helper adds support for finding a class given its name. I
> > use this to locate the frame buffer drivers and move them to the
> > keep-alive tree while suspending other drivers.
> >
> > +struct class * class_find(char * name)
> > +{
> > + struct class * this_class;
> > +
> > + if (!name)
> > + return NULL;
> > +
> > + list_for_each_entry(this_class, &class_subsys.kset.list, subsys.kset.kobj.entry) {
> > + if (!(strcmp(this_class->name, name)))
> > + return this_class;
> > + }
> > +
> > + return NULL;
> > +}
>
> Ick, no. I've been over this before with the fb people, and am not going
> to accept this patch (nevermind that it's broken...) See the lkml
> archives for more info on why I don't like this.

Please excuse my ignorance but I don't see how it's broken (their patch
just fills in a field that was left blank previously), and this patch
just makes use of that change. What's the point to device_class if we
don't use it?

That said, I do agree with using Pavel's new enum that includes
_SNAPSHOT and can see that it's a cleaner way in that it requires less
knowledge on suspend's part of what it wants to stay alive.

Regards,

Nigel

--
Nigel Cunningham
Pastoral Worker
Christian Reformed Church of Tuggeranong
PO Box 1004, Tuggeranong, ACT 2901

Many today claim to be tolerant. True tolerance, however, can cope with others
being intolerant.

2004-09-16 22:37:11

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH] Suspend2 Merge: Driver model patches 2/2

On Fri, Sep 17, 2004 at 08:18:47AM +1000, Nigel Cunningham wrote:
> On Fri, 2004-09-17 at 00:28, Greg KH wrote:
> > On Thu, Sep 16, 2004 at 08:58:51PM +1000, Nigel Cunningham wrote:
> > >
> > > This simple helper adds support for finding a class given its name. I
> > > use this to locate the frame buffer drivers and move them to the
> > > keep-alive tree while suspending other drivers.
> > >
> > > +struct class * class_find(char * name)
> > > +{
> > > + struct class * this_class;
> > > +
> > > + if (!name)
> > > + return NULL;
> > > +
> > > + list_for_each_entry(this_class, &class_subsys.kset.list, subsys.kset.kobj.entry) {
> > > + if (!(strcmp(this_class->name, name)))
> > > + return this_class;
> > > + }
> > > +
> > > + return NULL;
> > > +}
> >
> > Ick, no. I've been over this before with the fb people, and am not going
> > to accept this patch (nevermind that it's broken...) See the lkml
> > archives for more info on why I don't like this.
>
> Please excuse my ignorance but I don't see how it's broken

This function, as written is very broken. I will not accept it. Not to
mention the fact that the functionality this function proposes to offer
is not needed either.

> (their patch just fills in a field that was left blank previously),

What patch?

> and this patch just makes use of that change. What's the point to
> device_class if we don't use it?

I don't see a use of device_class in this function. I'm confused.

thanks,

greg k-h

2004-09-16 22:48:19

by Nigel Cunningham

[permalink] [raw]
Subject: Re: [PATCH] Suspend2 Merge: Driver model patches 2/2

Hi.

On Fri, 2004-09-17 at 08:35, Greg KH wrote:
> > > Ick, no. I've been over this before with the fb people, and am not going
> > > to accept this patch (nevermind that it's broken...) See the lkml
> > > archives for more info on why I don't like this.
> >
> > Please excuse my ignorance but I don't see how it's broken
>
> This function, as written is very broken. I will not accept it. Not to

What's broken? (I want to learn what I've done wrong that I'm not
seeing).

> mention the fact that the functionality this function proposes to offer
> is not needed either.
>
> > (their patch just fills in a field that was left blank previously),
>
> What patch?

Attached. Sorry if I wrongly assumed this was the patch you're talking
about.

> > and this patch just makes use of that change. What's the point to
> > device_class if we don't use it?
>
> I don't see a use of device_class in this function. I'm confused.

This patch finds the device_class that the frame buffer drivers
register. It gets called by suspend code I haven't posted yet, which
then moves the drivers in this class from one pm tree to another so that
the frame buffer drivers don't get suspended until it's time for the
atomic snapshot and can be resumed afterwards while we write the rest of
the image, without resuming all drivers. Given Pavel's work with the new
_SNAPSHOT flag, I guess this won't eventually be needed (provided, of
course that drivers do the right thing when called with _SNAPSHOT).

Regards,

Nigel
--
Nigel Cunningham
Pastoral Worker
Christian Reformed Church of Tuggeranong
PO Box 1004, Tuggeranong, ACT 2901

Many today claim to be tolerant. True tolerance, however, can cope with others
being intolerant.


Attachments:
204-frame-buffer-class-support (12.49 kB)

2004-09-16 23:15:42

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH] Suspend2 Merge: Driver model patches 2/2

On Fri, Sep 17, 2004 at 08:49:07AM +1000, Nigel Cunningham wrote:
> Hi.
>
> On Fri, 2004-09-17 at 08:35, Greg KH wrote:
> > > > Ick, no. I've been over this before with the fb people, and am not going
> > > > to accept this patch (nevermind that it's broken...) See the lkml
> > > > archives for more info on why I don't like this.
> > >
> > > Please excuse my ignorance but I don't see how it's broken
> >
> > This function, as written is very broken. I will not accept it. Not to
>
> What's broken? (I want to learn what I've done wrong that I'm not
> seeing).

- No locking when traversing the list.
- Reference count needs to be bumped before returning a pointer to the
object you found.

> > mention the fact that the functionality this function proposes to offer
> > is not needed either.
> >
> > > (their patch just fills in a field that was left blank previously),
> >
> > What patch?
>
> Attached. Sorry if I wrongly assumed this was the patch you're talking
> about.

Ah, no, I've never seen this one, thanks. But it looks sane, I don't
have a problem with it (sysfs will like it, it's not a suspend specific
patch at all.)

thanks,

greg k-h

2004-09-16 23:21:59

by Nigel Cunningham

[permalink] [raw]
Subject: Re: [PATCH] Suspend2 Merge: Driver model patches 2/2

Hi.

On Fri, 2004-09-17 at 09:07, Greg KH wrote:
> > What's broken? (I want to learn what I've done wrong that I'm not
> > seeing).
>
> - No locking when traversing the list.
> - Reference count needs to be bumped before returning a pointer to the
> object you found.

Ah. Fair enough. I haven't seen any problems because the locking is more
abstract: processes are frozen when this runs for suspend. I'll fix it,
but won't bother resubmitting it because Pavel's changes should obsolete
this stuff.

> > > mention the fact that the functionality this function proposes to offer
> > > is not needed either.
> > >
> > > > (their patch just fills in a field that was left blank previously),
> > >
> > > What patch?
> >
> > Attached. Sorry if I wrongly assumed this was the patch you're talking
> > about.
>
> Ah, no, I've never seen this one, thanks. But it looks sane, I don't
> have a problem with it (sysfs will like it, it's not a suspend specific
> patch at all.)

Antonio posted it to LKML last week IIRC, which is why I didn't include
it in the device driver patches. Given Pavel's changes (again), I'm in
two minds as to whether its needed. It's clearly the right thing to do,
but not needed at the moment. Then again, as we noted already, the whole
device_class thing doesn't get a lot of use at the moment.

Regards,

Nigel
--
Nigel Cunningham
Pastoral Worker
Christian Reformed Church of Tuggeranong
PO Box 1004, Tuggeranong, ACT 2901

Many today claim to be tolerant. True tolerance, however, can cope with others
being intolerant.

2004-09-17 20:47:57

by Pavel Machek

[permalink] [raw]
Subject: Re: [PATCH] Suspend2 Merge: Driver model patches 2/2

Hi!

> > Ah, no, I've never seen this one, thanks. But it looks sane, I don't
> > have a problem with it (sysfs will like it, it's not a suspend specific
> > patch at all.)
>
> Antonio posted it to LKML last week IIRC, which is why I didn't include
> it in the device driver patches. Given Pavel's changes (again), I'm in
> two minds as to whether its needed. It's clearly the right thing to do,
> but not needed at the moment. Then again, as we noted already, the whole

If it is not needed right now, go for simple solution and drop
that patch. Interested people can find it in list archives.
--
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms