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

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

[Oliver Hartkopp]

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:
  *
  * 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);
 
 	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;
 	}
 
-- 
2.28.0

[PATCH net-next 2/2] can: remove obsolete version strings

[Oliver Hartkopp]

As pointed out by Jakub Kicinski here:
https://marc.info/?l=linux-can&m=160229286216008
this patch removes the obsolete version information of the different
CAN protocols and the AF_CAN core module.

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 include/linux/can/core.h |  7 -------
 include/net/netns/can.h  |  1 -
 net/can/af_can.c         |  2 +-
 net/can/bcm.c            |  4 +---
 net/can/gw.c             |  4 +---
 net/can/isotp.c          |  4 +---
 net/can/proc.c           | 12 ------------
 net/can/raw.c            |  4 +---
 8 files changed, 5 insertions(+), 33 deletions(-)

diff --git a/include/linux/can/core.h b/include/linux/can/core.h
index 7da9f1f82e8e..5fb8d0e3f9c1 100644
--- a/include/linux/can/core.h
+++ b/include/linux/can/core.h
@@ -18,13 +18,6 @@
 #include <linux/skbuff.h>
 #include <linux/netdevice.h>
 
-#define CAN_VERSION "20170425"
-
-/* increment this number each time you change some user-space interface */
-#define CAN_ABI_VERSION "9"
-
-#define CAN_VERSION_STRING "rev " CAN_VERSION " abi " CAN_ABI_VERSION
-
 #define DNAME(dev) ((dev) ? (dev)->name : "any")
 
 /**
diff --git a/include/net/netns/can.h b/include/net/netns/can.h
index b6ab7d1530d7..52fbd8291a96 100644
--- a/include/net/netns/can.h
+++ b/include/net/netns/can.h
@@ -15,7 +15,6 @@ struct can_rcv_lists_stats;
 struct netns_can {
 #if IS_ENABLED(CONFIG_PROC_FS)
 	struct proc_dir_entry *proc_dir;
-	struct proc_dir_entry *pde_version;
 	struct proc_dir_entry *pde_stats;
 	struct proc_dir_entry *pde_reset_stats;
 	struct proc_dir_entry *pde_rcvlist_all;
diff --git a/net/can/af_can.c b/net/can/af_can.c
index b7d0f6500893..6373ab9c5507 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -875,7 +875,7 @@ static __init int can_init(void)
 		     offsetof(struct can_frame, data) !=
 		     offsetof(struct canfd_frame, data));
 
-	pr_info("can: controller area network core (" CAN_VERSION_STRING ")\n");
+	pr_info("can: controller area network core\n");
 
 	rcv_cache = kmem_cache_create("can_receiver", sizeof(struct receiver),
 				      0, 0, NULL);
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 4253915800e6..0e5c37be4a2b 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -81,8 +81,6 @@
 		     (CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG) : \
 		     (CAN_SFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG))
 
-#define CAN_BCM_VERSION "20170425"
-
 MODULE_DESCRIPTION("PF_CAN broadcast manager protocol");
 MODULE_LICENSE("Dual BSD/GPL");
 MODULE_AUTHOR("Oliver Hartkopp <oliver.hartkopp@volkswagen.de>");
@@ -1696,7 +1694,7 @@ static int __init bcm_module_init(void)
 {
 	int err;
 
-	pr_info("can: broadcast manager protocol (rev " CAN_BCM_VERSION " t)\n");
+	pr_info("can: broadcast manager protocol\n");
 
 	err = can_proto_register(&bcm_can_proto);
 	if (err < 0) {
diff --git a/net/can/gw.c b/net/can/gw.c
index 49b4e3d91ad6..6b790b6ff8d2 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -59,7 +59,6 @@
 #include <net/net_namespace.h>
 #include <net/sock.h>
 
-#define CAN_GW_VERSION "20190810"
 #define CAN_GW_NAME "can-gw"
 
 MODULE_DESCRIPTION("PF_CAN netlink gateway");
@@ -1194,8 +1193,7 @@ static __init int cgw_module_init(void)
 	/* sanitize given module parameter */
 	max_hops = clamp_t(unsigned int, max_hops, CGW_MIN_HOPS, CGW_MAX_HOPS);
 
-	pr_info("can: netlink gateway (rev " CAN_GW_VERSION ") max_hops=%d\n",
-		max_hops);
+	pr_info("can: netlink gateway - max_hops=%d\n",	max_hops);
 
 	ret = register_pernet_subsys(&cangw_pernet_ops);
 	if (ret)
diff --git a/net/can/isotp.c b/net/can/isotp.c
index bc3a722c200b..5338e990bde1 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -72,8 +72,6 @@
 #include <net/sock.h>
 #include <net/net_namespace.h>
 
-#define CAN_ISOTP_VERSION "20200928"
-
 MODULE_DESCRIPTION("PF_CAN isotp 15765-2:2016 protocol");
 MODULE_LICENSE("Dual BSD/GPL");
 MODULE_AUTHOR("Oliver Hartkopp <socketcan@hartkopp.net>");
@@ -1408,7 +1406,7 @@ static __init int isotp_module_init(void)
 {
 	int err;
 
-	pr_info("can: isotp protocol (rev " CAN_ISOTP_VERSION ")\n");
+	pr_info("can: isotp protocol\n");
 
 	err = can_proto_register(&isotp_can_proto);
 	if (err < 0)
diff --git a/net/can/proc.c b/net/can/proc.c
index a4eb06c9eb70..550928b8b8a2 100644
--- a/net/can/proc.c
+++ b/net/can/proc.c
@@ -54,7 +54,6 @@
  * proc filenames for the PF_CAN core
  */
 
-#define CAN_PROC_VERSION     "version"
 #define CAN_PROC_STATS       "stats"
 #define CAN_PROC_RESET_STATS "reset_stats"
 #define CAN_PROC_RCVLIST_ALL "rcvlist_all"
@@ -293,12 +292,6 @@ static int can_reset_stats_proc_show(struct seq_file *m, void *v)
 	return 0;
 }
 
-static int can_version_proc_show(struct seq_file *m, void *v)
-{
-	seq_printf(m, "%s\n", CAN_VERSION_STRING);
-	return 0;
-}
-
 static inline void can_rcvlist_proc_show_one(struct seq_file *m, int idx,
 					     struct net_device *dev,
 					     struct can_dev_rcv_lists *dev_rcv_lists)
@@ -441,8 +434,6 @@ void can_init_proc(struct net *net)
 	}
 
 	/* own procfs entries from the AF_CAN core */
-	net->can.pde_version = proc_create_net_single(CAN_PROC_VERSION, 0644,
-			net->can.proc_dir, can_version_proc_show, NULL);
 	net->can.pde_stats = proc_create_net_single(CAN_PROC_STATS, 0644,
 			net->can.proc_dir, can_stats_proc_show, NULL);
 	net->can.pde_reset_stats = proc_create_net_single(CAN_PROC_RESET_STATS,
@@ -471,9 +462,6 @@ void can_init_proc(struct net *net)
  */
 void can_remove_proc(struct net *net)
 {
-	if (net->can.pde_version)
-		remove_proc_entry(CAN_PROC_VERSION, net->can.proc_dir);
-
 	if (net->can.pde_stats)
 		remove_proc_entry(CAN_PROC_STATS, net->can.proc_dir);
 
diff --git a/net/can/raw.c b/net/can/raw.c
index ea70850f9152..6ec8aa1d0da4 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -55,8 +55,6 @@
 #include <net/sock.h>
 #include <net/net_namespace.h>
 
-#define CAN_RAW_VERSION CAN_VERSION
-
 MODULE_DESCRIPTION("PF_CAN raw protocol");
 MODULE_LICENSE("Dual BSD/GPL");
 MODULE_AUTHOR("Urs Thuermann <urs.thuermann@volkswagen.de>");
@@ -885,7 +883,7 @@ static __init int raw_module_init(void)
 {
 	int err;
 
-	pr_info("can: raw protocol (rev " CAN_RAW_VERSION ")\n");
+	pr_info("can: raw protocol\n");
 
 	err = can_proto_register(&raw_can_proto);
 	if (err < 0)
-- 
2.28.0

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

[Marc Kleine-Budde]

[-- 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 --]

Re: [PATCH net-next 2/2] can: remove obsolete version strings

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 801 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

Better use https://lore.kernel.org for this. The URL is constructed by adding
"/r/$MESSAGE_ID":


http://lore.kernel.org/r/20201009175751.5c54097f@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com

> this patch removes the obsolete version information of the different
> CAN protocols and the AF_CAN core module.
> 
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
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 --]