Documentation/firmware_class/firmware_sample_driver.c

PATCH In review Anders Blomdell 3 participants 5 messages Jun 27, 2007 – Jun 29, 2007

lore

Share & tools

lore

Anders Blomdell

author

2007-06-27 15:23 UTC #0 lore raw

PATCH 1/1 Documentation/firmware_class/firmware_sample_driver.c

Signed-off-by Anders Blomdell <[email protected]>

Minor modifications to make the example load and unload without Oops

This is what unpatched version generates with 2.6.21.3

kernel: Oops: 0000 [#2]
...

kernel: Call Trace:
kernel:  [<c04ae541>] sysfs_create_dir+0x49/0x63
kernel:  [<c04e76ae>] kobject_shadow_add+0xd5/0x17d
kernel:  [<c04e7908>] kobject_set_name+0x2b/0x92
kernel:  [<c0556444>] device_add+0x9a/0x58e
kernel:  [<c0620b65>] klist_init+0x23/0x2e
kernel:  [<c055bfee>] _request_firmware+0x116/0x29f
kernel:  [<c062149f>] wait_for_completion+0x8b/0x93
kernel:  [<c055c20b>] request_firmware+0xf/0x11
kernel:  [<e09420d9>] sample_init+0x59/0xc0 [firmware_sample_driver]
kernel:  [<c0446c92>] sys_init_module+0x16b7/0x17ee
kernel:  [<c04294de>] printk+0x0/0x1f
kernel:  [<c0488436>] mntput_no_expire+0x11/0x6e
kernel:  [<c0404fa8>] syscall_call+0x7/0xb

diff -u Documentation/firmware_class/firmware_sample_driver.c /tmp/fw/firmware_sample_driver.c
--- Documentation/firmware_class/firmware_sample_driver.c 2007-05-24
23:22:47.000000000 +0200
+++ /tmp/fw/firmware_sample_driver.c 2007-06-27 16:51:16.000000000 +0200
@@ -15,11 +15,16 @@

#include "linux/firmware.h"

+static void ghost_release(struct device *dev)
+{
+ printk(KERN_DEBUG "firmware_sample_driver: ghost_release\n");
+}
+

 static struct device ghost_device = {
        .bus_id    = "ghost0",

+ .release = ghost_release
};

-
static void sample_firmware_load(char *firmware, int size)

 {
        u8 buf[size+1];

@@ -97,16 +102,17 @@

static int sample_init(void)
{
- device_initialize(&ghost_device);
+ device_register(&ghost_device);
/* since there is no real hardware insertion I just call the
* sample probe functions here */
- sample_probe_specific();
+ /* sample_probe_specific(); */

        sample_probe_default();
-       sample_probe_async();
+       /*sample_probe_async();*/
        return 0;

}
static void __exit sample_exit(void)
{
+ device_unregister(&ghost_device);
}

module_init (sample_init);

Signature · 5 lines

Anders Blomdell                  Email: [email protected]
Department of Automatic Control
Lund University                  Phone:    +46 46 222 4625
P.O. Box 118                     Fax:      +46 46 138118
SE-221 00 Lund, Sweden

Marcel Holtmann

2007-06-27 17:51 UTC #1 lore raw

Hi Anders,

Quoted 31 lines

Signed-off-by Anders Blomdell <[email protected]>
Minor modifications to make the example load and unload without Oops
This is what unpatched version generates with 2.6.21.3
kernel: Oops: 0000 [#2]
...
kernel: Call Trace:
kernel:  [<c04ae541>] sysfs_create_dir+0x49/0x63
kernel:  [<c04e76ae>] kobject_shadow_add+0xd5/0x17d
kernel:  [<c04e7908>] kobject_set_name+0x2b/0x92
kernel:  [<c0556444>] device_add+0x9a/0x58e
kernel:  [<c0620b65>] klist_init+0x23/0x2e
kernel:  [<c055bfee>] _request_firmware+0x116/0x29f
kernel:  [<c062149f>] wait_for_completion+0x8b/0x93
kernel:  [<c055c20b>] request_firmware+0xf/0x11
kernel:  [<e09420d9>] sample_init+0x59/0xc0 [firmware_sample_driver]
kernel:  [<c0446c92>] sys_init_module+0x16b7/0x17ee
kernel:  [<c04294de>] printk+0x0/0x1f
kernel:  [<c0488436>] mntput_no_expire+0x11/0x6e
kernel:  [<c0404fa8>] syscall_call+0x7/0xb
diff -u Documentation/firmware_class/firmware_sample_driver.c /tmp/fw/firmware_sample_driver.c
--- Documentation/firmware_class/firmware_sample_driver.c 2007-05-24
23:22:47.000000000 +0200
+++ /tmp/fw/firmware_sample_driver.c 2007-06-27 16:51:16.000000000 +0200

Review line 22

Quoted 8 lines

(quoted diff) +5 -0

@@ -15,11 +15,16 @@
 
 #include "linux/firmware.h"
 
+static void ghost_release(struct device *dev)
+{
+       printk(KERN_DEBUG "firmware_sample_driver: ghost_release\n");
+}
+

I know it is a sample driver, but you can't do that. You have to allocate the device and then free it here.

Regards

Marcel

Anders Blomdell

author

2007-06-28 08:47 UTC #2 lore raw

Quoted 36 lines · Marcel Holtmann

Hi Anders,
Quoted 34 lines
Signed-off-by Anders Blomdell <[email protected]>
Minor modifications to make the example load and unload without Oops
This is what unpatched version generates with 2.6.21.3
kernel: Oops: 0000 [#2]
...
kernel: Call Trace:
kernel:  [<c04ae541>] sysfs_create_dir+0x49/0x63
kernel:  [<c04e76ae>] kobject_shadow_add+0xd5/0x17d
kernel:  [<c04e7908>] kobject_set_name+0x2b/0x92
kernel:  [<c0556444>] device_add+0x9a/0x58e
kernel:  [<c0620b65>] klist_init+0x23/0x2e
kernel:  [<c055bfee>] _request_firmware+0x116/0x29f
kernel:  [<c062149f>] wait_for_completion+0x8b/0x93
kernel:  [<c055c20b>] request_firmware+0xf/0x11
kernel:  [<e09420d9>] sample_init+0x59/0xc0 [firmware_sample_driver]
kernel:  [<c0446c92>] sys_init_module+0x16b7/0x17ee
kernel:  [<c04294de>] printk+0x0/0x1f
kernel:  [<c0488436>] mntput_no_expire+0x11/0x6e
kernel:  [<c0404fa8>] syscall_call+0x7/0xb
diff -u Documentation/firmware_class/firmware_sample_driver.c /tmp/fw/firmware_sample_driver.c
--- Documentation/firmware_class/firmware_sample_driver.c 2007-05-24
23:22:47.000000000 +0200
+++ /tmp/fw/firmware_sample_driver.c 2007-06-27 16:51:16.000000000 +0200
(quoted diff) +0 -0
@@ -15,11 +15,16 @@
 
 #include "linux/firmware.h"
 

Quoted 8 lines

Review line 22
Quoted 5 lines
(quoted diff) +5 -0
+static void ghost_release(struct device *dev)
+{
+       printk(KERN_DEBUG "firmware_sample_driver: ghost_release\n");
+}
+
I know it is a sample driver, but you can't do that. You have to allocate the device and then free it here.

Are you sure about this? The code in the example looks very similar to the code in "Linux Device Drivers" 3rd edition, p 382.

Regards

Anders Blomdell

Signature · 5 lines

Anders Blomdell                  Email: [email protected]
Department of Automatic Control
Lund University                  Phone:    +46 46 222 4625
P.O. Box 118                     Fax:      +46 46 138118
SE-221 00 Lund, Sweden

Satyam Sharma

2007-06-28 10:35 UTC #3 lore raw

In reply to Anders Blomdell

Hi,

[ It's good that you're trying to fix this, code in documentation should be setting standards, clearly. ]

Quoted 6 lines · Anders Blomdell

[...]
Minor modifications to make the example load and unload without Oops [...]
static int sample_init(void)
{
(quoted diff) +0 -1
-       device_initialize(&ghost_device);

Review

Quoted 10 lines

(quoted diff) +3 -2

+       device_register(&ghost_device);
        /* since there is no real hardware insertion I just call the
         * sample probe functions here */
-       sample_probe_specific();
+       /* sample_probe_specific(); */
        sample_probe_default();
-       sample_probe_async();
+       /*sample_probe_async();*/
        return 0;
 }

But IMO the above functions should be *fixed* to work properly instead of simply commenting them out. If they are un-fixable, why even keep the broken code in that file (only to mislead readers in future?). You might as well remove those calls (and the function definitions) completely. Best would be to fix them, of course.

My Rs. 0.02,
Satyam

Anders Blomdell

author

2007-06-29 17:50 UTC #4 lore raw

Quoted 23 lines · Satyam Sharma

Hi,
[ It's good that you're trying to fix this, code in documentation should be setting standards, clearly. ]
Quoted 6 lines · Anders Blomdell
[...]
Minor modifications to make the example load and unload without Oops [...]
static int sample_init(void)
{
(quoted diff) +0 -1
-       device_initialize(&ghost_device);
Review
Quoted 10 lines
(quoted diff) +3 -2
+       device_register(&ghost_device);
        /* since there is no real hardware insertion I just call the
         * sample probe functions here */
-       sample_probe_specific();
+       /* sample_probe_specific(); */
        sample_probe_default();
-       sample_probe_async();
+       /*sample_probe_async();*/
        return 0;
 }

Quoted 8 lines

But IMO the above functions should be *fixed* to work properly instead of simply commenting them out. If they are un-fixable, why even keep the broken code in that file (only to mislead readers in future?). You might as well remove those calls (and the function definitions) completely. Best would be to fix them, of course.
My Rs. 0.02,
Satyam

Signed-off-by Anders Blomdell <[email protected]>

Minor modifications to make the example load and unload without Oops, with suggestions from Satyam Sharma incorporated.

This is what unpatched version generates with 2.6.21.3

kernel: Oops: 0000 [#2]
...

kernel: Call Trace:
kernel:  [<c04ae541>] sysfs_create_dir+0x49/0x63
kernel:  [<c04e76ae>] kobject_shadow_add+0xd5/0x17d
kernel:  [<c04e7908>] kobject_set_name+0x2b/0x92
kernel:  [<c0556444>] device_add+0x9a/0x58e
kernel:  [<c0620b65>] klist_init+0x23/0x2e
kernel:  [<c055bfee>] _request_firmware+0x116/0x29f
kernel:  [<c062149f>] wait_for_completion+0x8b/0x93
kernel:  [<c055c20b>] request_firmware+0xf/0x11
kernel:  [<e09420d9>] sample_init+0x59/0xc0 [firmware_sample_driver]
kernel:  [<c0446c92>] sys_init_module+0x16b7/0x17ee
kernel:  [<c04294de>] printk+0x0/0x1f
kernel:  [<c0488436>] mntput_no_expire+0x11/0x6e
kernel:  [<c0404fa8>] syscall_call+0x7/0xb

#include "linux/firmware.h"

+static void ghost_release(struct device *dev)
+{
+ printk(KERN_DEBUG "firmware_sample_driver: ghost_release\n");
+}
+

 static struct device ghost_device = {
        .bus_id    = "ghost0",

+ .release = ghost_release
};

-
static void sample_firmware_load(char *firmware, int size)

 {
        u8 buf[size+1];
@@ -32,7 +37,7 @@
 {
        /* uses the default method to get the firmware */
         const struct firmware *fw_entry;

- printk(KERN_INFO "firmware_sample_driver: a ghost device got inserted
:)\n");
+ printk(KERN_INFO "firmware_sample_driver SYNC:\n");

         if(request_firmware(&fw_entry, "sample_driver_fw", &ghost_device)!=0)
        {

@@ -47,27 +52,9 @@

        /* finish setting up the device */

}
-static void sample_probe_specific(void)
-{
- /* Uses some specific hotplug support to get the firmware from
- * userspace directly into the hardware, or via some sysfs file */
-
- /* NOTE: This currently doesn't work */

- printk(KERN_INFO "firmware_sample_driver: a ghost device got inserted
:)\n");
+static char *context = "async context";

- if(request_firmware(NULL, "sample_driver_fw", &ghost_device)!=0)
- {
- printk(KERN_ERR
- "firmware_sample_driver: Firmware load failed\n");
- return;
- }
-
- /* request_firmware blocks until userspace finished, so at
- * this point the firmware should be already in the device */
-
- /* finish setting up the device */
-}
static void sample_probe_async_cont(const struct firmware *fw, void *context)

 {
        if(!fw){
@@ -76,7 +63,7 @@
                return;
        }

- printk(KERN_INFO "firmware_sample_driver: device pointer \"%s\"\n",
+ printk(KERN_INFO "firmware_sample_driver ASYNC: context \"%s\"\n",

               (char *)context);
        sample_firmware_load(fw->data, fw->size);
 }
@@ -84,9 +71,9 @@
 {
        /* Let's say that I can't sleep */
        int error;

- error = request_firmware_nowait (THIS_MODULE, FW_ACTION_NOHOTPLUG,
+ error = request_firmware_nowait (THIS_MODULE, FW_ACTION_HOTPLUG,
"sample_driver_fw", &ghost_device,
- "my device pointer",
+ context,

                                         sample_probe_async_cont);
        if(error){
                printk(KERN_ERR

@@ -97,16 +84,18 @@

static int sample_init(void)
{

-       device_initialize(&ghost_device);
+       int ret = device_register(&ghost_device);
+       if (ret == 0) {
        /* since there is no real hardware insertion I just call the
         * sample probe functions here */
-       sample_probe_specific();
        sample_probe_default();
        sample_probe_async();
-       return 0;
+       }
+       return ret;

}
static void __exit sample_exit(void)
{
+ device_unregister(&ghost_device);
}

module_init (sample_init);

Signature · 5 lines

Anders Blomdell                  Email: [email protected]
Department of Automatic Control
Lund University                  Phone:    +46 46 222 4625
P.O. Box 118                     Fax:      +46 46 138118
SE-221 00 Lund, Sweden