Re: [PATCH 5/6] media: dvb_frontend: better document the -EPERM condition

2017-09-20 Thread 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 
> ---
>  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 

thanks,
-- Shuah


[PATCH 5/6] media: dvb_frontend: better document the -EPERM condition

2017-09-19 Thread Mauro Carvalho Chehab
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 
---
 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