Re: [PATCH net-next 1/2] can-isotp: implement cleanups / improvements from review

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1.1: Type: text/plain, Size: 4523 bytes --]

On 10/10/20 10:49 PM, Oliver Hartkopp wrote:
> As pointed out by Jakub Kicinski here:
> https://marc.info/?l=linux-can&m=160229286216008
> this patch addresses the remarked issues:
> 
> - remove empty lines in comment
> - remove default=y for CAN_ISOTP in Kconfig
> - make use of pr_notice_once()
> - use GFP_KERNEL instead of gfp_any() in soft hrtimer context
> 
> The version strings in the CAN subsystem are removed by a separate patch.
> 
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
>  include/uapi/linux/can/isotp.h |  4 +---
>  net/can/Kconfig                |  3 ++-
>  net/can/isotp.c                | 14 +++++++-------
>  3 files changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/include/uapi/linux/can/isotp.h b/include/uapi/linux/can/isotp.h
> index 553006509f4e..accf0efa46f4 100644
> --- a/include/uapi/linux/can/isotp.h
> +++ b/include/uapi/linux/can/isotp.h
> @@ -151,8 +151,7 @@ struct can_isotp_ll_options {
>  #define CAN_ISOTP_DEFAULT_LL_TX_DL	CAN_MAX_DLEN
>  #define CAN_ISOTP_DEFAULT_LL_TX_FLAGS	0
>  
> -/*
> - * Remark on CAN_ISOTP_DEFAULT_RECV_* values:
> +/* Remark on CAN_ISOTP_DEFAULT_RECV_* values:

I think for uapi headers we use the default commenting style, not the netdev one.

>   *
>   * We can strongly assume, that the Linux Kernel implementation of
>   * CAN_ISOTP is capable to run with BS=0, STmin=0 and WFTmax=0.
> @@ -160,7 +159,6 @@ struct can_isotp_ll_options {
>   * these default settings can be changed via sockopts.
>   * For that reason the STmin value is intentionally _not_ checked for
>   * consistency and copied directly into the flow control (FC) frame.
> - *
>   */
>  
>  #endif /* !_UAPI_CAN_ISOTP_H */
> diff --git a/net/can/Kconfig b/net/can/Kconfig
> index 021fe03a8ed6..224e5e0283a9 100644
> --- a/net/can/Kconfig
> +++ b/net/can/Kconfig
> @@ -57,7 +57,6 @@ source "net/can/j1939/Kconfig"
>  
>  config CAN_ISOTP
>  	tristate "ISO 15765-2:2016 CAN transport protocol"
> -	default y
>  	help
>  	  CAN Transport Protocols offer support for segmented Point-to-Point
>  	  communication between CAN nodes via two defined CAN Identifiers.
> @@ -67,6 +66,8 @@ config CAN_ISOTP
>  	  vehicle diagnosis (UDS, ISO 14229) or IP-over-CAN traffic.
>  	  This protocol driver implements data transfers according to
>  	  ISO 15765-2:2016 for 'classic' CAN and CAN FD frame types.
> +	  If you want to perform automotive vehicle diagnostic services (UDS),
> +	  say 'y'.
>  
>  source "drivers/net/can/Kconfig"
>  
> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index e6ff032b5426..bc3a722c200b 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -222,8 +222,8 @@ static int isotp_send_fc(struct sock *sk, int ae, u8 flowstatus)
>  
>  	can_send_ret = can_send(nskb, 1);
>  	if (can_send_ret)
> -		printk_once(KERN_NOTICE "can-isotp: %s: can_send_ret %d\n",
> -			    __func__, can_send_ret);
> +		pr_notice_once("can-isotp: %s: can_send_ret %d\n",
> +			       __func__, can_send_ret);

please define a pr_fmt for the "can-isotp: " prefix.

>  
>  	dev_put(dev);
>  
> @@ -769,7 +769,7 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
>  
>  isotp_tx_burst:
>  		skb = alloc_skb(so->ll.mtu + sizeof(struct can_skb_priv),
> -				gfp_any());
> +				GFP_KERNEL);
>  		if (!skb) {
>  			dev_put(dev);
>  			break;
> @@ -798,8 +798,8 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
>  
>  		can_send_ret = can_send(skb, 1);
>  		if (can_send_ret)
> -			printk_once(KERN_NOTICE "can-isotp: %s: can_send_ret %d\n",
> -				    __func__, can_send_ret);
> +			pr_notice_once("can-isotp: %s: can_send_ret %d\n",
> +				       __func__, can_send_ret);
>  
>  		if (so->tx.idx >= so->tx.len) {
>  			/* we are done */
> @@ -942,8 +942,8 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>  	err = can_send(skb, 1);
>  	dev_put(dev);
>  	if (err) {
> -		printk_once(KERN_NOTICE "can-isotp: %s: can_send_ret %d\n",
> -			    __func__, err);
> +		pr_notice_once("can-isotp: %s: can_send_ret %d\n",
> +			       __func__, err);
>  		return err;
>  	}
>  
> 

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]