diff mbox series

[v8,03/22] counter: 104-quad-8: Return error when invalid mode during ceiling_write

Message ID 2be0c071e7730ea16b8faa1efcd3d0d908e7f9db.1613131238.git.vilhelm.gray@gmail.com (mailing list archive)
State New
Headers show
Series Introduce the Counter character device interface | expand

Commit Message

William Breathitt Gray Feb. 12, 2021, 12:13 p.m. UTC
The 104-QUAD-8 only has two count modes where a ceiling value makes
sense: Range Limit and Modulo-N. Outside of these two modes, setting a
ceiling value is an invalid operation -- so let's report it as such by
returning -EINVAL.

Fixes: fc069262261c ("counter: 104-quad-8: Add lock guards - generic interface")
Cc: Syed Nayyar Waris <syednwaris@gmail.com>
Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
---
 drivers/counter/104-quad-8.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

David Lechner Feb. 20, 2021, 4:30 p.m. UTC | #1
On 2/12/21 6:13 AM, William Breathitt Gray wrote:
> The 104-QUAD-8 only has two count modes where a ceiling value makes
> sense: Range Limit and Modulo-N. Outside of these two modes, setting a
> ceiling value is an invalid operation -- so let's report it as such by
> returning -EINVAL.
> 
> Fixes: fc069262261c ("counter: 104-quad-8: Add lock guards - generic interface")
> Cc: Syed Nayyar Waris <syednwaris@gmail.com>
> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> ---
>   drivers/counter/104-quad-8.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c
> index 9691f8612be8..f0608b21196a 100644
> --- a/drivers/counter/104-quad-8.c
> +++ b/drivers/counter/104-quad-8.c
> @@ -714,13 +714,14 @@ static ssize_t quad8_count_ceiling_write(struct counter_device *counter,
>   	switch (priv->count_mode[count->id]) {
>   	case 1:
>   	case 3:
> +		mutex_unlock(&priv->lock);
>   		quad8_preset_register_set(priv, count->id, ceiling);
> -		break;
> +		return len;
>   	}
>   
>   	mutex_unlock(&priv->lock);
>   
> -	return len;
> +	return -EINVAL;
>   }
>   
>   static ssize_t quad8_count_preset_enable_read(struct counter_device *counter,
> 

What happens if the register is written to when in the wrong mode?

If it doesn't hurt anything, I would suggest always writing the register
instead so that users don't have to know worry about the fact that the mode
has to be set first.
William Breathitt Gray Feb. 22, 2021, 2:06 a.m. UTC | #2
On Sat, Feb 20, 2021 at 10:30:03AM -0600, David Lechner wrote:
> On 2/12/21 6:13 AM, William Breathitt Gray wrote:
> > The 104-QUAD-8 only has two count modes where a ceiling value makes
> > sense: Range Limit and Modulo-N. Outside of these two modes, setting a
> > ceiling value is an invalid operation -- so let's report it as such by
> > returning -EINVAL.
> > 
> > Fixes: fc069262261c ("counter: 104-quad-8: Add lock guards - generic interface")
> > Cc: Syed Nayyar Waris <syednwaris@gmail.com>
> > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> > ---
> >   drivers/counter/104-quad-8.c | 5 +++--
> >   1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c
> > index 9691f8612be8..f0608b21196a 100644
> > --- a/drivers/counter/104-quad-8.c
> > +++ b/drivers/counter/104-quad-8.c
> > @@ -714,13 +714,14 @@ static ssize_t quad8_count_ceiling_write(struct counter_device *counter,
> >   	switch (priv->count_mode[count->id]) {
> >   	case 1:
> >   	case 3:
> > +		mutex_unlock(&priv->lock);
> >   		quad8_preset_register_set(priv, count->id, ceiling);
> > -		break;
> > +		return len;
> >   	}
> >   
> >   	mutex_unlock(&priv->lock);
> >   
> > -	return len;
> > +	return -EINVAL;
> >   }
> >   
> >   static ssize_t quad8_count_preset_enable_read(struct counter_device *counter,
> > 
> 
> What happens if the register is written to when in the wrong mode?
> 
> If it doesn't hurt anything, I would suggest always writing the register
> instead so that users don't have to know worry about the fact that the mode
> has to be set first.

Unfortunately this register is used for two different functions: as a
ceiling limit and as a preset value. When we're in the preset mode, the
value of this register will be used to set the counter value when an
interrupt occurs; whereas in ceiling mode, this value is used as the
upper counting limit for the counter. We should only set this value when
in the correct mode, lest we end clobbering the value for the other mode
(e.g. user sets ceiling value, but driver unexpectantly changes the
preset value).

William Breathitt Gray
diff mbox series

Patch

diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c
index 9691f8612be8..f0608b21196a 100644
--- a/drivers/counter/104-quad-8.c
+++ b/drivers/counter/104-quad-8.c
@@ -714,13 +714,14 @@  static ssize_t quad8_count_ceiling_write(struct counter_device *counter,
 	switch (priv->count_mode[count->id]) {
 	case 1:
 	case 3:
+		mutex_unlock(&priv->lock);
 		quad8_preset_register_set(priv, count->id, ceiling);
-		break;
+		return len;
 	}
 
 	mutex_unlock(&priv->lock);
 
-	return len;
+	return -EINVAL;
 }
 
 static ssize_t quad8_count_preset_enable_read(struct counter_device *counter,