* [PATCH 0/3] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS @ 2012-07-09 19:56 Ira W. Snyder 2012-07-09 19:56 ` [PATCH 1/3] can: make the echo stack keep packet information longer Ira W. Snyder ` (4 more replies) 0 siblings, 5 replies; 16+ messages in thread From: Ira W. Snyder @ 2012-07-09 19:56 UTC (permalink / raw) To: linux-can; +Cc: Ira W. Snyder From: "Ira W. Snyder" <iws@ovro.caltech.edu> This is another different approach to fixing the Janz ICAN3 support for CAN_RAW_RECV_OWN_MSGS. The can_put_echo_skb() function is changed to always keep all packets until can_get_echo_skb() is called. Previously, it would drop packets if they were not needed to be looped back. This makes it possible for the new function can_cmp_echo_skb() to work with hardware-assisted loopback support on the Janz ICAN3, which does not have TX-complete interrupts of any kind. Since we are now storing packets even in the non-loopback case, there is some extra memory overhead, to store the extra packets between the calls to can_put_echo_skb() and can_get_echo_skb(). After this patch series is applied, the SocketCAN tst-rcv-own-msgs test passes. Performance is rougly 15% less than using the previously posted patch: [PATCH ALTERNATE VERSION 1/1] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS This performance drop is due to the extra overhead of receiving the echo packets from the card itself. This involves one extra interrupt for each packet sent, and the associated overhead of running ican3_napi() for each packet sent. Ira W. Snyder (3): can: make the echo stack keep packet information longer can: add can_cmp_echo_skb() for echo skb comparison can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS drivers/net/can/dev.c | 75 ++++++++++++++++++++++++++++++----------- drivers/net/can/janz-ican3.c | 56 +++++++++++++++++-------------- include/linux/can/dev.h | 2 + 3 files changed, 87 insertions(+), 46 deletions(-) -- 1.7.8.6 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/3] can: make the echo stack keep packet information longer 2012-07-09 19:56 [PATCH 0/3] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS Ira W. Snyder @ 2012-07-09 19:56 ` Ira W. Snyder 2012-07-09 21:09 ` Oliver Hartkopp 2012-07-09 19:56 ` [PATCH 2/3] can: add can_cmp_echo_skb() for echo skb comparison Ira W. Snyder ` (3 subsequent siblings) 4 siblings, 1 reply; 16+ messages in thread From: Ira W. Snyder @ 2012-07-09 19:56 UTC (permalink / raw) To: linux-can; +Cc: Ira W. Snyder From: "Ira W. Snyder" <iws@ovro.caltech.edu> In order for the can_cmp_echo_skb() function to be able to detect packets received via the hardware loopback feature, all packets must be saved. This means we cannot drop non-loopback packets until reception time. --- drivers/net/can/dev.c | 41 +++++++++++++++++++++-------------------- 1 files changed, 21 insertions(+), 20 deletions(-) diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c index c5fe3a3..98e6c50 100644 --- a/drivers/net/can/dev.c +++ b/drivers/net/can/dev.c @@ -282,12 +282,6 @@ void 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) { - kfree_skb(skb); - return; - } - if (!priv->echo_skb[idx]) { struct sock *srcsk = skb->sk; @@ -303,12 +297,6 @@ void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, skb->sk = srcsk; - /* make settings for echo to reduce code in irq context */ - skb->protocol = htons(ETH_P_CAN); - skb->pkt_type = PACKET_BROADCAST; - skb->ip_summed = CHECKSUM_UNNECESSARY; - skb->dev = dev; - /* save this skb for tx interrupt echo handling */ priv->echo_skb[idx] = skb; } else { @@ -329,21 +317,34 @@ EXPORT_SYMBOL_GPL(can_put_echo_skb); unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx) { struct can_priv *priv = netdev_priv(dev); + struct sk_buff *skb = priv->echo_skb[idx]; + struct can_frame *cf; + u8 dlc; BUG_ON(idx >= priv->echo_skb_max); - if (priv->echo_skb[idx]) { - struct sk_buff *skb = priv->echo_skb[idx]; - struct can_frame *cf = (struct can_frame *)skb->data; - u8 dlc = cf->can_dlc; + if (!skb) + return 0; - netif_rx(priv->echo_skb[idx]); - priv->echo_skb[idx] = NULL; + priv->echo_skb[idx] = NULL; + cf = (struct can_frame *)skb->data; - return dlc; + /* check flag whether this packet has to be looped back */ + if (!(dev->flags & IFF_ECHO) || skb->pkt_type != PACKET_LOOPBACK) { + kfree_skb(skb); + return 0; } - return 0; + /* make settings for echo to reduce code in irq context */ + skb->protocol = htons(ETH_P_CAN); + skb->pkt_type = PACKET_BROADCAST; + skb->ip_summed = CHECKSUM_UNNECESSARY; + skb->dev = dev; + + dlc = cf->can_dlc; + netif_rx(skb); + + return dlc; } EXPORT_SYMBOL_GPL(can_get_echo_skb); -- 1.7.8.6 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] can: make the echo stack keep packet information longer 2012-07-09 19:56 ` [PATCH 1/3] can: make the echo stack keep packet information longer Ira W. Snyder @ 2012-07-09 21:09 ` Oliver Hartkopp 0 siblings, 0 replies; 16+ messages in thread From: Oliver Hartkopp @ 2012-07-09 21:09 UTC (permalink / raw) To: Ira W. Snyder; +Cc: linux-can On 09.07.2012 21:56, Ira W. Snyder wrote: > From: "Ira W. Snyder" <iws@ovro.caltech.edu> > > In order for the can_cmp_echo_skb() function to be able to detect > packets received via the hardware loopback feature, all packets must be > saved. This means we cannot drop non-loopback packets until reception > time. > --- > drivers/net/can/dev.c | 41 +++++++++++++++++++++-------------------- > 1 files changed, 21 insertions(+), 20 deletions(-) > > diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c > index c5fe3a3..98e6c50 100644 > --- a/drivers/net/can/dev.c > +++ b/drivers/net/can/dev.c > @@ -282,12 +282,6 @@ void 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) { > - kfree_skb(skb); > - return; > - } > - > if (!priv->echo_skb[idx]) { > struct sock *srcsk = skb->sk; > > @@ -303,12 +297,6 @@ void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, > > skb->sk = srcsk; > > - /* make settings for echo to reduce code in irq context */ > - skb->protocol = htons(ETH_P_CAN); > - skb->pkt_type = PACKET_BROADCAST; > - skb->ip_summed = CHECKSUM_UNNECESSARY; > - skb->dev = dev; > - > /* save this skb for tx interrupt echo handling */ > priv->echo_skb[idx] = skb; > } else { > @@ -329,21 +317,34 @@ EXPORT_SYMBOL_GPL(can_put_echo_skb); > unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx) > { > struct can_priv *priv = netdev_priv(dev); > + struct sk_buff *skb = priv->echo_skb[idx]; > + struct can_frame *cf; > + u8 dlc; > > BUG_ON(idx >= priv->echo_skb_max); > > - if (priv->echo_skb[idx]) { > - struct sk_buff *skb = priv->echo_skb[idx]; > - struct can_frame *cf = (struct can_frame *)skb->data; > - u8 dlc = cf->can_dlc; > + if (!skb) > + return 0; > > - netif_rx(priv->echo_skb[idx]); > - priv->echo_skb[idx] = NULL; > + priv->echo_skb[idx] = NULL; > + cf = (struct can_frame *)skb->data; > > - return dlc; > + /* check flag whether this packet has to be looped back */ > + if (!(dev->flags & IFF_ECHO) || skb->pkt_type != PACKET_LOOPBACK) { > + kfree_skb(skb); > + return 0; > } > > - return 0; > + /* make settings for echo to reduce code in irq context */ This comment became pointless :-) > + skb->protocol = htons(ETH_P_CAN); > + skb->pkt_type = PACKET_BROADCAST; > + skb->ip_summed = CHECKSUM_UNNECESSARY; > + skb->dev = dev; Hm - it's proably worth to check, if we really need to set all these values or if skb->pkt_type = PACKET_BROADCAST; would just be enough. Will do so - but this is not relevant for your current patch. > + > + dlc = cf->can_dlc; > + netif_rx(skb); > + > + return dlc; > } > EXPORT_SYMBOL_GPL(can_get_echo_skb); > Regards, Oliver ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/3] can: add can_cmp_echo_skb() for echo skb comparison 2012-07-09 19:56 [PATCH 0/3] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS Ira W. Snyder 2012-07-09 19:56 ` [PATCH 1/3] can: make the echo stack keep packet information longer Ira W. Snyder @ 2012-07-09 19:56 ` Ira W. Snyder 2012-07-09 19:56 ` [PATCH 3/3] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS Ira W. Snyder ` (2 subsequent siblings) 4 siblings, 0 replies; 16+ messages in thread From: Ira W. Snyder @ 2012-07-09 19:56 UTC (permalink / raw) To: linux-can; +Cc: Ira W. Snyder From: "Ira W. Snyder" <iws@ovro.caltech.edu> This function allows drivers to compare an incoming skb against the echo skb stack. On CAN hardware which has support for hardware loopback, this allows drivers to support the CAN_RAW_RECV_OWN_MSGS flag correctly. Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu> --- drivers/net/can/dev.c | 34 ++++++++++++++++++++++++++++++++++ include/linux/can/dev.h | 2 ++ 2 files changed, 36 insertions(+), 0 deletions(-) diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c index 98e6c50..626b152 100644 --- a/drivers/net/can/dev.c +++ b/drivers/net/can/dev.c @@ -349,6 +349,40 @@ unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx) EXPORT_SYMBOL_GPL(can_get_echo_skb); /* + * Compare an skb with an existing echo skb + * + * This function will be used on devices which have a hardware loopback. + * On these devices, this function can be used to compare a received skb + * with the saved echo skbs so that the hardware echo skb can be dropped. + * + * Returns true if the skb's are identical, false otherwise. + */ +bool can_cmp_echo_skb(struct sk_buff *skb, struct net_device *dev, + unsigned int idx) +{ + struct can_priv *priv = netdev_priv(dev); + struct can_frame *cf = (struct can_frame *)skb->data; + + BUG_ON(idx >= priv->echo_skb_max); + + if (priv->echo_skb[idx]) { + struct sk_buff *echo_skb = priv->echo_skb[idx]; + struct can_frame *echo_cf = (struct can_frame *)echo_skb->data; + + if (cf->can_id != echo_cf->can_id) + return false; + + if (cf->can_dlc != echo_cf->can_dlc) + return false; + + return memcmp(cf->data, echo_cf->data, cf->can_dlc) == 0; + } + + return false; +} +EXPORT_SYMBOL_GPL(can_cmp_echo_skb); + +/* * Remove the skb from the stack and free it. * * The function is typically called when TX failed. diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h index 5d2efe7..904a938 100644 --- a/include/linux/can/dev.h +++ b/include/linux/can/dev.h @@ -93,6 +93,8 @@ void can_bus_off(struct net_device *dev); void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, unsigned int idx); unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx); +bool can_cmp_echo_skb(struct sk_buff *skb, struct net_device *dev, + unsigned int idx); void can_free_echo_skb(struct net_device *dev, unsigned int idx); struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf); -- 1.7.8.6 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/3] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS 2012-07-09 19:56 [PATCH 0/3] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS Ira W. Snyder 2012-07-09 19:56 ` [PATCH 1/3] can: make the echo stack keep packet information longer Ira W. Snyder 2012-07-09 19:56 ` [PATCH 2/3] can: add can_cmp_echo_skb() for echo skb comparison Ira W. Snyder @ 2012-07-09 19:56 ` Ira W. Snyder 2012-07-09 20:57 ` Oliver Hartkopp 2012-07-09 21:05 ` [PATCH 0/3] " Oliver Hartkopp 2012-07-10 6:09 ` Wolfgang Grandegger 4 siblings, 1 reply; 16+ messages in thread From: Ira W. Snyder @ 2012-07-09 19:56 UTC (permalink / raw) To: linux-can; +Cc: Ira W. Snyder From: "Ira W. Snyder" <iws@ovro.caltech.edu> The Janz VMOD-ICAN3 firmware does not support any sort of TX-done notification or interrupt. The driver previously used the hardware loopback to attempt to work around this deficiency, but this caused all sockets to receive all messages, even if CAN_RAW_RECV_OWN_MSGS is off. Using the new function can_cmp_echo_skb(), we can drop the loopback messages and return the original skbs. This fixes the issues with CAN_RAW_RECV_OWN_MSGS. Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu> --- drivers/net/can/janz-ican3.c | 56 ++++++++++++++++++++++------------------- 1 files changed, 30 insertions(+), 26 deletions(-) diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c index 08c893c..a49af40 100644 --- a/drivers/net/can/janz-ican3.c +++ b/drivers/net/can/janz-ican3.c @@ -235,7 +235,6 @@ struct ican3_dev { /* fast host interface */ unsigned int fastrx_start; - unsigned int fastrx_int; unsigned int fastrx_num; unsigned int fasttx_start; unsigned int fasttx_num; @@ -454,7 +453,6 @@ static void __devinit ican3_init_fast_host_interface(struct ican3_dev *mod) /* save the start recv page */ mod->fastrx_start = mod->free_page; mod->fastrx_num = 0; - mod->fastrx_int = 0; /* build a single fast tohost queue descriptor */ memset(&desc, 0, sizeof(desc)); @@ -1150,6 +1148,21 @@ static int ican3_recv_skb(struct ican3_dev *mod) /* convert the ICAN3 frame into Linux CAN format */ ican3_to_can_frame(mod, &desc, cf); + /* + * If this is an ECHO frame received from the hardware loopback + * feature, use the skb saved in the ECHO stack instead. This allows + * the Linux CAN core to support CAN_RAW_RECV_OWN_MSGS correctly. + * + * Also, the netdevice queue needs to be allowed to send packets again. + */ + if (can_cmp_echo_skb(skb, ndev, 0)) { + stats->rx_packets++; + stats->rx_bytes += can_get_echo_skb(ndev, 0); + kfree_skb(skb); + netif_wake_queue(ndev); + goto err_noalloc; + } + /* receive the skb, update statistics */ netif_receive_skb(skb); stats->rx_packets++; @@ -1177,7 +1190,6 @@ static int ican3_napi(struct napi_struct *napi, int budget) { struct ican3_dev *mod = container_of(napi, struct ican3_dev, napi); struct ican3_msg msg; - unsigned long flags; int received = 0; int ret; @@ -1204,14 +1216,6 @@ static int ican3_napi(struct napi_struct *napi, int budget) if (received < budget) napi_complete(napi); - spin_lock_irqsave(&mod->lock, flags); - - /* Wake up the transmit queue if necessary */ - if (netif_queue_stopped(mod->ndev) && ican3_txok(mod)) - netif_wake_queue(mod->ndev); - - spin_unlock_irqrestore(&mod->lock, flags); - /* re-enable interrupt generation */ iowrite8(1 << mod->num, &mod->ctrl->int_enable); return received; @@ -1422,12 +1426,14 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev) void __iomem *desc_addr; unsigned long flags; + if (can_dropped_invalid_skb(ndev, skb)) + return NETDEV_TX_OK; + spin_lock_irqsave(&mod->lock, flags); /* check that we can actually transmit */ if (!ican3_txok(mod)) { - dev_err(mod->dev, "no free descriptors, stopping queue\n"); - netif_stop_queue(ndev); + dev_err(mod->dev, "BUG: no free descriptors\n"); spin_unlock_irqrestore(&mod->lock, flags); return NETDEV_TX_BUSY; } @@ -1442,6 +1448,16 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev) can_frame_to_ican3(mod, cf, &desc); /* + * This hardware doesn't have TX-done notifications, so we'll try and + * emulate it the best we can using ECHO skbs. Add the skb to the ECHO + * stack and stop transmitting packets. Upon packet reception, check + * if the ECHO skb and received skb match, and use that to wake the + * queue. + */ + netif_stop_queue(ndev); + can_put_echo_skb(skb, ndev, 0); + + /* * the programming manual says that you must set the IVALID bit, then * interrupt, then set the valid bit. Quite weird, but it seems to be * required for this to work @@ -1462,18 +1478,6 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev) /* update statistics */ stats->tx_packets++; stats->tx_bytes += cf->can_dlc; - kfree_skb(skb); - - /* - * This hardware doesn't have TX-done notifications, so we'll try and - * emulate it the best we can using ECHO skbs. Get the next TX - * descriptor, and see if we have room to send. If not, stop the queue. - * It will be woken when the ECHO skb for the current packet is recv'd. - */ - - /* copy the control bits of the descriptor */ - if (!ican3_txok(mod)) - netif_stop_queue(ndev); spin_unlock_irqrestore(&mod->lock, flags); return NETDEV_TX_OK; @@ -1654,7 +1658,7 @@ static int __devinit ican3_probe(struct platform_device *pdev) dev = &pdev->dev; /* allocate the CAN device and private data */ - ndev = alloc_candev(sizeof(*mod), 0); + ndev = alloc_candev(sizeof(*mod), 1); if (!ndev) { dev_err(dev, "unable to allocate CANdev\n"); ret = -ENOMEM; -- 1.7.8.6 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS 2012-07-09 19:56 ` [PATCH 3/3] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS Ira W. Snyder @ 2012-07-09 20:57 ` Oliver Hartkopp 2012-07-09 21:16 ` Ira W. Snyder 0 siblings, 1 reply; 16+ messages in thread From: Oliver Hartkopp @ 2012-07-09 20:57 UTC (permalink / raw) To: Ira W. Snyder; +Cc: linux-can On 09.07.2012 21:56, Ira W. Snyder wrote: > From: "Ira W. Snyder" <iws@ovro.caltech.edu> > > The Janz VMOD-ICAN3 firmware does not support any sort of TX-done > notification or interrupt. The driver previously used the hardware > loopback to attempt to work around this deficiency, but this caused all > sockets to receive all messages, even if CAN_RAW_RECV_OWN_MSGS is off. > > Using the new function can_cmp_echo_skb(), we can drop the loopback > messages and return the original skbs. This fixes the issues with > CAN_RAW_RECV_OWN_MSGS. > > Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu> > --- > drivers/net/can/janz-ican3.c | 56 ++++++++++++++++++++++------------------- > 1 files changed, 30 insertions(+), 26 deletions(-) > > diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c > index 08c893c..a49af40 100644 > --- a/drivers/net/can/janz-ican3.c > +++ b/drivers/net/can/janz-ican3.c > @@ -235,7 +235,6 @@ struct ican3_dev { > > /* fast host interface */ > unsigned int fastrx_start; > - unsigned int fastrx_int; > unsigned int fastrx_num; > unsigned int fasttx_start; > unsigned int fasttx_num; > @@ -454,7 +453,6 @@ static void __devinit ican3_init_fast_host_interface(struct ican3_dev *mod) > /* save the start recv page */ > mod->fastrx_start = mod->free_page; > mod->fastrx_num = 0; > - mod->fastrx_int = 0; > > /* build a single fast tohost queue descriptor */ > memset(&desc, 0, sizeof(desc)); > @@ -1150,6 +1148,21 @@ static int ican3_recv_skb(struct ican3_dev *mod) > /* convert the ICAN3 frame into Linux CAN format */ > ican3_to_can_frame(mod, &desc, cf); > > + /* > + * If this is an ECHO frame received from the hardware loopback > + * feature, use the skb saved in the ECHO stack instead. This allows > + * the Linux CAN core to support CAN_RAW_RECV_OWN_MSGS correctly. > + * > + * Also, the netdevice queue needs to be allowed to send packets again. > + */ > + if (can_cmp_echo_skb(skb, ndev, 0)) { > + stats->rx_packets++; > + stats->rx_bytes += can_get_echo_skb(ndev, 0); > + kfree_skb(skb); > + netif_wake_queue(ndev); > + goto err_noalloc; > + } > + > /* receive the skb, update statistics */ > netif_receive_skb(skb); > stats->rx_packets++; > @@ -1177,7 +1190,6 @@ static int ican3_napi(struct napi_struct *napi, int budget) > { > struct ican3_dev *mod = container_of(napi, struct ican3_dev, napi); > struct ican3_msg msg; > - unsigned long flags; > int received = 0; > int ret; > > @@ -1204,14 +1216,6 @@ static int ican3_napi(struct napi_struct *napi, int budget) > if (received < budget) > napi_complete(napi); > > - spin_lock_irqsave(&mod->lock, flags); > - > - /* Wake up the transmit queue if necessary */ > - if (netif_queue_stopped(mod->ndev) && ican3_txok(mod)) > - netif_wake_queue(mod->ndev); > - > - spin_unlock_irqrestore(&mod->lock, flags); > - > /* re-enable interrupt generation */ > iowrite8(1 << mod->num, &mod->ctrl->int_enable); > return received; > @@ -1422,12 +1426,14 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev) > void __iomem *desc_addr; > unsigned long flags; > > + if (can_dropped_invalid_skb(ndev, skb)) > + return NETDEV_TX_OK; > + > spin_lock_irqsave(&mod->lock, flags); > > /* check that we can actually transmit */ > if (!ican3_txok(mod)) { > - dev_err(mod->dev, "no free descriptors, stopping queue\n"); > - netif_stop_queue(ndev); > + dev_err(mod->dev, "BUG: no free descriptors\n"); > spin_unlock_irqrestore(&mod->lock, flags); > return NETDEV_TX_BUSY; > } > @@ -1442,6 +1448,16 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev) > can_frame_to_ican3(mod, cf, &desc); > > /* > + * This hardware doesn't have TX-done notifications, so we'll try and > + * emulate it the best we can using ECHO skbs. Add the skb to the ECHO > + * stack and stop transmitting packets. Upon packet reception, check > + * if the ECHO skb and received skb match, and use that to wake the > + * queue. > + */ > + netif_stop_queue(ndev); > + can_put_echo_skb(skb, ndev, 0); > + > + /* > * the programming manual says that you must set the IVALID bit, then > * interrupt, then set the valid bit. Quite weird, but it seems to be > * required for this to work > @@ -1462,18 +1478,6 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev) > /* update statistics */ > stats->tx_packets++; > stats->tx_bytes += cf->can_dlc; I think these statistic updates double the traffic as you do them in ican3_recv_skb() again. > - kfree_skb(skb); > - > - /* > - * This hardware doesn't have TX-done notifications, so we'll try and > - * emulate it the best we can using ECHO skbs. Get the next TX > - * descriptor, and see if we have room to send. If not, stop the queue. > - * It will be woken when the ECHO skb for the current packet is recv'd. > - */ > - > - /* copy the control bits of the descriptor */ > - if (!ican3_txok(mod)) > - netif_stop_queue(ndev); > > spin_unlock_irqrestore(&mod->lock, flags); > return NETDEV_TX_OK; > @@ -1654,7 +1658,7 @@ static int __devinit ican3_probe(struct platform_device *pdev) > dev = &pdev->dev; > > /* allocate the CAN device and private data */ > - ndev = alloc_candev(sizeof(*mod), 0); > + ndev = alloc_candev(sizeof(*mod), 1); > if (!ndev) { > dev_err(dev, "unable to allocate CANdev\n"); > ret = -ENOMEM; ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS 2012-07-09 20:57 ` Oliver Hartkopp @ 2012-07-09 21:16 ` Ira W. Snyder 2012-07-10 6:40 ` Wolfgang Grandegger 0 siblings, 1 reply; 16+ messages in thread From: Ira W. Snyder @ 2012-07-09 21:16 UTC (permalink / raw) To: Oliver Hartkopp; +Cc: linux-can On Mon, Jul 09, 2012 at 10:57:28PM +0200, Oliver Hartkopp wrote: > On 09.07.2012 21:56, Ira W. Snyder wrote: > > > From: "Ira W. Snyder" <iws@ovro.caltech.edu> > > > > The Janz VMOD-ICAN3 firmware does not support any sort of TX-done > > notification or interrupt. The driver previously used the hardware > > loopback to attempt to work around this deficiency, but this caused all > > sockets to receive all messages, even if CAN_RAW_RECV_OWN_MSGS is off. > > > > Using the new function can_cmp_echo_skb(), we can drop the loopback > > messages and return the original skbs. This fixes the issues with > > CAN_RAW_RECV_OWN_MSGS. > > > > Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu> > > --- > > drivers/net/can/janz-ican3.c | 56 ++++++++++++++++++++++------------------- > > 1 files changed, 30 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c > > index 08c893c..a49af40 100644 > > --- a/drivers/net/can/janz-ican3.c > > +++ b/drivers/net/can/janz-ican3.c > > @@ -235,7 +235,6 @@ struct ican3_dev { > > > > /* fast host interface */ > > unsigned int fastrx_start; > > - unsigned int fastrx_int; > > unsigned int fastrx_num; > > unsigned int fasttx_start; > > unsigned int fasttx_num; > > @@ -454,7 +453,6 @@ static void __devinit ican3_init_fast_host_interface(struct ican3_dev *mod) > > /* save the start recv page */ > > mod->fastrx_start = mod->free_page; > > mod->fastrx_num = 0; > > - mod->fastrx_int = 0; > > > > /* build a single fast tohost queue descriptor */ > > memset(&desc, 0, sizeof(desc)); > > @@ -1150,6 +1148,21 @@ static int ican3_recv_skb(struct ican3_dev *mod) > > /* convert the ICAN3 frame into Linux CAN format */ > > ican3_to_can_frame(mod, &desc, cf); > > > > + /* > > + * If this is an ECHO frame received from the hardware loopback > > + * feature, use the skb saved in the ECHO stack instead. This allows > > + * the Linux CAN core to support CAN_RAW_RECV_OWN_MSGS correctly. > > + * > > + * Also, the netdevice queue needs to be allowed to send packets again. > > + */ > > + if (can_cmp_echo_skb(skb, ndev, 0)) { > > + stats->rx_packets++; > > + stats->rx_bytes += can_get_echo_skb(ndev, 0); > > + kfree_skb(skb); > > + netif_wake_queue(ndev); > > + goto err_noalloc; > > + } > > + > > /* receive the skb, update statistics */ > > netif_receive_skb(skb); > > stats->rx_packets++; > > @@ -1177,7 +1190,6 @@ static int ican3_napi(struct napi_struct *napi, int budget) > > { > > struct ican3_dev *mod = container_of(napi, struct ican3_dev, napi); > > struct ican3_msg msg; > > - unsigned long flags; > > int received = 0; > > int ret; > > > > @@ -1204,14 +1216,6 @@ static int ican3_napi(struct napi_struct *napi, int budget) > > if (received < budget) > > napi_complete(napi); > > > > - spin_lock_irqsave(&mod->lock, flags); > > - > > - /* Wake up the transmit queue if necessary */ > > - if (netif_queue_stopped(mod->ndev) && ican3_txok(mod)) > > - netif_wake_queue(mod->ndev); > > - > > - spin_unlock_irqrestore(&mod->lock, flags); > > - > > /* re-enable interrupt generation */ > > iowrite8(1 << mod->num, &mod->ctrl->int_enable); > > return received; > > @@ -1422,12 +1426,14 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev) > > void __iomem *desc_addr; > > unsigned long flags; > > > > + if (can_dropped_invalid_skb(ndev, skb)) > > + return NETDEV_TX_OK; > > + > > spin_lock_irqsave(&mod->lock, flags); > > > > /* check that we can actually transmit */ > > if (!ican3_txok(mod)) { > > - dev_err(mod->dev, "no free descriptors, stopping queue\n"); > > - netif_stop_queue(ndev); > > + dev_err(mod->dev, "BUG: no free descriptors\n"); > > spin_unlock_irqrestore(&mod->lock, flags); > > return NETDEV_TX_BUSY; > > } > > @@ -1442,6 +1448,16 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev) > > can_frame_to_ican3(mod, cf, &desc); > > > > /* > > + * This hardware doesn't have TX-done notifications, so we'll try and > > + * emulate it the best we can using ECHO skbs. Add the skb to the ECHO > > + * stack and stop transmitting packets. Upon packet reception, check > > + * if the ECHO skb and received skb match, and use that to wake the > > + * queue. > > + */ > > + netif_stop_queue(ndev); > > + can_put_echo_skb(skb, ndev, 0); > > + > > + /* > > * the programming manual says that you must set the IVALID bit, then > > * interrupt, then set the valid bit. Quite weird, but it seems to be > > * required for this to work > > @@ -1462,18 +1478,6 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev) > > /* update statistics */ > > stats->tx_packets++; > > stats->tx_bytes += cf->can_dlc; > > > I think these statistic updates double the traffic as you do them in > ican3_recv_skb() again. > How exactly are the RX and TX packet counters supposed to work? There are several cases to consider. For each case, should the rx, tx, none, or both sets of counters updated? 1) When ican3_xmit() is called 2) When ican3_recv_skb() is called with an ECHO packet (sent from the local host, and looped back by the hardware) 3) When ican3_recv_skb() is called with a packet from an external host (sent from a different device on the CAN bus) The current code does the following: 1) tx counters only 2) rx counters only (we *did* receive a packet from hardware, after all) 3) rx counters only Do you want the following: 1) tx counters only 2) none 3) rx counters only I can also come up with good reasons for the following: 1) none (the packet has only been queued, not sucessfully transmitted) 2) tx counters only (we *did* successfully transmit a packet) 3) rx counters only Please clarify. Thanks, Ira > > - kfree_skb(skb); > > - > > - /* > > - * This hardware doesn't have TX-done notifications, so we'll try and > > - * emulate it the best we can using ECHO skbs. Get the next TX > > - * descriptor, and see if we have room to send. If not, stop the queue. > > - * It will be woken when the ECHO skb for the current packet is recv'd. > > - */ > > - > > - /* copy the control bits of the descriptor */ > > - if (!ican3_txok(mod)) > > - netif_stop_queue(ndev); > > > > spin_unlock_irqrestore(&mod->lock, flags); > > return NETDEV_TX_OK; > > @@ -1654,7 +1658,7 @@ static int __devinit ican3_probe(struct platform_device *pdev) > > dev = &pdev->dev; > > > > /* allocate the CAN device and private data */ > > - ndev = alloc_candev(sizeof(*mod), 0); > > + ndev = alloc_candev(sizeof(*mod), 1); > > if (!ndev) { > > dev_err(dev, "unable to allocate CANdev\n"); > > ret = -ENOMEM; > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS 2012-07-09 21:16 ` Ira W. Snyder @ 2012-07-10 6:40 ` Wolfgang Grandegger 0 siblings, 0 replies; 16+ messages in thread From: Wolfgang Grandegger @ 2012-07-10 6:40 UTC (permalink / raw) To: Ira W. Snyder; +Cc: Oliver Hartkopp, linux-can On 07/09/2012 11:16 PM, Ira W. Snyder wrote: > On Mon, Jul 09, 2012 at 10:57:28PM +0200, Oliver Hartkopp wrote: >> On 09.07.2012 21:56, Ira W. Snyder wrote: >> >>> From: "Ira W. Snyder" <iws@ovro.caltech.edu> >>> >>> The Janz VMOD-ICAN3 firmware does not support any sort of TX-done >>> notification or interrupt. The driver previously used the hardware >>> loopback to attempt to work around this deficiency, but this caused all >>> sockets to receive all messages, even if CAN_RAW_RECV_OWN_MSGS is off. >>> >>> Using the new function can_cmp_echo_skb(), we can drop the loopback >>> messages and return the original skbs. This fixes the issues with >>> CAN_RAW_RECV_OWN_MSGS. >>> >>> Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu> >>> --- >>> drivers/net/can/janz-ican3.c | 56 ++++++++++++++++++++++------------------- >>> 1 files changed, 30 insertions(+), 26 deletions(-) >>> >>> diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c >>> index 08c893c..a49af40 100644 >>> --- a/drivers/net/can/janz-ican3.c >>> +++ b/drivers/net/can/janz-ican3.c >>> @@ -235,7 +235,6 @@ struct ican3_dev { >>> >>> /* fast host interface */ >>> unsigned int fastrx_start; >>> - unsigned int fastrx_int; >>> unsigned int fastrx_num; >>> unsigned int fasttx_start; >>> unsigned int fasttx_num; >>> @@ -454,7 +453,6 @@ static void __devinit ican3_init_fast_host_interface(struct ican3_dev *mod) >>> /* save the start recv page */ >>> mod->fastrx_start = mod->free_page; >>> mod->fastrx_num = 0; >>> - mod->fastrx_int = 0; >>> >>> /* build a single fast tohost queue descriptor */ >>> memset(&desc, 0, sizeof(desc)); >>> @@ -1150,6 +1148,21 @@ static int ican3_recv_skb(struct ican3_dev *mod) >>> /* convert the ICAN3 frame into Linux CAN format */ >>> ican3_to_can_frame(mod, &desc, cf); >>> >>> + /* >>> + * If this is an ECHO frame received from the hardware loopback >>> + * feature, use the skb saved in the ECHO stack instead. This allows >>> + * the Linux CAN core to support CAN_RAW_RECV_OWN_MSGS correctly. >>> + * >>> + * Also, the netdevice queue needs to be allowed to send packets again. >>> + */ >>> + if (can_cmp_echo_skb(skb, ndev, 0)) { >>> + stats->rx_packets++; >>> + stats->rx_bytes += can_get_echo_skb(ndev, 0); >>> + kfree_skb(skb); >>> + netif_wake_queue(ndev); >>> + goto err_noalloc; >>> + } >>> + >>> /* receive the skb, update statistics */ >>> netif_receive_skb(skb); >>> stats->rx_packets++; >>> @@ -1177,7 +1190,6 @@ static int ican3_napi(struct napi_struct *napi, int budget) >>> { >>> struct ican3_dev *mod = container_of(napi, struct ican3_dev, napi); >>> struct ican3_msg msg; >>> - unsigned long flags; >>> int received = 0; >>> int ret; >>> >>> @@ -1204,14 +1216,6 @@ static int ican3_napi(struct napi_struct *napi, int budget) >>> if (received < budget) >>> napi_complete(napi); >>> >>> - spin_lock_irqsave(&mod->lock, flags); >>> - >>> - /* Wake up the transmit queue if necessary */ >>> - if (netif_queue_stopped(mod->ndev) && ican3_txok(mod)) >>> - netif_wake_queue(mod->ndev); >>> - >>> - spin_unlock_irqrestore(&mod->lock, flags); >>> - >>> /* re-enable interrupt generation */ >>> iowrite8(1 << mod->num, &mod->ctrl->int_enable); >>> return received; >>> @@ -1422,12 +1426,14 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev) >>> void __iomem *desc_addr; >>> unsigned long flags; >>> >>> + if (can_dropped_invalid_skb(ndev, skb)) >>> + return NETDEV_TX_OK; >>> + >>> spin_lock_irqsave(&mod->lock, flags); >>> >>> /* check that we can actually transmit */ >>> if (!ican3_txok(mod)) { >>> - dev_err(mod->dev, "no free descriptors, stopping queue\n"); >>> - netif_stop_queue(ndev); >>> + dev_err(mod->dev, "BUG: no free descriptors\n"); >>> spin_unlock_irqrestore(&mod->lock, flags); >>> return NETDEV_TX_BUSY; >>> } >>> @@ -1442,6 +1448,16 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev) >>> can_frame_to_ican3(mod, cf, &desc); >>> >>> /* >>> + * This hardware doesn't have TX-done notifications, so we'll try and >>> + * emulate it the best we can using ECHO skbs. Add the skb to the ECHO >>> + * stack and stop transmitting packets. Upon packet reception, check >>> + * if the ECHO skb and received skb match, and use that to wake the >>> + * queue. >>> + */ >>> + netif_stop_queue(ndev); >>> + can_put_echo_skb(skb, ndev, 0); >>> + >>> + /* >>> * the programming manual says that you must set the IVALID bit, then >>> * interrupt, then set the valid bit. Quite weird, but it seems to be >>> * required for this to work >>> @@ -1462,18 +1478,6 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev) >>> /* update statistics */ >>> stats->tx_packets++; >>> stats->tx_bytes += cf->can_dlc; >> >> >> I think these statistic updates double the traffic as you do them in >> ican3_recv_skb() again. >> > > How exactly are the RX and TX packet counters supposed to work? The RX and TX packet and byte counters should be set once when a CAN message has been *successfully* received or transmitted. For TX this is usually done when the TX done interrupt is handled. Looped backed packets (via echo skb) are not counted. > There are several cases to consider. For each case, should the rx, tx, > none, or both sets of counters updated? > > 1) When ican3_xmit() is called > 2) When ican3_recv_skb() is called with an ECHO packet (sent from the > local host, and looped back by the hardware) > 3) When ican3_recv_skb() is called with a packet from an external host > (sent from a different device on the CAN bus) Only messages from and to the CAN bus should be counted. > The current code does the following: > 1) tx counters only > 2) rx counters only (we *did* receive a packet from hardware, after all) > 3) rx counters only > > Do you want the following: > 1) tx counters only > 2) none > 3) rx counters only > > I can also come up with good reasons for the following: > 1) none (the packet has only been queued, not sucessfully transmitted) > 2) tx counters only (we *did* successfully transmit a packet) > 3) rx counters only That above handling shpuld be fine. We do *not* count looped back messages and the counters should be incremented on *success* (which might not always be possible). For example the EMS USB does also not provide a TX done notification: http://lxr.linux.no/#linux+v3.4.4/drivers/net/can/usb/ems_usb.c#L497 Wolfgang. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS 2012-07-09 19:56 [PATCH 0/3] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS Ira W. Snyder ` (2 preceding siblings ...) 2012-07-09 19:56 ` [PATCH 3/3] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS Ira W. Snyder @ 2012-07-09 21:05 ` Oliver Hartkopp 2012-07-09 21:29 ` Ira W. Snyder 2012-07-10 6:09 ` Wolfgang Grandegger 4 siblings, 1 reply; 16+ messages in thread From: Oliver Hartkopp @ 2012-07-09 21:05 UTC (permalink / raw) To: Ira W. Snyder; +Cc: linux-can Hello Ira, looks good to me in general ... Let's wait for other comments though. Does it work without problems? On 09.07.2012 21:56, Ira W. Snyder wrote: > From: "Ira W. Snyder" <iws@ovro.caltech.edu> > > This is another different approach to fixing the Janz ICAN3 support for > CAN_RAW_RECV_OWN_MSGS. > > The can_put_echo_skb() function is changed to always keep all packets > until can_get_echo_skb() is called. Previously, it would drop packets if > they were not needed to be looped back. This makes it possible for the > new function can_cmp_echo_skb() to work with hardware-assisted loopback > support on the Janz ICAN3, which does not have TX-complete interrupts of > any kind. > > Since we are now storing packets even in the non-loopback case, there is > some extra memory overhead, to store the extra packets between the calls > to can_put_echo_skb() and can_get_echo_skb(). > > After this patch series is applied, the SocketCAN tst-rcv-own-msgs test > passes. > > Performance is rougly 15% less than using the previously posted patch: > [PATCH ALTERNATE VERSION 1/1] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS > > This performance drop is due to the extra overhead of receiving the echo > packets from the card itself. This involves one extra interrupt for each > packet sent, and the associated overhead of running ican3_napi() for > each packet sent. > If it works without problems you might try to use 2 or 3 echo_skbs for testing to ensure the Janz card gets lined with outgoing traffic without waiting for each single sent frame. I assume this would make the implementation slightly complexer but may solve the performance drop on the wire. > Ira W. Snyder (3): > can: make the echo stack keep packet information longer > can: add can_cmp_echo_skb() for echo skb comparison > can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS > > drivers/net/can/dev.c | 75 ++++++++++++++++++++++++++++++----------- > drivers/net/can/janz-ican3.c | 56 +++++++++++++++++-------------- > include/linux/can/dev.h | 2 + > 3 files changed, 87 insertions(+), 46 deletions(-) > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS 2012-07-09 21:05 ` [PATCH 0/3] " Oliver Hartkopp @ 2012-07-09 21:29 ` Ira W. Snyder 0 siblings, 0 replies; 16+ messages in thread From: Ira W. Snyder @ 2012-07-09 21:29 UTC (permalink / raw) To: Oliver Hartkopp; +Cc: linux-can On Mon, Jul 09, 2012 at 11:05:33PM +0200, Oliver Hartkopp wrote: > Hello Ira, > > looks good to me in general ... > > Let's wait for other comments though. > > Does it work without problems? > There are no problems that I could find. Hooking two cards together and running cangen on both simultaneously at the maximum packet generation rate worked fine. It worked in both loopback (no option specified) and non-loopback (-x) mode. The previous patch which used can_cmp_echo_skb() did not handle non-loopback mode correctly. This version seems to receive more ENOBUFS errors back to cangen than the "ALTERNATE VERSION" which removed the netif_stop_queue() and netif_wake_queue() functions. I don't know if that is a problem or not. Overall, it looks good to me. Thanks for the comments on the other patches, Ira > On 09.07.2012 21:56, Ira W. Snyder wrote: > > > From: "Ira W. Snyder" <iws@ovro.caltech.edu> > > > > This is another different approach to fixing the Janz ICAN3 support for > > CAN_RAW_RECV_OWN_MSGS. > > > > The can_put_echo_skb() function is changed to always keep all packets > > until can_get_echo_skb() is called. Previously, it would drop packets if > > they were not needed to be looped back. This makes it possible for the > > new function can_cmp_echo_skb() to work with hardware-assisted loopback > > support on the Janz ICAN3, which does not have TX-complete interrupts of > > any kind. > > > > Since we are now storing packets even in the non-loopback case, there is > > some extra memory overhead, to store the extra packets between the calls > > to can_put_echo_skb() and can_get_echo_skb(). > > > > After this patch series is applied, the SocketCAN tst-rcv-own-msgs test > > passes. > > > > Performance is rougly 15% less than using the previously posted patch: > > [PATCH ALTERNATE VERSION 1/1] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS > > > > This performance drop is due to the extra overhead of receiving the echo > > packets from the card itself. This involves one extra interrupt for each > > packet sent, and the associated overhead of running ican3_napi() for > > each packet sent. > > > > > If it works without problems you might try to use 2 or 3 echo_skbs for testing > to ensure the Janz card gets lined with outgoing traffic without waiting for > each single sent frame. > > I assume this would make the implementation slightly complexer but may solve > the performance drop on the wire. > > > Ira W. Snyder (3): > > can: make the echo stack keep packet information longer > > can: add can_cmp_echo_skb() for echo skb comparison > > can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS > > > > drivers/net/can/dev.c | 75 ++++++++++++++++++++++++++++++----------- > > drivers/net/can/janz-ican3.c | 56 +++++++++++++++++-------------- > > include/linux/can/dev.h | 2 + > > 3 files changed, 87 insertions(+), 46 deletions(-) > > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS 2012-07-09 19:56 [PATCH 0/3] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS Ira W. Snyder ` (3 preceding siblings ...) 2012-07-09 21:05 ` [PATCH 0/3] " Oliver Hartkopp @ 2012-07-10 6:09 ` Wolfgang Grandegger 2012-07-10 6:59 ` Wolfgang Grandegger 4 siblings, 1 reply; 16+ messages in thread From: Wolfgang Grandegger @ 2012-07-10 6:09 UTC (permalink / raw) To: Ira W. Snyder; +Cc: linux-can Hi Ira, On 07/09/2012 09:56 PM, Ira W. Snyder wrote: > From: "Ira W. Snyder" <iws@ovro.caltech.edu> > > This is another different approach to fixing the Janz ICAN3 support for > CAN_RAW_RECV_OWN_MSGS. > > The can_put_echo_skb() function is changed to always keep all packets > until can_get_echo_skb() is called. Previously, it would drop packets if > they were not needed to be looped back. This makes it possible for the > new function can_cmp_echo_skb() to work with hardware-assisted loopback > support on the Janz ICAN3, which does not have TX-complete interrupts of > any kind. > > Since we are now storing packets even in the non-loopback case, there is > some extra memory overhead, to store the extra packets between the calls > to can_put_echo_skb() and can_get_echo_skb(). Well, I don't like such device specific quirk going into the common interface. > After this patch series is applied, the SocketCAN tst-rcv-own-msgs test > passes. > > Performance is rougly 15% less than using the previously posted patch: > [PATCH ALTERNATE VERSION 1/1] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS Oops, that's a lot and a clear contra. > This performance drop is due to the extra overhead of receiving the echo > packets from the card itself. This involves one extra interrupt for each > packet sent, and the associated overhead of running ican3_napi() for > each packet sent. Do we really want that. I agree that CAN_RAW_RECV_OWN_MSGS should work as expected but we should not try hard to partially fix the timing issue. Why do we not just use the default protocol callback (flags &= !IFF_ECHO) if the hardware cannot do it better. Wolfgang. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS 2012-07-10 6:09 ` Wolfgang Grandegger @ 2012-07-10 6:59 ` Wolfgang Grandegger 2012-07-10 15:22 ` Ira W. Snyder 0 siblings, 1 reply; 16+ messages in thread From: Wolfgang Grandegger @ 2012-07-10 6:59 UTC (permalink / raw) To: Ira W. Snyder; +Cc: linux-can On 07/10/2012 08:09 AM, Wolfgang Grandegger wrote: > Hi Ira, > > On 07/09/2012 09:56 PM, Ira W. Snyder wrote: >> From: "Ira W. Snyder" <iws@ovro.caltech.edu> >> >> This is another different approach to fixing the Janz ICAN3 support for >> CAN_RAW_RECV_OWN_MSGS. >> >> The can_put_echo_skb() function is changed to always keep all packets >> until can_get_echo_skb() is called. Previously, it would drop packets if >> they were not needed to be looped back. This makes it possible for the >> new function can_cmp_echo_skb() to work with hardware-assisted loopback >> support on the Janz ICAN3, which does not have TX-complete interrupts of >> any kind. >> >> Since we are now storing packets even in the non-loopback case, there is >> some extra memory overhead, to store the extra packets between the calls >> to can_put_echo_skb() and can_get_echo_skb(). > > Well, I don't like such device specific quirk going into the common > interface. Be aware, all other drivers will suffer from that change! >> After this patch series is applied, the SocketCAN tst-rcv-own-msgs test >> passes. >> >> Performance is rougly 15% less than using the previously posted patch: >> [PATCH ALTERNATE VERSION 1/1] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS Are the 15% only relevant if CAN_RAW_RECV_OWN_MSGS is off? > Oops, that's a lot and a clear contra. > >> This performance drop is due to the extra overhead of receiving the echo >> packets from the card itself. This involves one extra interrupt for each >> packet sent, and the associated overhead of running ican3_napi() for >> each packet sent. > > Do we really want that. I agree that CAN_RAW_RECV_OWN_MSGS should work > as expected but we should not try hard to partially fix the timing > issue. Why do we not just use the default protocol callback (flags &= > !IFF_ECHO) if the hardware cannot do it better. Well, getting a looped back message even if it did not go out to the bus is not nice either. What about a device specific solution where you do the dropping of looped back messages only if CAN_RAW_RECV_OWN_MSGS is off. Wolfgang, ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS 2012-07-10 6:59 ` Wolfgang Grandegger @ 2012-07-10 15:22 ` Ira W. Snyder 2012-07-10 19:19 ` Oliver Hartkopp 2012-07-10 20:34 ` Wolfgang Grandegger 0 siblings, 2 replies; 16+ messages in thread From: Ira W. Snyder @ 2012-07-10 15:22 UTC (permalink / raw) To: Wolfgang Grandegger; +Cc: linux-can On Tue, Jul 10, 2012 at 08:59:09AM +0200, Wolfgang Grandegger wrote: > On 07/10/2012 08:09 AM, Wolfgang Grandegger wrote: > > Hi Ira, > > > > On 07/09/2012 09:56 PM, Ira W. Snyder wrote: > >> From: "Ira W. Snyder" <iws@ovro.caltech.edu> > >> > >> This is another different approach to fixing the Janz ICAN3 support for > >> CAN_RAW_RECV_OWN_MSGS. > >> > >> The can_put_echo_skb() function is changed to always keep all packets > >> until can_get_echo_skb() is called. Previously, it would drop packets if > >> they were not needed to be looped back. This makes it possible for the > >> new function can_cmp_echo_skb() to work with hardware-assisted loopback > >> support on the Janz ICAN3, which does not have TX-complete interrupts of > >> any kind. > >> > >> Since we are now storing packets even in the non-loopback case, there is > >> some extra memory overhead, to store the extra packets between the calls > >> to can_put_echo_skb() and can_get_echo_skb(). > > > > Well, I don't like such device specific quirk going into the common > > interface. > > Be aware, all other drivers will suffer from that change! > I understand that. If you prefer, I can copy can_put_echo_skb() and can_get_echo_skb() with my modifications into the janz-ican3 driver itself, and leave the generic ones alone. > >> After this patch series is applied, the SocketCAN tst-rcv-own-msgs test > >> passes. > >> > >> Performance is rougly 15% less than using the previously posted patch: > >> [PATCH ALTERNATE VERSION 1/1] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS > > Are the 15% only relevant if CAN_RAW_RECV_OWN_MSGS is off? > The performance difference happens in all cases. The ican3 hardware doesn't have a TX-done interrupt, but the programming guide does promise that the hardware loopback will not return the echo packet until it has been successfully transmitted onto the bus. It does appear to work correctly. I talked with my coworker who is more knowledgable about the CAN bus. We decided we like this patch as written, since we will be able to get more accurate timing information by sending a packet and timing how long it takes to receive it back using CAN_RAW_RECV_OWN_MSGS. > > Oops, that's a lot and a clear contra. > > > >> This performance drop is due to the extra overhead of receiving the echo > >> packets from the card itself. This involves one extra interrupt for each > >> packet sent, and the associated overhead of running ican3_napi() for > >> each packet sent. > > > > Do we really want that. I agree that CAN_RAW_RECV_OWN_MSGS should work > > as expected but we should not try hard to partially fix the timing > > issue. Why do we not just use the default protocol callback (flags &= > > !IFF_ECHO) if the hardware cannot do it better. > > Well, getting a looped back message even if it did not go out to the bus > is not nice either. > Right. As noted above, we have decided that we like this patch, since it only loops back packets that have actually made it onto the bus. > What about a device specific solution where you do the dropping of > looped back messages only if CAN_RAW_RECV_OWN_MSGS is off. > Isn't this option per-socket, rather than per-device? I don't have any idea how I'd implement this suggestion. Thanks, Ira > Wolfgang, ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS 2012-07-10 15:22 ` Ira W. Snyder @ 2012-07-10 19:19 ` Oliver Hartkopp 2012-07-10 20:34 ` Wolfgang Grandegger 1 sibling, 0 replies; 16+ messages in thread From: Oliver Hartkopp @ 2012-07-10 19:19 UTC (permalink / raw) To: Ira W. Snyder, Wolfgang Grandegger; +Cc: linux-can Hello Wolfgang, hello Ira, On 10.07.2012 17:22, Ira W. Snyder wrote: > On Tue, Jul 10, 2012 at 08:59:09AM +0200, Wolfgang Grandegger wrote: >>>> Since we are now storing packets even in the non-loopback case, there is >>>> some extra memory overhead, to store the extra packets between the calls >>>> to can_put_echo_skb() and can_get_echo_skb(). >>> >>> Well, I don't like such device specific quirk going into the common >>> interface. >> >> Be aware, all other drivers will suffer from that change! >> > > I understand that. If you prefer, I can copy can_put_echo_skb() and > can_get_echo_skb() with my modifications into the janz-ican3 driver > itself, and leave the generic ones alone. > i also thought about probable impacts on other drivers and to me the idea to include the changes in the ican3 driver is ok. The general infrastucture of echo_skb[] can be re-used - but the access to these skbs can be done ican3 specific. E.g. the BUG_ON() statements may become obsolete then. >>>> After this patch series is applied, the SocketCAN tst-rcv-own-msgs test >>>> passes. >>>> >>>> Performance is rougly 15% less than using the previously posted patch: >>>> [PATCH ALTERNATE VERSION 1/1] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS >> >> Are the 15% only relevant if CAN_RAW_RECV_OWN_MSGS is off? >> > > The performance difference happens in all cases. The ican3 hardware > doesn't have a TX-done interrupt, but the programming guide does promise > that the hardware loopback will not return the echo packet until it has > been successfully transmitted onto the bus. It does appear to work > correctly. > > I talked with my coworker who is more knowledgable about the CAN bus. We > decided we like this patch as written, since we will be able to get more > accurate timing information by sending a packet and timing how long it > takes to receive it back using CAN_RAW_RECV_OWN_MSGS. > Good choice :-) From the practical point you get into problems, when your CAN driver doesn't support IFF_ECHO (e.g. the slcan driver or the legacy PEAK drivers) as people call you on the phone and talk about traffic seen by candump but no traffic on the bus %-( I would suggest to have about 2-3 echo_skbs in flight. This allows to push the tx side of the ican3 to it's max. throughput. On the receiver side you just need to compare up to 3 frames then and when >= 2 echo_skbs become free (NULL) you restart the tx queue. Should be feasible with low effort ... >> What about a device specific solution where you do the dropping of >> looped back messages only if CAN_RAW_RECV_OWN_MSGS is off. >> > > Isn't this option per-socket, rather than per-device? yes. > > I don't have any idea how I'd implement this suggestion. > > Thanks, > Ira Regards, Oliver ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS 2012-07-10 15:22 ` Ira W. Snyder 2012-07-10 19:19 ` Oliver Hartkopp @ 2012-07-10 20:34 ` Wolfgang Grandegger 1 sibling, 0 replies; 16+ messages in thread From: Wolfgang Grandegger @ 2012-07-10 20:34 UTC (permalink / raw) To: Ira W. Snyder; +Cc: linux-can On 07/10/2012 05:22 PM, Ira W. Snyder wrote: > On Tue, Jul 10, 2012 at 08:59:09AM +0200, Wolfgang Grandegger wrote: >> On 07/10/2012 08:09 AM, Wolfgang Grandegger wrote: >>> Hi Ira, >>> >>> On 07/09/2012 09:56 PM, Ira W. Snyder wrote: >>>> From: "Ira W. Snyder" <iws@ovro.caltech.edu> >>>> >>>> This is another different approach to fixing the Janz ICAN3 support for >>>> CAN_RAW_RECV_OWN_MSGS. >>>> >>>> The can_put_echo_skb() function is changed to always keep all packets >>>> until can_get_echo_skb() is called. Previously, it would drop packets if >>>> they were not needed to be looped back. This makes it possible for the >>>> new function can_cmp_echo_skb() to work with hardware-assisted loopback >>>> support on the Janz ICAN3, which does not have TX-complete interrupts of >>>> any kind. >>>> >>>> Since we are now storing packets even in the non-loopback case, there is >>>> some extra memory overhead, to store the extra packets between the calls >>>> to can_put_echo_skb() and can_get_echo_skb(). >>> >>> Well, I don't like such device specific quirk going into the common >>> interface. >> >> Be aware, all other drivers will suffer from that change! >> > > I understand that. If you prefer, I can copy can_put_echo_skb() and > can_get_echo_skb() with my modifications into the janz-ican3 driver > itself, and leave the generic ones alone. Definitely, this is special handling for that device only. Till now we have no other device needing a similar quirk. >>>> After this patch series is applied, the SocketCAN tst-rcv-own-msgs test >>>> passes. >>>> >>>> Performance is rougly 15% less than using the previously posted patch: >>>> [PATCH ALTERNATE VERSION 1/1] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS >> >> Are the 15% only relevant if CAN_RAW_RECV_OWN_MSGS is off? >> > > The performance difference happens in all cases. The ican3 hardware > doesn't have a TX-done interrupt, but the programming guide does promise > that the hardware loopback will not return the echo packet until it has > been successfully transmitted onto the bus. It does appear to work > correctly. > > I talked with my coworker who is more knowledgable about the CAN bus. We > decided we like this patch as written, since we will be able to get more > accurate timing information by sending a packet and timing how long it > takes to receive it back using CAN_RAW_RECV_OWN_MSGS. > >>> Oops, that's a lot and a clear contra. >>> >>>> This performance drop is due to the extra overhead of receiving the echo >>>> packets from the card itself. This involves one extra interrupt for each >>>> packet sent, and the associated overhead of running ican3_napi() for >>>> each packet sent. >>> >>> Do we really want that. I agree that CAN_RAW_RECV_OWN_MSGS should work >>> as expected but we should not try hard to partially fix the timing >>> issue. Why do we not just use the default protocol callback (flags &= >>> !IFF_ECHO) if the hardware cannot do it better. >> >> Well, getting a looped back message even if it did not go out to the bus >> is not nice either. >> > > Right. As noted above, we have decided that we like this patch, since it > only loops back packets that have actually made it onto the bus. > >> What about a device specific solution where you do the dropping of >> looped back messages only if CAN_RAW_RECV_OWN_MSGS is off. >> > > Isn't this option per-socket, rather than per-device? > > I don't have any idea how I'd implement this suggestion. Hm, the xmit function does know if the packet should be looped back. If not, the skb will be freed: http://lxr.linux.no/#linux+v3.4.4/drivers/net/can/dev.c#L285 Can't that be used to decide if the packet should be stored, restored and removed if it matches a looped back message? Wolfgang. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 0/3] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS @ 2012-07-12 16:15 Ira W. Snyder 0 siblings, 0 replies; 16+ messages in thread From: Ira W. Snyder @ 2012-07-12 16:15 UTC (permalink / raw) To: linux-can; +Cc: Ira W. Snyder From: "Ira W. Snyder" <iws@ovro.caltech.edu> Per reviewer feedback, I have moved the janz-ican3 driver-specific modifications to the can_put_echo_skb() and can_get_echo_skb() into the janz-ican3 driver itself. The first patch brings support up to the level of the previously posted patch series. The second patch modifies the driver-specific versions of can_put_echo_skb() and can_get_echo_skb() to use an SKB queue. This removes the burden of index management, and allows increased TX buffer queue length. The third patch adds support for CAN_CTRLMODE_ONE_SHOT. My coworker has requested this, and it seems like a good enhancement. Ira W. Snyder (3): can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS can: janz-ican3: increase tx buffer size can: janz-ican3: add support for one shot mode drivers/net/can/janz-ican3.c | 164 +++++++++++++++++++++++++++++++++++------- 1 files changed, 137 insertions(+), 27 deletions(-) -- 1.7.8.6 ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2012-07-12 16:15 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-07-09 19:56 [PATCH 0/3] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS Ira W. Snyder 2012-07-09 19:56 ` [PATCH 1/3] can: make the echo stack keep packet information longer Ira W. Snyder 2012-07-09 21:09 ` Oliver Hartkopp 2012-07-09 19:56 ` [PATCH 2/3] can: add can_cmp_echo_skb() for echo skb comparison Ira W. Snyder 2012-07-09 19:56 ` [PATCH 3/3] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS Ira W. Snyder 2012-07-09 20:57 ` Oliver Hartkopp 2012-07-09 21:16 ` Ira W. Snyder 2012-07-10 6:40 ` Wolfgang Grandegger 2012-07-09 21:05 ` [PATCH 0/3] " Oliver Hartkopp 2012-07-09 21:29 ` Ira W. Snyder 2012-07-10 6:09 ` Wolfgang Grandegger 2012-07-10 6:59 ` Wolfgang Grandegger 2012-07-10 15:22 ` Ira W. Snyder 2012-07-10 19:19 ` Oliver Hartkopp 2012-07-10 20:34 ` Wolfgang Grandegger 2012-07-12 16:15 Ira W. Snyder
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.