| Message ID | 20201223202053.131157-1-trix@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | nfp: remove h from printk format specifier | expand |
On Wed, Dec 23, 2020 at 12:20:53PM -0800, trix@redhat.com wrote: > From: Tom Rix <trix@redhat.com> > > This change fixes the checkpatch warning described in this commit > commit cbacb5ab0aa0 ("docs: printk-formats: Stop encouraging use of unnecessary %h[xudi] and %hh[xudi]") > > Standard integer promotion is already done and %hx and %hhx is useless > so do not encourage the use of %hh[xudi] or %h[xudi]. > > Signed-off-by: Tom Rix <trix@redhat.com> Hi Tom, This patch looks appropriate for net-next, which is currently closed. The changes look fine, but I'm curious to know if its intentionally that the following was left alone in ethernet/netronome/nfp/nfp_net_ethtool.c:nfp_net_get_nspinfo() snprintf(version, ETHTOOL_FWVERS_LEN, "%hu.%hu" If the above was not intentional then perhaps you could respin with that updated and resubmit when net-next re-opens. Feel free to add: Reviewed-by: Simon Horman <simon.horman@netronome.com>
On 12/24/20 12:21 PM, Simon Horman wrote: > On Wed, Dec 23, 2020 at 12:20:53PM -0800, trix@redhat.com wrote: >> From: Tom Rix <trix@redhat.com> >> >> This change fixes the checkpatch warning described in this commit >> commit cbacb5ab0aa0 ("docs: printk-formats: Stop encouraging use of unnecessary %h[xudi] and %hh[xudi]") >> >> Standard integer promotion is already done and %hx and %hhx is useless >> so do not encourage the use of %hh[xudi] or %h[xudi]. >> >> Signed-off-by: Tom Rix <trix@redhat.com> > Hi Tom, > > This patch looks appropriate for net-next, which is currently closed. > > The changes look fine, but I'm curious to know if its intentionally that > the following was left alone in ethernet/netronome/nfp/nfp_net_ethtool.c:nfp_net_get_nspinfo() > > snprintf(version, ETHTOOL_FWVERS_LEN, "%hu.%hu" I am limiting changes to logging functions, what is roughly in checkpatch. I can add this snprintf in if you want. Tom > > If the above was not intentional then perhaps you could respin with that > updated and resubmit when net-next re-opens. Feel free to add: > > Reviewed-by: Simon Horman <simon.horman@netronome.com> >
On Thu, 2020-12-24 at 14:14 -0800, Tom Rix wrote: > On 12/24/20 12:21 PM, Simon Horman wrote: > > On Wed, Dec 23, 2020 at 12:20:53PM -0800, trix@redhat.com wrote: > > > From: Tom Rix <trix@redhat.com> > > > > > > This change fixes the checkpatch warning described in this commit > > > commit cbacb5ab0aa0 ("docs: printk-formats: Stop encouraging use of unnecessary %h[xudi] and %hh[xudi]") > > > > > > Standard integer promotion is already done and %hx and %hhx is useless > > > so do not encourage the use of %hh[xudi] or %h[xudi]. > > > > > > Signed-off-by: Tom Rix <trix@redhat.com> > > Hi Tom, > > > > This patch looks appropriate for net-next, which is currently closed. > > > > The changes look fine, but I'm curious to know if its intentionally that > > the following was left alone in ethernet/netronome/nfp/nfp_net_ethtool.c:nfp_net_get_nspinfo() > > > > snprintf(version, ETHTOOL_FWVERS_LEN, "%hu.%hu" > > I am limiting changes to logging functions, what is roughly in checkpatch. > > I can add this snprintf in if you want. I'm a bit confused here Tom. I thought your clang-tidy script was looking for anything marked with __printf() that is using %h[idux] or %hh[idux]. Wouldn't snprintf qualify for this already? include/linux/kernel.h-extern __printf(3, 4) include/linux/kernel.h:int snprintf(char *buf, size_t size, const char *fmt, ...); Kernel code doesn't use a signed char or short with %hx or %hu very often but in case you didn't already know, any signed char/short emitted with anything like %hx or %hu needs to be left alone as sign extension occurs so: signed char foo = -1; printk("%hx", foo); emits ffff but printk("%x", foo); emits ffffffff An example: $ gcc -x c - #include <stdio.h> #include <stdlib.h> int main(int argc, char **argv) { signed short i = -1; printf("hx: %hx\n", i); printf("x: %x\n", i); printf("hu: %hu\n", i); printf("u: %u\n", i); return 0; } $ ./a.out hx: ffff x: ffffffff hu: 65535 u: 4294967295 $
On 12/24/20 2:39 PM, Joe Perches wrote: > On Thu, 2020-12-24 at 14:14 -0800, Tom Rix wrote: >> On 12/24/20 12:21 PM, Simon Horman wrote: >>> On Wed, Dec 23, 2020 at 12:20:53PM -0800, trix@redhat.com wrote: >>>> From: Tom Rix <trix@redhat.com> >>>> >>>> This change fixes the checkpatch warning described in this commit >>>> commit cbacb5ab0aa0 ("docs: printk-formats: Stop encouraging use of unnecessary %h[xudi] and %hh[xudi]") >>>> >>>> Standard integer promotion is already done and %hx and %hhx is useless >>>> so do not encourage the use of %hh[xudi] or %h[xudi]. >>>> >>>> Signed-off-by: Tom Rix <trix@redhat.com> >>> Hi Tom, >>> >>> This patch looks appropriate for net-next, which is currently closed. >>> >>> The changes look fine, but I'm curious to know if its intentionally that >>> the following was left alone in ethernet/netronome/nfp/nfp_net_ethtool.c:nfp_net_get_nspinfo() >>> >>> snprintf(version, ETHTOOL_FWVERS_LEN, "%hu.%hu" >> I am limiting changes to logging functions, what is roughly in checkpatch. >> >> I can add this snprintf in if you want. > I'm a bit confused here Tom. > > I thought your clang-tidy script was looking for anything marked with > __printf() that is using %h[idux] or %hh[idux]. Yes, it uses the format attribute to find the logging functions. > > Wouldn't snprintf qualify for this already? > > include/linux/kernel.h-extern __printf(3, 4) > include/linux/kernel.h:int snprintf(char *buf, size_t size, const char *fmt, ...); Yes, this is found. But since snprintf is not really a logging function, I ignore these. If someone asks for them not to be ignored in a specific change, I will do that. > > Kernel code doesn't use a signed char or short with %hx or %hu very often > but in case you didn't already know, any signed char/short emitted with > anything like %hx or %hu needs to be left alone as sign extension occurs so: Yes, this would also effect checkpatch. Tom > > signed char foo = -1; > printk("%hx", foo); > > emits ffff but > > printk("%x", foo); > > emits ffffffff > > An example: > > $ gcc -x c - > #include <stdio.h> > #include <stdlib.h> > > int main(int argc, char **argv) > { > signed short i = -1; > printf("hx: %hx\n", i); > printf("x: %x\n", i); > printf("hu: %hu\n", i); > printf("u: %u\n", i); > return 0; > } > > $ ./a.out > hx: ffff > x: ffffffff > hu: 65535 > u: 4294967295 > > $ > >
On Fri, 2020-12-25 at 06:56 -0800, Tom Rix wrote: > On 12/24/20 2:39 PM, Joe Perches wrote: [] > > Kernel code doesn't use a signed char or short with %hx or %hu very often > > but in case you didn't already know, any signed char/short emitted with > > anything like %hx or %hu needs to be left alone as sign extension occurs so: > > Yes, this would also effect checkpatch. Of course but checkpatch is stupid and doesn't know types so it just assumes that the type argument is not signed. In general, that's a reasonable but imperfect assumption. coccinelle could probably do this properly as it's a much better parser. clang-tidy should be able to as well.
On 12/25/20 9:06 AM, Joe Perches wrote: > On Fri, 2020-12-25 at 06:56 -0800, Tom Rix wrote: >> On 12/24/20 2:39 PM, Joe Perches wrote: > [] >>> Kernel code doesn't use a signed char or short with %hx or %hu very often >>> but in case you didn't already know, any signed char/short emitted with >>> anything like %hx or %hu needs to be left alone as sign extension occurs so: >> Yes, this would also effect checkpatch. > Of course but checkpatch is stupid and doesn't know types > so it just assumes that the type argument is not signed. > > In general, that's a reasonable but imperfect assumption. > > coccinelle could probably do this properly as it's a much > better parser. clang-tidy should be able to as well. > Ok. But types not matching the format string is a larger problem. Has there been an effort to clean these up ? Tom
On Fri, 2020-12-25 at 14:13 -0800, Tom Rix wrote: > On 12/25/20 9:06 AM, Joe Perches wrote: > > On Fri, 2020-12-25 at 06:56 -0800, Tom Rix wrote: > > > On 12/24/20 2:39 PM, Joe Perches wrote: > > [] > > > > Kernel code doesn't use a signed char or short with %hx or %hu very often > > > > but in case you didn't already know, any signed char/short emitted with > > > > anything like %hx or %hu needs to be left alone as sign extension occurs so: > > > Yes, this would also effect checkpatch. > > Of course but checkpatch is stupid and doesn't know types > > so it just assumes that the type argument is not signed. > > > > In general, that's a reasonable but imperfect assumption. > > > > coccinelle could probably do this properly as it's a much > > better parser. clang-tidy should be able to as well. > > > Ok. > > But types not matching the format string is a larger problem. > > Has there been an effort to clean these up ? Not really no. __printf already does a reasonable job for that. The biggest issue for format type mismatches is the %p<foo> extensions. __printf can only verify that the argument is a pointer, not necessarily the 'right' type of pointed to object. There are overflow possibilities like '"%*ph", len, pointer' where pointer may not have len bytes available and, for instance, mismatched uses of %pI4 and %pI6 where %pI4 expects a pointer to 4 bytes and %pI6 expects a pointer to 16 bytes. Anyway it's not that easy a problem to analyze.
From: Tom Rix > Sent: 25 December 2020 14:57 ... > > Kernel code doesn't use a signed char or short with %hx or %hu very often > > but in case you didn't already know, any signed char/short emitted with > > anything like %hx or %hu needs to be left alone as sign extension occurs so: > > Yes, this would also effect checkpatch. Does the kernel printf do the masking for %hx and %hhx? A quick check I did showed that (at least some versions of) glibc does. But the printf builtin in bash doesn't. If the masking is there then %h[diux] and %hh[diux] are valid even though the varargs supplied parameter is always extended to at least the size of int. This is even true if the parameter might be large. For instance doing: ..., "%hh02x:%hh02x:%hh02x:%hh02x", x >> 24, x >> 16, x >> 8, x); will generate slightly smaller code than masking the passed values. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/verifier.c b/drivers/net/ethernet/netronome/nfp/bpf/verifier.c index e92ee510fd52..2681b5d56a38 100644 --- a/drivers/net/ethernet/netronome/nfp/bpf/verifier.c +++ b/drivers/net/ethernet/netronome/nfp/bpf/verifier.c @@ -828,7 +828,7 @@ int nfp_bpf_opt_replace_insn(struct bpf_verifier_env *env, u32 off, return 0; } - pr_vlog(env, "unsupported instruction replacement %hhx -> %hhx\n", + pr_vlog(env, "unsupported instruction replacement %x -> %x\n", meta->insn.code, insn->code); return -EINVAL; } diff --git a/drivers/net/ethernet/netronome/nfp/crypto/tls.c b/drivers/net/ethernet/netronome/nfp/crypto/tls.c index 84d66d138c3d..697317d60d29 100644 --- a/drivers/net/ethernet/netronome/nfp/crypto/tls.c +++ b/drivers/net/ethernet/netronome/nfp/crypto/tls.c @@ -486,7 +486,7 @@ int nfp_net_tls_rx_resync_req(struct net_device *netdev, th = pkt + req->l4_offset; if ((u8 *)&th[1] > (u8 *)pkt + pkt_len) { - netdev_warn_once(netdev, "invalid TLS RX resync request (l3_off: %hhu l4_off: %hhu pkt_len: %u)\n", + netdev_warn_once(netdev, "invalid TLS RX resync request (l3_off: %u l4_off: %u pkt_len: %u)\n", req->l3_offset, req->l4_offset, pkt_len); err = -EINVAL; goto err_cnt_ign; @@ -507,7 +507,7 @@ int nfp_net_tls_rx_resync_req(struct net_device *netdev, break; #endif default: - netdev_warn_once(netdev, "invalid TLS RX resync request (l3_off: %hhu l4_off: %hhu ipver: %u)\n", + netdev_warn_once(netdev, "invalid TLS RX resync request (l3_off: %u l4_off: %u ipver: %u)\n", req->l3_offset, req->l4_offset, iph->version); err = -EINVAL; goto err_cnt_ign; diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_mutex.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_mutex.c index 7bc17b94ac60..041801f0e668 100644 --- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_mutex.c +++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_mutex.c @@ -195,7 +195,7 @@ int nfp_cpp_mutex_lock(struct nfp_cpp_mutex *mutex) if (time_is_before_eq_jiffies(warn_at)) { warn_at = jiffies + NFP_MUTEX_WAIT_NEXT_WARN * HZ; nfp_warn(mutex->cpp, - "Warning: waiting for NFP mutex [depth:%hd target:%d addr:%llx key:%08x]\n", + "Warning: waiting for NFP mutex [depth:%d target:%d addr:%llx key:%08x]\n", mutex->depth, mutex->target, mutex->address, mutex->key); } diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.c index 10e7d8b21c46..06d03081a4dd 100644 --- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.c +++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.c @@ -247,12 +247,12 @@ static int nfp_nsp_check(struct nfp_nsp *state) state->ver.minor = FIELD_GET(NSP_STATUS_MINOR, reg); if (state->ver.major != NSP_MAJOR) { - nfp_err(cpp, "Unsupported ABI %hu.%hu\n", + nfp_err(cpp, "Unsupported ABI %u.%u\n", state->ver.major, state->ver.minor); return -EINVAL; } if (state->ver.minor < NSP_MINOR) { - nfp_err(cpp, "ABI too old to support NIC operation (%u.%hu < %u.%u), please update the management FW on the flash\n", + nfp_err(cpp, "ABI too old to support NIC operation (%u.%u < %u.%u), please update the management FW on the flash\n", NSP_MAJOR, state->ver.minor, NSP_MAJOR, NSP_MINOR); return -EINVAL; } @@ -662,7 +662,7 @@ nfp_nsp_command_buf(struct nfp_nsp *nsp, struct nfp_nsp_command_buf_arg *arg) int err; if (nsp->ver.minor < 13) { - nfp_err(cpp, "NSP: Code 0x%04x with buffer not supported (ABI %hu.%hu)\n", + nfp_err(cpp, "NSP: Code 0x%04x with buffer not supported (ABI %u.%u)\n", arg->arg.code, nsp->ver.major, nsp->ver.minor); return -EOPNOTSUPP; }