[PATCH 0/5] last-minute misc ddbridge related changed

[PATCH 0/5] last-minute misc ddbridge related changed

[Daniel Scheller]

From: Daniel Scheller <d.scheller@gmx.net>

This small series improves on a few things related to the recently merged
ddbridge driver update:

stv0910:
  * add an explanation for the mutex_lock needs in gate_ctrl() and release
    the lock in case of I2C ctrl errors
  * announce 100Ksyms/s as minimum symbol rate
ddbridge:
  * fix the teardown and deregistration order wrt the i2c_client usage
  * fix most warnings reported by sparse (sparse still reports four
    spinlock/context warnings, that code will be kept as-is - there
    won't be locking problems since the checked pointers won't suddenly
    vanish)
staging/cxd2099:
  * disable buffer mode by default and make that controllable by a module
    parameter (suggested, requested and tested by Jasmin, modparm switch
    done by me)

Please pull these patches in for the 4.14 merge window, together with the
two ([1], [2]) remaining ones on patchwork - thank you very much!

[1] https://patchwork.linuxtv.org/patch/43350/
[2] https://patchwork.linuxtv.org/patch/43202/

Daniel Scheller (5):
  [media] dvb-frontends/stv0910: release lock on gate_ctrl() failure
  [media] ddbridge: fix teardown/deregistration order in
    ddb_input_detach()
  [media] ddbridge: fix sparse warnings
  [media] staging/cxd2099: Add module parameter for buffer mode
  [media] dvb-frontends/stv0910: change minsymrate to 100Ksyms/s

 drivers/media/dvb-frontends/stv0910.c      | 27 +++++++++++++++++-----
 drivers/media/pci/ddbridge/ddbridge-core.c | 37 +++++++++++++++---------------
 drivers/media/pci/ddbridge/ddbridge-io.h   | 12 +++++-----
 drivers/staging/media/cxd2099/cxd2099.c    | 21 +++++++++--------
 4 files changed, 57 insertions(+), 40 deletions(-)

-- 
2.13.0

[PATCH 1/5] [media] dvb-frontends/stv0910: release lock on gate_ctrl() failure

[Daniel Scheller]

From: Daniel Scheller <d.scheller@gmx.net>

Whenever write_reg() fails to open/close the demod's I2C gate, release the
lock to avoid deadlocking situations. If I2c gate open failed, there's no
need to hold a lock, and if close fails, the mutex_unlock() at the end of
the function is never reached, leaving the mutex_lock in locked state,
which in turn will cause potential for deadlocks. Thus, release the lock
on failure.

While we're touching gate_ctrl(), add some explanation about the need for
locking and the shared I2C bus/gate.

Cc: Julia Lawall <julia.lawall@lip6.fr>
Signed-off-by: Daniel Scheller <d.scheller@gmx.net>
---
 drivers/media/dvb-frontends/stv0910.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/media/dvb-frontends/stv0910.c b/drivers/media/dvb-frontends/stv0910.c
index d1ae9553f74c..0d4a6a115159 100644
--- a/drivers/media/dvb-frontends/stv0910.c
+++ b/drivers/media/dvb-frontends/stv0910.c
@@ -1221,17 +1221,32 @@ static int gate_ctrl(struct dvb_frontend *fe, int enable)
 	struct stv *state = fe->demodulator_priv;
 	u8 i2crpt = state->i2crpt & ~0x86;
 
-	if (enable)
-		mutex_lock(&state->base->i2c_lock);
+	/*
+	 * mutex_lock note: Concurrent I2C gate bus accesses must be
+	 * prevented (STV0910 = dual demod on a single IC with a single I2C
+	 * gate/bus, and two tuners attached), similar to most (if not all)
+	 * other I2C host interfaces/busses.
+	 *
+	 * enable=1 (open I2C gate) will grab the lock
+	 * enable=0 (close I2C gate) releases the lock
+	 */
 
-	if (enable)
+	if (enable) {
+		mutex_lock(&state->base->i2c_lock);
 		i2crpt |= 0x80;
-	else
+	} else {
 		i2crpt |= 0x02;
+	}
 
 	if (write_reg(state, state->nr ? RSTV0910_P2_I2CRPT :
-		      RSTV0910_P1_I2CRPT, i2crpt) < 0)
+		      RSTV0910_P1_I2CRPT, i2crpt) < 0) {
+		/* don't hold the I2C bus lock on failure */
+		mutex_unlock(&state->base->i2c_lock);
+		dev_err(&state->base->i2c->dev,
+			"%s() write_reg failure (enable=%d)\n",
+			__func__, enable);
 		return -EIO;
+	}
 
 	state->i2crpt = i2crpt;
 
-- 
2.13.0

[PATCH 2/5] [media] ddbridge: fix teardown/deregistration order in ddb_input_detach()

[Daniel Scheller]

From: Daniel Scheller <d.scheller@gmx.net>

Brought to attention by Matthias Schwarzott <zzam@gentoo.org> by fixing
possible use-after-free faults in some demod drivers:

In ddb_input_detach(), the i2c_client is unregistered and removed before
dvb frontends are unregistered and detached. While no use-after-free issue
was observed so far, there is another issue with this:

dvb->attached keeps track of the state of the input/output registration,
and the i2c_client unregistration takes place only if everything was
successful (dvb->attached == 0x31). If for some reason an error occurred
during the frontend setup, that value stays at 0x20. In the following
error handling and cleanup, ddb_input_detach() will skip down to that
state, leaving the i2c_client registered, causing refcount issues.

Fix this by moving the i2c_client deregistration down to case 0x20.

Cc: Matthias Schwarzott <zzam@gentoo.org>
Signed-off-by: Daniel Scheller <d.scheller@gmx.net>
---
 drivers/media/pci/ddbridge/ddbridge-core.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/media/pci/ddbridge/ddbridge-core.c b/drivers/media/pci/ddbridge/ddbridge-core.c
index 2464bde1c432..281b6739b0c1 100644
--- a/drivers/media/pci/ddbridge/ddbridge-core.c
+++ b/drivers/media/pci/ddbridge/ddbridge-core.c
@@ -1255,11 +1255,6 @@ static void dvb_input_detach(struct ddb_input *input)
 
 	switch (dvb->attached) {
 	case 0x31:
-		client = dvb->i2c_client[0];
-		if (client) {
-			module_put(client->dev.driver->owner);
-			i2c_unregister_device(client);
-		}
 		if (dvb->fe2)
 			dvb_unregister_frontend(dvb->fe2);
 		if (dvb->fe)
@@ -1273,6 +1268,12 @@ static void dvb_input_detach(struct ddb_input *input)
 		dvb->fe = dvb->fe2 = NULL;
 		/* fallthrough */
 	case 0x20:
+		client = dvb->i2c_client[0];
+		if (client) {
+			module_put(client->dev.driver->owner);
+			i2c_unregister_device(client);
+		}
+
 		dvb_net_release(&dvb->dvbnet);
 		/* fallthrough */
 	case 0x12:
-- 
2.13.0

[PATCH 3/5] [media] ddbridge: fix sparse warnings

[Daniel Scheller]

From: Daniel Scheller <d.scheller@gmx.net>

Fix several

  drivers/media/pci/ddbridge/ddbridge-core.c: warning: symbol ... was not declared. Should it be static?
  drivers/media/pci/ddbridge/ddbridge-core.c: warning: Using plain integer as NULL pointer
  drivers/media/pci/ddbridge/ddbridge-io.h: warning: cast removes address space of expression
  drivers/media/pci/ddbridge/ddbridge-io.h: warning: incorrect type in argument 1 (different address spaces)

at multiple places.

Cc: Ralph Metzler <rjkm@metzlerbros.de>
Signed-off-by: Daniel Scheller <d.scheller@gmx.net>
---
 drivers/media/pci/ddbridge/ddbridge-core.c | 26 +++++++++++++-------------
 drivers/media/pci/ddbridge/ddbridge-io.h   | 12 ++++++------
 2 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/media/pci/ddbridge/ddbridge-core.c b/drivers/media/pci/ddbridge/ddbridge-core.c
index 281b6739b0c1..f4bd4908acdd 100644
--- a/drivers/media/pci/ddbridge/ddbridge-core.c
+++ b/drivers/media/pci/ddbridge/ddbridge-core.c
@@ -69,7 +69,7 @@ MODULE_PARM_DESC(adapter_alloc,
 
 /****************************************************************************/
 
-DEFINE_MUTEX(redirect_lock);
+static DEFINE_MUTEX(redirect_lock);
 
 struct workqueue_struct *ddb_wq;
 
@@ -135,8 +135,8 @@ static void ddb_redirect_dma(struct ddb *dev,
 
 static int ddb_unredirect(struct ddb_port *port)
 {
-	struct ddb_input *oredi, *iredi = 0;
-	struct ddb_output *iredo = 0;
+	struct ddb_input *oredi, *iredi = NULL;
+	struct ddb_output *iredo = NULL;
 
 	/* dev_info(port->dev->dev,
 	 * "unredirect %d.%d\n", port->dev->nr, port->nr);
@@ -160,14 +160,14 @@ static int ddb_unredirect(struct ddb_port *port)
 				ddb_redirect_dma(oredi->port->dev,
 						 oredi->dma, iredo->dma);
 			}
-			port->input[0]->redo = 0;
+			port->input[0]->redo = NULL;
 			ddb_set_dma_table(port->input[0]);
 		}
 		oredi->redi = iredi;
-		port->input[0]->redi = 0;
+		port->input[0]->redi = NULL;
 	}
-	oredi->redo = 0;
-	port->output->redi = 0;
+	oredi->redo = NULL;
+	port->output->redi = NULL;
 
 	ddb_set_dma_table(oredi);
 done:
@@ -209,7 +209,7 @@ static int ddb_redirect(u32 i, u32 p)
 	if (input2) {
 		if (input->redi) {
 			input2->redi = input->redi;
-			input->redi = 0;
+			input->redi = NULL;
 		} else
 			input2->redi = input;
 	}
@@ -811,11 +811,11 @@ static const struct file_operations ci_fops = {
 	.open    = ts_open,
 	.release = ts_release,
 	.poll    = ts_poll,
-	.mmap    = 0,
+	.mmap    = NULL,
 };
 
 static struct dvb_device dvbdev_ci = {
-	.priv    = 0,
+	.priv    = NULL,
 	.readers = 1,
 	.writers = 1,
 	.users   = 2,
@@ -2053,7 +2053,7 @@ static struct dvb_ca_en50221 en_templ = {
 
 static void ci_attach(struct ddb_port *port)
 {
-	struct ddb_ci *ci = 0;
+	struct ddb_ci *ci = NULL;
 
 	ci = kzalloc(sizeof(*ci), GFP_KERNEL);
 	if (!ci)
@@ -2206,7 +2206,7 @@ static void ci_xo2_attach(struct ddb_port *port)
 /****************************************************************************/
 /****************************************************************************/
 
-struct cxd2099_cfg cxd_cfg = {
+static struct cxd2099_cfg cxd_cfg = {
 	.bitrate =  72000,
 	.adr     =  0x40,
 	.polarity = 1,
@@ -3445,7 +3445,7 @@ int ddb_device_create(struct ddb *dev)
 	if (res) {
 		ddb_device_attrs_del(dev);
 		device_destroy(&ddb_class, MKDEV(ddb_major, dev->nr));
-		ddbs[dev->nr] = 0;
+		ddbs[dev->nr] = NULL;
 		dev->ddb_dev = ERR_PTR(-ENODEV);
 	} else
 		ddb_num++;
diff --git a/drivers/media/pci/ddbridge/ddbridge-io.h b/drivers/media/pci/ddbridge/ddbridge-io.h
index ce92e9484075..a4c6bbe09168 100644
--- a/drivers/media/pci/ddbridge/ddbridge-io.h
+++ b/drivers/media/pci/ddbridge/ddbridge-io.h
@@ -27,32 +27,32 @@
 
 static inline u32 ddblreadl(struct ddb_link *link, u32 adr)
 {
-	return readl((char *) (link->dev->regs + (adr)));
+	return readl(link->dev->regs + adr);
 }
 
 static inline void ddblwritel(struct ddb_link *link, u32 val, u32 adr)
 {
-	writel(val, (char *) (link->dev->regs + (adr)));
+	writel(val, link->dev->regs + adr);
 }
 
 static inline u32 ddbreadl(struct ddb *dev, u32 adr)
 {
-	return readl((char *) (dev->regs + (adr)));
+	return readl(dev->regs + adr);
 }
 
 static inline void ddbwritel(struct ddb *dev, u32 val, u32 adr)
 {
-	writel(val, (char *) (dev->regs + (adr)));
+	writel(val, dev->regs + adr);
 }
 
 static inline void ddbcpyto(struct ddb *dev, u32 adr, void *src, long count)
 {
-	return memcpy_toio((char *) (dev->regs + adr), src, count);
+	return memcpy_toio(dev->regs + adr, src, count);
 }
 
 static inline void ddbcpyfrom(struct ddb *dev, void *dst, u32 adr, long count)
 {
-	return memcpy_fromio(dst, (char *) (dev->regs + adr), count);
+	return memcpy_fromio(dst, dev->regs + adr, count);
 }
 
 static inline u32 safe_ddbreadl(struct ddb *dev, u32 adr)
-- 
2.13.0

[PATCH 4/5] [media] staging/cxd2099: Add module parameter for buffer mode

[Daniel Scheller]

From: Daniel Scheller <d.scheller@gmx.net>

The buffer mode of the cxd2099 driver requires more work regarding error
handling and thus can cause issues in some cases, so disable it by default
and make that mode of operation controllable by users via a module
parameter (ie. 'modprobe cxd2099 buffermode=1' enables current behaviour).

The upstream codebase also has the buffer mode disabled by default, so
we should match this (but users still can test things out using the
modparm).

Signed-off-by: Daniel Scheller <d.scheller@gmx.net>
Tested-by: Jasmin Jessich <jasmin@anw.at>
Signed-off-by: Jasmin Jessich <jasmin@anw.at>
---
 drivers/staging/media/cxd2099/cxd2099.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/media/cxd2099/cxd2099.c b/drivers/staging/media/cxd2099/cxd2099.c
index f28916ea69f1..3e30f4864e2b 100644
--- a/drivers/staging/media/cxd2099/cxd2099.c
+++ b/drivers/staging/media/cxd2099/cxd2099.c
@@ -33,8 +33,9 @@
 
 #include "cxd2099.h"
 
-/* comment this line to deactivate the cxd2099ar buffer mode */
-#define BUFFER_MODE 1
+static int buffermode;
+module_param(buffermode, int, 0444);
+MODULE_PARM_DESC(buffermode, "Enable use of the CXD2099AR buffer mode (default: disabled)");
 
 static int read_data(struct dvb_ca_en50221 *ca, int slot, u8 *ebuf, int ecount);
 
@@ -221,7 +222,6 @@ static int write_reg(struct cxd *ci, u8 reg, u8 val)
 	return write_regm(ci, reg, val, 0xff);
 }
 
-#ifdef BUFFER_MODE
 static int write_block(struct cxd *ci, u8 adr, u8 *data, u16 n)
 {
 	int status = 0;
@@ -248,7 +248,6 @@ static int write_block(struct cxd *ci, u8 adr, u8 *data, u16 n)
 	}
 	return status;
 }
-#endif
 
 static void set_mode(struct cxd *ci, int mode)
 {
@@ -642,8 +641,6 @@ static int read_data(struct dvb_ca_en50221 *ca, int slot, u8 *ebuf, int ecount)
 	return len;
 }
 
-#ifdef BUFFER_MODE
-
 static int write_data(struct dvb_ca_en50221 *ca, int slot, u8 *ebuf, int ecount)
 {
 	struct cxd *ci = ca->data;
@@ -658,7 +655,6 @@ static int write_data(struct dvb_ca_en50221 *ca, int slot, u8 *ebuf, int ecount)
 	mutex_unlock(&ci->lock);
 	return ecount;
 }
-#endif
 
 static struct dvb_ca_en50221 en_templ = {
 	.read_attribute_mem  = read_attribute_mem,
@@ -669,11 +665,8 @@ static struct dvb_ca_en50221 en_templ = {
 	.slot_shutdown       = slot_shutdown,
 	.slot_ts_enable      = slot_ts_enable,
 	.poll_slot_status    = poll_slot_status,
-#ifdef BUFFER_MODE
 	.read_data           = read_data,
 	.write_data          = write_data,
-#endif
-
 };
 
 struct dvb_ca_en50221 *cxd2099_attach(struct cxd2099_cfg *cfg,
@@ -703,6 +696,14 @@ struct dvb_ca_en50221 *cxd2099_attach(struct cxd2099_cfg *cfg,
 	ci->en.data = ci;
 	init(ci);
 	dev_info(&i2c->dev, "Attached CXD2099AR at %02x\n", ci->cfg.adr);
+
+	if (!buffermode) {
+		ci->en.read_data = NULL;
+		ci->en.write_data = NULL;
+	} else {
+		dev_info(&i2c->dev, "Using CXD2099AR buffer mode");
+	}
+
 	return &ci->en;
 }
 EXPORT_SYMBOL(cxd2099_attach);
-- 
2.13.0

[PATCH 5/5] [media] dvb-frontends/stv0910: change minsymrate to 100Ksyms/s

[Daniel Scheller]

From: Daniel Scheller <d.scheller@gmx.net>

The demodulator supports symbol rates as low as 100Ksyms/s - the demod
setup in start() already handles such low symbol rates and reviewers
of stv0910 equipped cards even found and tested transponders with
SRs in that range. So, announce this in the fe_ops.

Cc: Ralph Metzler <rjkm@metzlerbros.de>
Cc: Richard Scobie <r.scobie@clear.net.nz>
Signed-off-by: Daniel Scheller <d.scheller@gmx.net>
---
 drivers/media/dvb-frontends/stv0910.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/dvb-frontends/stv0910.c b/drivers/media/dvb-frontends/stv0910.c
index 0d4a6a115159..8bf855c301f5 100644
--- a/drivers/media/dvb-frontends/stv0910.c
+++ b/drivers/media/dvb-frontends/stv0910.c
@@ -1698,7 +1698,7 @@ static const struct dvb_frontend_ops stv0910_ops = {
 		.frequency_max		= 2150000,
 		.frequency_stepsize	= 0,
 		.frequency_tolerance	= 0,
-		.symbol_rate_min	= 1000000,
+		.symbol_rate_min	= 100000,
 		.symbol_rate_max	= 70000000,
 		.caps			= FE_CAN_INVERSION_AUTO |
 					  FE_CAN_FEC_AUTO       |
-- 
2.13.0