| Message ID | 20201010204909.2059-1-socketcan@hartkopp.net |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next,1/2] can-isotp: implement cleanups / improvements from review | expand |
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
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; }
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(-)