Fix issues in em28xx

While testing a PCTV tripleSTICK 292e, a couple of issues were discovered. Firstly, disconnecting the device causes a lockdep splat, which is addressed in the first patch.

Secondly, a number of kernel messages are missing their terminating newline characters, and these are spread over a range of commits and a range of kernel versions. Therefore, I've split the fixes up by offending commit, which should make backporting to stable trees easier. (Some need to be applied to 3.14 and on, some to 3.15 and on, etc.)

It isn't clear who is the maintainer for this driver; there is no MAINTAINERS entry. If there is a maintainer, please ensure that they add themselves to this critical file. Thanks.

 drivers/media/usb/em28xx/em28xx-audio.c |  8 ++++----
 drivers/media/usb/em28xx/em28xx-core.c  |  4 ++--
 drivers/media/usb/em28xx/em28xx-dvb.c   | 14 +++++++-------
 drivers/media/usb/em28xx/em28xx-input.c |  9 ++++-----
 drivers/media/usb/em28xx/em28xx-video.c |  6 +++---
 5 files changed, 20 insertions(+), 21 deletions(-)

FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Removing the em28xx-rc module results in the following lockdep splat, which is caused by trying to call cancel_delayed_work_sync() on an uninitialised delayed work. Fix this by ensuring we always initialise the work.

INFO: trying to register non-static key.
the code is fine but needs lockdep annotation.
turning off the locking correctness validator.

CPU: 0 PID: 2183 Comm: rmmod Not tainted 3.18.0+ #1464
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
Backtrace:
[<c0012228>] (dump_backtrace) from [<c00123c0>] (show_stack+0x18/0x1c)
 r6:c1419d2c r5:00000000 r4:00000000 r3:00000000
[<c00123a8>] (show_stack) from [<c06e2550>] (dump_stack+0x7c/0x98)
[<c06e24d4>] (dump_stack) from [<c0061c94>] (__lock_acquire+0x16d4/0x1bb0)
 r4:edf19f74 r3:df049380
[<c00605c0>] (__lock_acquire) from [<c00626d4>] (lock_acquire+0xb0/0x124)
 r10:00000000 r9:c003ba90 r8:00000000 r7:00000000 r6:00000000 r5:edf19f74
 r4:00000000
[<c0062624>] (lock_acquire) from [<c003bad4>] (flush_work+0x44/0x264)
 r10:00000000 r9:eaa86000 r8:edf190b0 r7:edf19f74 r6:00000001 r5:edf19f64
 r4:00000000
[<c003ba90>] (flush_work) from [<c003d8f0>] (__cancel_work_timer+0x8c/0x124)
 r7:00000000 r6:00000001 r5:00000000 r4:edf19f64
[<c003d864>] (__cancel_work_timer) from [<c003d99c>] (cancel_delayed_work_sync+0x14/0x18)
 r7:00000000 r6:eccc3600 r5:00000000 r4:edf19000
[<c003d988>] (cancel_delayed_work_sync) from [<bf0b5c10>] (em28xx_ir_fini+0x48/0xd8 [em28xx_rc])
[<bf0b5bc8>] (em28xx_ir_fini [em28xx_rc]) from [<bf08a0a8>] (em28xx_unregister_extension+0x40/0x94 [em28xx])
 r8:c000edc4 r7:00000081 r6:bf092bf4 r5:bf0b6a2c r4:edf19000 r3:bf0b5bc8
[<bf08a068>] (em28xx_unregister_extension [em28xx]) from [<bf0b64dc>] (em28xx_rc_unregister+0x14/0x1c [em28xx_rc])
 r6:00000800 r5:00000000 r4:bf0b6a50 r3:bf0b64c8
[<bf0b64c8>] (em28xx_rc_unregister [em28xx_rc]) from [<c0096710>] (SyS_delete_module+0x11c/0x180)
[<c00965f4>] (SyS_delete_module) from [<c000ec00>] (ret_fast_syscall+0x0/0x48)
 r6:00000001 r5:beb0f813 r4:b8b17d00

---
 drivers/media/usb/em28xx/em28xx-input.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index d8dc03aadfbd..ef36c49ef166 100644
@@ -654,8 +654,6 @@ static void em28xx_init_buttons(struct em28xx *dev)
 	if (dev->num_button_polling_addresses) {
 		memset(dev->button_polling_last_values, 0,
 		       EM28XX_NUM_BUTTON_ADDRESSES_MAX);
-		INIT_DELAYED_WORK(&dev->buttons_query_work,
-				  em28xx_query_buttons);
 		schedule_delayed_work(&dev->buttons_query_work,
 				      msecs_to_jiffies(dev->button_polling_interval));
 	}

@@ -689,6 +687,7 @@ static int em28xx_ir_init(struct em28xx *dev)
 	}
 
 	kref_get(&dev->ref);
+	INIT_DELAYED_WORK(&dev->buttons_query_work, em28xx_query_buttons);
 
 	if (dev->board.buttons)
 		em28xx_init_buttons(dev);

1.8.3.1

I guess you won't mind if I mis-spell your name too...

Provided all places that touch buttons_query_work are properly updated that's fine, but to me that is fragile and asking for trouble. It's far better to ensure that everything is properly initialised so you don't have to remember to conditionalise every single reference to a work struct.

In any case, delayed work struct initialisation is cheap - it doesn't involve any additional memory, it only initialises the various members of the struct (and the lockdep information for the static key) so there really is no argument against always initialising delayed works or normal works, timers, etc to avoid these kinds of bugs.

Anything to keep the code simple is a good thing.

FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Am 20.12.2014 um 15:51 schrieb Russell King - ARM Linux:

Wow... it seems to be very easy to offend you...
Sorry, that was definitely not my intention. I did not do this on purpose. It was just a simple mistake and I will try to avoid it in the future.

Fair enough !

Regards,
Frank Schäfer

The lockdep splat addressed in a previous commit revealed that at least one message in em28xx-input.c was missing a new line:

em28178 #0: Closing input extensionINFO: trying to register non-static key.

Further inspection shows several other messages also miss a new line. These will be fixed in a subsequent patch.

---
 drivers/media/usb/em28xx/em28xx-audio.c | 2 +-
 drivers/media/usb/em28xx/em28xx-dvb.c   | 2 +-
 drivers/media/usb/em28xx/em28xx-input.c | 2 +-
 drivers/media/usb/em28xx/em28xx-video.c | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

index 44ae1e0661e6..52dc9d70da72 100644
@@ -981,7 +981,7 @@ static int em28xx_audio_fini(struct em28xx *dev)
 		return 0;
 	}
 
-	em28xx_info("Closing audio extension");
+	em28xx_info("Closing audio extension\n");
 
 	if (dev->adev.sndcard) {
 		snd_card_disconnect(dev->adev.sndcard);

index 9877b699c6bc..80c384c390e2 100644
@@ -1724,7 +1724,7 @@ static int em28xx_dvb_fini(struct em28xx *dev)
 	if (!dev->dvb)
 		return 0;
 
-	em28xx_info("Closing DVB extension");
+	em28xx_info("Closing DVB extension\n");
 
 	dvb = dev->dvb;
 	client = dvb->i2c_client_tuner;

index ef36c49ef166..aea22deadc0a 100644
@@ -832,7 +832,7 @@ static int em28xx_ir_fini(struct em28xx *dev)
 		return 0;
 	}
 
-	em28xx_info("Closing input extension");
+	em28xx_info("Closing input extension\n");
 
 	em28xx_shutdown_buttons(dev);
 

index cf7f58b76292..3b8c464bf25a 100644
@@ -1958,7 +1958,7 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
 	if (v4l2 == NULL)
 		return 0;
 
-	em28xx_info("Closing video extension");
+	em28xx_info("Closing video extension\n");
 
 	mutex_lock(&dev->lock);
 

1.8.3.1

Inspection shows that newlines are missing from several kernel messages in em28xx-input. Fix these.

---
 drivers/media/usb/em28xx/em28xx-input.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index aea22deadc0a..4007356d991d 100644
@@ -861,7 +861,7 @@ static int em28xx_ir_suspend(struct em28xx *dev)
 	if (dev->is_audio_only)
 		return 0;
 
-	em28xx_info("Suspending input extension");
+	em28xx_info("Suspending input extension\n");
 	if (ir)
 		cancel_delayed_work_sync(&ir->work);
 	cancel_delayed_work_sync(&dev->buttons_query_work);

@@ -878,7 +878,7 @@ static int em28xx_ir_resume(struct em28xx *dev)
 	if (dev->is_audio_only)
 		return 0;
 
-	em28xx_info("Resuming input extension");
+	em28xx_info("Resuming input extension\n");
 	/* if suspend calls ir_raw_event_unregister(), the should call
 	   ir_raw_event_register() */
 	if (ir)

1.8.3.1

Inspection shows that newlines are missing from several kernel messages in em28xx-core. Fix these.

---
 drivers/media/usb/em28xx/em28xx-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 86461a708abe..37456079f490 100644
@@ -1125,7 +1125,7 @@ int em28xx_suspend_extension(struct em28xx *dev)
 {
 	const struct em28xx_ops *ops = NULL;
 
-	em28xx_info("Suspending extensions");
+	em28xx_info("Suspending extensions\n");
 	mutex_lock(&em28xx_devlist_mutex);
 	list_for_each_entry(ops, &em28xx_extension_devlist, next) {
 		if (ops->suspend)

@@ -1139,7 +1139,7 @@ int em28xx_resume_extension(struct em28xx *dev)
 {
 	const struct em28xx_ops *ops = NULL;
 
-	em28xx_info("Resuming extensions");
+	em28xx_info("Resuming extensions\n");
 	mutex_lock(&em28xx_devlist_mutex);
 	list_for_each_entry(ops, &em28xx_extension_devlist, next) {
 		if (ops->resume)

1.8.3.1

Inspection shows that newlines are missing from several kernel messages in em28xx-audio. Fix these.

---
 drivers/media/usb/em28xx/em28xx-audio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 52dc9d70da72..82d58eb3d32a 100644
@@ -820,7 +820,7 @@ static int em28xx_audio_urb_init(struct em28xx *dev)
 	if (urb_size > ep_size * npackets)
 		npackets = DIV_ROUND_UP(urb_size, ep_size);
 
-	em28xx_info("Number of URBs: %d, with %d packets and %d size",
+	em28xx_info("Number of URBs: %d, with %d packets and %d size\n",
 		    num_urb, npackets, urb_size);
 
 	/* Estimate the bytes per period */

1.8.3.1

Inspection shows that newlines are missing from several kernel messages in em28xx-audio. Fix these.

---
 drivers/media/usb/em28xx/em28xx-audio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 82d58eb3d32a..49a5f9532bd8 100644
@@ -1005,7 +1005,7 @@ static int em28xx_audio_suspend(struct em28xx *dev)
 	if (dev->usb_audio_type != EM28XX_USB_AUDIO_VENDOR)
 		return 0;
 
-	em28xx_info("Suspending audio extension");
+	em28xx_info("Suspending audio extension\n");
 	em28xx_deinit_isoc_audio(dev);
 	atomic_set(&dev->adev.stream_started, 0);
 	return 0;

@@ -1019,7 +1019,7 @@ static int em28xx_audio_resume(struct em28xx *dev)
 	if (dev->usb_audio_type != EM28XX_USB_AUDIO_VENDOR)
 		return 0;
 
-	em28xx_info("Resuming audio extension");
+	em28xx_info("Resuming audio extension\n");
 	/* Nothing to do other than schedule_work() ?? */
 	schedule_work(&dev->adev.wq_trigger);
 	return 0;

1.8.3.1

Inspection shows that newlines are missing from several kernel messages in em28xx-dvb. Fix these.

---
 drivers/media/usb/em28xx/em28xx-dvb.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

index 80c384c390e2..aee70d483264 100644
@@ -1775,17 +1775,17 @@ static int em28xx_dvb_suspend(struct em28xx *dev)
 	if (!dev->board.has_dvb)
 		return 0;
 
-	em28xx_info("Suspending DVB extension");
+	em28xx_info("Suspending DVB extension\n");
 	if (dev->dvb) {
 		struct em28xx_dvb *dvb = dev->dvb;
 
 		if (dvb->fe[0]) {
 			ret = dvb_frontend_suspend(dvb->fe[0]);
-			em28xx_info("fe0 suspend %d", ret);
+			em28xx_info("fe0 suspend %d\n", ret);
 		}
 		if (dvb->fe[1]) {
 			dvb_frontend_suspend(dvb->fe[1]);
-			em28xx_info("fe1 suspend %d", ret);
+			em28xx_info("fe1 suspend %d\n", ret);
 		}
 	}
 

@@ -1802,18 +1802,18 @@ static int em28xx_dvb_resume(struct em28xx *dev)
 	if (!dev->board.has_dvb)
 		return 0;
 
-	em28xx_info("Resuming DVB extension");
+	em28xx_info("Resuming DVB extension\n");
 	if (dev->dvb) {
 		struct em28xx_dvb *dvb = dev->dvb;
 
 		if (dvb->fe[0]) {
 			ret = dvb_frontend_resume(dvb->fe[0]);
-			em28xx_info("fe0 resume %d", ret);
+			em28xx_info("fe0 resume %d\n", ret);
 		}
 
 		if (dvb->fe[1]) {
 			ret = dvb_frontend_resume(dvb->fe[1]);
-			em28xx_info("fe1 resume %d", ret);
+			em28xx_info("fe1 resume %d\n", ret);
 		}
 	}
 

1.8.3.1

Inspection shows that newlines are missing from several kernel messages in em28xx-video. Fix these.

---
 drivers/media/usb/em28xx/em28xx-video.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 3b8c464bf25a..f220c1f376e3 100644
@@ -2007,7 +2007,7 @@ static int em28xx_v4l2_suspend(struct em28xx *dev)
 	if (!dev->has_video)
 		return 0;
 
-	em28xx_info("Suspending video extension");
+	em28xx_info("Suspending video extension\n");
 	em28xx_stop_urbs(dev);
 	return 0;
 }

@@ -2020,7 +2020,7 @@ static int em28xx_v4l2_resume(struct em28xx *dev)
 	if (!dev->has_video)
 		return 0;
 
-	em28xx_info("Resuming video extension");
+	em28xx_info("Resuming video extension\n");
 	/* what do we do here */
 	return 0;
 }

1.8.3.1

Am 20.12.2014 um 13:44 schrieb Russell King - ARM Linux:

(line 3598)

EM28XX VIDEO4LINUX DRIVER

M:    Mauro Carvalho Chehab <[email protected]>
L:    [email protected]
W:    http://linuxtv.org
T:    git git://linuxtv.org/media_tree.git
S:    Maintained
F:    drivers/media/usb/em28xx/

That's fine then - I thought my scripts picked Mauro up as the drivers/media maintainer rather than the driver author.

FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Am 20.12.2014 um 13:44 schrieb Russell King - ARM Linux: