Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] usb: Replace a < b ? a : b construct with min_t(type, a, b) in drivers/usb
@ 2019-06-17 23:30 dmg
  2019-06-18  6:49 ` Greg KH
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: dmg @ 2019-06-17 23:30 UTC (permalink / raw)
  To: linux-usb; +Cc: gregkh, Daniel M German

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] usb: Replace a < b ? a : b construct with min_t(type, a, b) in drivers/usb
  2019-06-17 23:30 [PATCH] usb: Replace a < b ? a : b construct with min_t(type, a, b) in drivers/usb dmg
@ 2019-06-18  6:49 ` Greg KH
  2019-06-18 15:00   ` dmg
  2019-06-18  7:15 ` Felipe Balbi
  2019-06-18 10:03 ` David Laight
  2 siblings, 1 reply; 6+ messages in thread
From: Greg KH @ 2019-06-18  6:49 UTC (permalink / raw)
  To: dmg; +Cc: linux-usb

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] usb: Replace a < b ? a : b construct with min_t(type, a, b) in drivers/usb
  2019-06-17 23:30 [PATCH] usb: Replace a < b ? a : b construct with min_t(type, a, b) in drivers/usb dmg
  2019-06-18  6:49 ` Greg KH
@ 2019-06-18  7:15 ` Felipe Balbi
  2019-06-18 10:03 ` David Laight
  2 siblings, 0 replies; 6+ messages in thread
From: Felipe Balbi @ 2019-06-18  7:15 UTC (permalink / raw)
  To: dmg, linux-usb; +Cc: gregkh, Daniel M German

[-- 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 --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH] usb: Replace a < b ? a : b construct with min_t(type, a, b) in drivers/usb
  2019-06-17 23:30 [PATCH] usb: Replace a < b ? a : b construct with min_t(type, a, b) in drivers/usb dmg
  2019-06-18  6:49 ` Greg KH
  2019-06-18  7:15 ` Felipe Balbi
@ 2019-06-18 10:03 ` David Laight
  2 siblings, 0 replies; 6+ messages in thread
From: David Laight @ 2019-06-18 10:03 UTC (permalink / raw)
  To: dmg, linux-usb; +Cc: gregkh

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)


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] usb: Replace a < b ? a : b construct with min_t(type, a, b) in drivers/usb
  2019-06-18  6:49 ` Greg KH
@ 2019-06-18 15:00   ` dmg
  2019-06-18 15:26     ` Greg KH
  0 siblings, 1 reply; 6+ messages in thread
From: dmg @ 2019-06-18 15:00 UTC (permalink / raw)
  To: Greg KH; +Cc: linux-usb


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 .

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] usb: Replace a < b ? a : b construct with min_t(type, a, b) in drivers/usb
  2019-06-18 15:00   ` dmg
@ 2019-06-18 15:26     ` Greg KH
  0 siblings, 0 replies; 6+ messages in thread
From: Greg KH @ 2019-06-18 15:26 UTC (permalink / raw)
  To: dmg; +Cc: linux-usb

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-17 23:30 [PATCH] usb: Replace a < b ? a : b construct with min_t(type, a, b) in drivers/usb dmg
2019-06-18  6:49 ` Greg KH
2019-06-18 15:00   ` dmg
2019-06-18 15:26     ` Greg KH
2019-06-18  7:15 ` Felipe Balbi
2019-06-18 10:03 ` David Laight

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org linux-usb@archiver.kernel.org
	public-inbox-index linux-usb


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/ public-inbox