| Message ID | 20190617233050.21409-1-dmg@turingmachine.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: Replace a < b ? a : b construct with min_t(type, a, b) in drivers/usb | expand |
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
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
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)
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 .
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
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;