[PATCH 0/6] some fixes and cleanups for the em28xx-based HVR-930C

[PATCH 0/6] some fixes and cleanups for the em28xx-based HVR-930C

[Mauro Carvalho Chehab]

This patch series addresses some issues with suspend2ram of devices
based on DRX-K.

With this patch series, it is now possible to suspend to ram while
streaming. At resume, the stream will continue to play.

While doing that, I added a few other changes:

- I moved the init code to .init. That is an initial step to fix
  suspend to disk;

- There's a fix to an issue that happens at xc5000 removal (sent
  already as a RFC patch);

- A dprintk change at his logic to not require both a boot parameter and
  a dynamic_printk enablement. It also re-adds __func__ to the printks,
  that got previously removed;

- It removes the unused mfe_sharing var from the dvb attach logic.

Mauro Carvalho Chehab (6):
  [media] em28xx: remove firmware before releasing xc5000 priv state
  [media] drxk: Fix debug printks
  [media] em28xx-dvb: remove unused mfe_sharing
  [media] em28xx-dvb: handle to stop/start streaming at PM
  [media] em28xx: move board-specific init code
  [media] drxk: move device init code to .init callback

 drivers/media/dvb-frontends/drxk_hard.c | 117 ++++++++++++++++----------------
 drivers/media/tuners/xc5000.c           |   2 +-
 drivers/media/usb/em28xx/em28xx-dvb.c   |  45 ++++++++----
 3 files changed, 92 insertions(+), 72 deletions(-)

-- 
1.9.3

[PATCH 1/6] [media] em28xx: remove firmware before releasing xc5000 priv state

[Mauro Carvalho Chehab]

hybrid_tuner_release_state() can free the priv state, so we need to
release the firmware before calling it.

root@debian:~# rmmod em28xx_dvb em28xx_v4l drxk em28xx
[ 1992.790039] em2884 #0: Closing DVB extension
[ 1992.797623] xc5000 2-0061: destroying instance
[ 1992.799595] general protection fault: 0000 [#1] PREEMPT SMP
[ 1992.800032] Modules linked in: drxk em28xx_dvb(-) em28xx_v4l videobuf2_vmalloc videobuf2_memops videobuf2_core em28xx tveeprom
[ 1992.800032] CPU: 3 PID: 2095 Comm: rmmod Not tainted 3.17.0-rc5-00734-g214635f-dirty #91
[ 1992.800032] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
[ 1992.800032] task: ffff88003cf700d0 ti: ffff88003c398000 task.ti: ffff88003c398000
[ 1992.800032] RIP: 0010:[<ffffffff81320721>]  [<ffffffff81320721>] release_firmware+0x2c/0x52
[ 1992.800032] RSP: 0000:ffff88003c39be58  EFLAGS: 00010246
[ 1992.800032] RAX: ffffffff8188d408 RBX: 6b6b6b6b6b6b6b6b RCX: 0000000000000006
[ 1992.800032] RDX: 0000000000000004 RSI: ffff88003cf70860 RDI: 6b6b6b6b6b6b6b6b
[ 1992.800032] RBP: ffff88003c39be60 R08: 0000000000000200 R09: 0000000000000001
[ 1992.800032] R10: ffff88003c39bcc0 R11: ffffffff82ac6100 R12: ffff88003bc5a000
[ 1992.800032] R13: ffff88003d6a81c8 R14: 00007fd6e977d090 R15: 0000000000000800
[ 1992.800032] FS:  00007fd6e8633700(0000) GS:ffff88003e200000(0000) knlGS:0000000000000000
[ 1992.800032] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[ 1992.800032] CR2: 0000000000400184 CR3: 000000003b996000 CR4: 00000000000006a0
[ 1992.800032] Stack:
[ 1992.800032]  ffff88003befcc00 ffff88003c39be80 ffffffff813cbd36 ffff88003bc5a000
[ 1992.800032]  ffff88003baa2000 ffff88003c39bea0 ffffffff813e2f75 ffff88003d6a8000
[ 1992.800032]  ffff88003baa2000 ffff88003c39bec8 ffffffffa0046804 ffff88003baa2000
[ 1992.800032] Call Trace:
[ 1992.800032]  [<ffffffff813cbd36>] xc5000_release+0xa0/0xbf
[ 1992.800032]  [<ffffffff813e2f75>] dvb_frontend_detach+0x35/0x7d
[ 1992.800032]  [<ffffffffa0046804>] em28xx_dvb_fini+0x195/0x1d0 [em28xx_dvb]
[ 1992.800032]  [<ffffffffa0009211>] em28xx_unregister_extension+0x3d/0x79 [em28xx]
[ 1992.800032]  [<ffffffffa0048e20>] em28xx_dvb_unregister+0x10/0x1f0 [em28xx_dvb]
[ 1992.800032]  [<ffffffff810942e8>] SyS_delete_module+0x141/0x19e
[ 1992.800032]  [<ffffffff81488792>] system_call_fastpath+0x16/0x1b
[ 1992.800032] Code: 48 85 ff 48 c7 c0 08 d4 88 81 48 89 e5 53 48 89 fb 74 3b 48 3d 08 d4 88 81 74 10 48 8b 50 08 48 39 53 08 74 18 48 83 c0 18 eb e8 <48> 8b 7b 18 48 85 ff 75 13 48 8b 7b 08 e8 32 16 de ff 48 89 df
[ 1992.800032] RIP  [<ffffffff81320721>] release_firmware+0x2c/0x52
[ 1992.800032]  RSP <ffff88003c39be58>
[ 1992.867774] ---[ end trace 499f4df0704fd661 ]---

Reported-by: Johannes Stezenbach <js@linuxtv.org>
Cc: stable@vger.kernel.org
Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

diff --git a/drivers/media/tuners/xc5000.c b/drivers/media/tuners/xc5000.c
index e44c8aba6074..803a0e63d47e 100644
--- a/drivers/media/tuners/xc5000.c
+++ b/drivers/media/tuners/xc5000.c
@@ -1333,9 +1333,9 @@ static int xc5000_release(struct dvb_frontend *fe)
 
 	if (priv) {
 		cancel_delayed_work(&priv->timer_sleep);
-		hybrid_tuner_release_state(priv);
 		if (priv->firmware)
 			release_firmware(priv->firmware);
+		hybrid_tuner_release_state(priv);
 	}
 
 	mutex_unlock(&xc5000_list_mutex);
-- 
1.9.3

[PATCH 2/6] [media] drxk: Fix debug printks

[Mauro Carvalho Chehab]

This patch partially reverts 0fb220f2a5cb. What happened is
that the conversion of debug messages to use pr_debug() was a
bad idea, because one needing to debug would need to both
enable debug level via a modprobe parameter, and then to
enable the dynamic printk's.

So, for now, let's use printk() directly at dprintk().

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

diff --git a/drivers/media/dvb-frontends/drxk_hard.c b/drivers/media/dvb-frontends/drxk_hard.c
index 672195147d01..f140b835c414 100644
--- a/drivers/media/dvb-frontends/drxk_hard.c
+++ b/drivers/media/dvb-frontends/drxk_hard.c
@@ -166,9 +166,9 @@ static unsigned int debug;
 module_param(debug, int, 0644);
 MODULE_PARM_DESC(debug, "enable debug messages");
 
-#define dprintk(level, fmt, arg...) do {			\
-if (debug >= level)						\
-	pr_debug(fmt, ##arg);					\
+#define dprintk(level, fmt, arg...) do {				\
+if (debug >= level)							\
+	printk(KERN_DEBUG KBUILD_MODNAME ": %s " fmt, __func__, ##arg);	\
 } while (0)
 
 
-- 
1.9.3

[PATCH 3/6] [media] em28xx-dvb: remove unused mfe_sharing

[Mauro Carvalho Chehab]

This field is not used on this driver anymore. Remove it.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
index 9682c52d67d1..65a456d2f454 100644
--- a/drivers/media/usb/em28xx/em28xx-dvb.c
+++ b/drivers/media/usb/em28xx/em28xx-dvb.c
@@ -1047,7 +1047,7 @@ static void em28xx_unregister_dvb(struct em28xx_dvb *dvb)
 
 static int em28xx_dvb_init(struct em28xx *dev)
 {
-	int result = 0, mfe_shared = 0;
+	int result = 0;
 	struct em28xx_dvb *dvb;
 
 	if (dev->is_audio_only) {
@@ -1624,9 +1624,6 @@ static int em28xx_dvb_init(struct em28xx *dev)
 	if (result < 0)
 		goto out_free;
 
-	/* MFE lock */
-	dvb->adapter.mfe_shared = mfe_shared;
-
 	em28xx_info("DVB extension successfully initialized\n");
 
 	kref_get(&dev->ref);
-- 
1.9.3

[PATCH 4/6] [media] em28xx-dvb: handle to stop/start streaming at PM

[Mauro Carvalho Chehab]

Before suspending, we should be stoping streaming, as it
makes no sense to deliver pending URBs after resume. Also,
we need to properly reinitialize the streaming after resume,
otherwise if suspend happens while streaming, it won't be
returning back.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
index 65a456d2f454..3d19607bd8f0 100644
--- a/drivers/media/usb/em28xx/em28xx-dvb.c
+++ b/drivers/media/usb/em28xx/em28xx-dvb.c
@@ -1719,6 +1719,9 @@ static int em28xx_dvb_suspend(struct em28xx *dev)
 	if (dev->dvb) {
 		struct em28xx_dvb *dvb = dev->dvb;
 
+		if (dvb->nfeeds)
+			em28xx_stop_streaming(dvb);
+
 		if (dvb->fe[0]) {
 			ret = dvb_frontend_suspend(dvb->fe[0]);
 			em28xx_info("fe0 suspend %d", ret);
@@ -1755,6 +1758,9 @@ static int em28xx_dvb_resume(struct em28xx *dev)
 			ret = dvb_frontend_resume(dvb->fe[1]);
 			em28xx_info("fe1 resume %d", ret);
 		}
+
+		if (dvb->nfeeds)
+			em28xx_start_streaming(dvb);
 	}
 
 	return 0;
-- 
1.9.3

[PATCH 5/6] [media] em28xx: move board-specific init code

[Mauro Carvalho Chehab]

Some drivers are doing some board-specific init.

The same init is also needed during restore, so, let's
move this code to a separate function.

No functional changes should be noticed so far

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
index 3d19607bd8f0..ff46ba46a34d 100644
--- a/drivers/media/usb/em28xx/em28xx-dvb.c
+++ b/drivers/media/usb/em28xx/em28xx-dvb.c
@@ -1045,6 +1045,29 @@ static void em28xx_unregister_dvb(struct em28xx_dvb *dvb)
 	dvb_unregister_adapter(&dvb->adapter);
 }
 
+static void em28xx_dvb_board_init(struct em28xx *dev, struct em28xx_dvb *dvb)
+{
+	/* init frontend */
+	switch (dev->model) {
+	case EM2884_BOARD_HAUPPAUGE_WINTV_HVR_930C:
+		hauppauge_hvr930c_init(dev);
+		break;
+	case EM2884_BOARD_TERRATEC_H5:
+		terratec_h5_init(dev);
+		break;
+	case EM2884_BOARD_PCTV_510E:
+	case EM2884_BOARD_PCTV_520E:
+		pctv_520e_init(dev);
+		break;
+	case EM2884_BOARD_CINERGY_HTC_STICK:
+		terratec_htc_stick_init(dev);
+		break;
+	case EM2884_BOARD_TERRATEC_HTC_USB_XS:
+		terratec_htc_usb_xs_init(dev);
+		break;
+	}
+}
+
 static int em28xx_dvb_init(struct em28xx *dev)
 {
 	int result = 0;
@@ -1093,6 +1116,8 @@ static int em28xx_dvb_init(struct em28xx *dev)
 
 	mutex_lock(&dev->lock);
 	em28xx_set_mode(dev, EM28XX_DIGITAL_MODE);
+	em28xx_dvb_board_init(dev, dvb);
+
 	/* init frontend */
 	switch (dev->model) {
 	case EM2874_BOARD_LEADERSHIP_ISDBT:
@@ -1266,7 +1291,6 @@ static int em28xx_dvb_init(struct em28xx *dev)
 	case EM2884_BOARD_HAUPPAUGE_WINTV_HVR_930C:
 	{
 		struct xc5000_config cfg;
-		hauppauge_hvr930c_init(dev);
 
 		dvb->fe[0] = dvb_attach(drxk_attach,
 					&hauppauge_930c_drxk, &dev->i2c_adap[dev->def_i2c_bus]);
@@ -1298,8 +1322,6 @@ static int em28xx_dvb_init(struct em28xx *dev)
 		break;
 	}
 	case EM2884_BOARD_TERRATEC_H5:
-		terratec_h5_init(dev);
-
 		dvb->fe[0] = dvb_attach(drxk_attach, &terratec_h5_drxk, &dev->i2c_adap[dev->def_i2c_bus]);
 		if (!dvb->fe[0]) {
 			result = -EINVAL;
@@ -1363,8 +1385,6 @@ static int em28xx_dvb_init(struct em28xx *dev)
 		break;
 	case EM2884_BOARD_PCTV_510E:
 	case EM2884_BOARD_PCTV_520E:
-		pctv_520e_init(dev);
-
 		/* attach demodulator */
 		dvb->fe[0] = dvb_attach(drxk_attach, &pctv_520e_drxk,
 				&dev->i2c_adap[dev->def_i2c_bus]);
@@ -1381,8 +1401,6 @@ static int em28xx_dvb_init(struct em28xx *dev)
 		}
 		break;
 	case EM2884_BOARD_CINERGY_HTC_STICK:
-		terratec_htc_stick_init(dev);
-
 		/* attach demodulator */
 		dvb->fe[0] = dvb_attach(drxk_attach, &terratec_htc_stick_drxk,
 					&dev->i2c_adap[dev->def_i2c_bus]);
@@ -1400,8 +1418,6 @@ static int em28xx_dvb_init(struct em28xx *dev)
 		}
 		break;
 	case EM2884_BOARD_TERRATEC_HTC_USB_XS:
-		terratec_htc_usb_xs_init(dev);
-
 		/* attach demodulator */
 		dvb->fe[0] = dvb_attach(drxk_attach, &terratec_htc_stick_drxk,
 					&dev->i2c_adap[dev->def_i2c_bus]);
-- 
1.9.3

[PATCH 6/6] [media] drxk: move device init code to .init callback

[Mauro Carvalho Chehab]

Instead of initializing the board during frontend attach, do it
latter, when DVB core calls the .init() callback.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

diff --git a/drivers/media/dvb-frontends/drxk_hard.c b/drivers/media/dvb-frontends/drxk_hard.c
index f140b835c414..18c499acb4c8 100644
--- a/drivers/media/dvb-frontends/drxk_hard.c
+++ b/drivers/media/dvb-frontends/drxk_hard.c
@@ -541,7 +541,7 @@ error:
 }
 
 
-static int init_state(struct drxk_state *state)
+static void init_state(struct drxk_state *state)
 {
 	/*
 	 * FIXME: most (all?) of the values bellow should be moved into
@@ -773,7 +773,6 @@ static int init_state(struct drxk_state *state)
 
 	state->m_rfmirror = (ul_rf_mirror == 0);
 	state->m_if_agc_pol = false;
-	return 0;
 }
 
 static int drxx_open(struct drxk_state *state)
@@ -6053,11 +6052,33 @@ error:
 
 static int init_drxk(struct drxk_state *state)
 {
+	struct dtv_frontend_properties *p;
 	int status = 0, n = 0;
 	enum drx_power_mode power_mode = DRXK_POWER_DOWN_OFDM;
 	u16 driver_version;
 
 	dprintk(1, "\n");
+
+	/* Initialize stats */
+	p = &state->frontend.dtv_property_cache;
+	p->strength.len = 1;
+	p->cnr.len = 1;
+	p->block_error.len = 1;
+	p->block_count.len = 1;
+	p->pre_bit_error.len = 1;
+	p->pre_bit_count.len = 1;
+	p->post_bit_error.len = 1;
+	p->post_bit_count.len = 1;
+
+	p->strength.stat[0].scale = FE_SCALE_RELATIVE;
+	p->cnr.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
+	p->block_error.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
+	p->block_count.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
+	p->pre_bit_error.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
+	p->pre_bit_count.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
+	p->post_bit_error.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
+	p->post_bit_count.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
+
 	if ((state->m_drxk_state == DRXK_UNINITIALIZED)) {
 		drxk_i2c_lock(state);
 		status = power_up_device(state);
@@ -6276,33 +6297,32 @@ error:
 	return status;
 }
 
-static void load_firmware_cb(const struct firmware *fw,
-			     void *context)
+static void load_firmware_cb(struct drxk_state *state)
 {
-	struct drxk_state *state = context;
+	const struct firmware *fw = NULL;
+	int status;
 
-	dprintk(1, ": %s\n", fw ? "firmware loaded" : "firmware not loaded");
+	status = request_firmware(&fw, state->microcode_name,
+				  state->i2c->dev.parent);
+	if (status < 0)
+		fw = NULL;
+
+	dprintk(1, ": firmware %s %s\n", state->microcode_name,
+		fw ? "loaded" : "not loaded");
 	if (!fw) {
 		pr_err("Could not load firmware file %s.\n",
 			state->microcode_name);
 		pr_info("Copy %s to your hotplug directory!\n",
 			state->microcode_name);
-		state->microcode_name = NULL;
+		pr_info("Trying to use the internal firmware, but this may not work well. Be warned.\n");
 
 		/*
-		 * As firmware is now load asynchronous, it is not possible
-		 * anymore to fail at frontend attach. We might silently
-		 * return here, and hope that the driver won't crash.
-		 * We might also change all DVB callbacks to return -ENODEV
-		 * if the device is not initialized.
 		 * As the DRX-K devices have their own internal firmware,
 		 * let's just hope that it will match a firmware revision
 		 * compatible with this driver and proceed.
 		 */
 	}
 	state->fw = fw;
-
-	init_drxk(state);
 }
 
 static void drxk_release(struct dvb_frontend *fe)
@@ -6737,6 +6757,23 @@ static int drxk_get_tune_settings(struct dvb_frontend *fe,
 	}
 }
 
+static int drxk_init(struct dvb_frontend *fe)
+{
+	struct drxk_state *state = fe->demodulator_priv;
+
+	/*
+	 * FIXME: If we let init to run during resume, the frontend
+	 * won't work anymore, on a suspend2ram. Letting it to be
+	 * initialized also doesn't help for suspend2disk. So, let's
+	 * just return for now. Probably, we need to do something else
+	 * during suspend in order to allow to reload the firmware here.
+	 */
+	if (fe->exit == DVB_FE_DEVICE_RESUME)
+		return 0;
+
+	return init_drxk(state);
+}
+
 static struct dvb_frontend_ops drxk_ops = {
 	/* .delsys will be filled dynamically */
 	.info = {
@@ -6760,6 +6797,7 @@ static struct dvb_frontend_ops drxk_ops = {
 	.release = drxk_release,
 	.sleep = drxk_sleep,
 	.i2c_gate_ctrl = drxk_gate_ctrl,
+	.init = drxk_init,
 
 	.set_frontend = drxk_set_parameters,
 	.get_tune_settings = drxk_get_tune_settings,
@@ -6773,10 +6811,8 @@ static struct dvb_frontend_ops drxk_ops = {
 struct dvb_frontend *drxk_attach(const struct drxk_config *config,
 				 struct i2c_adapter *i2c)
 {
-	struct dtv_frontend_properties *p;
 	struct drxk_state *state = NULL;
 	u8 adr = config->adr;
-	int status;
 
 	dprintk(1, "\n");
 	state = kzalloc(sizeof(struct drxk_state), GFP_KERNEL);
@@ -6802,7 +6838,6 @@ struct dvb_frontend *drxk_attach(const struct drxk_config *config,
 		state->m_dvbc_static_clk = true;
 	}
 
-
 	if (config->mpeg_out_clk_strength)
 		state->m_ts_clockk_strength = config->mpeg_out_clk_strength & 0x07;
 	else
@@ -6827,48 +6862,14 @@ struct dvb_frontend *drxk_attach(const struct drxk_config *config,
 	memcpy(&state->frontend.ops, &drxk_ops, sizeof(drxk_ops));
 	state->frontend.demodulator_priv = state;
 
-	init_state(state);
-
 	/* Load firmware and initialize DRX-K */
-	if (state->microcode_name) {
-		const struct firmware *fw = NULL;
+	if (state->microcode_name)
+		load_firmware_cb(state);
 
-		status = request_firmware(&fw, state->microcode_name,
-					  state->i2c->dev.parent);
-		if (status < 0)
-			fw = NULL;
-		load_firmware_cb(fw, state);
-	} else if (init_drxk(state) < 0)
-		goto error;
+	init_state(state);
 
-
-	/* Initialize stats */
-	p = &state->frontend.dtv_property_cache;
-	p->strength.len = 1;
-	p->cnr.len = 1;
-	p->block_error.len = 1;
-	p->block_count.len = 1;
-	p->pre_bit_error.len = 1;
-	p->pre_bit_count.len = 1;
-	p->post_bit_error.len = 1;
-	p->post_bit_count.len = 1;
-
-	p->strength.stat[0].scale = FE_SCALE_RELATIVE;
-	p->cnr.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
-	p->block_error.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
-	p->block_count.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
-	p->pre_bit_error.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
-	p->pre_bit_count.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
-	p->post_bit_error.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
-	p->post_bit_count.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
-
-	pr_info("frontend initialized.\n");
+	pr_info("frontend attached.\n");
 	return &state->frontend;
-
-error:
-	pr_err("not found\n");
-	kfree(state);
-	return NULL;
 }
 EXPORT_SYMBOL(drxk_attach);
 
-- 
1.9.3

Re: [PATCH 0/6] some fixes and cleanups for the em28xx-based HVR-930C

[Johannes Stezenbach]

Hi Mauro,

On Sun, Sep 28, 2014 at 11:23:17PM -0300, Mauro Carvalho Chehab wrote:
> This patch series addresses some issues with suspend2ram of devices
> based on DRX-K.
> 
> With this patch series, it is now possible to suspend to ram while
> streaming. At resume, the stream will continue to play.
> 
> While doing that, I added a few other changes:
> 
> - I moved the init code to .init. That is an initial step to fix
>   suspend to disk;
> 
> - There's a fix to an issue that happens at xc5000 removal (sent
>   already as a RFC patch);
> 
> - A dprintk change at his logic to not require both a boot parameter and
>   a dynamic_printk enablement. It also re-adds __func__ to the printks,
>   that got previously removed;
> 
> - It removes the unused mfe_sharing var from the dvb attach logic.
> 
> Mauro Carvalho Chehab (6):
>   [media] em28xx: remove firmware before releasing xc5000 priv state
>   [media] drxk: Fix debug printks
>   [media] em28xx-dvb: remove unused mfe_sharing
>   [media] em28xx-dvb: handle to stop/start streaming at PM
>   [media] em28xx: move board-specific init code
>   [media] drxk: move device init code to .init callback
> 
>  drivers/media/dvb-frontends/drxk_hard.c | 117 ++++++++++++++++----------------
>  drivers/media/tuners/xc5000.c           |   2 +-
>  drivers/media/usb/em28xx/em28xx-dvb.c   |  45 ++++++++----
>  3 files changed, 92 insertions(+), 72 deletions(-)


Disregarding your mails from the "em28xx breaks after hibernate"
that hibernate doesn't work for you, I decided to give these
changes a try on top of today's media_tree.git
(cf3167c -> 3.17.0-rc5-00741-g9a3fbd8), still inside qemu
(can't upgrade/reboot my main machine right now).

Works!  For hibernate, using "echo reboot >/sys/power/disk", so
the host driver cannot interfere with the qemu driver during hibernate.
Qemu causes several USB resets to the device during
hibernate -> resume, but the USB power is not cut.
It works even while running dvbv5-zap and streaming to mplayer.

I tried both suspend-to-ram and hibernate a couple of times,
at least in Qemu it all works.

There are a lot of drxk debug prints now enabled by default,
not sure if that was intentional.


Thanks,
Johannes

Re: [PATCH 0/6] some fixes and cleanups for the em28xx-based HVR-930C

[Mauro Carvalho Chehab]

Em Mon, 29 Sep 2014 19:44:30 +0200
Johannes Stezenbach <js@linuxtv.org> escreveu:

> Hi Mauro,
> 
> On Sun, Sep 28, 2014 at 11:23:17PM -0300, Mauro Carvalho Chehab wrote:
> > This patch series addresses some issues with suspend2ram of devices
> > based on DRX-K.
> > 
> > With this patch series, it is now possible to suspend to ram while
> > streaming. At resume, the stream will continue to play.
> > 
> > While doing that, I added a few other changes:
> > 
> > - I moved the init code to .init. That is an initial step to fix
> >   suspend to disk;
> > 
> > - There's a fix to an issue that happens at xc5000 removal (sent
> >   already as a RFC patch);
> > 
> > - A dprintk change at his logic to not require both a boot parameter and
> >   a dynamic_printk enablement. It also re-adds __func__ to the printks,
> >   that got previously removed;
> > 
> > - It removes the unused mfe_sharing var from the dvb attach logic.
> > 
> > Mauro Carvalho Chehab (6):
> >   [media] em28xx: remove firmware before releasing xc5000 priv state
> >   [media] drxk: Fix debug printks
> >   [media] em28xx-dvb: remove unused mfe_sharing
> >   [media] em28xx-dvb: handle to stop/start streaming at PM
> >   [media] em28xx: move board-specific init code
> >   [media] drxk: move device init code to .init callback
> > 
> >  drivers/media/dvb-frontends/drxk_hard.c | 117 ++++++++++++++++----------------
> >  drivers/media/tuners/xc5000.c           |   2 +-
> >  drivers/media/usb/em28xx/em28xx-dvb.c   |  45 ++++++++----
> >  3 files changed, 92 insertions(+), 72 deletions(-)
> 
> 
> Disregarding your mails from the "em28xx breaks after hibernate"
> that hibernate doesn't work for you, I decided to give these
> changes a try on top of today's media_tree.git
> (cf3167c -> 3.17.0-rc5-00741-g9a3fbd8), still inside qemu
> (can't upgrade/reboot my main machine right now).

Well, I think I was not clear: It doesn't work for me when 
I power down the USB or the machine after suspended. If I keep
the device energized, it works ;)

> Works!  For hibernate, using "echo reboot >/sys/power/disk", so
> the host driver cannot interfere with the qemu driver during hibernate.
> Qemu causes several USB resets to the device during
> hibernate -> resume, but the USB power is not cut.
> It works even while running dvbv5-zap and streaming to mplayer.

Thanks for testing it! it is great to have a separate test for
the patches.

Could I add a tested-by tag on those patches?

> I tried both suspend-to-ram and hibernate a couple of times,
> at least in Qemu it all works.
> 
> There are a lot of drxk debug prints now enabled by default,
> not sure if that was intentional.

You're probably probing the device with debug=1, right?

Before the patches, debug=1 were not working well, because:

- it would require to also manually enable the debug lines via
  dynamic_printk sysfs nodes;

- the called function where not displayed. Weveral printks there
  are just dprintk(1, "\n"), used as a way to trace the drxk
  driver. With the old way, this were just printing:
	  drxk: <empty line>

We should likely do some cleanup in some future, removing those
empty printks, as function trace can easily be done via perf, 
and to remove the debug level, using just dynamic_printk.

Regards,
Mauro

Re: [PATCH 0/6] some fixes and cleanups for the em28xx-based HVR-930C

[Johannes Stezenbach]

On Mon, Sep 29, 2014 at 03:30:18PM -0300, Mauro Carvalho Chehab wrote:
> Em Mon, 29 Sep 2014 19:44:30 +0200
> Johannes Stezenbach <js@linuxtv.org> escreveu:
> 
> > Disregarding your mails from the "em28xx breaks after hibernate"
> > that hibernate doesn't work for you, I decided to give these
> > changes a try on top of today's media_tree.git
> > (cf3167c -> 3.17.0-rc5-00741-g9a3fbd8), still inside qemu
> > (can't upgrade/reboot my main machine right now).
> 
> Well, I think I was not clear: It doesn't work for me when 
> I power down the USB or the machine after suspended. If I keep
> the device energized, it works ;)

Ah, OK.  I'll try to test with power removed tomorrow.

> > Works!  For hibernate, using "echo reboot >/sys/power/disk", so
> > the host driver cannot interfere with the qemu driver during hibernate.
> > Qemu causes several USB resets to the device during
> > hibernate -> resume, but the USB power is not cut.
> > It works even while running dvbv5-zap and streaming to mplayer.
> 
> Thanks for testing it! it is great to have a separate test for
> the patches.
> 
> Could I add a tested-by tag on those patches?

sure

> > I tried both suspend-to-ram and hibernate a couple of times,
> > at least in Qemu it all works.
> > 
> > There are a lot of drxk debug prints now enabled by default,
> > not sure if that was intentional.
> 
> You're probably probing the device with debug=1, right?

right, forgot I had added this to the kernel command line
in my qemu start script


Thanks,
Johannes

Re: [PATCH 0/6] some fixes and cleanups for the em28xx-based HVR-930C

[Johannes Stezenbach]

On Mon, Sep 29, 2014 at 08:44:28PM +0200, Johannes Stezenbach wrote:
> On Mon, Sep 29, 2014 at 03:30:18PM -0300, Mauro Carvalho Chehab wrote:
> > Em Mon, 29 Sep 2014 19:44:30 +0200
> > Johannes Stezenbach <js@linuxtv.org> escreveu:
> > 
> > > Disregarding your mails from the "em28xx breaks after hibernate"
> > > that hibernate doesn't work for you, I decided to give these
> > > changes a try on top of today's media_tree.git
> > > (cf3167c -> 3.17.0-rc5-00741-g9a3fbd8), still inside qemu
> > > (can't upgrade/reboot my main machine right now).
> > 
> > Well, I think I was not clear: It doesn't work for me when 
> > I power down the USB or the machine after suspended. If I keep
> > the device energized, it works ;)
> 
> Ah, OK.  I'll try to test with power removed tomorrow.

I test again in qemu, but this time rmmod and blacklist em28xx
on the host, and unplug HVR-930C during hibernate.  As you
said, it breaks.  Log fter resume:

[   83.308267] usb 1-1: reset high-speed USB device number 2 using ehci-pci
[   83.598182] em2884 #0: Resuming extensions
[   83.599187] em2884 #0: Resuming video extensionem2884 #0: Resuming DVB extension
[   83.604115] xc5000: I2C read failed
[   83.607091] xc5000: I2C write failed (len=3)
[   83.607985] xc5000: firmware upload failed...
[   83.608766]  - too many retries. Giving up
[   83.609553] em2884 #0: fe0 resume -22
[   83.615533] PM: restore of devices complete after 937.567 msecs
[   83.617278] PM: Image restored successfully.
[   83.618262] PM: Basic memory bitmaps freed
[   83.619097] Restarting tasks ... done.
[   83.622320] xc5000: I2C read failed
[   83.623197] xc5000: I2C write failed (len=3)
[   83.623198] xc5000: firmware upload failed...
[   83.623198]  - too many retries. Giving up
[   83.624071] drxk: i2c read error at addr 0x29
[   83.624072] drxk: Error -6 on mpegts_stop
[   83.624073] drxk: Error -6 on start
[   84.621531] drxk: i2c read error at addr 0x29
[   84.623426] drxk: Error -6 on get_dvbt_lock_status
[   84.625477] drxk: Error -6 on get_lock_status


Johannes

Re: [PATCH 0/6] some fixes and cleanups for the em28xx-based HVR-930C

[Mauro Carvalho Chehab]

Em Tue, 30 Sep 2014 09:38:10 +0200
Johannes Stezenbach <js@linuxtv.org> escreveu:

> On Mon, Sep 29, 2014 at 08:44:28PM +0200, Johannes Stezenbach wrote:
> > On Mon, Sep 29, 2014 at 03:30:18PM -0300, Mauro Carvalho Chehab wrote:
> > > Em Mon, 29 Sep 2014 19:44:30 +0200
> > > Johannes Stezenbach <js@linuxtv.org> escreveu:
> > > 
> > > > Disregarding your mails from the "em28xx breaks after hibernate"
> > > > that hibernate doesn't work for you, I decided to give these
> > > > changes a try on top of today's media_tree.git
> > > > (cf3167c -> 3.17.0-rc5-00741-g9a3fbd8), still inside qemu
> > > > (can't upgrade/reboot my main machine right now).
> > > 
> > > Well, I think I was not clear: It doesn't work for me when 
> > > I power down the USB or the machine after suspended. If I keep
> > > the device energized, it works ;)
> > 
> > Ah, OK.  I'll try to test with power removed tomorrow.
> 
> I test again in qemu, but this time rmmod and blacklist em28xx
> on the host, and unplug HVR-930C during hibernate.  As you
> said, it breaks.  Log fter resume:
> 
> [   83.308267] usb 1-1: reset high-speed USB device number 2 using ehci-pci
> [   83.598182] em2884 #0: Resuming extensions
> [   83.599187] em2884 #0: Resuming video extensionem2884 #0: Resuming DVB extension
> [   83.604115] xc5000: I2C read failed
> [   83.607091] xc5000: I2C write failed (len=3)
> [   83.607985] xc5000: firmware upload failed...
> [   83.608766]  - too many retries. Giving up
> [   83.609553] em2884 #0: fe0 resume -22
> [   83.615533] PM: restore of devices complete after 937.567 msecs
> [   83.617278] PM: Image restored successfully.
> [   83.618262] PM: Basic memory bitmaps freed
> [   83.619097] Restarting tasks ... done.
> [   83.622320] xc5000: I2C read failed
> [   83.623197] xc5000: I2C write failed (len=3)
> [   83.623198] xc5000: firmware upload failed...
> [   83.623198]  - too many retries. Giving up
> [   83.624071] drxk: i2c read error at addr 0x29
> [   83.624072] drxk: Error -6 on mpegts_stop
> [   83.624073] drxk: Error -6 on start
> [   84.621531] drxk: i2c read error at addr 0x29
> [   84.623426] drxk: Error -6 on get_dvbt_lock_status
> [   84.625477] drxk: Error -6 on get_lock_status

Yeah, this is what I was expecting. I have already a patch that would fix
the issue with xc5000 (the issue is actually at em28xx, that needs to do
a full initialization of the device), but something else is needed to put
drxk into a reliable state.

I am traveling during this week (without the HVR-930C or RF generators).
So, I can't touch on it. I'll seek for some time to try to fix this issue. 

Regards,
Mauro

Re: [PATCH 0/6] some fixes and cleanups for the em28xx-based HVR-930C

[Antti Palosaari]

On 09/30/2014 12:14 PM, Mauro Carvalho Chehab wrote:
> Em Tue, 30 Sep 2014 09:38:10 +0200
> Johannes Stezenbach <js@linuxtv.org> escreveu:
>
>> On Mon, Sep 29, 2014 at 08:44:28PM +0200, Johannes Stezenbach wrote:
>>> On Mon, Sep 29, 2014 at 03:30:18PM -0300, Mauro Carvalho Chehab wrote:

>>> Ah, OK.  I'll try to test with power removed tomorrow.
>>
>> I test again in qemu, but this time rmmod and blacklist em28xx
>> on the host, and unplug HVR-930C during hibernate.  As you
>> said, it breaks.  Log fter resume:
>>
>> [   83.308267] usb 1-1: reset high-speed USB device number 2 using ehci-pci
>> [   83.598182] em2884 #0: Resuming extensions
>> [   83.599187] em2884 #0: Resuming video extensionem2884 #0: Resuming DVB extension
>> [   83.604115] xc5000: I2C read failed
>> [   83.607091] xc5000: I2C write failed (len=3)
>> [   83.607985] xc5000: firmware upload failed...
>> [   83.608766]  - too many retries. Giving up
>> [   83.609553] em2884 #0: fe0 resume -22
>> [   83.615533] PM: restore of devices complete after 937.567 msecs
>> [   83.617278] PM: Image restored successfully.
>> [   83.618262] PM: Basic memory bitmaps freed
>> [   83.619097] Restarting tasks ... done.
>> [   83.622320] xc5000: I2C read failed
>> [   83.623197] xc5000: I2C write failed (len=3)
>> [   83.623198] xc5000: firmware upload failed...
>> [   83.623198]  - too many retries. Giving up
>> [   83.624071] drxk: i2c read error at addr 0x29
>> [   83.624072] drxk: Error -6 on mpegts_stop
>> [   83.624073] drxk: Error -6 on start
>> [   84.621531] drxk: i2c read error at addr 0x29
>> [   84.623426] drxk: Error -6 on get_dvbt_lock_status
>> [   84.625477] drxk: Error -6 on get_lock_status
>
> Yeah, this is what I was expecting. I have already a patch that would fix
> the issue with xc5000 (the issue is actually at em28xx, that needs to do
> a full initialization of the device), but something else is needed to put
> drxk into a reliable state.

As a general level, we have to get rid of all kind of hacks we have used 
in our DTV drivers in order to make things properly and reliable in a 
long term. There is far away too many hacks, which leads all the time 
that kind of problems when some new functionality is added.

That means:
* get rid of I2C gate control and use I2C mux instead
* get rid of homemade clock configs and use clock framework
* get rid of homemade GPIO things and use kernel GPIO framework
* get rid of all hackish solutions to enable HW power and reset, likely 
using regulator framework
* get rid of homemade DVB driver and use I2C/SPI etc models offered by 
core kernel

Basically we should replace all the interfaces drivers needs something 
well known and standard. Individual driver should not be aware, nor need 
to know any HW connections behind busses it uses - it should trust it 
gets all the things when it needs. When it access I2C bus, bus must be 
there fully operational. When it wakes up from sleep and request power 
and clock, those should be delivered without any knowledge about 
underlying connections - it is up to framework to offer those.

Implementing things properly using well known internal APIs / 
frameworks, without own hacks, is only way to get things work properly 
in a long ran. (PS. I haven't followed that discussion, but these kind 
of issues looks just like a coming from hacks I mentioned.).

Amen.
Antti

-- 
http://palosaari.fi/