* [PATCH can-next] can: dev: always create TX echo skb @ 2021-03-09 21:19 Marc Kleine-Budde 2021-03-18 10:03 ` Vincent MAILHOL 0 siblings, 1 reply; 4+ messages in thread From: Marc Kleine-Budde @ 2021-03-09 21:19 UTC (permalink / raw) To: linux-can; +Cc: kernel, Oliver Hartkopp, Marc Kleine-Budde So far the creation of the TX echo skb was optional and can be controlled by the local sender of a CAN frame. It turns out that the TX echo CAN skb can be piggybacked to carry information in the driver from the TX- to the TX-complete handler. Several drivers already use the return value of can_get_echo_skb() (which is the length of the data field in the CAN frame) for their number of transferred bytes statistics. The statistics are not working if CAN echo skbs are disabled. Another use case is to calculate and set the CAN frame length on the wire, which is needed for BQL support in both the TX and TX-completion handler. For now in can_put_echo_skb(), which is called from the TX handler, the skb carrying the CAN frame is discarded if no TX echo is requested, leading to the above illustrated problems. This patch changes the can_put_echo_skb() function, so that the echo skb is always generated. If the sender requests no echo, the echo skb is consumed in __can_get_echo_skb() without being passed into the RX handler of the networking stack, but the CAN data length and CAN frame length information is properly returned. Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> --- drivers/net/can/dev/skb.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c index 6a64fe410987..22b0472a5fad 100644 --- a/drivers/net/can/dev/skb.c +++ b/drivers/net/can/dev/skb.c @@ -45,7 +45,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, BUG_ON(idx >= priv->echo_skb_max); /* check flag whether this packet has to be looped back */ - if (!(dev->flags & IFF_ECHO) || skb->pkt_type != PACKET_LOOPBACK || + if (!(dev->flags & IFF_ECHO) || (skb->protocol != htons(ETH_P_CAN) && skb->protocol != htons(ETH_P_CANFD))) { kfree_skb(skb); @@ -58,7 +58,6 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, return -ENOMEM; /* make settings for echo to reduce code in irq context */ - skb->pkt_type = PACKET_BROADCAST; skb->ip_summed = CHECKSUM_UNNECESSARY; skb->dev = dev; @@ -111,6 +110,13 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr, priv->echo_skb[idx] = NULL; + if (skb->pkt_type == PACKET_LOOPBACK) { + skb->pkt_type = PACKET_BROADCAST; + } else { + dev_consume_skb_any(skb); + return NULL; + } + return skb; } -- 2.30.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH can-next] can: dev: always create TX echo skb 2021-03-09 21:19 [PATCH can-next] can: dev: always create TX echo skb Marc Kleine-Budde @ 2021-03-18 10:03 ` Vincent MAILHOL 2021-03-18 10:21 ` Oliver Hartkopp 0 siblings, 1 reply; 4+ messages in thread From: Vincent MAILHOL @ 2021-03-18 10:03 UTC (permalink / raw) To: Marc Kleine-Budde; +Cc: linux-can, kernel, Oliver Hartkopp Hi Marc, On Wed. 10 Mar 2021 at 06:19, Marc Kleine-Budde <mkl@pengutronix.de> wrote: > > So far the creation of the TX echo skb was optional and can be > controlled by the local sender of a CAN frame. > > It turns out that the TX echo CAN skb can be piggybacked to carry > information in the driver from the TX- to the TX-complete handler. > > Several drivers already use the return value of > can_get_echo_skb() (which is the length of the data field in the CAN > frame) for their number of transferred bytes statistics. The > statistics are not working if CAN echo skbs are disabled. > > Another use case is to calculate and set the CAN frame length on the > wire, which is needed for BQL support in both the TX and TX-completion > handler. > > For now in can_put_echo_skb(), which is called from the TX handler, > the skb carrying the CAN frame is discarded if no TX echo is > requested, leading to the above illustrated problems. > > This patch changes the can_put_echo_skb() function, so that the echo > skb is always generated. If the sender requests no echo, the echo skb > is consumed in __can_get_echo_skb() without being passed into the RX > handler of the networking stack, but the CAN data length and CAN frame > length information is properly returned. > > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> > --- > drivers/net/can/dev/skb.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c > index 6a64fe410987..22b0472a5fad 100644 > --- a/drivers/net/can/dev/skb.c > +++ b/drivers/net/can/dev/skb.c > @@ -45,7 +45,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, > BUG_ON(idx >= priv->echo_skb_max); > > /* check flag whether this packet has to be looped back */ > - if (!(dev->flags & IFF_ECHO) || skb->pkt_type != PACKET_LOOPBACK || > + if (!(dev->flags & IFF_ECHO) || > (skb->protocol != htons(ETH_P_CAN) && > skb->protocol != htons(ETH_P_CANFD))) { > kfree_skb(skb); > @@ -58,7 +58,6 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, > return -ENOMEM; > > /* make settings for echo to reduce code in irq context */ > - skb->pkt_type = PACKET_BROADCAST; > skb->ip_summed = CHECKSUM_UNNECESSARY; > skb->dev = dev; > > @@ -111,6 +110,13 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr, > > priv->echo_skb[idx] = NULL; > > + if (skb->pkt_type == PACKET_LOOPBACK) { > + skb->pkt_type = PACKET_BROADCAST; > + } else { > + dev_consume_skb_any(skb); > + return NULL; > + } > + > return skb; > } I do see any particular issues on this patch at the moment, however, while looking at the TX echo functionality, it reminded me of a point which has always been a bit unclear to me: the CAN_CTRLMODE_LOOPBACK. So let me go a bit off topic. Like all other controller's mode, I would expect the CAN_CTRLMODE_LOOPBACK flag to do two things: - Announce that the device is capable of doing loopback - Control this feature (enable/disable it) But, by default, this flag is set to 0 unless the user explicitly passes the "loopback on" argument when configuring the device. So isn't this supposed to be an issue for all the drivers which expect to get a TX loopback in order to trigger the TX completion handler? Personally, for my driver, I would like to use the can_set_static_ctrlmode() to force CAN_CTRLMODE_LOOPBACK so that I do not need a different TX completion logic when CAN_CTRLMODE_LOOPBACK is off. The issue is that because CAN_CTRLMODE_LOOPBACK is per default off, doing: can_set_static_ctrlmode(netdev, CAN_CTRLMODE_LOOPBACK); would lead to a RTNETLINK answers: Operation not supported when configuring the device unless "loopback on" is explicitly passed on the command line. At the moment, I have the feeling that many drivers just ignore the value of this flag and activate the loopback regardless. Do you think that it would make sense to set CAN_CTRLMODE_LOOPBACK by default to on? Yours sincerely, Vincent ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH can-next] can: dev: always create TX echo skb 2021-03-18 10:03 ` Vincent MAILHOL @ 2021-03-18 10:21 ` Oliver Hartkopp 2021-03-18 10:55 ` Vincent MAILHOL 0 siblings, 1 reply; 4+ messages in thread From: Oliver Hartkopp @ 2021-03-18 10:21 UTC (permalink / raw) To: Vincent MAILHOL, Marc Kleine-Budde; +Cc: linux-can, kernel Hi Vincent, On 18.03.21 11:03, Vincent MAILHOL wrote: > Hi Marc, > > On Wed. 10 Mar 2021 at 06:19, Marc Kleine-Budde <mkl@pengutronix.de> wrote: >> >> So far the creation of the TX echo skb was optional and can be >> controlled by the local sender of a CAN frame. >> >> It turns out that the TX echo CAN skb can be piggybacked to carry >> information in the driver from the TX- to the TX-complete handler. >> >> Several drivers already use the return value of >> can_get_echo_skb() (which is the length of the data field in the CAN >> frame) for their number of transferred bytes statistics. The >> statistics are not working if CAN echo skbs are disabled. >> >> Another use case is to calculate and set the CAN frame length on the >> wire, which is needed for BQL support in both the TX and TX-completion >> handler. >> >> For now in can_put_echo_skb(), which is called from the TX handler, >> the skb carrying the CAN frame is discarded if no TX echo is >> requested, leading to the above illustrated problems. >> >> This patch changes the can_put_echo_skb() function, so that the echo >> skb is always generated. If the sender requests no echo, the echo skb >> is consumed in __can_get_echo_skb() without being passed into the RX >> handler of the networking stack, but the CAN data length and CAN frame >> length information is properly returned. >> >> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> >> --- >> drivers/net/can/dev/skb.c | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c >> index 6a64fe410987..22b0472a5fad 100644 >> --- a/drivers/net/can/dev/skb.c >> +++ b/drivers/net/can/dev/skb.c >> @@ -45,7 +45,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, >> BUG_ON(idx >= priv->echo_skb_max); >> >> /* check flag whether this packet has to be looped back */ >> - if (!(dev->flags & IFF_ECHO) || skb->pkt_type != PACKET_LOOPBACK || >> + if (!(dev->flags & IFF_ECHO) || >> (skb->protocol != htons(ETH_P_CAN) && >> skb->protocol != htons(ETH_P_CANFD))) { >> kfree_skb(skb); >> @@ -58,7 +58,6 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, >> return -ENOMEM; >> >> /* make settings for echo to reduce code in irq context */ >> - skb->pkt_type = PACKET_BROADCAST; >> skb->ip_summed = CHECKSUM_UNNECESSARY; >> skb->dev = dev; >> >> @@ -111,6 +110,13 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr, >> >> priv->echo_skb[idx] = NULL; >> >> + if (skb->pkt_type == PACKET_LOOPBACK) { >> + skb->pkt_type = PACKET_BROADCAST; >> + } else { >> + dev_consume_skb_any(skb); >> + return NULL; >> + } >> + >> return skb; >> } > > I do see any particular issues on this patch at the moment, > however, while looking at the TX echo functionality, it reminded > me of a point which has always been a bit unclear to me: the > CAN_CTRLMODE_LOOPBACK. So let me go a bit off topic. > > Like all other controller's mode, I would expect the > CAN_CTRLMODE_LOOPBACK flag to do two things: > - Announce that the device is capable of doing loopback > - Control this feature (enable/disable it) > > But, by default, this flag is set to 0 unless the user > explicitly passes the "loopback on" argument when configuring > the device. > > So isn't this supposed to be an issue for all the drivers which > expect to get a TX loopback in order to trigger the TX completion > handler? > > Personally, for my driver, I would like to use the > can_set_static_ctrlmode() to force CAN_CTRLMODE_LOOPBACK so that > I do not need a different TX completion logic when > CAN_CTRLMODE_LOOPBACK is off. > > The issue is that because CAN_CTRLMODE_LOOPBACK is per default > off, doing: > can_set_static_ctrlmode(netdev, CAN_CTRLMODE_LOOPBACK); > would lead to a > RTNETLINK answers: Operation not supported > when configuring the device unless "loopback on" is explicitly > passed on the command line. > > At the moment, I have the feeling that many drivers just ignore > the value of this flag and activate the loopback regardless. > > Do you think that it would make sense to set > CAN_CTRLMODE_LOOPBACK by default to on? Definitely not. CAN_CTRLMODE_LOOPBACK defines that the CAN controller establishes a shortcut below the rx/tx bitstream engines on chip level. So the attached CAN transceiver is not in operation anymore. Please do not mix up CAN_CTRLMODE_LOOPBACK with the IFF_ECHO flag that echoes CAN frames in the sending host to reflect the correct CAN traffic e.g. in candump. Best, Oliver ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH can-next] can: dev: always create TX echo skb 2021-03-18 10:21 ` Oliver Hartkopp @ 2021-03-18 10:55 ` Vincent MAILHOL 0 siblings, 0 replies; 4+ messages in thread From: Vincent MAILHOL @ 2021-03-18 10:55 UTC (permalink / raw) To: Oliver Hartkopp; +Cc: Marc Kleine-Budde, linux-can, kernel On Thur. 18 Mar 2021 at 19:21, Oliver Hartkopp <socketcan@hartkopp.net> wrote: > > Hi Vincent, > > On 18.03.21 11:03, Vincent MAILHOL wrote: > > Hi Marc, > > > > On Wed. 10 Mar 2021 at 06:19, Marc Kleine-Budde <mkl@pengutronix.de> wrote: > >> > >> So far the creation of the TX echo skb was optional and can be > >> controlled by the local sender of a CAN frame. > >> > >> It turns out that the TX echo CAN skb can be piggybacked to carry > >> information in the driver from the TX- to the TX-complete handler. > >> > >> Several drivers already use the return value of > >> can_get_echo_skb() (which is the length of the data field in the CAN > >> frame) for their number of transferred bytes statistics. The > >> statistics are not working if CAN echo skbs are disabled. > >> > >> Another use case is to calculate and set the CAN frame length on the > >> wire, which is needed for BQL support in both the TX and TX-completion > >> handler. > >> > >> For now in can_put_echo_skb(), which is called from the TX handler, > >> the skb carrying the CAN frame is discarded if no TX echo is > >> requested, leading to the above illustrated problems. > >> > >> This patch changes the can_put_echo_skb() function, so that the echo > >> skb is always generated. If the sender requests no echo, the echo skb > >> is consumed in __can_get_echo_skb() without being passed into the RX > >> handler of the networking stack, but the CAN data length and CAN frame > >> length information is properly returned. > >> > >> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> > >> --- > >> drivers/net/can/dev/skb.c | 10 ++++++++-- > >> 1 file changed, 8 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c > >> index 6a64fe410987..22b0472a5fad 100644 > >> --- a/drivers/net/can/dev/skb.c > >> +++ b/drivers/net/can/dev/skb.c > >> @@ -45,7 +45,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, > >> BUG_ON(idx >= priv->echo_skb_max); > >> > >> /* check flag whether this packet has to be looped back */ > >> - if (!(dev->flags & IFF_ECHO) || skb->pkt_type != PACKET_LOOPBACK || > >> + if (!(dev->flags & IFF_ECHO) || > >> (skb->protocol != htons(ETH_P_CAN) && > >> skb->protocol != htons(ETH_P_CANFD))) { > >> kfree_skb(skb); > >> @@ -58,7 +58,6 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, > >> return -ENOMEM; > >> > >> /* make settings for echo to reduce code in irq context */ > >> - skb->pkt_type = PACKET_BROADCAST; > >> skb->ip_summed = CHECKSUM_UNNECESSARY; > >> skb->dev = dev; > >> > >> @@ -111,6 +110,13 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr, > >> > >> priv->echo_skb[idx] = NULL; > >> > >> + if (skb->pkt_type == PACKET_LOOPBACK) { > >> + skb->pkt_type = PACKET_BROADCAST; > >> + } else { > >> + dev_consume_skb_any(skb); > >> + return NULL; > >> + } > >> + > >> return skb; > >> } > > > > I do see any particular issues on this patch at the moment, > > however, while looking at the TX echo functionality, it reminded > > me of a point which has always been a bit unclear to me: the > > CAN_CTRLMODE_LOOPBACK. So let me go a bit off topic. > > > > Like all other controller's mode, I would expect the > > CAN_CTRLMODE_LOOPBACK flag to do two things: > > - Announce that the device is capable of doing loopback > > - Control this feature (enable/disable it) > > > > But, by default, this flag is set to 0 unless the user > > explicitly passes the "loopback on" argument when configuring > > the device. > > > > So isn't this supposed to be an issue for all the drivers which > > expect to get a TX loopback in order to trigger the TX completion > > handler? > > > > Personally, for my driver, I would like to use the > > can_set_static_ctrlmode() to force CAN_CTRLMODE_LOOPBACK so that > > I do not need a different TX completion logic when > > CAN_CTRLMODE_LOOPBACK is off. > > > > The issue is that because CAN_CTRLMODE_LOOPBACK is per default > > off, doing: > > can_set_static_ctrlmode(netdev, CAN_CTRLMODE_LOOPBACK); > > would lead to a > > RTNETLINK answers: Operation not supported > > when configuring the device unless "loopback on" is explicitly > > passed on the command line. > > > > At the moment, I have the feeling that many drivers just ignore > > the value of this flag and activate the loopback regardless. > > > > Do you think that it would make sense to set > > CAN_CTRLMODE_LOOPBACK by default to on? > > Definitely not. > > CAN_CTRLMODE_LOOPBACK defines that the CAN controller establishes a > shortcut below the rx/tx bitstream engines on chip level. So the > attached CAN transceiver is not in operation anymore. > > Please do not mix up CAN_CTRLMODE_LOOPBACK with the IFF_ECHO flag that > echoes CAN frames in the sending host to reflect the correct CAN traffic > e.g. in candump. OK. I indeed totally misunderstood the meaning of that CAN_CTRLMODE_LOOPBACK. Thanks for the clarification. Now, I understand the meaning. Yours sincerely, Vincent ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-03-18 10:56 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-09 21:19 [PATCH can-next] can: dev: always create TX echo skb Marc Kleine-Budde 2021-03-18 10:03 ` Vincent MAILHOL 2021-03-18 10:21 ` Oliver Hartkopp 2021-03-18 10:55 ` Vincent MAILHOL
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).