[PATCH] usb: Replace a < b ? a : b construct with min_t(type, a, b) in drivers/usb

[PATCH] usb: Replace a < b ? a : b construct with min_t(type, a, b) in drivers/usb

[dmg]

From: Daniel M German <dmg@turingmachine.org>

Use min_t to find the minimum of two values instead of using the ?: operator.

This change does not alter functionality. It is merely cosmetic intended to
improve the readability of the code.

Signed-off-by: Daniel M German <dmg@turingmachine.org>
---
 drivers/usb/gadget/function/u_ether.c | 2 +-
 drivers/usb/misc/adutux.c             | 2 +-
 drivers/usb/storage/realtek_cr.c      | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
index 737bd77a575d..f6ba46684ddb 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -1006,7 +1006,7 @@ int gether_get_ifname(struct net_device *net, char *name, int len)
 	rtnl_lock();
 	ret = snprintf(name, len, "%s\n", netdev_name(net));
 	rtnl_unlock();
-	return ret < len ? ret : len;
+	return min_t(int, ret, len);
 }
 EXPORT_SYMBOL_GPL(gether_get_ifname);
 
diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index 9465fb95d70a..4a9fa3152f2a 100644
--- a/drivers/usb/misc/adutux.c
+++ b/drivers/usb/misc/adutux.c
@@ -379,7 +379,7 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
 
 		if (data_in_secondary) {
 			/* drain secondary buffer */
-			int amount = bytes_to_read < data_in_secondary ? bytes_to_read : data_in_secondary;
+			int amount = min_t(size_t, bytes_to_read, data_in_secondary);
 			i = copy_to_user(buffer, dev->read_buffer_secondary+dev->secondary_head, amount);
 			if (i) {
 				retval = -EFAULT;
diff --git a/drivers/usb/storage/realtek_cr.c b/drivers/usb/storage/realtek_cr.c
index cc794e25a0b6..15ce54bde600 100644
--- a/drivers/usb/storage/realtek_cr.c
+++ b/drivers/usb/storage/realtek_cr.c
@@ -260,7 +260,7 @@ static int rts51x_bulk_transport(struct us_data *us, u8 lun,
 	 * was really transferred and what the device tells us
 	 */
 	if (residue)
-		residue = residue < buf_len ? residue : buf_len;
+		residue = min_t(unsigned int, residue, buf_len);
 
 	if (act_len)
 		*act_len = buf_len - residue;
-- 
2.20.1

Re: [PATCH] usb: Replace a < b ? a : b construct with min_t(type, a, b) in drivers/usb

[Greg KH]

On Mon, Jun 17, 2019 at 04:30:50PM -0700, dmg@turingmachine.org wrote:
> From: Daniel M German <dmg@turingmachine.org>
> 
> Use min_t to find the minimum of two values instead of using the ?: operator.

Why is min_t() needed for all of these and not just min()?

> 
> This change does not alter functionality. It is merely cosmetic intended to
> improve the readability of the code.
> 
> Signed-off-by: Daniel M German <dmg@turingmachine.org>
> ---
>  drivers/usb/gadget/function/u_ether.c | 2 +-
>  drivers/usb/misc/adutux.c             | 2 +-
>  drivers/usb/storage/realtek_cr.c      | 2 +-

Can you break this up into one patch per driver?  That way you can
include the proper maintainers/reviewers when you resend them.

thanks,

greg k-h

Re: [PATCH] usb: Replace a < b ? a : b construct with min_t(type, a, b) in drivers/usb

[dmg]

Greg KH <gregkh@linuxfoundation.org> writes:

> On Mon, Jun 17, 2019 at 04:30:50PM -0700, dmg@turingmachine.org wrote:
>> From: Daniel M German <dmg@turingmachine.org>
>>
>> Use min_t to find the minimum of two values instead of using the ?: operator.
>
> Why is min_t() needed for all of these and not just min()?

The use of min triggers a compilation warning (see below), which min_t is supposed to
address (from min_t comment: 'min()/max() macros that also do strict type-checking.. See the
"unnecessary" pointer comparison.", from include/linux/kernel')

   In file included from drivers/usb/misc/adutux.c:19:
   drivers/usb/misc/adutux.c: In function ‘adu_read’:
   ./include/linux/kernel.h:818:29: warning: comparison of distinct pointer types lacks a cast
      (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
                                ^~
   ./include/linux/kernel.h:832:4: note: in expansion of macro ‘__typecheck’
      (__typecheck(x, y) && __no_side_effects(x, y))
       ^~~~~~~~~~~
   ./include/linux/kernel.h:842:24: note: in expansion of macro ‘__safe_cmp’
     __builtin_choose_expr(__safe_cmp(x, y), \
                           ^~~~~~~~~~
   ./include/linux/kernel.h:851:19: note: in expansion of macro ‘__careful_cmp’
    #define min(x, y) __careful_cmp(x, y, <)
                      ^~~~~~~~~~~~~
   drivers/usb/misc/adutux.c:382:34: note: in expansion of macro ‘min’
                        int amount = min(bytes_to_read, data_in_secondary);
                                     ^~~

[...]

> Can you break this up into one patch per driver?  That way you can
> include the proper maintainers/reviewers when you resend them.
>

I will split the patch as instructed.



> thanks,
>
> greg k-h


--
Daniel M. German                  "Beauty is the first test; there is no
                                   permanent place in the world for ugly
   G. H. Hardy ->                  mathematics."
http://turingmachine.org/
http://silvernegative.com/
dmg (at) uvic (dot) ca
replace (at) with @ and (dot) with .

Re: [PATCH] usb: Replace a < b ? a : b construct with min_t(type, a, b) in drivers/usb

[Greg KH]

On Tue, Jun 18, 2019 at 08:00:28AM -0700, dmg wrote:
> 
> Greg KH <gregkh@linuxfoundation.org> writes:
> 
> > On Mon, Jun 17, 2019 at 04:30:50PM -0700, dmg@turingmachine.org wrote:
> >> From: Daniel M German <dmg@turingmachine.org>
> >>
> >> Use min_t to find the minimum of two values instead of using the ?: operator.
> >
> > Why is min_t() needed for all of these and not just min()?
> 
> The use of min triggers a compilation warning (see below), which min_t is supposed to
> address (from min_t comment: 'min()/max() macros that also do strict type-checking.. See the
> "unnecessary" pointer comparison.", from include/linux/kernel')
> 
>    In file included from drivers/usb/misc/adutux.c:19:
>    drivers/usb/misc/adutux.c: In function ‘adu_read’:
>    ./include/linux/kernel.h:818:29: warning: comparison of distinct pointer types lacks a cast
>       (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
>                                 ^~
>    ./include/linux/kernel.h:832:4: note: in expansion of macro ‘__typecheck’
>       (__typecheck(x, y) && __no_side_effects(x, y))
>        ^~~~~~~~~~~
>    ./include/linux/kernel.h:842:24: note: in expansion of macro ‘__safe_cmp’
>      __builtin_choose_expr(__safe_cmp(x, y), \
>                            ^~~~~~~~~~
>    ./include/linux/kernel.h:851:19: note: in expansion of macro ‘__careful_cmp’
>     #define min(x, y) __careful_cmp(x, y, <)
>                       ^~~~~~~~~~~~~
>    drivers/usb/misc/adutux.c:382:34: note: in expansion of macro ‘min’
>                         int amount = min(bytes_to_read, data_in_secondary);
>                                      ^~~

Yes, but is it needed for all of these?

And what about just changing the types of those variables to be the
same?  Does the cast have to be there?

thanks,

greg k-h

Re: [PATCH] usb: Replace a < b ? a : b construct with min_t(type, a, b) in drivers/usb

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 690 bytes --]


Hi,

dmg@turingmachine.org writes:
> From: Daniel M German <dmg@turingmachine.org>
>
> Use min_t to find the minimum of two values instead of using the ?: operator.
>
> This change does not alter functionality. It is merely cosmetic intended to
> improve the readability of the code.
>
> Signed-off-by: Daniel M German <dmg@turingmachine.org>
> ---
>  drivers/usb/gadget/function/u_ether.c | 2 +-
>  drivers/usb/misc/adutux.c             | 2 +-
>  drivers/usb/storage/realtek_cr.c      | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)

could you split this into three patches? One per driver? That way they
can be reviewed separately.

Thanks

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

RE: [PATCH] usb: Replace a < b ? a : b construct with min_t(type, a, b) in drivers/usb

[David Laight]

From: dmg@turingmachine.org
> Sent: 18 June 2019 00:31
> Use min_t to find the minimum of two values instead of using the ?: operator.
> 
> This change does not alter functionality. It is merely cosmetic intended to
> improve the readability of the code.
> 
> Signed-off-by: Daniel M German <dmg@turingmachine.org>
> ---
>  drivers/usb/gadget/function/u_ether.c | 2 +-
>  drivers/usb/misc/adutux.c             | 2 +-
>  drivers/usb/storage/realtek_cr.c      | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
> index 737bd77a575d..f6ba46684ddb 100644
> --- a/drivers/usb/gadget/function/u_ether.c
> +++ b/drivers/usb/gadget/function/u_ether.c
> @@ -1006,7 +1006,7 @@ int gether_get_ifname(struct net_device *net, char *name, int len)
>  	rtnl_lock();
>  	ret = snprintf(name, len, "%s\n", netdev_name(net));
>  	rtnl_unlock();
> -	return ret < len ? ret : len;
> +	return min_t(int, ret, len);
>  }

I'm not sure using min() or min_t() helps readability here.

In any case that code fragment looks broken!
Were buf[] too small the length returned would include a '\0'.
Now it is quite likely that the overflow is actually impossible
(provided buf[] has room for the '\n').

OTOH the 'correct' fix is to replace the snprintf() with scnprintf()
and remove the 'min()' completely.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)