Re: [PATCH v7 12/13] can: peak_usb: dump undecodable URBs once and rate limit decode errors

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] For an undecodable URB, the decoder's error message and the hex
  dump of the same URB are now gated by two separate net_ratelimit()…

--

can: peak_usb: dump undecodable URBs once and rate limit decode errors

This patch limits the hex dump of an undecodable URB to the
urb->actual_length bytes received. It removes the extra dumps from the
PCAN-USB FD and Pro decoders so that only peak_usb_read_bulk_callback()
dumps the URB. It also puts net_ratelimit() in front of the dump and the
decoder error messages.

[ ... ]

> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> index e5198ec5c760d..1d306160fa354 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> @@ -266,10 +266,10 @@ static void peak_usb_read_bulk_callback(struct urb *urb)
>  		/* handle these kinds of msgs only if _start callback called */
>  		if (test_bit(PCAN_USB_STATE_STARTED, &dev->state)) {
>  			err = dev->adapter->dev_decode_buf(dev, urb);
> -			if (err)
> +			if (err && net_ratelimit())
>  				pcan_dump_mem("received usb message",
>  					      urb->transfer_buffer,
> -					      urb->transfer_buffer_length);
> +					      urb->actual_length);
>  		}
>  	}

[Severity: Low]
Can the decoder's error message and the dump of the same URB get split
up by two separate net_ratelimit() decisions?

Both calls use the single global net_ratelimit_state (5 * HZ interval,
burst of 10). For an undecodable URB, pcan_usb_fd_decode_buf() or
pcan_usb_pro_decode_buf() first calls net_ratelimit() for its own
message. peak_usb_read_bulk_callback() then calls it again for the dump.

For example, in pcan_usb_pro_decode_buf():

    if (!sizeof_rec) {
        if (net_ratelimit())
            netdev_err(netdev,
                       "got unsupported rec in usb msg:\n");
        err = -ENOTSUPP;
        break;
    }

Suppose the decoder's message takes the last token in the burst. The
dump in peak_usb_read_bulk_callback() is then suppressed. The "got
unsupported rec in usb msg:" line, which ends in a colon, gets logged
without the dump that should follow it. The reverse can also happen:
the dump appears without its message. The same applies to "bad msg hdr
len", "got frag rec" and "got short rec" in the FD and Pro decoders.

The per-record "unhandled msg type" and "unhandled rec type" messages
printed earlier in the loop for the same URB also use up tokens, and so
does other networking code. That makes it more likely that the dump for
a later fatal error in the URB is dropped.

Before this patch, the FD and Pro decoders always printed the dump right
after the message. Would making one rate-limit decision per URB, either
in the core or passed down to the decoders, keep each message and its
dump together?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261030-canfd_check_channel_idx-v7-0-fc4a3faf3acf%40peak-system.fr