Here's the patch as we discussed.
diff -ur --new-file a/drivers/base/interface.c b/drivers/base/interface.c
--- a/drivers/base/interface.c Wed Aug 14 17:09:28 2002
+++ b/drivers/base/interface.c Fri Aug 16 22:03:18 2002
@@ -88,8 +88,20 @@
static DEVICE_ATTR(power,"power",S_IWUSR | S_IRUGO,
device_read_power,device_write_power);
+
+static ssize_t device_read_driver(struct device * dev, char * buf,
size_t count, loff_t off)
+{
+ if (dev->driver)
+ return off ? 0 : sprintf(buf,"%s\n",dev->driver->name);
+ else
+ return 0;
+}
+
+static DEVICE_ATTR(driver,"driver",S_IRUGO,device_read_driver,NULL);
+
struct device_attribute * device_default_files[] = {
&dev_attr_name,
&dev_attr_power,
+ &dev_attr_driver,
NULL,
};
Also if You're interested here's the write support for "driver".
diff -ur --new-file a/drivers/base/base.h b/drivers/base/base.h
--- a/drivers/base/base.h Fri Aug 16 12:20:18 2002
+++ b/drivers/base/base.h Fri Aug 16 22:17:26 2002
@@ -26,3 +26,5 @@
extern int driver_attach(struct device_driver * drv);
extern void driver_detach(struct device_driver * drv);
+extern int do_driver_detach(struct device * dev, struct device_driver *
drv);
+extern int do_driver_attach(struct device * dev, void * data);
diff -ur --new-file a/drivers/base/core.c b/drivers/base/core.c
--- a/drivers/base/core.c Wed Aug 14 17:09:28 2002
+++ b/drivers/base/core.c Fri Aug 16 22:16:30 2002
@@ -98,7 +98,7 @@
static void device_detach(struct device * dev)
{
- struct device_driver * drv;
+ struct device_driver * drv;
if (dev->driver) {
lock_device(dev);
@@ -117,7 +117,7 @@
}
}
-static int do_driver_attach(struct device * dev, void * data)
+int do_driver_attach(struct device * dev, void * data)
{
struct device_driver * drv = (struct device_driver *)data;
int error = 0;
@@ -134,7 +134,7 @@
return bus_for_each_dev(drv->bus,drv,do_driver_attach);
}
-static int do_driver_detach(struct device * dev, struct device_driver *
drv)
+int do_driver_detach(struct device * dev, struct device_driver * drv)
{
lock_device(dev);
if (dev->driver == drv) {
diff -ur --new-file a/drivers/base/interface.c b/drivers/base/interface.c
--- a/drivers/base/interface.c Fri Aug 16 22:06:41 2002
+++ b/drivers/base/interface.c Fri Aug 16 22:15:29 2002
@@ -8,6 +8,7 @@
#include <linux/device.h>
#include <linux/err.h>
#include <linux/stat.h>
+#include "base.h"
static ssize_t device_read_name(struct device * dev, char * buf, size_t
count, loff_t off)
{
@@ -97,7 +98,44 @@
return 0;
}
-static DEVICE_ATTR(driver,"driver",S_IRUGO,device_read_driver,NULL);
+struct device_driver * find_driver_by_name(struct bus_type * bus, char
* name)
+{
+ struct list_head * pos;
+ struct device_driver * drv;
+ list_for_each (pos, &bus->drivers)
+ {
+ drv = list_entry(pos, struct device_driver, bus_list);
+ if (!strncmp(drv->name,name,strlen(name) - 1))
+ return drv;
+
+ }
+ return NULL;
+
+}
+
+static ssize_t device_write_driver(struct device * dev, char * buf,
size_t count, loff_t off)
+{
+ struct device_driver * drv = NULL;
+ int error = 0;
+ if (off)
+ return 0;
+ if (!dev->bus)
+ return count;
+ if (!dev->driver)
+ {
+ drv = find_driver_by_name(dev->bus, buf);
+ if (drv)
+ error = do_driver_attach(dev,drv);
+
+ } else if (!strnicmp(buf,"remove",6))
+ {
+ error = do_driver_detach(dev, dev->driver);
+ }
+ return error < 0 ? error : count;
+}
+
+static DEVICE_ATTR(driver,"driver",S_IWUSR | S_IRUGO,
+ device_read_driver,device_write_driver);
struct device_attribute * device_default_files[] = {
&dev_attr_name,
I look forward to hearing from you.
Thanks,
Adam
PS: Would you be interested in a patch that would port the pnpbios
driver to the driver model?
On Fri, Aug 16, 2002 at 10:36:00PM +0000, Adam Belay wrote:
> Here's the patch as we discussed.
Um, your email client mangled the patch, dropping tabs and wrapped
lines.
> +static ssize_t device_read_driver(struct device * dev, char * buf,
> size_t count, loff_t off)
> +{
> + if (dev->driver)
> + return off ? 0 : sprintf(buf,"%s\n",dev->driver->name);
> + else
> + return 0;
> +}
Isn't this info already in the "name" file of a driver?
> +struct device_driver * find_driver_by_name(struct bus_type * bus, char
> * name)
> +{
> + struct list_head * pos;
> + struct device_driver * drv;
> + list_for_each (pos, &bus->drivers)
> + {
> + drv = list_entry(pos, struct device_driver, bus_list);
> + if (!strncmp(drv->name,name,strlen(name) - 1))
> + return drv;
> +
> + }
> + return NULL;
> +
> +}
Not that I agree with this patch at all, but you might want to go read
Documentation/CodingStyle to fix up your brace placement properly.
thanks,
greg k-h
Adam Belay wrote:
> +static ssize_t device_read_driver(struct device * dev, char * buf,
> size_t count, loff_t off)
> +{
> + if (dev->driver)
> + return off ? 0 : sprintf(buf,"%s\n",dev->driver->name);
You aren't checking that the name will fit in the supplied buffer - what
is there isn't enough space? Shouldn't you either use snprintf, a
strncpy, or a maximum field width in the sprintf?
diff -ur --new-file a/drivers/base/base.h b/drivers/base/base.h
--- a/drivers/base/base.h Fri Aug 16 12:20:18 2002
+++ b/drivers/base/base.h Fri Aug 16 22:17:26 2002
@@ -26,3 +26,5 @@
extern int driver_attach(struct device_driver * drv);
extern void driver_detach(struct device_driver * drv);
+extern int do_driver_detach(struct device * dev, struct device_driver * drv);
+extern int do_driver_attach(struct device * dev, void * data);
diff -ur --new-file a/drivers/base/core.c b/drivers/base/core.c
--- a/drivers/base/core.c Wed Aug 14 17:09:28 2002
+++ b/drivers/base/core.c Fri Aug 16 22:16:30 2002
@@ -98,7 +98,7 @@
static void device_detach(struct device * dev)
{
- struct device_driver * drv;
+ struct device_driver * drv;
if (dev->driver) {
lock_device(dev);
@@ -117,7 +117,7 @@
}
}
-static int do_driver_attach(struct device * dev, void * data)
+int do_driver_attach(struct device * dev, void * data)
{
struct device_driver * drv = (struct device_driver *)data;
int error = 0;
@@ -134,7 +134,7 @@
return bus_for_each_dev(drv->bus,drv,do_driver_attach);
}
-static int do_driver_detach(struct device * dev, struct device_driver * drv)
+int do_driver_detach(struct device * dev, struct device_driver * drv)
{
lock_device(dev);
if (dev->driver == drv) {
diff -ur --new-file a/drivers/base/interface.c b/drivers/base/interface.c
--- a/drivers/base/interface.c Fri Aug 16 22:06:41 2002
+++ b/drivers/base/interface.c Fri Aug 16 22:15:29 2002
@@ -8,6 +8,7 @@
#include <linux/device.h>
#include <linux/err.h>
#include <linux/stat.h>
+#include "base.h"
static ssize_t device_read_name(struct device * dev, char * buf, size_t count, loff_t off)
{
@@ -97,7 +98,44 @@
return 0;
}
-static DEVICE_ATTR(driver,"driver",S_IRUGO,device_read_driver,NULL);
+struct device_driver * find_driver_by_name(struct bus_type * bus, char * name)
+{
+ struct list_head * pos;
+ struct device_driver * drv;
+ list_for_each (pos, &bus->drivers)
+ {
+ drv = list_entry(pos, struct device_driver, bus_list);
+ if (!strncmp(drv->name,name,strlen(name) - 1))
+ return drv;
+
+ }
+ return NULL;
+
+}
+
+static ssize_t device_write_driver(struct device * dev, char * buf, size_t count, loff_t off)
+{
+ struct device_driver * drv = NULL;
+ int error = 0;
+ if (off)
+ return 0;
+ if (!dev->bus)
+ return count;
+ if (!dev->driver)
+ {
+ drv = find_driver_by_name(dev->bus, buf);
+ if (drv)
+ error = do_driver_attach(dev,drv);
+
+ } else if (!strnicmp(buf,"remove",6))
+ {
+ error = do_driver_detach(dev, dev->driver);
+ }
+ return error < 0 ? error : count;
+}
+
+static DEVICE_ATTR(driver,"driver",S_IWUSR | S_IRUGO,
+ device_read_driver,device_write_driver);
struct device_attribute * device_default_files[] = {
&dev_attr_name,
On Sat, Aug 17, 2002 at 02:10:34PM +0000, Adam Belay wrote:
>
>
> [email protected] wrote:
>
> >
> >Um, your email client mangled the patch, dropping tabs and wrapped
> >lines.
> >
> Thanks for pointing this out. I'll send it as an attachment this time.
> My current client has been causing me a lot of trouble, is there one
> you would suggest I use?
I like mutt, but that's just my opinion :)
Hm, in looking at your attachments, they will not apply either. All the
tabs are gone, something's wrong with your originals. Did you cut and
paste to generate them?
> >
> >Isn't this info already in the "name" file of a driver?
> >
>
> I'm probably just confused but I'm not sure what you mean. This patch
> does the following, as shown previously:
>
> example:
> #cd /driverfs/root/pci0/00:00.0
> #cat driver
> agpgart
Ah, got it, nevermind :)
thanks,
greg k-h
On Fri, 2002-08-16 at 23:36, Adam Belay wrote:
> +static ssize_t device_read_driver(struct device * dev, char * buf,
> size_t count, loff_t off)
> +{
> + if (dev->driver)
> + return off ? 0 : sprintf(buf,"%s\n",dev->driver->name);
> + else
> + return 0;
Unless you can prove without doubt between now and the end of time the
size is sufficient use snprintf
[email protected] wrote:
>On Sat, Aug 17, 2002 at 02:10:34PM +0000, Adam Belay wrote:
>
>>
>>[email protected] wrote:
>>
>>>Um, your email client mangled the patch, dropping tabs and wrapped
>>>lines.
>>>
>>Thanks for pointing this out. I'll send it as an attachment this time.
>> My current client has been causing me a lot of trouble, is there one
>>you would suggest I use?
>>
>
>I like mutt, but that's just my opinion :)
>
>Hm, in looking at your attachments, they will not apply either. All the
>tabs are gone, something's wrong with your originals. Did you cut and
>paste to generate them?
>
I downloaded my patches through the mailing list and applied them:
bash-2.05a$ cat ./driver.patch | patch -p1 -l -d linux
patching file drivers/base/interface.c
bash-2.05a$ cat ./driver2.patch | patch -p1 -l -d linux
patching file drivers/base/base.h
patching file drivers/base/core.c
patching file drivers/base/interface.c
It applies cleanly but . . .
You're right the tabs are gone although when I applied my originals they
weren't. I hate netscape navigator. I gzipped them so netscape can't
mess them up. In the meantime I'm going to download mutt. Thanks for
your help. Let me know if the patch works this time. Also after
looking at the interface code I realized that not just my code used
sprintf. Do you think they should all use snprintf instead or is the
probability of a driver attribute exceeding the one page buffer size so
low that it doesn't matter?
Also I was wondering if you think resource management variables (irq,
io, dma, etc) should live in the device structure like power management
variables do? Global resource management seams interesting to me,
although there already is a proc interface that does list resources, I'm
wondering if the driver model is a good place to put such an interface?
Thanks,
-Adam
On Sat, Aug 17, 2002 at 10:32:30PM +0000, Adam Belay wrote:
> Let me know if the patch works this time.
Yes, your patch does work this time, but the coding style is still wrong :)
thanks,
greg k-h
On Sat, Aug 17, 2002 at 10:32:30PM +0000, Adam Belay wrote:
> Also after looking at the interface code I realized that not just my
> code used sprintf. Do you think they should all use snprintf instead
> or is the probability of a driver attribute exceeding the one page
> buffer size so low that it doesn't matter?
snprintf is always a good idea to be using.
> Also I was wondering if you think resource management variables (irq,
> io, dma, etc) should live in the device structure like power management
> variables do?
Lots of different devices do not have irq, io, and dma assigned to them
(like every USB device). These values should be on a per-bus type (i.e.
most pci devices _do_ have those types of values.
> Global resource management seams interesting to me, although there
> already is a proc interface that does list resources, I'm wondering if
> the driver model is a good place to put such an interface?
Yes it is a good place to put them, as almost every /proc file that does
not deal with processes will eventually be moving to this fs.
thanks,
greg k-h
> I downloaded my patches through the mailing list and applied them:
>
> bash-2.05a$ cat ./driver.patch | patch -p1 -l -d linux
> patching file drivers/base/interface.c
> bash-2.05a$ cat ./driver2.patch | patch -p1 -l -d linux
> patching file drivers/base/base.h
> patching file drivers/base/core.c
> patching file drivers/base/interface.c
>
> It applies cleanly but . . .
patch -l does not imply cleanly. That will ignore the whitespace munging
that your MUA is doing.
> You're right the tabs are gone although when I applied my originals they
> weren't. I hate netscape navigator. I gzipped them so netscape can't
> mess them up. In the meantime I'm going to download mutt. Thanks for
> your help. Let me know if the patch works this time. Also after
> looking at the interface code I realized that not just my code used
> sprintf. Do you think they should all use snprintf instead or is the
> probability of a driver attribute exceeding the one page buffer size so
> low that it doesn't matter?
They should use snprintf. Thanks for pointing that out.
> Also I was wondering if you think resource management variables (irq,
> io, dma, etc) should live in the device structure like power management
> variables do? Global resource management seams interesting to me,
> although there already is a proc interface that does list resources, I'm
> wondering if the driver model is a good place to put such an interface?
Yes. We talked about doing that from the very beginning, and were going to
see how things worked out. There was some dicussion about this at OLS,
too. But, I'm not sure it's ready for it yet.
What would be nice would be some way to cleanly represent conditional
attributes of devices, like resource and power management. I think I
almost have something with the device interface stuff, but I fear it's a
fine line to cross over into Abstraction Hell...
</tangent>
-pat
> Also if You're interested here's the write support for "driver".
This just doesn't make sense. When a device is inserted or a driver is
loaded, the bus tries to attach devices to a driver by comparing the
hardware ID of a device to the list of supported IDs of a driver. This is
handled at the bus level because the format of the ID and the semantics
for matching them are bus-specific.
Regardless, it happens automatically. It just doesn't make sense to let
people try and bind a device to some random driver.
Suppose we did support this. If you write the name of a driver to a file,
we search the bus's list of drivers for a match. We then let the bus
compare the hardware IDs and call probe if it matches.
One big problem is that the IDs in the driver are marked __devinitdata, so
they're thrown away after init (if hotplugging is not enabled). So, we
would have to change every driver.
Besides, it just doesn't make sense. If $user wants to use a different
or third party driver, let them rmmod and insmod.
> PS: Would you be interested in a patch that would port the pnpbios
> driver to the driver model?
Yes.
-pat
[email protected] wrote:
>
>patch -l does not imply cleanly. That will ignore the whitespace munging
>that your MUA is doing.
>
You're right it's incorrect to say cleanly
>>also I was wondering if you think resource management variables (irq,
>>io, dma, etc) should live in the device structure like power management
>>variables do? Global resource management seams interesting to me,
>>although there already is a proc interface that does list resources, I'm
>>wondering if the driver model is a good place to put such an interface?
>>
>
>Yes. We talked about doing that from the very beginning, and were going to
>see how things worked out. There was some discussion about this at OLS,
>too. But, I'm not sure it's ready for it yet.
>
That's great, I think there's a big advantage doing this because if this
data lies in the driver model code then it's very easy to standardize
interfaces for hardware and power management
>
>
>What would be nice would be some way to cleanly represent conditional
>attributes of devices, like resource and power management. I think I
>almost have something with the device interface stuff, but I fear it's a
>fine line to cross over into Abstraction Hell...
>
Could you please send me a patch, if there is one, concerning your work
with conditional attributes, I'd love to take a look. If we could work
something out like this it would solve all kinds of problems. I'll look
into it. Remember that devfs patch I had a while ago. Instead of using
devfs handles I could use kdev_t and export the major and minor through
a conditional attribute. If so should a list of major and minor pairs
be in one file?
Also when you say conditional attributes do you mean conditional in the
device structure as well. In other words do you mean a list or hash of
attributes in the device structure?
Thanks,
Adam
[email protected] wrote:
>>Also if You're interested here's the write support for "driver".
>>
>
>Suppose we did support this. If you write the name of a driver to a file,
>we search the bus's list of drivers for a match. We then let the bus
>compare the hardware IDs and call probe if it matches.
>
That's exactly how my code works.
>
>
>One big problem is that the IDs in the driver are marked __devinitdata, so
>they're thrown away after init (if hotplugging is not enabled). So, we
>would have to change every driver.
>
Found that out when I tested binding the agpgart driver.
>
>
>Besides, it just doesn't make sense. If $user wants to use a different
>or third party driver, let them rmmod and insmod.
>
Ok, I guess that makes sense. My interface was primarily for special
cases anyway. What does need to be done is a user level program that
finds and loads the proper modules automatically. Maybe we could use
the existing hotplug scripts or we could even start from scratch. Maybe
we should make a file in the source tree where driver developers can
list their supported hardware IDs but more importantly documentation on
the attributes registered into driverfs.
>
>
>>PS: Would you be interested in a patch that would port the pnpbios
>>driver to the driver model?
>>
>
>Yes.
>
great!
Thanks,
Adam
On Mon, Aug 19, 2002 at 03:50:57PM +0000, Adam Belay wrote:
> What does need to be done is a user level program that finds and loads
> the proper modules automatically. Maybe we could use the existing
> hotplug scripts or we could even start from scratch.
Um, the current hotplug scripts, and my diethotplug program already do
this. Why write another user program to do this? What problem are you
trying to solve?
> Maybe we should make a file in the source tree where driver developers
> can list their supported hardware IDs but more importantly
> documentation on the attributes registered into driverfs.
The support hardware ids are already in the source code, and can be
easily extracted, that is what depmod(8) does to build the modules.*map
files. See the linux-hotplug documentation at
http://linux-hotplug.sf.net/ and this article:
http://www.linuxjournal.com/article.php?sid=5604
on how all of this works. I need to move that article into the
Documentation/DocBook directory someday soon...
driverfs documentation could be retrieved with the DEVICE_ATTR macro, if
we _really_ throught this would be useful.
thanks,
greg k-h
[email protected] wrote:
>
>Um, the current hotplug scripts, and my diethotplug program already do
>this. Why write another user program to do this? What problem are you
>trying to solve?
>
Nevermind, I wasn't very aware of them. Looking at them now they look
like they'd be great for this. By the way, is diethotplug a space
efficient binary version of the hotplug scripts or is there more to it
then that?
Thanks,
Adam
On Mon, Aug 19, 2002 at 09:54:35PM +0000, Adam Belay wrote:
> By the way, is diethotplug a space efficient binary version of the
> hotplug scripts or is there more to it then that?
Yes it is a space efficient version (the resulting binary is usually
300% smaller than the original modules.*map files being used.) It
achieves these space savings at the expense of flexibility, the binary
is always tied to a specific kernel version.
Embedded and rescue disk distros are using the program, as a replacement
for the hotplug scripts, but the primary purpose is for the program to
live in initramfs and be used as part of the normal kernel boot process.
thanks,
greg k-h
On Mon, Aug 19, 2002 at 08:32:55PM -0700, Greg KH wrote:
> On Mon, Aug 19, 2002 at 09:54:35PM +0000, Adam Belay wrote:
> > By the way, is diethotplug a space efficient binary version of the
> > hotplug scripts or is there more to it then that?
>
> Yes it is a space efficient version (the resulting binary is usually
> 300% smaller than the original modules.*map files being used.) It
> achieves these space savings at the expense of flexibility, the binary
> is always tied to a specific kernel version.
My apologies if you meant 30%
<rant>
Arrgh! Please explain the math. How can something be more
than 100% smaller than something else.
I know it is OT but this marketing math really bugs me.
i've yet to see anyone explain how y = x - (3x) where x < 0
and y <= 0 can yield a positive number without first proving
-1 == 1.
</rant>
--
________________________________________________________________
J.W. Schultz Pegasystems Technologies
email address: [email protected]
Remember Cernan and Schmitt
On Mon, Aug 19, 2002 at 11:51:59PM -0700, jw schultz wrote:
> On Mon, Aug 19, 2002 at 08:32:55PM -0700, Greg KH wrote:
> > On Mon, Aug 19, 2002 at 09:54:35PM +0000, Adam Belay wrote:
> > > By the way, is diethotplug a space efficient binary version of the
> > > hotplug scripts or is there more to it then that?
> >
> > Yes it is a space efficient version (the resulting binary is usually
> > 300% smaller than the original modules.*map files being used.) It
> > achieves these space savings at the expense of flexibility, the binary
> > is always tied to a specific kernel version.
>
> My apologies if you meant 30%
Very sorry, I ment that the original modules.*map files are 300% larger
than the diethotplug binary.
Hm, for the options I use in 2.5.31 I get these results:
modules.pcimap 6273 bytes
modules.usbmap 98513 bytes
modules.ieee1394map 73 bytes
----------------------------------
total 104859 bytes
diethotplug binary 16332 bytes
Looks like the modules.*map files are closer to 650% larger :)
thanks,
greg k-h