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@xxxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxx>
thanks,
-- Shuah
