dvb-core: Handle failures to create devices

PATCH In review Simon Arlott 2 participants 6 messages May 1, 2007 – May 6, 2007 Media (V4L/DVB)

Share & tools

Simon Arlott

author

2007-05-01 17:12 UTC #0 lore raw

PATCH 1/1 dvb-core: Handle failures to create devices

dvb-core is not started early enough when device drivers that use dvb are compiled in so device_register_device fails (silently) since dvb_class is NULL, this runs dvb_init using subsys_initclass instead of module_init.

dvb_register_device will now check the return value of class_device_create.

Signed-off-by Simon Arlott <[email protected]>

---
drivers/media/dvb/dvb-core/dvbdev.c | 11 +++++++++--

1 files changed, 9 insertions(+), 2 deletions(-)

drivers/media/dvb/dvb-core/dvbdev.c +0 -0

index 7683983..b859a60 100644
@@ -201,6 +201,7 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,

	struct dvb_device *dvbdev;
	struct file_operations *dvbdevfops;

+	struct class_device *clsdev;
	int id;

	mutex_lock(&dvbdev_register_lock);

@@ -242,9 +243,15 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,

	mutex_unlock(&dvbdev_register_lock);

- class_device_create(dvb_class, NULL, MKDEV(DVB_MAJOR, nums2minor(adap->num, type, id)),
+ clsdev = class_device_create(dvb_class, NULL, MKDEV(DVB_MAJOR, nums2minor(adap->num, type, id)),

			    adap->device, "dvb%d.%s%d", adap->num, dnames[type], id);

+ if (IS_ERR(clsdev)) {
+ printk("%s: failed to create device dvb%d.%s%d (%ld)\n", __FUNCTION__,
+ adap->num, dnames[type], id, PTR_ERR(clsdev));
+ return PTR_ERR(clsdev);
+ }
+

	dprintk("DVB: register adapter%d/%s%d @ minor: %i (0x%02x)\n",
		adap->num, dnames[type], id, nums2minor(adap->num, type, id),
		nums2minor(adap->num, type, id));
@@ -431,7 +438,7 @@ static void __exit exit_dvbdev(void)
	unregister_chrdev_region(MKDEV(DVB_MAJOR, 0), MAX_DVB_MINORS);
}

-module_init(init_dvbdev);
+subsys_initcall(init_dvbdev);
module_exit(exit_dvbdev);

MODULE_DESCRIPTION("DVB Core Driver");

Signature · 4 lines

1.5.0.1

-- 
Simon Arlott

Simon Arlott

author

2007-05-01 17:14 UTC #1 lore raw

Quoted 1 line · Simon Arlott

dvb_class is NULL, this runs dvb_init using subsys_initclass instead of module_init.

That should of course be subsystem_initCALL not "initclass".

Signature · 1 line

Simon Arlott

Trent Piepho

2007-05-01 21:21 UTC #2 lore raw

In reply to Simon Arlott

Quoted 5 lines · Simon Arlott

dvb-core is not started early enough when device drivers that use dvb are compiled in so device_register_device fails (silently) since dvb_class is NULL, this runs dvb_init using subsys_initclass instead of module_init.
dvb_register_device will now check the return value of class_device_create.

Good catch.

Review

Quoted 3 lines

(quoted diff) +3 -0

+		printk("%s: failed to create device dvb%d.%s%d (%ld)\n", __FUNCTION__,
+			adap->num, dnames[type], id, PTR_ERR(clsdev));
+		return PTR_ERR(clsdev);

printk(KERN_ERR "...."

Simon Arlott

author

2007-05-01 21:33 UTC #3 lore raw

dvb_register_device will now check the return value of class_device_create. All the printks had missing level prefixes so I've fixed these too.

Signed-off-by Simon Arlott <[email protected]>

---

Quoted 8 lines · Trent Piepho

Quoted 5 lines · Simon Arlott
dvb-core is not started early enough when device drivers that use dvb are compiled in so device_register_device fails (silently) since dvb_class is NULL, this runs dvb_init using subsys_initclass instead of module_init.
dvb_register_device will now check the return value of class_device_create.
Good catch.

Thanks, it's only been 11 months since I first pointed it out.

Quoted 5 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+		printk("%s: failed to create device dvb%d.%s%d (%ld)\n", __FUNCTION__,
+			adap->num, dnames[type], id, PTR_ERR(clsdev));
+		return PTR_ERR(clsdev);

printk(KERN_ERR "...."

I copied another printk from that file without thinking.

drivers/media/dvb/dvb-core/dvbdev.c | 21 ++++++++++++++-------

1 files changed, 14 insertions(+), 7 deletions(-)

drivers/media/dvb/dvb-core/dvbdev.c +0 -0

index 7683983..297da62 100644
@@ -201,6 +201,7 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,

	struct dvb_device *dvbdev;
	struct file_operations *dvbdevfops;

+	struct class_device *clsdev;
	int id;

	mutex_lock(&dvbdev_register_lock);
@@ -208,7 +209,7 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
	if ((id = dvbdev_get_free_id (adap, type)) < 0){
		mutex_unlock(&dvbdev_register_lock);
		*pdvbdev = NULL;
-		printk ("%s: could get find free device id...\n", __FUNCTION__);
+		printk(KERN_ERR "%s: could get find free device id...\n", __FUNCTION__);
		return -ENFILE;
	}

@@ -242,10 +243,16 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,

	mutex_unlock(&dvbdev_register_lock);

- class_device_create(dvb_class, NULL, MKDEV(DVB_MAJOR, nums2minor(adap->num, type, id)),
+ clsdev = class_device_create(dvb_class, NULL, MKDEV(DVB_MAJOR, nums2minor(adap->num, type, id)),

			    adap->device, "dvb%d.%s%d", adap->num, dnames[type], id);

- dprintk("DVB: register adapter%d/%s%d @ minor: %i (0x%02x)\n",
+ if (IS_ERR(clsdev)) {
+ printk(KERN_ERR "%s: failed to create device dvb%d.%s%d (%ld)\n", __FUNCTION__,
+ adap->num, dnames[type], id, PTR_ERR(clsdev));
+ return PTR_ERR(clsdev);
+ }
+
+ dprintk(KERN_DEBUG "DVB: register adapter%d/%s%d @ minor: %i (0x%02x)\n",

		adap->num, dnames[type], id, nums2minor(adap->num, type, id),
		nums2minor(adap->num, type, id));

@@ -304,7 +311,7 @@ int dvb_register_adapter(struct dvb_adapter *adap, const char *name, struct modu

	memset (adap, 0, sizeof(struct dvb_adapter));
	INIT_LIST_HEAD (&adap->device_list);

- printk ("DVB: registering new adapter (%s).\n", name);
+ printk(KERN_INFO "DVB: registering new adapter (%s).\n", name);

	adap->num = num;
	adap->name = name;
@@ -400,13 +407,13 @@ static int __init init_dvbdev(void)
	dev_t dev = MKDEV(DVB_MAJOR, 0);

	if ((retval = register_chrdev_region(dev, MAX_DVB_MINORS, "DVB")) != 0) {
-		printk("dvb-core: unable to get major %d\n", DVB_MAJOR);
+		printk(KERN_ERR "dvb-core: unable to get major %d\n", DVB_MAJOR);
		return retval;
	}

	cdev_init(&dvb_device_cdev, &dvb_device_fops);
	if ((retval = cdev_add(&dvb_device_cdev, dev, MAX_DVB_MINORS)) != 0) {
-		printk("dvb-core: unable to get major %d\n", DVB_MAJOR);
+		printk(KERN_ERR "dvb-core: unable to get major %d\n", DVB_MAJOR);
		goto error;
	}

@@ -431,7 +438,7 @@ static void __exit exit_dvbdev(void)

	unregister_chrdev_region(MKDEV(DVB_MAJOR, 0), MAX_DVB_MINORS);
}

-module_init(init_dvbdev);
+subsys_initcall(init_dvbdev);
module_exit(exit_dvbdev);

MODULE_DESCRIPTION("DVB Core Driver");

Signature · 5 lines

1.5.0.1

-- 
Simon Arlott
09F911029D74E35BD84156C5635688C0

Trent Piepho

2007-05-06 22:50 UTC #4 lore raw

I've tested this and can confirm it works. dvb_class will be set too late without the change to subsys_initcall.

Quoted 2 lines · Simon Arlott

dvb-core is not started early enough when device drivers that use dvb are compiled in so device_register_device fails (silently) since dvb_class is

^dvb_register_device

Quoted 4 lines

NULL, this runs dvb_init using subsys_initcall instead of module_init.
dvb_register_device will now check the return value of class_device_create. All the printks had missing level prefixes so I've fixed these too.

Probably better to make this a separate patch, since it's not related.

Review

Quoted 2 lines

(quoted diff) +1 -1

-		printk ("%s: could get find free device id...\n", __FUNCTION__);
+		printk(KERN_ERR "%s: could get find free device id...\n", __FUNCTION__);

"couldn't find free device id"

If it's ok with you, I'll import your patch as two seperate patches with the spelling errors fixed?

Review

Quoted 3 lines

(quoted diff) +1 -0

+	dprintk(KERN_DEBUG "DVB: register adapter%d/%s%d @ minor: %i (0x%02x)\n",
 		adap->num, dnames[type], id, nums2minor(adap->num, type, id),
 		nums2minor(adap->num, type, id));

The dvb-core dvbdev_debug parameter does nothing but turn on this one single dprintk. I'm tempted to just delete it.

Simon Arlott

author

2007-05-06 22:58 UTC #5 lore raw

Quoted 16 lines · Trent Piepho

I've tested this and can confirm it works. dvb_class will be set too late without the change to subsys_initcall.
Quoted 2 lines · Simon Arlott
dvb-core is not started early enough when device drivers that use dvb are compiled in so device_register_device fails (silently) since dvb_class is
^dvb_register_device
Quoted 4 lines
NULL, this runs dvb_init using subsys_initcall instead of module_init.
dvb_register_device will now check the return value of class_device_create. All the printks had missing level prefixes so I've fixed these too.
Probably better to make this a separate patch, since it's not related.

Quoted 7 lines

Review
Quoted 2 lines
(quoted diff) +1 -1
-		printk ("%s: could get find free device id...\n", __FUNCTION__);
+		printk(KERN_ERR "%s: could get find free device id...\n", __FUNCTION__);
"couldn't find free device id"
If it's ok with you, I'll import your patch as two seperate patches with the spelling errors fixed?

That's fine with me.

Quoted 6 lines

Review
Quoted 3 lines
(quoted diff) +1 -0
+	dprintk(KERN_DEBUG "DVB: register adapter%d/%s%d @ minor: %i (0x%02x)\n",
 		adap->num, dnames[type], id, nums2minor(adap->num, type, id),
 		nums2minor(adap->num, type, id));
The dvb-core dvbdev_debug parameter does nothing but turn on this one single dprintk. I'm tempted to just delete it.

Just always output that information, or remove it? (I have it enabled, although I'm not sure if there's any point).

Signature · 1 line

Simon Arlott