All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/6] media: dvb_frontend: cleanup dvb_frontend_ioctl_properties()
@ 2017-09-19 13:42 Mauro Carvalho Chehab
  2017-09-19 13:42 ` [PATCH 2/6] media: dvb_frontend: cleanup ioctl handling logic Mauro Carvalho Chehab
                   ` (5 more replies)
  0 siblings, 6 replies; 12+ messages in thread
From: Mauro Carvalho Chehab @ 2017-09-19 13:42 UTC (permalink / raw)
  To: Linux Media Mailing List
  Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab, Max Kellermann,
	Shuah Khan, Colin Ian King

Use a switch() on this function, just like on other ioctl
handlers and handle parameters inside each part of the
switch.

That makes it easier to integrate with the already existing
ioctl handler function.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/dvb-core/dvb_frontend.c | 83 +++++++++++++++++++++--------------
 1 file changed, 51 insertions(+), 32 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index 8abe4f541a36..725cb1c8a088 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -1971,21 +1971,25 @@ static int dvb_frontend_ioctl_properties(struct file *file,
 	struct dvb_frontend *fe = dvbdev->priv;
 	struct dvb_frontend_private *fepriv = fe->frontend_priv;
 	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
-	int err = 0;
-
-	struct dtv_properties *tvps = parg;
-	struct dtv_property *tvp = NULL;
-	int i;
+	int err, i;
 
 	dev_dbg(fe->dvb->device, "%s:\n", __func__);
 
-	if (cmd == FE_SET_PROPERTY) {
-		dev_dbg(fe->dvb->device, "%s: properties.num = %d\n", __func__, tvps->num);
-		dev_dbg(fe->dvb->device, "%s: properties.props = %p\n", __func__, tvps->props);
+	switch(cmd) {
+	case FE_SET_PROPERTY: {
+		struct dtv_properties *tvps = parg;
+		struct dtv_property *tvp = NULL;
 
-		/* Put an arbitrary limit on the number of messages that can
-		 * be sent at once */
-		if ((tvps->num == 0) || (tvps->num > DTV_IOCTL_MAX_MSGS))
+		dev_dbg(fe->dvb->device, "%s: properties.num = %d\n",
+			__func__, tvps->num);
+		dev_dbg(fe->dvb->device, "%s: properties.props = %p\n",
+			__func__, tvps->props);
+
+		/*
+		 * Put an arbitrary limit on the number of messages that can
+		 * be sent at once
+		 */
+		if (!tvps->num || (tvps->num > DTV_IOCTL_MAX_MSGS))
 			return -EINVAL;
 
 		tvp = memdup_user(tvps->props, tvps->num * sizeof(*tvp));
@@ -1994,23 +1998,34 @@ static int dvb_frontend_ioctl_properties(struct file *file,
 
 		for (i = 0; i < tvps->num; i++) {
 			err = dtv_property_process_set(fe, tvp + i, file);
-			if (err < 0)
-				goto out;
+			if (err < 0) {
+				kfree(tvp);
+				return err;
+			}
 			(tvp + i)->result = err;
 		}
 
 		if (c->state == DTV_TUNE)
 			dev_dbg(fe->dvb->device, "%s: Property cache is full, tuning\n", __func__);
 
-	} else if (cmd == FE_GET_PROPERTY) {
+		kfree(tvp);
+		break;
+	}
+	case FE_GET_PROPERTY: {
+		struct dtv_properties *tvps = parg;
+		struct dtv_property *tvp = NULL;
 		struct dtv_frontend_properties getp = fe->dtv_property_cache;
 
-		dev_dbg(fe->dvb->device, "%s: properties.num = %d\n", __func__, tvps->num);
-		dev_dbg(fe->dvb->device, "%s: properties.props = %p\n", __func__, tvps->props);
+		dev_dbg(fe->dvb->device, "%s: properties.num = %d\n",
+			__func__, tvps->num);
+		dev_dbg(fe->dvb->device, "%s: properties.props = %p\n",
+			__func__, tvps->props);
 
-		/* Put an arbitrary limit on the number of messages that can
-		 * be sent at once */
-		if ((tvps->num == 0) || (tvps->num > DTV_IOCTL_MAX_MSGS))
+		/*
+		 * Put an arbitrary limit on the number of messages that can
+		 * be sent at once
+		 */
+		if (!tvps->num || (tvps->num > DTV_IOCTL_MAX_MSGS))
 			return -EINVAL;
 
 		tvp = memdup_user(tvps->props, tvps->num * sizeof(*tvp));
@@ -2025,28 +2040,32 @@ static int dvb_frontend_ioctl_properties(struct file *file,
 		 */
 		if (fepriv->state != FESTATE_IDLE) {
 			err = dtv_get_frontend(fe, &getp, NULL);
-			if (err < 0)
-				goto out;
+			if (err < 0) {
+				kfree(tvp);
+				return err;
+			}
 		}
 		for (i = 0; i < tvps->num; i++) {
 			err = dtv_property_process_get(fe, &getp, tvp + i, file);
-			if (err < 0)
-				goto out;
+			if (err < 0) {
+				kfree(tvp);
+				return err;
+			}
 			(tvp + i)->result = err;
 		}
 
 		if (copy_to_user((void __user *)tvps->props, tvp,
 				 tvps->num * sizeof(struct dtv_property))) {
-			err = -EFAULT;
-			goto out;
+			kfree(tvp);
+			return -EFAULT;
 		}
-
-	} else
-		err = -EOPNOTSUPP;
-
-out:
-	kfree(tvp);
-	return err;
+		kfree(tvp);
+		break;
+	}
+	default:
+		return -ENOTSUPP;
+	} /* switch */
+	return 0;
 }
 
 static int dtv_set_frontend(struct dvb_frontend *fe)
-- 
2.13.5

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/6] media: dvb_frontend: cleanup ioctl handling logic
  2017-09-19 13:42 [PATCH 1/6] media: dvb_frontend: cleanup dvb_frontend_ioctl_properties() Mauro Carvalho Chehab
@ 2017-09-19 13:42 ` Mauro Carvalho Chehab
  2017-09-20 20:58   ` Shuah Khan
  2017-09-19 13:42 ` [PATCH 3/6] media: dvb_frontend: get rid of proprierty cache's state Mauro Carvalho Chehab
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 12+ messages in thread
From: Mauro Carvalho Chehab @ 2017-09-19 13:42 UTC (permalink / raw)
  To: Linux Media Mailing List
  Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab, Max Kellermann,
	Shuah Khan, Colin Ian King

Currently, there are two handlers for ioctls:
 - dvb_frontend_ioctl_properties()
 - dvb_frontend_ioctl_legacy()

Despite their names, both handles non-legacy DVB ioctls.

Besides that, there's no reason why to not handle all ioctls
on a single handler function.

So, merge them into a single function (dvb_frontend_handle_ioctl)
and reorganize the ioctl's to indicate what's the current DVB
API and what's deprecated.

Despite the big diff, the handling logic for each ioctl is the
same as before.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/dvb-core/dvb_frontend.c | 402 +++++++++++++++++-----------------
 1 file changed, 195 insertions(+), 207 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index 725cb1c8a088..cbe697a094d2 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -1315,10 +1315,8 @@ static int dtv_get_frontend(struct dvb_frontend *fe,
 	return 0;
 }
 
-static int dvb_frontend_ioctl_legacy(struct file *file,
-			unsigned int cmd, void *parg);
-static int dvb_frontend_ioctl_properties(struct file *file,
-			unsigned int cmd, void *parg);
+static int dvb_frontend_handle_ioctl(struct file *file,
+				     unsigned int cmd, void *parg);
 
 static int dtv_property_process_get(struct dvb_frontend *fe,
 				    const struct dtv_frontend_properties *c,
@@ -1816,12 +1814,12 @@ static int dtv_property_process_set(struct dvb_frontend *fe,
 		break;
 	case DTV_VOLTAGE:
 		c->voltage = tvp->u.data;
-		r = dvb_frontend_ioctl_legacy(file, FE_SET_VOLTAGE,
+		r = dvb_frontend_handle_ioctl(file, FE_SET_VOLTAGE,
 			(void *)c->voltage);
 		break;
 	case DTV_TONE:
 		c->sectone = tvp->u.data;
-		r = dvb_frontend_ioctl_legacy(file, FE_SET_TONE,
+		r = dvb_frontend_handle_ioctl(file, FE_SET_TONE,
 			(void *)c->sectone);
 		break;
 	case DTV_CODE_RATE_HP:
@@ -1928,14 +1926,13 @@ static int dtv_property_process_set(struct dvb_frontend *fe,
 	return r;
 }
 
-static int dvb_frontend_ioctl(struct file *file,
-			unsigned int cmd, void *parg)
+static int dvb_frontend_ioctl(struct file *file, unsigned int cmd, void *parg)
 {
 	struct dvb_device *dvbdev = file->private_data;
 	struct dvb_frontend *fe = dvbdev->priv;
 	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
 	struct dvb_frontend_private *fepriv = fe->frontend_priv;
-	int err = -EOPNOTSUPP;
+	int err;
 
 	dev_dbg(fe->dvb->device, "%s: (%d)\n", __func__, _IOC_NR(cmd));
 	if (down_interruptible(&fepriv->sem))
@@ -1953,121 +1950,13 @@ static int dvb_frontend_ioctl(struct file *file,
 		return -EPERM;
 	}
 
-	if ((cmd == FE_SET_PROPERTY) || (cmd == FE_GET_PROPERTY))
-		err = dvb_frontend_ioctl_properties(file, cmd, parg);
-	else {
-		c->state = DTV_UNDEFINED;
-		err = dvb_frontend_ioctl_legacy(file, cmd, parg);
-	}
+	c->state = DTV_UNDEFINED;
+	err = dvb_frontend_handle_ioctl(file, cmd, parg);
 
 	up(&fepriv->sem);
 	return err;
 }
 
-static int dvb_frontend_ioctl_properties(struct file *file,
-			unsigned int cmd, void *parg)
-{
-	struct dvb_device *dvbdev = file->private_data;
-	struct dvb_frontend *fe = dvbdev->priv;
-	struct dvb_frontend_private *fepriv = fe->frontend_priv;
-	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
-	int err, i;
-
-	dev_dbg(fe->dvb->device, "%s:\n", __func__);
-
-	switch(cmd) {
-	case FE_SET_PROPERTY: {
-		struct dtv_properties *tvps = parg;
-		struct dtv_property *tvp = NULL;
-
-		dev_dbg(fe->dvb->device, "%s: properties.num = %d\n",
-			__func__, tvps->num);
-		dev_dbg(fe->dvb->device, "%s: properties.props = %p\n",
-			__func__, tvps->props);
-
-		/*
-		 * Put an arbitrary limit on the number of messages that can
-		 * be sent at once
-		 */
-		if (!tvps->num || (tvps->num > DTV_IOCTL_MAX_MSGS))
-			return -EINVAL;
-
-		tvp = memdup_user(tvps->props, tvps->num * sizeof(*tvp));
-		if (IS_ERR(tvp))
-			return PTR_ERR(tvp);
-
-		for (i = 0; i < tvps->num; i++) {
-			err = dtv_property_process_set(fe, tvp + i, file);
-			if (err < 0) {
-				kfree(tvp);
-				return err;
-			}
-			(tvp + i)->result = err;
-		}
-
-		if (c->state == DTV_TUNE)
-			dev_dbg(fe->dvb->device, "%s: Property cache is full, tuning\n", __func__);
-
-		kfree(tvp);
-		break;
-	}
-	case FE_GET_PROPERTY: {
-		struct dtv_properties *tvps = parg;
-		struct dtv_property *tvp = NULL;
-		struct dtv_frontend_properties getp = fe->dtv_property_cache;
-
-		dev_dbg(fe->dvb->device, "%s: properties.num = %d\n",
-			__func__, tvps->num);
-		dev_dbg(fe->dvb->device, "%s: properties.props = %p\n",
-			__func__, tvps->props);
-
-		/*
-		 * Put an arbitrary limit on the number of messages that can
-		 * be sent at once
-		 */
-		if (!tvps->num || (tvps->num > DTV_IOCTL_MAX_MSGS))
-			return -EINVAL;
-
-		tvp = memdup_user(tvps->props, tvps->num * sizeof(*tvp));
-		if (IS_ERR(tvp))
-			return PTR_ERR(tvp);
-
-		/*
-		 * Let's use our own copy of property cache, in order to
-		 * avoid mangling with DTV zigzag logic, as drivers might
-		 * return crap, if they don't check if the data is available
-		 * before updating the properties cache.
-		 */
-		if (fepriv->state != FESTATE_IDLE) {
-			err = dtv_get_frontend(fe, &getp, NULL);
-			if (err < 0) {
-				kfree(tvp);
-				return err;
-			}
-		}
-		for (i = 0; i < tvps->num; i++) {
-			err = dtv_property_process_get(fe, &getp, tvp + i, file);
-			if (err < 0) {
-				kfree(tvp);
-				return err;
-			}
-			(tvp + i)->result = err;
-		}
-
-		if (copy_to_user((void __user *)tvps->props, tvp,
-				 tvps->num * sizeof(struct dtv_property))) {
-			kfree(tvp);
-			return -EFAULT;
-		}
-		kfree(tvp);
-		break;
-	}
-	default:
-		return -ENOTSUPP;
-	} /* switch */
-	return 0;
-}
-
 static int dtv_set_frontend(struct dvb_frontend *fe)
 {
 	struct dvb_frontend_private *fepriv = fe->frontend_priv;
@@ -2205,59 +2094,102 @@ static int dtv_set_frontend(struct dvb_frontend *fe)
 }
 
 
-static int dvb_frontend_ioctl_legacy(struct file *file,
-			unsigned int cmd, void *parg)
+static int dvb_frontend_handle_ioctl(struct file *file,
+				     unsigned int cmd, void *parg)
 {
 	struct dvb_device *dvbdev = file->private_data;
 	struct dvb_frontend *fe = dvbdev->priv;
 	struct dvb_frontend_private *fepriv = fe->frontend_priv;
 	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
-	int err = -EOPNOTSUPP;
+	int i, err;
 
-	switch (cmd) {
-	case FE_GET_INFO: {
-		struct dvb_frontend_info* info = parg;
+	dev_dbg(fe->dvb->device, "%s:\n", __func__);
 
-		memcpy(info, &fe->ops.info, sizeof(struct dvb_frontend_info));
-		dvb_frontend_get_frequency_limits(fe, &info->frequency_min, &info->frequency_max);
+	switch(cmd) {
+	case FE_SET_PROPERTY: {
+		struct dtv_properties *tvps = parg;
+		struct dtv_property *tvp = NULL;
+
+		dev_dbg(fe->dvb->device, "%s: properties.num = %d\n",
+			__func__, tvps->num);
+		dev_dbg(fe->dvb->device, "%s: properties.props = %p\n",
+			__func__, tvps->props);
+
+		/*
+		 * Put an arbitrary limit on the number of messages that can
+		 * be sent at once
+		 */
+		if (!tvps->num || (tvps->num > DTV_IOCTL_MAX_MSGS))
+			return -EINVAL;
+
+		tvp = memdup_user(tvps->props, tvps->num * sizeof(*tvp));
+		if (IS_ERR(tvp))
+			return PTR_ERR(tvp);
+
+		for (i = 0; i < tvps->num; i++) {
+			err = dtv_property_process_set(fe, tvp + i, file);
+			if (err < 0) {
+				kfree(tvp);
+				return err;
+			}
+			(tvp + i)->result = err;
+		}
+
+		if (c->state == DTV_TUNE)
+			dev_dbg(fe->dvb->device, "%s: Property cache is full, tuning\n", __func__);
+
+		kfree(tvp);
+		break;
+	}
+	case FE_GET_PROPERTY: {
+		struct dtv_properties *tvps = parg;
+		struct dtv_property *tvp = NULL;
+		struct dtv_frontend_properties getp = fe->dtv_property_cache;
+
+		dev_dbg(fe->dvb->device, "%s: properties.num = %d\n",
+			__func__, tvps->num);
+		dev_dbg(fe->dvb->device, "%s: properties.props = %p\n",
+			__func__, tvps->props);
+
+		/*
+		 * Put an arbitrary limit on the number of messages that can
+		 * be sent at once
+		 */
+		if (!tvps->num || (tvps->num > DTV_IOCTL_MAX_MSGS))
+			return -EINVAL;
+
+		tvp = memdup_user(tvps->props, tvps->num * sizeof(*tvp));
+		if (IS_ERR(tvp))
+			return PTR_ERR(tvp);
 
 		/*
-		 * Associate the 4 delivery systems supported by DVBv3
-		 * API with their DVBv5 counterpart. For the other standards,
-		 * use the closest type, assuming that it would hopefully
-		 * work with a DVBv3 application.
-		 * It should be noticed that, on multi-frontend devices with
-		 * different types (terrestrial and cable, for example),
-		 * a pure DVBv3 application won't be able to use all delivery
-		 * systems. Yet, changing the DVBv5 cache to the other delivery
-		 * system should be enough for making it work.
+		 * Let's use our own copy of property cache, in order to
+		 * avoid mangling with DTV zigzag logic, as drivers might
+		 * return crap, if they don't check if the data is available
+		 * before updating the properties cache.
 		 */
-		switch (dvbv3_type(c->delivery_system)) {
-		case DVBV3_QPSK:
-			info->type = FE_QPSK;
-			break;
-		case DVBV3_ATSC:
-			info->type = FE_ATSC;
-			break;
-		case DVBV3_QAM:
-			info->type = FE_QAM;
-			break;
-		case DVBV3_OFDM:
-			info->type = FE_OFDM;
-			break;
-		default:
-			dev_err(fe->dvb->device,
-					"%s: doesn't know how to handle a DVBv3 call to delivery system %i\n",
-					__func__, c->delivery_system);
-			fe->ops.info.type = FE_OFDM;
+		if (fepriv->state != FESTATE_IDLE) {
+			err = dtv_get_frontend(fe, &getp, NULL);
+			if (err < 0) {
+				kfree(tvp);
+				return err;
+			}
+		}
+		for (i = 0; i < tvps->num; i++) {
+			err = dtv_property_process_get(fe, &getp, tvp + i, file);
+			if (err < 0) {
+				kfree(tvp);
+				return err;
+			}
+			(tvp + i)->result = err;
 		}
-		dev_dbg(fe->dvb->device, "%s: current delivery system on cache: %d, V3 type: %d\n",
-				 __func__, c->delivery_system, fe->ops.info.type);
 
-		/* Set CAN_INVERSION_AUTO bit on in other than oneshot mode */
-		if (!(fepriv->tune_mode_flags & FE_TUNE_MODE_ONESHOT))
-			info->caps |= FE_CAN_INVERSION_AUTO;
-		err = 0;
+		if (copy_to_user((void __user *)tvps->props, tvp,
+				 tvps->num * sizeof(struct dtv_property))) {
+			kfree(tvp);
+			return -EFAULT;
+		}
+		kfree(tvp);
 		break;
 	}
 
@@ -2278,42 +2210,6 @@ static int dvb_frontend_ioctl_legacy(struct file *file,
 		break;
 	}
 
-	case FE_READ_BER:
-		if (fe->ops.read_ber) {
-			if (fepriv->thread)
-				err = fe->ops.read_ber(fe, (__u32 *) parg);
-			else
-				err = -EAGAIN;
-		}
-		break;
-
-	case FE_READ_SIGNAL_STRENGTH:
-		if (fe->ops.read_signal_strength) {
-			if (fepriv->thread)
-				err = fe->ops.read_signal_strength(fe, (__u16 *) parg);
-			else
-				err = -EAGAIN;
-		}
-		break;
-
-	case FE_READ_SNR:
-		if (fe->ops.read_snr) {
-			if (fepriv->thread)
-				err = fe->ops.read_snr(fe, (__u16 *) parg);
-			else
-				err = -EAGAIN;
-		}
-		break;
-
-	case FE_READ_UNCORRECTED_BLOCKS:
-		if (fe->ops.read_ucblocks) {
-			if (fepriv->thread)
-				err = fe->ops.read_ucblocks(fe, (__u32 *) parg);
-			else
-				err = -EAGAIN;
-		}
-		break;
-
 	case FE_DISEQC_RESET_OVERLOAD:
 		if (fe->ops.diseqc_reset_overload) {
 			err = fe->ops.diseqc_reset_overload(fe);
@@ -2365,6 +2261,23 @@ static int dvb_frontend_ioctl_legacy(struct file *file,
 		}
 		break;
 
+	case FE_DISEQC_RECV_SLAVE_REPLY:
+		if (fe->ops.diseqc_recv_slave_reply)
+			err = fe->ops.diseqc_recv_slave_reply(fe, (struct dvb_diseqc_slave_reply*) parg);
+		break;
+
+	case FE_ENABLE_HIGH_LNB_VOLTAGE:
+		if (fe->ops.enable_high_lnb_voltage)
+			err = fe->ops.enable_high_lnb_voltage(fe, (long) parg);
+		break;
+
+	case FE_SET_FRONTEND_TUNE_MODE:
+		fepriv->tune_mode_flags = (unsigned long) parg;
+		err = 0;
+		break;
+
+	/* DEPRECATED dish control ioctls */
+
 	case FE_DISHNETWORK_SEND_LEGACY_CMD:
 		if (fe->ops.dishnetwork_send_legacy_command) {
 			err = fe->ops.dishnetwork_send_legacy_command(fe,
@@ -2430,15 +2343,91 @@ static int dvb_frontend_ioctl_legacy(struct file *file,
 		}
 		break;
 
-	case FE_DISEQC_RECV_SLAVE_REPLY:
-		if (fe->ops.diseqc_recv_slave_reply)
-			err = fe->ops.diseqc_recv_slave_reply(fe, (struct dvb_diseqc_slave_reply*) parg);
+	/* DEPRECATED statistics ioctls */
+
+	case FE_READ_BER:
+		if (fe->ops.read_ber) {
+			if (fepriv->thread)
+				err = fe->ops.read_ber(fe, (__u32 *) parg);
+			else
+				err = -EAGAIN;
+		}
+		break;
+
+	case FE_READ_SIGNAL_STRENGTH:
+		if (fe->ops.read_signal_strength) {
+			if (fepriv->thread)
+				err = fe->ops.read_signal_strength(fe, (__u16 *) parg);
+			else
+				err = -EAGAIN;
+		}
 		break;
 
-	case FE_ENABLE_HIGH_LNB_VOLTAGE:
-		if (fe->ops.enable_high_lnb_voltage)
-			err = fe->ops.enable_high_lnb_voltage(fe, (long) parg);
+	case FE_READ_SNR:
+		if (fe->ops.read_snr) {
+			if (fepriv->thread)
+				err = fe->ops.read_snr(fe, (__u16 *) parg);
+			else
+				err = -EAGAIN;
+		}
+		break;
+
+	case FE_READ_UNCORRECTED_BLOCKS:
+		if (fe->ops.read_ucblocks) {
+			if (fepriv->thread)
+				err = fe->ops.read_ucblocks(fe, (__u32 *) parg);
+			else
+				err = -EAGAIN;
+		}
+		break;
+
+	/* DEPRECATED DVBv3 ioctls */
+
+	case FE_GET_INFO: {
+		struct dvb_frontend_info* info = parg;
+
+		memcpy(info, &fe->ops.info, sizeof(struct dvb_frontend_info));
+		dvb_frontend_get_frequency_limits(fe, &info->frequency_min, &info->frequency_max);
+
+		/*
+		 * Associate the 4 delivery systems supported by DVBv3
+		 * API with their DVBv5 counterpart. For the other standards,
+		 * use the closest type, assuming that it would hopefully
+		 * work with a DVBv3 application.
+		 * It should be noticed that, on multi-frontend devices with
+		 * different types (terrestrial and cable, for example),
+		 * a pure DVBv3 application won't be able to use all delivery
+		 * systems. Yet, changing the DVBv5 cache to the other delivery
+		 * system should be enough for making it work.
+		 */
+		switch (dvbv3_type(c->delivery_system)) {
+		case DVBV3_QPSK:
+			info->type = FE_QPSK;
+			break;
+		case DVBV3_ATSC:
+			info->type = FE_ATSC;
+			break;
+		case DVBV3_QAM:
+			info->type = FE_QAM;
+			break;
+		case DVBV3_OFDM:
+			info->type = FE_OFDM;
+			break;
+		default:
+			dev_err(fe->dvb->device,
+					"%s: doesn't know how to handle a DVBv3 call to delivery system %i\n",
+					__func__, c->delivery_system);
+			fe->ops.info.type = FE_OFDM;
+		}
+		dev_dbg(fe->dvb->device, "%s: current delivery system on cache: %d, V3 type: %d\n",
+				 __func__, c->delivery_system, fe->ops.info.type);
+
+		/* Set CAN_INVERSION_AUTO bit on in other than oneshot mode */
+		if (!(fepriv->tune_mode_flags & FE_TUNE_MODE_ONESHOT))
+			info->caps |= FE_CAN_INVERSION_AUTO;
+		err = 0;
 		break;
+	}
 
 	case FE_SET_FRONTEND:
 		err = dvbv3_set_delivery_system(fe);
@@ -2466,11 +2455,10 @@ static int dvb_frontend_ioctl_legacy(struct file *file,
 		err = dtv_get_frontend(fe, &getp, parg);
 		break;
 	}
-	case FE_SET_FRONTEND_TUNE_MODE:
-		fepriv->tune_mode_flags = (unsigned long) parg;
-		err = 0;
-		break;
-	}
+
+	default:
+		return -ENOTSUPP;
+	} /* switch */
 
 	return err;
 }
-- 
2.13.5

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/6] media: dvb_frontend: get rid of proprierty cache's state
  2017-09-19 13:42 [PATCH 1/6] media: dvb_frontend: cleanup dvb_frontend_ioctl_properties() Mauro Carvalho Chehab
  2017-09-19 13:42 ` [PATCH 2/6] media: dvb_frontend: cleanup ioctl handling logic Mauro Carvalho Chehab
@ 2017-09-19 13:42 ` Mauro Carvalho Chehab
  2017-09-20 21:08   ` Shuah Khan
  2017-09-19 13:42 ` [PATCH 4/6] media: dvb_frontend.h: fix alignment at the cache properties Mauro Carvalho Chehab
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 12+ messages in thread
From: Mauro Carvalho Chehab @ 2017-09-19 13:42 UTC (permalink / raw)
  To: Linux Media Mailing List
  Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab, Max Kellermann,
	Shuah Khan, Colin Ian King, Michael Ira Krufky, Sakari Ailus

In the past, I guess the idea was to use state in order to
allow an autofush logic. However, in the current code, it is
used only for debug messages, on a poor man's solution, as
there's already a debug message to indicate when the properties
got flushed.

So, just get rid of it for good.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/dvb-core/dvb_frontend.c | 20 ++++++--------------
 drivers/media/dvb-core/dvb_frontend.h |  5 -----
 2 files changed, 6 insertions(+), 19 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index cbe697a094d2..d0a17d67ab1b 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -951,8 +951,6 @@ static int dvb_frontend_clear_cache(struct dvb_frontend *fe)
 	memset(c, 0, offsetof(struct dtv_frontend_properties, strength));
 	c->delivery_system = delsys;
 
-	c->state = DTV_CLEAR;
-
 	dev_dbg(fe->dvb->device, "%s: Clearing cache for delivery system %d\n",
 			__func__, c->delivery_system);
 
@@ -1775,13 +1773,13 @@ static int dtv_property_process_set(struct dvb_frontend *fe,
 		dvb_frontend_clear_cache(fe);
 		break;
 	case DTV_TUNE:
-		/* interpret the cache of data, build either a traditional frontend
-		 * tunerequest so we can pass validation in the FE_SET_FRONTEND
-		 * ioctl.
+		/*
+		 * Use the cached Digital TV properties to tune the
+		 * frontend
 		 */
-		c->state = tvp->cmd;
-		dev_dbg(fe->dvb->device, "%s: Finalised property cache\n",
-				__func__);
+		dev_dbg(fe->dvb->device,
+			"%s: Setting the frontend from property cache\n",
+			__func__);
 
 		r = dtv_set_frontend(fe);
 		break;
@@ -1930,7 +1928,6 @@ static int dvb_frontend_ioctl(struct file *file, unsigned int cmd, void *parg)
 {
 	struct dvb_device *dvbdev = file->private_data;
 	struct dvb_frontend *fe = dvbdev->priv;
-	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
 	struct dvb_frontend_private *fepriv = fe->frontend_priv;
 	int err;
 
@@ -1950,7 +1947,6 @@ static int dvb_frontend_ioctl(struct file *file, unsigned int cmd, void *parg)
 		return -EPERM;
 	}
 
-	c->state = DTV_UNDEFINED;
 	err = dvb_frontend_handle_ioctl(file, cmd, parg);
 
 	up(&fepriv->sem);
@@ -2134,10 +2130,6 @@ static int dvb_frontend_handle_ioctl(struct file *file,
 			}
 			(tvp + i)->result = err;
 		}
-
-		if (c->state == DTV_TUNE)
-			dev_dbg(fe->dvb->device, "%s: Property cache is full, tuning\n", __func__);
-
 		kfree(tvp);
 		break;
 	}
diff --git a/drivers/media/dvb-core/dvb_frontend.h b/drivers/media/dvb-core/dvb_frontend.h
index 852b91ba49d2..1bdeb10f0d78 100644
--- a/drivers/media/dvb-core/dvb_frontend.h
+++ b/drivers/media/dvb-core/dvb_frontend.h
@@ -620,11 +620,6 @@ struct dtv_frontend_properties {
 	struct dtv_fe_stats	post_bit_count;
 	struct dtv_fe_stats	block_error;
 	struct dtv_fe_stats	block_count;
-
-	/* private: */
-	/* Cache State */
-	u32			state;
-
 };
 
 #define DVB_FE_NO_EXIT  0
-- 
2.13.5

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 4/6] media: dvb_frontend.h: fix alignment at the cache properties
  2017-09-19 13:42 [PATCH 1/6] media: dvb_frontend: cleanup dvb_frontend_ioctl_properties() Mauro Carvalho Chehab
  2017-09-19 13:42 ` [PATCH 2/6] media: dvb_frontend: cleanup ioctl handling logic Mauro Carvalho Chehab
  2017-09-19 13:42 ` [PATCH 3/6] media: dvb_frontend: get rid of proprierty cache's state Mauro Carvalho Chehab
@ 2017-09-19 13:42 ` Mauro Carvalho Chehab
  2017-09-19 13:42 ` [PATCH 5/6] media: dvb_frontend: better document the -EPERM condition Mauro Carvalho Chehab
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: Mauro Carvalho Chehab @ 2017-09-19 13:42 UTC (permalink / raw)
  To: Linux Media Mailing List
  Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab, Max Kellermann,
	Michael Ira Krufky, Sakari Ailus

There are too much tabs on several properties, for no good
reason. That sounds confusing while reading the struct, so
adjust them.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/dvb-core/dvb_frontend.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.h b/drivers/media/dvb-core/dvb_frontend.h
index 1bdeb10f0d78..d273ed2f72c9 100644
--- a/drivers/media/dvb-core/dvb_frontend.h
+++ b/drivers/media/dvb-core/dvb_frontend.h
@@ -557,15 +557,15 @@ struct dtv_frontend_properties {
 
 	enum fe_sec_voltage	voltage;
 	enum fe_sec_tone_mode	sectone;
-	enum fe_spectral_inversion	inversion;
-	enum fe_code_rate		fec_inner;
+	enum fe_spectral_inversion inversion;
+	enum fe_code_rate	fec_inner;
 	enum fe_transmit_mode	transmission_mode;
 	u32			bandwidth_hz;	/* 0 = AUTO */
 	enum fe_guard_interval	guard_interval;
-	enum fe_hierarchy		hierarchy;
+	enum fe_hierarchy	hierarchy;
 	u32			symbol_rate;
-	enum fe_code_rate		code_rate_HP;
-	enum fe_code_rate		code_rate_LP;
+	enum fe_code_rate	code_rate_HP;
+	enum fe_code_rate	code_rate_LP;
 
 	enum fe_pilot		pilot;
 	enum fe_rolloff		rolloff;
-- 
2.13.5

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 5/6] media: dvb_frontend: better document the -EPERM condition
  2017-09-19 13:42 [PATCH 1/6] media: dvb_frontend: cleanup dvb_frontend_ioctl_properties() Mauro Carvalho Chehab
                   ` (2 preceding siblings ...)
  2017-09-19 13:42 ` [PATCH 4/6] media: dvb_frontend.h: fix alignment at the cache properties Mauro Carvalho Chehab
@ 2017-09-19 13:42 ` Mauro Carvalho Chehab
  2017-09-20 21:12   ` Shuah Khan
  2017-09-19 13:42 ` [PATCH 6/6] media: dvb_frontend: fix return values for FE_SET_PROPERTY Mauro Carvalho Chehab
  2017-09-20 20:11 ` [PATCH 1/6] media: dvb_frontend: cleanup dvb_frontend_ioctl_properties() Shuah Khan
  5 siblings, 1 reply; 12+ messages in thread
From: Mauro Carvalho Chehab @ 2017-09-19 13:42 UTC (permalink / raw)
  To: Linux Media Mailing List
  Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab, Max Kellermann,
	Shuah Khan, Colin Ian King

Two readonly ioctls can't be allowed if the frontend device
is opened in read only mode. Explain why.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/dvb-core/dvb_frontend.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index d0a17d67ab1b..db3f8c597a24 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -1940,9 +1940,23 @@ static int dvb_frontend_ioctl(struct file *file, unsigned int cmd, void *parg)
 		return -ENODEV;
 	}
 
-	if ((file->f_flags & O_ACCMODE) == O_RDONLY &&
-	    (_IOC_DIR(cmd) != _IOC_READ || cmd == FE_GET_EVENT ||
-	     cmd == FE_DISEQC_RECV_SLAVE_REPLY)) {
+	/*
+	 * If the frontend is opened in read-only mode, only the ioctls
+	 * that don't interfere at the tune logic should be accepted.
+	 * That allows an external application to monitor the DVB QoS and
+	 * statistics parameters.
+	 *
+	 * That matches all _IOR() ioctls, except for two special cases:
+	 *   - FE_GET_EVENT is part of the tuning logic on a DVB application;
+	 *   - FE_DISEQC_RECV_SLAVE_REPLY is part of DiSEqC 2.0
+	 *     setup
+	 * So, those two ioctls should also return -EPERM, as otherwise
+	 * reading from them would interfere with a DVB tune application
+	 */
+	if ((file->f_flags & O_ACCMODE) == O_RDONLY
+	    && (_IOC_DIR(cmd) != _IOC_READ
+		|| cmd == FE_GET_EVENT
+		|| cmd == FE_DISEQC_RECV_SLAVE_REPLY)) {
 		up(&fepriv->sem);
 		return -EPERM;
 	}
-- 
2.13.5

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 6/6] media: dvb_frontend: fix return values for FE_SET_PROPERTY
  2017-09-19 13:42 [PATCH 1/6] media: dvb_frontend: cleanup dvb_frontend_ioctl_properties() Mauro Carvalho Chehab
                   ` (3 preceding siblings ...)
  2017-09-19 13:42 ` [PATCH 5/6] media: dvb_frontend: better document the -EPERM condition Mauro Carvalho Chehab
@ 2017-09-19 13:42 ` Mauro Carvalho Chehab
  2017-09-20 20:11 ` [PATCH 1/6] media: dvb_frontend: cleanup dvb_frontend_ioctl_properties() Shuah Khan
  5 siblings, 0 replies; 12+ messages in thread
From: Mauro Carvalho Chehab @ 2017-09-19 13:42 UTC (permalink / raw)
  To: Linux Media Mailing List
  Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab, Max Kellermann,
	Shuah Khan, Colin Ian King

There are several problems with regards to the return of
FE_SET_PROPERTY. The original idea were to return per-property
return codes via tvp->result field, and to return an updated
set of values.

However, that never worked. What's actually implemented is:

- the FE_SET_PROPERTY implementation doesn't call .get_frontend
  callback in order to get the actual parameters after return;

- the tvp->result field is only filled if there's no error.
  So, it is always filled with zero;

- FE_SET_PROPERTY doesn't call memdup_user() nor any other
  copy_to_user() function. So, any changes at the properies
  will be lost;

- FE_SET_PROPERTY is declared as a write-only ioctl (IOW).

While we could fix the above, it could cause regressions.

So, let's just assume what the code really does, updating
the documentation accordingly and removing the logic that
would update the discarded tvp->result.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 Documentation/media/uapi/dvb/fe-get-property.rst | 7 +++++--
 drivers/media/dvb-core/dvb_frontend.c            | 2 --
 include/uapi/linux/dvb/frontend.h                | 2 +-
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/Documentation/media/uapi/dvb/fe-get-property.rst b/Documentation/media/uapi/dvb/fe-get-property.rst
index 948d2ba84f2c..b69741d9cedf 100644
--- a/Documentation/media/uapi/dvb/fe-get-property.rst
+++ b/Documentation/media/uapi/dvb/fe-get-property.rst
@@ -48,8 +48,11 @@ depends on the delivery system and on the device:
 
    -  This call requires read/write access to the device.
 
-   -  At return, the values are updated to reflect the actual parameters
-      used.
+.. note::
+
+   At return, the values aren't updated to reflect the actual
+   parameters used. If the actual parameters are needed, an explicit
+   call to ``FE_GET_PROPERTY`` is needed.
 
 -  ``FE_GET_PROPERTY:``
 
diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index db3f8c597a24..4b5acc9d0124 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -2142,7 +2142,6 @@ static int dvb_frontend_handle_ioctl(struct file *file,
 				kfree(tvp);
 				return err;
 			}
-			(tvp + i)->result = err;
 		}
 		kfree(tvp);
 		break;
@@ -2187,7 +2186,6 @@ static int dvb_frontend_handle_ioctl(struct file *file,
 				kfree(tvp);
 				return err;
 			}
-			(tvp + i)->result = err;
 		}
 
 		if (copy_to_user((void __user *)tvps->props, tvp,
diff --git a/include/uapi/linux/dvb/frontend.h b/include/uapi/linux/dvb/frontend.h
index 861cacd5711f..6bc26f35217b 100644
--- a/include/uapi/linux/dvb/frontend.h
+++ b/include/uapi/linux/dvb/frontend.h
@@ -830,7 +830,7 @@ struct dtv_fe_stats {
  * @cmd:	Digital TV command.
  * @reserved:	Not used.
  * @u:		Union with the values for the command.
- * @result:	Result of the command set (currently unused).
+ * @result:	Unused
  *
  * The @u union may have either one of the values below:
  *
-- 
2.13.5

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/6] media: dvb_frontend: cleanup dvb_frontend_ioctl_properties()
  2017-09-19 13:42 [PATCH 1/6] media: dvb_frontend: cleanup dvb_frontend_ioctl_properties() Mauro Carvalho Chehab
                   ` (4 preceding siblings ...)
  2017-09-19 13:42 ` [PATCH 6/6] media: dvb_frontend: fix return values for FE_SET_PROPERTY Mauro Carvalho Chehab
@ 2017-09-20 20:11 ` Shuah Khan
  2017-09-20 20:27   ` Mauro Carvalho Chehab
  5 siblings, 1 reply; 12+ messages in thread
From: Shuah Khan @ 2017-09-20 20:11 UTC (permalink / raw)
  To: Mauro Carvalho Chehab, Linux Media Mailing List
  Cc: Mauro Carvalho Chehab, Max Kellermann, Colin Ian King,
	Shuah Khan, Shuah Khan

On 09/19/2017 07:42 AM, Mauro Carvalho Chehab wrote:
> Use a switch() on this function, just like on other ioctl
> handlers and handle parameters inside each part of the
> switch.
> 
> That makes it easier to integrate with the already existing
> ioctl handler function.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>

The change looks good. Couple of comments below:

> ---
>  drivers/media/dvb-core/dvb_frontend.c | 83 +++++++++++++++++++++--------------
>  1 file changed, 51 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
> index 8abe4f541a36..725cb1c8a088 100644
> --- a/drivers/media/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb-core/dvb_frontend.c
> @@ -1971,21 +1971,25 @@ static int dvb_frontend_ioctl_properties(struct file *file,
>  	struct dvb_frontend *fe = dvbdev->priv;
>  	struct dvb_frontend_private *fepriv = fe->frontend_priv;
>  	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
> -	int err = 0;
> -
> -	struct dtv_properties *tvps = parg;
> -	struct dtv_property *tvp = NULL;
> -	int i;
> +	int err, i;
>  
>  	dev_dbg(fe->dvb->device, "%s:\n", __func__);
>  
> -	if (cmd == FE_SET_PROPERTY) {
> -		dev_dbg(fe->dvb->device, "%s: properties.num = %d\n", __func__, tvps->num);
> -		dev_dbg(fe->dvb->device, "%s: properties.props = %p\n", __func__, tvps->props);
> +	switch(cmd) {
> +	case FE_SET_PROPERTY: {
> +		struct dtv_properties *tvps = parg;
> +		struct dtv_property *tvp = NULL;
>  
> -		/* Put an arbitrary limit on the number of messages that can
> -		 * be sent at once */
> -		if ((tvps->num == 0) || (tvps->num > DTV_IOCTL_MAX_MSGS))
> +		dev_dbg(fe->dvb->device, "%s: properties.num = %d\n",
> +			__func__, tvps->num);
> +		dev_dbg(fe->dvb->device, "%s: properties.props = %p\n",
> +			__func__, tvps->props);
> +
> +		/*
> +		 * Put an arbitrary limit on the number of messages that can
> +		 * be sent at once
> +		 */
> +		if (!tvps->num || (tvps->num > DTV_IOCTL_MAX_MSGS))
>  			return -EINVAL;
>  
>  		tvp = memdup_user(tvps->props, tvps->num * sizeof(*tvp));
> @@ -1994,23 +1998,34 @@ static int dvb_frontend_ioctl_properties(struct file *file,
>  
>  		for (i = 0; i < tvps->num; i++) {
>  			err = dtv_property_process_set(fe, tvp + i, file);
> -			if (err < 0)
> -				goto out;
> +			if (err < 0) {
> +				kfree(tvp);
> +				return err;
> +			}
>  			(tvp + i)->result = err;
>  		}
>  
>  		if (c->state == DTV_TUNE)
>  			dev_dbg(fe->dvb->device, "%s: Property cache is full, tuning\n", __func__);
>  
> -	} else if (cmd == FE_GET_PROPERTY) {
> +		kfree(tvp);
> +		break;
> +	}
> +	case FE_GET_PROPERTY: {
> +		struct dtv_properties *tvps = parg;
> +		struct dtv_property *tvp = NULL;
>  		struct dtv_frontend_properties getp = fe->dtv_property_cache;
>  
> -		dev_dbg(fe->dvb->device, "%s: properties.num = %d\n", __func__, tvps->num);
> -		dev_dbg(fe->dvb->device, "%s: properties.props = %p\n", __func__, tvps->props);
> +		dev_dbg(fe->dvb->device, "%s: properties.num = %d\n",
> +			__func__, tvps->num);
> +		dev_dbg(fe->dvb->device, "%s: properties.props = %p\n",
> +			__func__, tvps->props);
>  
> -		/* Put an arbitrary limit on the number of messages that can
> -		 * be sent at once */
> -		if ((tvps->num == 0) || (tvps->num > DTV_IOCTL_MAX_MSGS))
> +		/*
> +		 * Put an arbitrary limit on the number of messages that can
> +		 * be sent at once
> +		 */
> +		if (!tvps->num || (tvps->num > DTV_IOCTL_MAX_MSGS))
>  			return -EINVAL;
>  
>  		tvp = memdup_user(tvps->props, tvps->num * sizeof(*tvp));
> @@ -2025,28 +2040,32 @@ static int dvb_frontend_ioctl_properties(struct file *file,
>  		 */
>  		if (fepriv->state != FESTATE_IDLE) {
>  			err = dtv_get_frontend(fe, &getp, NULL);
> -			if (err < 0)
> -				goto out;
> +			if (err < 0) {
> +				kfree(tvp);
> +				return err;
> +			}

Could avoid duplicate code keeping out logic perhaps? Is there a reason
for removing this?

>  		}
>  		for (i = 0; i < tvps->num; i++) {
>  			err = dtv_property_process_get(fe, &getp, tvp + i, file);
> -			if (err < 0)
> -				goto out;
> +			if (err < 0) {
> +				kfree(tvp);
> +				return err;
> +			}
>  			(tvp + i)->result = err;
>  		}
>  
>  		if (copy_to_user((void __user *)tvps->props, tvp,
>  				 tvps->num * sizeof(struct dtv_property))) {
> -			err = -EFAULT;
> -			goto out;
> +			kfree(tvp);
> +			return -EFAULT;
>  		}

Could avoid duplicate code keeping out logic perhaps? Is there a reason
for removing this?

> -
> -	} else
> -		err = -EOPNOTSUPP;
> -
> -out:
> -	kfree(tvp);
> -	return err;
> +		kfree(tvp);
> +		break;
> +	}
> +	default:
> +		return -ENOTSUPP;
> +	} /* switch */
> +	return 0;
>  }
>  
>  static int dtv_set_frontend(struct dvb_frontend *fe)
> 

Reviewed-by: Shuah Khan <shuahkh@osg.samsung.com>

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/6] media: dvb_frontend: cleanup dvb_frontend_ioctl_properties()
  2017-09-20 20:11 ` [PATCH 1/6] media: dvb_frontend: cleanup dvb_frontend_ioctl_properties() Shuah Khan
@ 2017-09-20 20:27   ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 12+ messages in thread
From: Mauro Carvalho Chehab @ 2017-09-20 20:27 UTC (permalink / raw)
  To: Shuah Khan
  Cc: Linux Media Mailing List, Mauro Carvalho Chehab, Max Kellermann,
	Colin Ian King, Shuah Khan

Em Wed, 20 Sep 2017 14:11:39 -0600
Shuah Khan <shuah@kernel.org> escreveu:

> On 09/19/2017 07:42 AM, Mauro Carvalho Chehab wrote:
> > Use a switch() on this function, just like on other ioctl
> > handlers and handle parameters inside each part of the
> > switch.
> > 
> > That makes it easier to integrate with the already existing
> > ioctl handler function.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>  
> 
> The change looks good. Couple of comments below:
> 
> > ---
> >  drivers/media/dvb-core/dvb_frontend.c | 83 +++++++++++++++++++++--------------
> >  1 file changed, 51 insertions(+), 32 deletions(-)
> > 
> > diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
> > index 8abe4f541a36..725cb1c8a088 100644
> > --- a/drivers/media/dvb-core/dvb_frontend.c
> > +++ b/drivers/media/dvb-core/dvb_frontend.c
> > @@ -1971,21 +1971,25 @@ static int dvb_frontend_ioctl_properties(struct file *file,
> >  	struct dvb_frontend *fe = dvbdev->priv;
> >  	struct dvb_frontend_private *fepriv = fe->frontend_priv;
> >  	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
> > -	int err = 0;
> > -
> > -	struct dtv_properties *tvps = parg;
> > -	struct dtv_property *tvp = NULL;
> > -	int i;
> > +	int err, i;
> >  
> >  	dev_dbg(fe->dvb->device, "%s:\n", __func__);
> >  
> > -	if (cmd == FE_SET_PROPERTY) {
> > -		dev_dbg(fe->dvb->device, "%s: properties.num = %d\n", __func__, tvps->num);
> > -		dev_dbg(fe->dvb->device, "%s: properties.props = %p\n", __func__, tvps->props);
> > +	switch(cmd) {
> > +	case FE_SET_PROPERTY: {
> > +		struct dtv_properties *tvps = parg;
> > +		struct dtv_property *tvp = NULL;
> >  
> > -		/* Put an arbitrary limit on the number of messages that can
> > -		 * be sent at once */
> > -		if ((tvps->num == 0) || (tvps->num > DTV_IOCTL_MAX_MSGS))
> > +		dev_dbg(fe->dvb->device, "%s: properties.num = %d\n",
> > +			__func__, tvps->num);
> > +		dev_dbg(fe->dvb->device, "%s: properties.props = %p\n",
> > +			__func__, tvps->props);
> > +
> > +		/*
> > +		 * Put an arbitrary limit on the number of messages that can
> > +		 * be sent at once
> > +		 */
> > +		if (!tvps->num || (tvps->num > DTV_IOCTL_MAX_MSGS))
> >  			return -EINVAL;
> >  
> >  		tvp = memdup_user(tvps->props, tvps->num * sizeof(*tvp));
> > @@ -1994,23 +1998,34 @@ static int dvb_frontend_ioctl_properties(struct file *file,
> >  
> >  		for (i = 0; i < tvps->num; i++) {
> >  			err = dtv_property_process_set(fe, tvp + i, file);
> > -			if (err < 0)
> > -				goto out;
> > +			if (err < 0) {
> > +				kfree(tvp);
> > +				return err;
> > +			}
> >  			(tvp + i)->result = err;
> >  		}
> >  
> >  		if (c->state == DTV_TUNE)
> >  			dev_dbg(fe->dvb->device, "%s: Property cache is full, tuning\n", __func__);
> >  
> > -	} else if (cmd == FE_GET_PROPERTY) {
> > +		kfree(tvp);
> > +		break;
> > +	}
> > +	case FE_GET_PROPERTY: {
> > +		struct dtv_properties *tvps = parg;
> > +		struct dtv_property *tvp = NULL;
> >  		struct dtv_frontend_properties getp = fe->dtv_property_cache;
> >  
> > -		dev_dbg(fe->dvb->device, "%s: properties.num = %d\n", __func__, tvps->num);
> > -		dev_dbg(fe->dvb->device, "%s: properties.props = %p\n", __func__, tvps->props);
> > +		dev_dbg(fe->dvb->device, "%s: properties.num = %d\n",
> > +			__func__, tvps->num);
> > +		dev_dbg(fe->dvb->device, "%s: properties.props = %p\n",
> > +			__func__, tvps->props);
> >  
> > -		/* Put an arbitrary limit on the number of messages that can
> > -		 * be sent at once */
> > -		if ((tvps->num == 0) || (tvps->num > DTV_IOCTL_MAX_MSGS))
> > +		/*
> > +		 * Put an arbitrary limit on the number of messages that can
> > +		 * be sent at once
> > +		 */
> > +		if (!tvps->num || (tvps->num > DTV_IOCTL_MAX_MSGS))
> >  			return -EINVAL;
> >  
> >  		tvp = memdup_user(tvps->props, tvps->num * sizeof(*tvp));
> > @@ -2025,28 +2040,32 @@ static int dvb_frontend_ioctl_properties(struct file *file,
> >  		 */
> >  		if (fepriv->state != FESTATE_IDLE) {
> >  			err = dtv_get_frontend(fe, &getp, NULL);
> > -			if (err < 0)
> > -				goto out;
> > +			if (err < 0) {
> > +				kfree(tvp);
> > +				return err;
> > +			}  
> 
> Could avoid duplicate code keeping out logic perhaps? Is there a reason
> for removing this?

Yes. See the next patch :-)

Basically, the next patch remove dvb_frontend_ioctl_properties(), merging
it with another ioctl handler. On such handler, the error handling path
is different for each ioctl.

We might still use gotos there, but that would be messy.

> 
> >  		}
> >  		for (i = 0; i < tvps->num; i++) {
> >  			err = dtv_property_process_get(fe, &getp, tvp + i, file);
> > -			if (err < 0)
> > -				goto out;
> > +			if (err < 0) {
> > +				kfree(tvp);
> > +				return err;
> > +			}
> >  			(tvp + i)->result = err;
> >  		}
> >  
> >  		if (copy_to_user((void __user *)tvps->props, tvp,
> >  				 tvps->num * sizeof(struct dtv_property))) {
> > -			err = -EFAULT;
> > -			goto out;
> > +			kfree(tvp);
> > +			return -EFAULT;
> >  		}  
> 
> Could avoid duplicate code keeping out logic perhaps? Is there a reason
> for removing this?

Same as above.

> 
> > -
> > -	} else
> > -		err = -EOPNOTSUPP;
> > -
> > -out:
> > -	kfree(tvp);
> > -	return err;
> > +		kfree(tvp);
> > +		break;
> > +	}
> > +	default:
> > +		return -ENOTSUPP;
> > +	} /* switch */
> > +	return 0;
> >  }
> >  
> >  static int dtv_set_frontend(struct dvb_frontend *fe)
> >   
> 
> Reviewed-by: Shuah Khan <shuahkh@osg.samsung.com>
> 
> thanks,
> -- Shuah



Thanks,
Mauro

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/6] media: dvb_frontend: cleanup ioctl handling logic
  2017-09-19 13:42 ` [PATCH 2/6] media: dvb_frontend: cleanup ioctl handling logic Mauro Carvalho Chehab
@ 2017-09-20 20:58   ` Shuah Khan
  2017-09-20 21:16     ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 12+ messages in thread
From: Shuah Khan @ 2017-09-20 20:58 UTC (permalink / raw)
  To: Mauro Carvalho Chehab, Linux Media Mailing List
  Cc: Mauro Carvalho Chehab, Max Kellermann, Colin Ian King,
	Shuah Khan, Shuah Khan

H Mauro,

On 09/19/2017 07:42 AM, Mauro Carvalho Chehab wrote:
> Currently, there are two handlers for ioctls:
>  - dvb_frontend_ioctl_properties()
>  - dvb_frontend_ioctl_legacy()
> 
> Despite their names, both handles non-legacy DVB ioctls.
> 
> Besides that, there's no reason why to not handle all ioctls
> on a single handler function.
> 
> So, merge them into a single function (dvb_frontend_handle_ioctl)
> and reorganize the ioctl's to indicate what's the current DVB
> API and what's deprecated.
> 
> Despite the big diff, the handling logic for each ioctl is the
> same as before.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> ---
>  drivers/media/dvb-core/dvb_frontend.c | 402 +++++++++++++++++-----------------
>  1 file changed, 195 insertions(+), 207 deletions(-)
> 
> diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
> index 725cb1c8a088..cbe697a094d2 100644
> --- a/drivers/media/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb-core/dvb_frontend.c
> @@ -1315,10 +1315,8 @@ static int dtv_get_frontend(struct dvb_frontend *fe,
>  	return 0;
>  }
>  
> -static int dvb_frontend_ioctl_legacy(struct file *file,
> -			unsigned int cmd, void *parg);
> -static int dvb_frontend_ioctl_properties(struct file *file,
> -			unsigned int cmd, void *parg);
> +static int dvb_frontend_handle_ioctl(struct file *file,
> +				     unsigned int cmd, void *parg);
>  
>  static int dtv_property_process_get(struct dvb_frontend *fe,
>  				    const struct dtv_frontend_properties *c,
> @@ -1816,12 +1814,12 @@ static int dtv_property_process_set(struct dvb_frontend *fe,
>  		break;
>  	case DTV_VOLTAGE:
>  		c->voltage = tvp->u.data;
> -		r = dvb_frontend_ioctl_legacy(file, FE_SET_VOLTAGE,
> +		r = dvb_frontend_handle_ioctl(file, FE_SET_VOLTAGE,
>  			(void *)c->voltage);
>  		break;
>  	case DTV_TONE:
>  		c->sectone = tvp->u.data;
> -		r = dvb_frontend_ioctl_legacy(file, FE_SET_TONE,
> +		r = dvb_frontend_handle_ioctl(file, FE_SET_TONE,
>  			(void *)c->sectone);
>  		break;
>  	case DTV_CODE_RATE_HP:
> @@ -1928,14 +1926,13 @@ static int dtv_property_process_set(struct dvb_frontend *fe,
>  	return r;
>  }
>  
> -static int dvb_frontend_ioctl(struct file *file,
> -			unsigned int cmd, void *parg)
> +static int dvb_frontend_ioctl(struct file *file, unsigned int cmd, void *parg)
>  {
>  	struct dvb_device *dvbdev = file->private_data;
>  	struct dvb_frontend *fe = dvbdev->priv;
>  	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
>  	struct dvb_frontend_private *fepriv = fe->frontend_priv;
> -	int err = -EOPNOTSUPP;
> +	int err;
>  
>  	dev_dbg(fe->dvb->device, "%s: (%d)\n", __func__, _IOC_NR(cmd));
>  	if (down_interruptible(&fepriv->sem))
> @@ -1953,121 +1950,13 @@ static int dvb_frontend_ioctl(struct file *file,
>  		return -EPERM;
>  	}
>  
> -	if ((cmd == FE_SET_PROPERTY) || (cmd == FE_GET_PROPERTY))
> -		err = dvb_frontend_ioctl_properties(file, cmd, parg);
> -	else {
> -		c->state = DTV_UNDEFINED;
> -		err = dvb_frontend_ioctl_legacy(file, cmd, parg);
> -	}
> +	c->state = DTV_UNDEFINED;> +	err = dvb_frontend_handle_ioctl(file, cmd, parg);

With this change, c->state value gets changed unconditionally before
calling the ioctl.

dvb_frontend_ioctl_properties() has logic for c->state == DTV_TUNE.
Is it safe to set change c->state here? I think it should be set
only when cmd is != FE_SET_PROPERTY or FE_GET_PROPERTY??

>  
>  	up(&fepriv->sem);
>  	return err;
>  }
>  
> -static int dvb_frontend_ioctl_properties(struct file *file,
> -			unsigned int cmd, void *parg)
> -{
> -	struct dvb_device *dvbdev = file->private_data;
> -	struct dvb_frontend *fe = dvbdev->priv;
> -	struct dvb_frontend_private *fepriv = fe->frontend_priv;
> -	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
> -	int err, i;
> -
> -	dev_dbg(fe->dvb->device, "%s:\n", __func__);
> -
> -	switch(cmd) {
> -	case FE_SET_PROPERTY: {
> -		struct dtv_properties *tvps = parg;
> -		struct dtv_property *tvp = NULL;
> -
> -		dev_dbg(fe->dvb->device, "%s: properties.num = %d\n",
> -			__func__, tvps->num);
> -		dev_dbg(fe->dvb->device, "%s: properties.props = %p\n",
> -			__func__, tvps->props);
> -
> -		/*
> -		 * Put an arbitrary limit on the number of messages that can
> -		 * be sent at once
> -		 */
> -		if (!tvps->num || (tvps->num > DTV_IOCTL_MAX_MSGS))
> -			return -EINVAL;
> -
> -		tvp = memdup_user(tvps->props, tvps->num * sizeof(*tvp));
> -		if (IS_ERR(tvp))
> -			return PTR_ERR(tvp);
> -
> -		for (i = 0; i < tvps->num; i++) {
> -			err = dtv_property_process_set(fe, tvp + i, file);
> -			if (err < 0) {
> -				kfree(tvp);
> -				return err;
> -			}
> -			(tvp + i)->result = err;
> -		}
> -
> -		if (c->state == DTV_TUNE)
> -			dev_dbg(fe->dvb->device, "%s: Property cache is full, tuning\n", __func__);
> -
> -		kfree(tvp);
> -		break;
> -	}
> -	case FE_GET_PROPERTY: {
> -		struct dtv_properties *tvps = parg;
> -		struct dtv_property *tvp = NULL;
> -		struct dtv_frontend_properties getp = fe->dtv_property_cache;
> -
> -		dev_dbg(fe->dvb->device, "%s: properties.num = %d\n",
> -			__func__, tvps->num);
> -		dev_dbg(fe->dvb->device, "%s: properties.props = %p\n",
> -			__func__, tvps->props);
> -
> -		/*
> -		 * Put an arbitrary limit on the number of messages that can
> -		 * be sent at once
> -		 */
> -		if (!tvps->num || (tvps->num > DTV_IOCTL_MAX_MSGS))
> -			return -EINVAL;
> -
> -		tvp = memdup_user(tvps->props, tvps->num * sizeof(*tvp));
> -		if (IS_ERR(tvp))
> -			return PTR_ERR(tvp);
> -
> -		/*
> -		 * Let's use our own copy of property cache, in order to
> -		 * avoid mangling with DTV zigzag logic, as drivers might
> -		 * return crap, if they don't check if the data is available
> -		 * before updating the properties cache.
> -		 */
> -		if (fepriv->state != FESTATE_IDLE) {
> -			err = dtv_get_frontend(fe, &getp, NULL);
> -			if (err < 0) {
> -				kfree(tvp);
> -				return err;
> -			}
> -		}
> -		for (i = 0; i < tvps->num; i++) {
> -			err = dtv_property_process_get(fe, &getp, tvp + i, file);
> -			if (err < 0) {
> -				kfree(tvp);
> -				return err;
> -			}
> -			(tvp + i)->result = err;
> -		}
> -
> -		if (copy_to_user((void __user *)tvps->props, tvp,
> -				 tvps->num * sizeof(struct dtv_property))) {
> -			kfree(tvp);
> -			return -EFAULT;
> -		}
> -		kfree(tvp);
> -		break;
> -	}
> -	default:
> -		return -ENOTSUPP;
> -	} /* switch */
> -	return 0;
> -}
> -
>  static int dtv_set_frontend(struct dvb_frontend *fe)
>  {
>  	struct dvb_frontend_private *fepriv = fe->frontend_priv;
> @@ -2205,59 +2094,102 @@ static int dtv_set_frontend(struct dvb_frontend *fe)
>  }
>  
>  
> -static int dvb_frontend_ioctl_legacy(struct file *file,
> -			unsigned int cmd, void *parg)
> +static int dvb_frontend_handle_ioctl(struct file *file,
> +				     unsigned int cmd, void *parg)
>  {
>  	struct dvb_device *dvbdev = file->private_data;
>  	struct dvb_frontend *fe = dvbdev->priv;
>  	struct dvb_frontend_private *fepriv = fe->frontend_priv;
>  	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
> -	int err = -EOPNOTSUPP;
> +	int i, err;
>  
> -	switch (cmd) {
> -	case FE_GET_INFO: {
> -		struct dvb_frontend_info* info = parg;
> +	dev_dbg(fe->dvb->device, "%s:\n", __func__);
>  
> -		memcpy(info, &fe->ops.info, sizeof(struct dvb_frontend_info));
> -		dvb_frontend_get_frequency_limits(fe, &info->frequency_min, &info->frequency_max);
> +	switch(cmd) {
> +	case FE_SET_PROPERTY: {
> +		struct dtv_properties *tvps = parg;
> +		struct dtv_property *tvp = NULL;
> +
> +		dev_dbg(fe->dvb->device, "%s: properties.num = %d\n",
> +			__func__, tvps->num);
> +		dev_dbg(fe->dvb->device, "%s: properties.props = %p\n",
> +			__func__, tvps->props);
> +
> +		/*
> +		 * Put an arbitrary limit on the number of messages that can
> +		 * be sent at once
> +		 */
> +		if (!tvps->num || (tvps->num > DTV_IOCTL_MAX_MSGS))
> +			return -EINVAL;
> +
> +		tvp = memdup_user(tvps->props, tvps->num * sizeof(*tvp));
> +		if (IS_ERR(tvp))
> +			return PTR_ERR(tvp);
> +
> +		for (i = 0; i < tvps->num; i++) {
> +			err = dtv_property_process_set(fe, tvp + i, file);
> +			if (err < 0) {
> +				kfree(tvp);
> +				return err;
> +			}
> +			(tvp + i)->result = err;
> +		}
> +
> +		if (c->state == DTV_TUNE)
> +			dev_dbg(fe->dvb->device, "%s: Property cache is full, tuning\n", __func__);

With the c->state being set unconditionally to DTV_UNDEFINED, this will break.

> +
> +		kfree(tvp);
> +		break;
> +	}
> +	case FE_GET_PROPERTY: {
> +		struct dtv_properties *tvps = parg;
> +		struct dtv_property *tvp = NULL;
> +		struct dtv_frontend_properties getp = fe->dtv_property_cache;
> +
> +		dev_dbg(fe->dvb->device, "%s: properties.num = %d\n",
> +			__func__, tvps->num);
> +		dev_dbg(fe->dvb->device, "%s: properties.props = %p\n",
> +			__func__, tvps->props);
> +
> +		/*
> +		 * Put an arbitrary limit on the number of messages that can
> +		 * be sent at once
> +		 */
> +		if (!tvps->num || (tvps->num > DTV_IOCTL_MAX_MSGS))
> +			return -EINVAL;
> +
> +		tvp = memdup_user(tvps->props, tvps->num * sizeof(*tvp));
> +		if (IS_ERR(tvp))
> +			return PTR_ERR(tvp);
>  
>  		/*
> -		 * Associate the 4 delivery systems supported by DVBv3
> -		 * API with their DVBv5 counterpart. For the other standards,
> -		 * use the closest type, assuming that it would hopefully
> -		 * work with a DVBv3 application.
> -		 * It should be noticed that, on multi-frontend devices with
> -		 * different types (terrestrial and cable, for example),
> -		 * a pure DVBv3 application won't be able to use all delivery
> -		 * systems. Yet, changing the DVBv5 cache to the other delivery
> -		 * system should be enough for making it work.
> +		 * Let's use our own copy of property cache, in order to
> +		 * avoid mangling with DTV zigzag logic, as drivers might
> +		 * return crap, if they don't check if the data is available
> +		 * before updating the properties cache.
>  		 */
> -		switch (dvbv3_type(c->delivery_system)) {
> -		case DVBV3_QPSK:
> -			info->type = FE_QPSK;
> -			break;
> -		case DVBV3_ATSC:
> -			info->type = FE_ATSC;
> -			break;
> -		case DVBV3_QAM:
> -			info->type = FE_QAM;
> -			break;
> -		case DVBV3_OFDM:
> -			info->type = FE_OFDM;
> -			break;
> -		default:
> -			dev_err(fe->dvb->device,
> -					"%s: doesn't know how to handle a DVBv3 call to delivery system %i\n",
> -					__func__, c->delivery_system);
> -			fe->ops.info.type = FE_OFDM;
> +		if (fepriv->state != FESTATE_IDLE) {
> +			err = dtv_get_frontend(fe, &getp, NULL);
> +			if (err < 0) {
> +				kfree(tvp);
> +				return err;
> +			}
> +		}
> +		for (i = 0; i < tvps->num; i++) {
> +			err = dtv_property_process_get(fe, &getp, tvp + i, file);
> +			if (err < 0) {
> +				kfree(tvp);
> +				return err;
> +			}
> +			(tvp + i)->result = err;
>  		}
> -		dev_dbg(fe->dvb->device, "%s: current delivery system on cache: %d, V3 type: %d\n",
> -				 __func__, c->delivery_system, fe->ops.info.type);
>  
> -		/* Set CAN_INVERSION_AUTO bit on in other than oneshot mode */
> -		if (!(fepriv->tune_mode_flags & FE_TUNE_MODE_ONESHOT))
> -			info->caps |= FE_CAN_INVERSION_AUTO;
> -		err = 0;
> +		if (copy_to_user((void __user *)tvps->props, tvp,
> +				 tvps->num * sizeof(struct dtv_property))) {
> +			kfree(tvp);
> +			return -EFAULT;
> +		}
> +		kfree(tvp);
>  		break;
>  	}
>  
> @@ -2278,42 +2210,6 @@ static int dvb_frontend_ioctl_legacy(struct file *file,
>  		break;
>  	}
>  
> -	case FE_READ_BER:
> -		if (fe->ops.read_ber) {
> -			if (fepriv->thread)
> -				err = fe->ops.read_ber(fe, (__u32 *) parg);
> -			else
> -				err = -EAGAIN;
> -		}
> -		break;
> -
> -	case FE_READ_SIGNAL_STRENGTH:
> -		if (fe->ops.read_signal_strength) {
> -			if (fepriv->thread)
> -				err = fe->ops.read_signal_strength(fe, (__u16 *) parg);
> -			else
> -				err = -EAGAIN;
> -		}
> -		break;
> -
> -	case FE_READ_SNR:
> -		if (fe->ops.read_snr) {
> -			if (fepriv->thread)
> -				err = fe->ops.read_snr(fe, (__u16 *) parg);
> -			else
> -				err = -EAGAIN;
> -		}
> -		break;
> -
> -	case FE_READ_UNCORRECTED_BLOCKS:
> -		if (fe->ops.read_ucblocks) {
> -			if (fepriv->thread)
> -				err = fe->ops.read_ucblocks(fe, (__u32 *) parg);
> -			else
> -				err = -EAGAIN;
> -		}
> -		break;
> -
>  	case FE_DISEQC_RESET_OVERLOAD:
>  		if (fe->ops.diseqc_reset_overload) {
>  			err = fe->ops.diseqc_reset_overload(fe);
> @@ -2365,6 +2261,23 @@ static int dvb_frontend_ioctl_legacy(struct file *file,
>  		}
>  		break;
>  
> +	case FE_DISEQC_RECV_SLAVE_REPLY:
> +		if (fe->ops.diseqc_recv_slave_reply)
> +			err = fe->ops.diseqc_recv_slave_reply(fe, (struct dvb_diseqc_slave_reply*) parg);
> +		break;
> +
> +	case FE_ENABLE_HIGH_LNB_VOLTAGE:
> +		if (fe->ops.enable_high_lnb_voltage)
> +			err = fe->ops.enable_high_lnb_voltage(fe, (long) parg);
> +		break;
> +
> +	case FE_SET_FRONTEND_TUNE_MODE:
> +		fepriv->tune_mode_flags = (unsigned long) parg;
> +		err = 0;
> +		break;
> +
> +	/* DEPRECATED dish control ioctls */
> +
>  	case FE_DISHNETWORK_SEND_LEGACY_CMD:
>  		if (fe->ops.dishnetwork_send_legacy_command) {
>  			err = fe->ops.dishnetwork_send_legacy_command(fe,
> @@ -2430,15 +2343,91 @@ static int dvb_frontend_ioctl_legacy(struct file *file,
>  		}
>  		break;
>  
> -	case FE_DISEQC_RECV_SLAVE_REPLY:
> -		if (fe->ops.diseqc_recv_slave_reply)
> -			err = fe->ops.diseqc_recv_slave_reply(fe, (struct dvb_diseqc_slave_reply*) parg);
> +	/* DEPRECATED statistics ioctls */
> +
> +	case FE_READ_BER:
> +		if (fe->ops.read_ber) {
> +			if (fepriv->thread)
> +				err = fe->ops.read_ber(fe, (__u32 *) parg);
> +			else
> +				err = -EAGAIN;
> +		}
> +		break;
> +
> +	case FE_READ_SIGNAL_STRENGTH:
> +		if (fe->ops.read_signal_strength) {
> +			if (fepriv->thread)
> +				err = fe->ops.read_signal_strength(fe, (__u16 *) parg);
> +			else
> +				err = -EAGAIN;
> +		}
>  		break;
>  
> -	case FE_ENABLE_HIGH_LNB_VOLTAGE:
> -		if (fe->ops.enable_high_lnb_voltage)
> -			err = fe->ops.enable_high_lnb_voltage(fe, (long) parg);
> +	case FE_READ_SNR:
> +		if (fe->ops.read_snr) {
> +			if (fepriv->thread)
> +				err = fe->ops.read_snr(fe, (__u16 *) parg);
> +			else
> +				err = -EAGAIN;
> +		}
> +		break;
> +
> +	case FE_READ_UNCORRECTED_BLOCKS:
> +		if (fe->ops.read_ucblocks) {
> +			if (fepriv->thread)
> +				err = fe->ops.read_ucblocks(fe, (__u32 *) parg);
> +			else
> +				err = -EAGAIN;
> +		}
> +		break;
> +
> +	/* DEPRECATED DVBv3 ioctls */
> +
> +	case FE_GET_INFO: {
> +		struct dvb_frontend_info* info = parg;
> +
> +		memcpy(info, &fe->ops.info, sizeof(struct dvb_frontend_info));
> +		dvb_frontend_get_frequency_limits(fe, &info->frequency_min, &info->frequency_max);
> +
> +		/*
> +		 * Associate the 4 delivery systems supported by DVBv3
> +		 * API with their DVBv5 counterpart. For the other standards,
> +		 * use the closest type, assuming that it would hopefully
> +		 * work with a DVBv3 application.
> +		 * It should be noticed that, on multi-frontend devices with
> +		 * different types (terrestrial and cable, for example),
> +		 * a pure DVBv3 application won't be able to use all delivery
> +		 * systems. Yet, changing the DVBv5 cache to the other delivery
> +		 * system should be enough for making it work.
> +		 */
> +		switch (dvbv3_type(c->delivery_system)) {
> +		case DVBV3_QPSK:
> +			info->type = FE_QPSK;
> +			break;
> +		case DVBV3_ATSC:
> +			info->type = FE_ATSC;
> +			break;
> +		case DVBV3_QAM:
> +			info->type = FE_QAM;
> +			break;
> +		case DVBV3_OFDM:
> +			info->type = FE_OFDM;
> +			break;
> +		default:
> +			dev_err(fe->dvb->device,
> +					"%s: doesn't know how to handle a DVBv3 call to delivery system %i\n",
> +					__func__, c->delivery_system);
> +			fe->ops.info.type = FE_OFDM;
> +		}
> +		dev_dbg(fe->dvb->device, "%s: current delivery system on cache: %d, V3 type: %d\n",
> +				 __func__, c->delivery_system, fe->ops.info.type);
> +
> +		/* Set CAN_INVERSION_AUTO bit on in other than oneshot mode */
> +		if (!(fepriv->tune_mode_flags & FE_TUNE_MODE_ONESHOT))
> +			info->caps |= FE_CAN_INVERSION_AUTO;
> +		err = 0;
>  		break;
> +	}
>  
>  	case FE_SET_FRONTEND:
>  		err = dvbv3_set_delivery_system(fe);
> @@ -2466,11 +2455,10 @@ static int dvb_frontend_ioctl_legacy(struct file *file,
>  		err = dtv_get_frontend(fe, &getp, parg);
>  		break;
>  	}
> -	case FE_SET_FRONTEND_TUNE_MODE:
> -		fepriv->tune_mode_flags = (unsigned long) parg;
> -		err = 0;
> -		break;
> -	}
> +
> +	default:
> +		return -ENOTSUPP;
> +	} /* switch */
>  
>  	return err;
>  }
> 

Rest looks okay to me. With c->state issue addressed and/or explained:

Reviewed-by: Shuah Khan <shuahkh@osg.samsung.com>

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/6] media: dvb_frontend: get rid of proprierty cache's state
  2017-09-19 13:42 ` [PATCH 3/6] media: dvb_frontend: get rid of proprierty cache's state Mauro Carvalho Chehab
@ 2017-09-20 21:08   ` Shuah Khan
  0 siblings, 0 replies; 12+ messages in thread
From: Shuah Khan @ 2017-09-20 21:08 UTC (permalink / raw)
  To: Mauro Carvalho Chehab, Linux Media Mailing List
  Cc: Mauro Carvalho Chehab, Max Kellermann, Colin Ian King,
	Michael Ira Krufky, Sakari Ailus, Shuah Khan, Shuah Khan

On 09/19/2017 07:42 AM, Mauro Carvalho Chehab wrote:
> In the past, I guess the idea was to use state in order to
> allow an autofush logic. However, in the current code, it is
> used only for debug messages, on a poor man's solution, as
> there's already a debug message to indicate when the properties
> got flushed.
> 
> So, just get rid of it for good.

Okay now PATCH 2/3 makes sense. Looks good to me.

Reviewed-by: Shuah Khan <shuahkg@osg.samsung.com>

> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> ---
>  drivers/media/dvb-core/dvb_frontend.c | 20 ++++++--------------
>  drivers/media/dvb-core/dvb_frontend.h |  5 -----
>  2 files changed, 6 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
> index cbe697a094d2..d0a17d67ab1b 100644
> --- a/drivers/media/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb-core/dvb_frontend.c
> @@ -951,8 +951,6 @@ static int dvb_frontend_clear_cache(struct dvb_frontend *fe)
>  	memset(c, 0, offsetof(struct dtv_frontend_properties, strength));
>  	c->delivery_system = delsys;
>  
> -	c->state = DTV_CLEAR;
> -
>  	dev_dbg(fe->dvb->device, "%s: Clearing cache for delivery system %d\n",
>  			__func__, c->delivery_system);
>  
> @@ -1775,13 +1773,13 @@ static int dtv_property_process_set(struct dvb_frontend *fe,
>  		dvb_frontend_clear_cache(fe);
>  		break;
>  	case DTV_TUNE:
> -		/* interpret the cache of data, build either a traditional frontend
> -		 * tunerequest so we can pass validation in the FE_SET_FRONTEND
> -		 * ioctl.
> +		/*
> +		 * Use the cached Digital TV properties to tune the
> +		 * frontend
>  		 */
> -		c->state = tvp->cmd;
> -		dev_dbg(fe->dvb->device, "%s: Finalised property cache\n",
> -				__func__);
> +		dev_dbg(fe->dvb->device,
> +			"%s: Setting the frontend from property cache\n",
> +			__func__);
>  
>  		r = dtv_set_frontend(fe);
>  		break;
> @@ -1930,7 +1928,6 @@ static int dvb_frontend_ioctl(struct file *file, unsigned int cmd, void *parg)
>  {
>  	struct dvb_device *dvbdev = file->private_data;
>  	struct dvb_frontend *fe = dvbdev->priv;
> -	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
>  	struct dvb_frontend_private *fepriv = fe->frontend_priv;
>  	int err;
>  
> @@ -1950,7 +1947,6 @@ static int dvb_frontend_ioctl(struct file *file, unsigned int cmd, void *parg)
>  		return -EPERM;
>  	}
>  
> -	c->state = DTV_UNDEFINED;
>  	err = dvb_frontend_handle_ioctl(file, cmd, parg);
>  
>  	up(&fepriv->sem);
> @@ -2134,10 +2130,6 @@ static int dvb_frontend_handle_ioctl(struct file *file,
>  			}
>  			(tvp + i)->result = err;
>  		}
> -
> -		if (c->state == DTV_TUNE)
> -			dev_dbg(fe->dvb->device, "%s: Property cache is full, tuning\n", __func__);
> -
>  		kfree(tvp);
>  		break;
>  	}
> diff --git a/drivers/media/dvb-core/dvb_frontend.h b/drivers/media/dvb-core/dvb_frontend.h
> index 852b91ba49d2..1bdeb10f0d78 100644
> --- a/drivers/media/dvb-core/dvb_frontend.h
> +++ b/drivers/media/dvb-core/dvb_frontend.h
> @@ -620,11 +620,6 @@ struct dtv_frontend_properties {
>  	struct dtv_fe_stats	post_bit_count;
>  	struct dtv_fe_stats	block_error;
>  	struct dtv_fe_stats	block_count;
> -
> -	/* private: */
> -	/* Cache State */
> -	u32			state;
> -
>  };
>  
>  #define DVB_FE_NO_EXIT  0
> 

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 5/6] media: dvb_frontend: better document the -EPERM condition
  2017-09-19 13:42 ` [PATCH 5/6] media: dvb_frontend: better document the -EPERM condition Mauro Carvalho Chehab
@ 2017-09-20 21:12   ` Shuah Khan
  0 siblings, 0 replies; 12+ messages in thread
From: Shuah Khan @ 2017-09-20 21:12 UTC (permalink / raw)
  To: Mauro Carvalho Chehab, Linux Media Mailing List
  Cc: Mauro Carvalho Chehab, Max Kellermann, Colin Ian King,
	Shuah Khan, Shuah Khan

On 09/19/2017 07:42 AM, Mauro Carvalho Chehab wrote:
> Two readonly ioctls can't be allowed if the frontend device
> is opened in read only mode. Explain why.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> ---
>  drivers/media/dvb-core/dvb_frontend.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
> index d0a17d67ab1b..db3f8c597a24 100644
> --- a/drivers/media/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb-core/dvb_frontend.c
> @@ -1940,9 +1940,23 @@ static int dvb_frontend_ioctl(struct file *file, unsigned int cmd, void *parg)
>  		return -ENODEV;
>  	}
>  
> -	if ((file->f_flags & O_ACCMODE) == O_RDONLY &&
> -	    (_IOC_DIR(cmd) != _IOC_READ || cmd == FE_GET_EVENT ||
> -	     cmd == FE_DISEQC_RECV_SLAVE_REPLY)) {
> +	/*
> +	 * If the frontend is opened in read-only mode, only the ioctls
> +	 * that don't interfere at the tune logic should be accepted.

Nit:

with the tune logic

> +	 * That allows an external application to monitor the DVB QoS and
> +	 * statistics parameters.
> +	 *
> +	 * That matches all _IOR() ioctls, except for two special cases:
> +	 *   - FE_GET_EVENT is part of the tuning logic on a DVB application;
> +	 *   - FE_DISEQC_RECV_SLAVE_REPLY is part of DiSEqC 2.0
> +	 *     setup
> +	 * So, those two ioctls should also return -EPERM, as otherwise
> +	 * reading from them would interfere with a DVB tune application
> +	 */
> +	if ((file->f_flags & O_ACCMODE) == O_RDONLY
> +	    && (_IOC_DIR(cmd) != _IOC_READ
> +		|| cmd == FE_GET_EVENT
> +		|| cmd == FE_DISEQC_RECV_SLAVE_REPLY)) {
>  		up(&fepriv->sem);
>  		return -EPERM;
>  	}
> 

Looks good to me

Reviewed by: Shuah Khan <shuahkh@osg.samsung.com>

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/6] media: dvb_frontend: cleanup ioctl handling logic
  2017-09-20 20:58   ` Shuah Khan
@ 2017-09-20 21:16     ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 12+ messages in thread
From: Mauro Carvalho Chehab @ 2017-09-20 21:16 UTC (permalink / raw)
  To: Shuah Khan
  Cc: Linux Media Mailing List, Mauro Carvalho Chehab, Max Kellermann,
	Colin Ian King, Shuah Khan

Em Wed, 20 Sep 2017 14:58:12 -0600
Shuah Khan <shuah@kernel.org> escreveu:

> > +	c->state = DTV_UNDEFINED;> +	err = dvb_frontend_handle_ioctl(file, cmd, parg);  
> 
> With this change, c->state value gets changed unconditionally before
> calling the ioctl.
> 
> dvb_frontend_ioctl_properties() has logic for c->state == DTV_TUNE.
> Is it safe to set change c->state here? I think it should be set
> only when cmd is != FE_SET_PROPERTY or FE_GET_PROPERTY??

It doesn't mind. c->state is used just for debugging purposes. One of the
patches I made got rid of it.



Thanks,
Mauro

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2017-09-20 21:16 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-19 13:42 [PATCH 1/6] media: dvb_frontend: cleanup dvb_frontend_ioctl_properties() Mauro Carvalho Chehab
2017-09-19 13:42 ` [PATCH 2/6] media: dvb_frontend: cleanup ioctl handling logic Mauro Carvalho Chehab
2017-09-20 20:58   ` Shuah Khan
2017-09-20 21:16     ` Mauro Carvalho Chehab
2017-09-19 13:42 ` [PATCH 3/6] media: dvb_frontend: get rid of proprierty cache's state Mauro Carvalho Chehab
2017-09-20 21:08   ` Shuah Khan
2017-09-19 13:42 ` [PATCH 4/6] media: dvb_frontend.h: fix alignment at the cache properties Mauro Carvalho Chehab
2017-09-19 13:42 ` [PATCH 5/6] media: dvb_frontend: better document the -EPERM condition Mauro Carvalho Chehab
2017-09-20 21:12   ` Shuah Khan
2017-09-19 13:42 ` [PATCH 6/6] media: dvb_frontend: fix return values for FE_SET_PROPERTY Mauro Carvalho Chehab
2017-09-20 20:11 ` [PATCH 1/6] media: dvb_frontend: cleanup dvb_frontend_ioctl_properties() Shuah Khan
2017-09-20 20:27   ` Mauro Carvalho Chehab

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.