netdev - Re: [PATCH v4 4/7] can: replace can_dlc as variable/element for payload length

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMZ6RqJz+G-R6LF=jU22kcYPBwTCOB7XmcY+GTNLmfm+-9rvUw@mail.gmail.com>
Date:   Mon, 9 Nov 2020 21:59:57 +0900
From:   Vincent MAILHOL <mailhol.vincent@...adoo.fr>
To:     Oliver Hartkopp <socketcan@...tkopp.net>
Cc:     linux-can <linux-can@...r.kernel.org>,
        Marc Kleine-Budde <mkl@...gutronix.de>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH v4 4/7] can: replace can_dlc as variable/element for
 payload length

On Mon. 9 Nov 2020 at 19:26, Oliver Hartkopp wrote:
> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> index b2e8df8e4cb0..72671184a7a2 100644
> --- a/include/linux/can/dev.h
> +++ b/include/linux/can/dev.h
> @@ -183,12 +183,12 @@ static inline void can_set_static_ctrlmode(struct net_device *dev,
>         /* override MTU which was set by default in can_setup()? */
>         if (static_mode & CAN_CTRLMODE_FD)
>                 dev->mtu = CANFD_MTU;
>  }
>
> -/* get data length from can_dlc with sanitized can_dlc */
> -u8 can_dlc2len(u8 can_dlc);
> +/* get data length from raw data length code (DLC) */

/*
 * convert a given data length code (dlc) of an FD CAN frame into a
 * valid data length of max. 64 bytes.
 */

I missed this point during my previous review: the can_dlc2len() function
is only valid for CAN FD frames. Comments should reflect this fact.

> +u8 can_dlc2len(u8 dlc);

Concerning the name:
 * can_get_cc_len() converts a Classical CAN frame DLC into a data
   length.
 * can_dlc2len() converts an FD CAN frame DLC into a data length.

Just realized that both macro/function do similar things so we could
think of a similar naming as well.
 * Example 1: can_get_cc_len() and can_get_fd_len()
 * Example 2: can_cc_dlc2len() and can_fd_dlc2len()

Or we could simply leave things as they are, this is not a big issue
as long as the comments clearly state which one is for classical
frames and which one is for FD frames.

>
>  /* map the sanitized data length to an appropriate data length code */
>  u8 can_len2dlc(u8 len);

can_len2dlc() might be renamed (e.g. can_get_fd_dlc()) if Example 1
solution is chosen.

>  struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max,

Yours sincerely,
Vincent Mailhol