diff mbox series

[v1,1/1] pktcdvd: Use clamp_val() instead of min()+max()

Message ID 20230616142614.36206-1-andriy.shevchenko@linux.intel.com
State New
Headers
Series [v1,1/1] pktcdvd: Use clamp_val() instead of min()+max() |

Commit Message

Andy Shevchenko June 16, 2023, 2:26 p.m. UTC 
  In a couple of places replace min()+max() pair by clamp_val().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/block/pktcdvd.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

Comments

David Laight June 20, 2023, 12:06 p.m. UTC | #1 
From: Andy Shevchenko
> Sent: 16 June 2023 15:26
> 
> In a couple of places replace min()+max() pair by clamp_val().
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/block/pktcdvd.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
> index a1428538bda5..18a960bb6165 100644
> --- a/drivers/block/pktcdvd.c
> +++ b/drivers/block/pktcdvd.c
> @@ -208,14 +208,11 @@ static DEVICE_ATTR_RO(size);
>  static void init_write_congestion_marks(int* lo, int* hi)
>  {
>  	if (*hi > 0) {
> -		*hi = max(*hi, 500);
> -		*hi = min(*hi, 1000000);
> +		*hi = clamp_val(*hi, 500, 1000000);

(standard rant about minmax.h)

clamp_val() is pretty much broken by design.
It MIGHT be ok here but it casts both limits to the
type of the value being compared.
In general that is just plain wrong.

Like min_t() it is generally ok because the kernel only uses
unsigned values between 0 and MAXINT.

If min/max were ok, then using clamp() should also be ok.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Andy Shevchenko June 20, 2023, 1:24 p.m. UTC | #2 
On Tue, Jun 20, 2023 at 12:06:49PM +0000, David Laight wrote:

...

> > +		*hi = clamp_val(*hi, 500, 1000000);
> 
> (standard rant about minmax.h)
> 
> clamp_val() is pretty much broken by design.
> It MIGHT be ok here but it casts both limits to the
> type of the value being compared.
> In general that is just plain wrong.
> 
> Like min_t() it is generally ok because the kernel only uses
> unsigned values between 0 and MAXINT.
> 
> If min/max were ok, then using clamp() should also be ok.

Submit a patch to fix it, if you think you can make it better.
Obviously your comment can be addressed separately if we even
need that.
David Laight June 20, 2023, 1:35 p.m. UTC | #3 
From: 'Andy Shevchenko'
> Sent: 20 June 2023 14:25
> 
> On Tue, Jun 20, 2023 at 12:06:49PM +0000, David Laight wrote:
> 
> ...
> 
> > > +		*hi = clamp_val(*hi, 500, 1000000);
> >
> > (standard rant about minmax.h)
> >
> > clamp_val() is pretty much broken by design.
> > It MIGHT be ok here but it casts both limits to the
> > type of the value being compared.
> > In general that is just plain wrong.
> >
> > Like min_t() it is generally ok because the kernel only uses
> > unsigned values between 0 and MAXINT.
> >
> > If min/max were ok, then using clamp() should also be ok.
> 
> Submit a patch to fix it, if you think you can make it better.
> Obviously your comment can be addressed separately if we even
> need that.

Did you try using clamp() ?

To see why clamp_val() is broken consider?
	unsigned char val = 200;
	...
	xxx = clamp_val(val, 10, 300);
 
This sets xxx to 44 - not exactly expected.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Andy Shevchenko June 20, 2023, 3:03 p.m. UTC | #4 
On Tue, Jun 20, 2023 at 01:35:11PM +0000, David Laight wrote:
> From: 'Andy Shevchenko'
> > Sent: 20 June 2023 14:25
> > On Tue, Jun 20, 2023 at 12:06:49PM +0000, David Laight wrote:

...

> > > > +		*hi = clamp_val(*hi, 500, 1000000);
> > >
> > > (standard rant about minmax.h)
> > >
> > > clamp_val() is pretty much broken by design.
> > > It MIGHT be ok here but it casts both limits to the
> > > type of the value being compared.
> > > In general that is just plain wrong.
> > >
> > > Like min_t() it is generally ok because the kernel only uses
> > > unsigned values between 0 and MAXINT.
> > >
> > > If min/max were ok, then using clamp() should also be ok.
> > 
> > Submit a patch to fix it, if you think you can make it better.
> > Obviously your comment can be addressed separately if we even
> > need that.
> 
> Did you try using clamp() ?
> 
> To see why clamp_val() is broken consider?
> 	unsigned char val = 200;
> 	...
> 	xxx = clamp_val(val, 10, 300);
>  
> This sets xxx to 44 - not exactly expected.

Right, clamp_val() has to be improved.
However this is not the case in this driver.

I have just sent a v2 with clamp().
diff mbox series

Patch

diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index a1428538bda5..18a960bb6165 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -208,14 +208,11 @@  static DEVICE_ATTR_RO(size);
 static void init_write_congestion_marks(int* lo, int* hi)
 {
 	if (*hi > 0) {
-		*hi = max(*hi, 500);
-		*hi = min(*hi, 1000000);
+		*hi = clamp_val(*hi, 500, 1000000);
 		if (*lo <= 0)
 			*lo = *hi - 100;
-		else {
-			*lo = min(*lo, *hi - 100);
-			*lo = max(*lo, 100);
-		}
+		else
+			*lo = clamp_val(*lo, 100, *hi - 100);
 	} else {
 		*hi = -1;
 		*lo = -1;