[PATCH 0/2] media: fully initialize si2168 on resume only when necessary

[PATCH 0/2] media: fully initialize si2168 on resume only when necessary

[Lukas Middendorf]

For the si2168, the firmware file is loaded in si2168_init(). This function is
called at the beginning of actual device usage and on resume. si2168_probe()
does not include firmware loading and full device initialization. In case the
device is not used before suspend, the call on resume can be the first time the
firmware file is read. This is can lead to a system stall and is unreliable.
If the firmware has been loaded before suspend, the kernel firmware loader sets
up automatic caching which allows firmware loads to succeed on later resumes.

It is not useful to fully initialize the device on resume if it has not been
initialized previously. The device is not in active use and is not expected to
be in an initialized state. Therefore initialization should be skipped in this
case.

This patch series adds a separate resume callback to si2168 to skip init in
case the device has not been initialized previously. To allow this, separate
suspend and resume callback pointers have to be added to dvb_frontend_ops. The
new callbacks are only used if they are implemented in a driver, otherwise the
old sleep and init callback is used instead.

Similar dedicated init callback likely also have to be implemented for some
other dvb frontend drivers to prevent first-time firmware loading on resume.

This patch series replaces my earlier patch which calls firmware_request_cache
instead to explicitly set up firmware caching.

Lukas Middendorf (2):
  media dvb_frontend: add suspend and resume callbacks to
    dvb_frontend_ops
  media si2168: fully initialize si2168 on resume only when necessary

 drivers/media/dvb-core/dvb_frontend.c     |  8 ++++++--
 drivers/media/dvb-frontends/si2168.c      | 24 +++++++++++++++++++++++
 drivers/media/dvb-frontends/si2168_priv.h |  1 +
 include/media/dvb_frontend.h              | 13 ++++++++++--
 4 files changed, 42 insertions(+), 4 deletions(-)

-- 
2.31.1

[PATCH 1/2] media dvb_frontend: add suspend and resume callbacks to dvb_frontend_ops

[Lukas Middendorf]

While dvb_tuner_ops already has dedicated suspend and resume callbacks,
dvb_frontend_ops currently does not have them. Add those callbacks and
use them for suspend and resume. If they are not set, the old behavior
of calling sleep or init is used.

This allows dvb_frontend drivers to handle resume differently from init,
and suspend differently from sleep. No change is required for drivers
not needing this functionality.

Signed-off-by: Lukas Middendorf <kernel@tuxforce.de>
---
 drivers/media/dvb-core/dvb_frontend.c |  8 ++++++--
 include/media/dvb_frontend.h          | 13 +++++++++++--
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index fb35697dd93c..9e78191db5c6 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -2912,7 +2912,9 @@ int dvb_frontend_suspend(struct dvb_frontend *fe)
 	else if (fe->ops.tuner_ops.sleep)
 		ret = fe->ops.tuner_ops.sleep(fe);
 
-	if (fe->ops.sleep)
+	if (fe->ops.suspend)
+		ret = fe->ops.suspend(fe);
+	else if (fe->ops.sleep)
 		ret = fe->ops.sleep(fe);
 
 	return ret;
@@ -2928,7 +2930,9 @@ int dvb_frontend_resume(struct dvb_frontend *fe)
 		fe->id);
 
 	fe->exit = DVB_FE_DEVICE_RESUME;
-	if (fe->ops.init)
+	if (fe->ops.resume)
+		ret = fe->ops.resume(fe);
+	else if (fe->ops.init)
 		ret = fe->ops.init(fe);
 
 	if (fe->ops.tuner_ops.resume)
diff --git a/include/media/dvb_frontend.h b/include/media/dvb_frontend.h
index 0d76fa4551b3..e7c44870f20d 100644
--- a/include/media/dvb_frontend.h
+++ b/include/media/dvb_frontend.h
@@ -364,6 +364,10 @@ struct dvb_frontend_internal_info {
  *			allocated by the driver.
  * @init:		callback function used to initialize the tuner device.
  * @sleep:		callback function used to put the tuner to sleep.
+ * @suspend:		callback function used to inform that the Kernel will
+ *			suspend.
+ * @resume:		callback function used to inform that the Kernel is
+ *			resuming from suspend.
  * @write:		callback function used by some demod legacy drivers to
  *			allow other drivers to write data into their registers.
  *			Should not be used on new drivers.
@@ -443,6 +447,8 @@ struct dvb_frontend_ops {
 
 	int (*init)(struct dvb_frontend* fe);
 	int (*sleep)(struct dvb_frontend* fe);
+	int (*suspend)(struct dvb_frontend *fe);
+	int (*resume)(struct dvb_frontend *fe);
 
 	int (*write)(struct dvb_frontend* fe, const u8 buf[], int len);
 
@@ -755,7 +761,8 @@ void dvb_frontend_detach(struct dvb_frontend *fe);
  * &dvb_frontend_ops.tuner_ops.suspend\(\) is available, it calls it. Otherwise,
  * it will call &dvb_frontend_ops.tuner_ops.sleep\(\), if available.
  *
- * It will also call &dvb_frontend_ops.sleep\(\) to put the demod to suspend.
+ * It will also call &dvb_frontend_ops.suspend\(\) to put the demod to suspend,
+ * if available. Otherwise it will call &dvb_frontend_ops.sleep\(\).
  *
  * The drivers should also call dvb_frontend_suspend\(\) as part of their
  * handler for the &device_driver.suspend\(\).
@@ -769,7 +776,9 @@ int dvb_frontend_suspend(struct dvb_frontend *fe);
  *
  * This function resumes the usual operation of the tuner after resume.
  *
- * In order to resume the frontend, it calls the demod &dvb_frontend_ops.init\(\).
+ * In order to resume the frontend, it calls the demod
+ * &dvb_frontend_ops.resume\(\) if available. Otherwise it calls demod
+ * &dvb_frontend_ops.init\(\).
  *
  * If &dvb_frontend_ops.tuner_ops.resume\(\) is available, It, it calls it.
  * Otherwise,t will call &dvb_frontend_ops.tuner_ops.init\(\), if available.
-- 
2.31.1

[PATCH 2/2] media si2168: fully initialize si2168 on resume only when necessary

[Lukas Middendorf]

At connection time (or boot) in si2168_probe(), the firmware is not
loaded to the device and the device is not fully activated. It is
not useful or sensible to do this full initialization on resume in
case it has not been previously initialized and is expected to be
in this initialized state.
Calling si2168_init() and therefore reading the firmware file for
the first time during resume leads to problems and should be avoided.
It is however safe to read the firmware file once it has already been
read outside of a suspend/resume situation.

Add a staus flag 'initialized' to store whether si2168_init() has
successfully been called. If initialization fails (e.g. due to missing
firmware file), the flag is not set.
Register a separate si2168_resume callback which only calls
si2168_init() once the 'initialized' flag has been set and it is safe
to load the firmware at resume.
The first call to si2168_init() will now always happen when the device
is actually used for the first time and never during resume.
This avoids the unsafe firmware file reading and should also speed up
resume by skipping unnecessary device initialization.

Signed-off-by: Lukas Middendorf <kernel@tuxforce.de>
---
 drivers/media/dvb-frontends/si2168.c      | 24 +++++++++++++++++++++++
 drivers/media/dvb-frontends/si2168_priv.h |  1 +
 2 files changed, 25 insertions(+)

diff --git a/drivers/media/dvb-frontends/si2168.c b/drivers/media/dvb-frontends/si2168.c
index 14b93a7d3358..f4ea7a896cdf 100644
--- a/drivers/media/dvb-frontends/si2168.c
+++ b/drivers/media/dvb-frontends/si2168.c
@@ -527,6 +527,7 @@ static int si2168_init(struct dvb_frontend *fe)
 		goto err;
 
 	dev->warm = true;
+	dev->initialized=true;
 warm:
 	/* Init stats here to indicate which stats are supported */
 	c->cnr.len = 1;
@@ -548,6 +549,28 @@ static int si2168_init(struct dvb_frontend *fe)
 	return ret;
 }
 
+static int si2168_resume(struct dvb_frontend *fe)
+{
+	struct i2c_client *client = fe->demodulator_priv;
+	struct si2168_dev *dev = i2c_get_clientdata(client);
+	
+	/* check whether si2168_init() has been called successfully
+	 * outside of a resume cycle. Ony call it (and load firmware)
+	 * in this case. si2168_init() is only called during resume
+	 * once the device has actually been used. Otherwise, leave the
+	 * device untouched.
+	 */
+	if(dev->initialized) {
+		dev_dbg(&client->dev, "previsously initialized, call si2168_init()\n");
+		return si2168_init(fe);
+	}
+	else {
+		dev_dbg(&client->dev, "not initialized yet, skipping init on resume\n");
+		return 0;
+	}
+}
+
+
 static int si2168_sleep(struct dvb_frontend *fe)
 {
 	struct i2c_client *client = fe->demodulator_priv;
@@ -657,6 +680,7 @@ static const struct dvb_frontend_ops si2168_ops = {
 
 	.init = si2168_init,
 	.sleep = si2168_sleep,
+	.resume = si2168_resume,
 
 	.set_frontend = si2168_set_frontend,
 
diff --git a/drivers/media/dvb-frontends/si2168_priv.h b/drivers/media/dvb-frontends/si2168_priv.h
index 18bea5222082..007a02c7fee8 100644
--- a/drivers/media/dvb-frontends/si2168_priv.h
+++ b/drivers/media/dvb-frontends/si2168_priv.h
@@ -37,6 +37,7 @@ struct si2168_dev {
 	u8 ts_mode;
 	unsigned int active:1;
 	unsigned int warm:1;
+	unsigned int initialized:1;
 	unsigned int ts_clock_inv:1;
 	unsigned int ts_clock_gapped:1;
 	unsigned int spectral_inversion:1;
-- 
2.31.1