xc2028: firmware loading cleanup

From: Chris Pascoe <c.pascoe@itee.uq.edu.au>

Hold the private lock over set_config and set priv->firm_size to 0 after a
failed firmware load to prevent firmware accidentally being freed on us.

Clean up the firmware load/error messages somewhat and rename priv->version
to priv->firm_version to make it clear which "version" it is.

Signed-off-by: Chris Pascoe <c.pascoe@itee.uq.edu.au>
Signed-off-by: Mauro Carvalho Chehab <mchehab@infradead.org>

1 files changed, 38 insertions, 44 deletions

diff --git a/linux/drivers/media/video/tuner-xc2028.c b/linux/drivers/media/video/tuner-xc2028.c
index 23dcfcc24..fd8714faf 100644
--- a/linux/drivers/media/video/tuner-xc2028.c
+++ b/linux/drivers/media/video/tuner-xc2028.c
@@ -76,8 +76,7 @@ struct xc2028_data {
 
 	struct firmware_description *firm;
 	int			firm_size;
-
-	__u16			version;
+	__u16			firm_version;
 
 	struct xc2028_ctrl	ctrl;
 
@@ -252,6 +251,7 @@ static void free_firmware(struct xc2028_data *priv)
 	kfree(priv->firm);
 
 	priv->firm = NULL;
+	priv->firm_size = 0;
 	priv->need_load_generic = 1;
 }
 
@@ -266,7 +266,7 @@ static int load_all_firmwares(struct dvb_frontend *fe)
 
 	tuner_dbg("%s called\n", __FUNCTION__);
 
-	tuner_info("Reading firmware %s\n", priv->ctrl.fname);
+	tuner_dbg("Reading firmware %s\n", priv->ctrl.fname);
 	rc = request_firmware(&fw, priv->ctrl.fname,
 			      &priv->i2c_props.adap->dev);
 	if (rc < 0) {
@@ -282,40 +282,34 @@ static int load_all_firmwares(struct dvb_frontend *fe)
 	p = fw->data;
 	endp = p + fw->size;
 
-	if (fw->size < sizeof(name) - 1 + 2) {
-		tuner_err("Error: firmware size is zero!\n");
-		rc = -EINVAL;
-		goto done;
+	if (fw->size < sizeof(name) - 1 + 2 + 2) {
+		tuner_err("Error: firmware file %s has invalid size!\n",
+			  priv->ctrl.fname);
+		goto corrupt;
 	}
 
 	memcpy(name, p, sizeof(name) - 1);
 	name[sizeof(name) - 1] = 0;
 	p += sizeof(name) - 1;
 
-	priv->version = le16_to_cpu(*(__u16 *) p);
+	priv->firm_version = le16_to_cpu(*(__u16 *) p);
 	p += 2;
 
-	tuner_info("Firmware: %s, ver %d.%d\n", name,
-		   priv->version >> 8, priv->version & 0xff);
-
-	if (p + 2 > endp)
-		goto corrupt;
-
 	n_array = le16_to_cpu(*(__u16 *) p);
 	p += 2;
 
-	tuner_info("There are %d firmwares at %s\n",
-		   n_array, priv->ctrl.fname);
+	tuner_info("Loading %d firmware images from %s, type: %s, ver %d.%d\n",
+		   n_array, priv->ctrl.fname, name,
+		   priv->firm_version >> 8, priv->firm_version & 0xff);
 
 	priv->firm = kzalloc(sizeof(*priv->firm) * n_array, GFP_KERNEL);
-
-	if (!fw) {
-		tuner_err("Not enough memory for reading firmware.\n");
+	if (priv->firm == NULL) {
+		tuner_err("Not enough memory to load firmware file.\n");
 		rc = -ENOMEM;
-		goto done;
+		goto err;
 	}
-
 	priv->firm_size = n_array;
+
 	n = -1;
 	while (p < endp) {
 		__u32 type, size;
@@ -323,7 +317,8 @@ static int load_all_firmwares(struct dvb_frontend *fe)
 
 		n++;
 		if (n >= n_array) {
-			tuner_err("Too much firmwares at the file\n");
+			tuner_err("More firmware images in file than "
+				  "were expected!\n");
 			goto corrupt;
 		}
 
@@ -353,15 +348,17 @@ static int load_all_firmwares(struct dvb_frontend *fe)
 		}
 
 		priv->firm[n].ptr = kzalloc(size, GFP_KERNEL);
-		if (!priv->firm[n].ptr) {
-			tuner_err("Not enough memory.\n");
+		if (priv->firm[n].ptr == NULL) {
+			tuner_err("Not enough memory to load firmware file.\n");
 			rc = -ENOMEM;
 			goto err;
 		}
-		tuner_info("Reading firmware type ");
-		dump_firm_type(type);
-		printk("(%x), id %llx, size=%d.\n",
-			   type, (unsigned long long)id, size);
+		tuner_dbg("Reading firmware type ");
+		if (debug) {
+			dump_firm_type(type);
+			printk("(%x), id %llx, size=%d.\n",
+				   type, (unsigned long long)id, size);
+		}
 
 		memcpy(priv->firm[n].ptr, p, size);
 		priv->firm[n].type = type;
@@ -383,13 +380,13 @@ corrupt:
 	tuner_err("Error: firmware file is corrupted!\n");
 
 err:
-	tuner_info("Releasing loaded firmware file.\n");
-
+	tuner_info("Releasing partially loaded firmware file.\n");
 	free_firmware(priv);
 
 done:
 	release_firmware(fw);
-	tuner_dbg("Firmware files loaded.\n");
+	if (rc == 0)
+		tuner_dbg("Firmware files loaded.\n");
 
 	return rc;
 }
@@ -457,11 +454,6 @@ static int load_firmware(struct dvb_frontend *fe, unsigned int type,
 	printk("(%x), id %016llx.\n", type, (unsigned long long)*id);
 
 	p = priv->firm[pos].ptr;
-
-	if (!p) {
-		tuner_err("Firmware pointer were freed!");
-		return -EINVAL;
-	}
 	endp = p + priv->firm[pos].size;
 
 	while (p < endp) {
@@ -561,15 +553,10 @@ static int load_scode(struct dvb_frontend *fe, unsigned int type,
 
 	p = priv->firm[pos].ptr;
 
-	if (!p) {
-		tuner_err("Firmware pointer were freed!");
-		return -EINVAL;
-	}
-
 	if ((priv->firm[pos].size != 12 * 16) || (scode >= 16))
 		return -EINVAL;
 
-	if (priv->version < 0x0202)
+	if (priv->firm_version < 0x0202)
 		rc = send_seq(priv, {0x20, 0x00, 0x00, 0x00});
 	else
 		rc = send_seq(priv, {0xa0, 0x00, 0x00, 0x00});
@@ -800,7 +787,7 @@ static int generic_set_tv_freq(struct dvb_frontend *fe, u32 freq /* in Hz */ ,
 
 	/* CMD= Set frequency */
 
-	if (priv->version < 0x0202)
+	if (priv->firm_version < 0x0202)
 		rc = send_seq(priv, {0x00, 0x02, 0x00, 0x00});
 	else
 		rc = send_seq(priv, {0x80, 0x02, 0x00, 0x00});
@@ -885,6 +872,7 @@ static int xc2028_dvb_release(struct dvb_frontend *fe)
 
 		free_firmware(priv);
 		kfree(priv);
+		fe->tuner_priv = NULL;
 	}
 
 	mutex_unlock(&xc2028_list_mutex);
@@ -910,14 +898,18 @@ static int xc2028_set_config(struct dvb_frontend *fe, void *priv_cfg)
 
 	tuner_dbg("%s called\n", __FUNCTION__);
 
+	mutex_lock(&priv->lock);
+
 	priv->ctrl.type = p->type;
 
 	if (p->fname) {
 		kfree(priv->ctrl.fname);
 
 		priv->ctrl.fname = kmalloc(strlen(p->fname) + 1, GFP_KERNEL);
-		if (!priv->ctrl.fname)
+		if (priv->ctrl.fname == NULL) {
+			mutex_unlock(&priv->lock);
 			return -ENOMEM;
+		}
 
 		free_firmware(priv);
 		strcpy(priv->ctrl.fname, p->fname);
@@ -926,6 +918,8 @@ static int xc2028_set_config(struct dvb_frontend *fe, void *priv_cfg)
 	if (p->max_len > 0)
 		priv->max_len = p->max_len;
 
+	mutex_unlock(&priv->lock);
+
 	return 0;
 }