From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wolfgang Grandegger Subject: Re: [PATCH 3/3] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS Date: Tue, 10 Jul 2012 08:40:37 +0200 Message-ID: <4FFBCE65.8030605@grandegger.com> References: <1341863790-5645-1-git-send-email-iws@ovro.caltech.edu> <1341863790-5645-4-git-send-email-iws@ovro.caltech.edu> <4FFB45B8.10400@hartkopp.net> <20120709211638.GB20561@ovro.caltech.edu> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: Received: from ngcobalt02.manitu.net ([217.11.48.102]:37627 "EHLO ngcobalt02.manitu.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752043Ab2GJGko (ORCPT ); Tue, 10 Jul 2012 02:40:44 -0400 In-Reply-To: <20120709211638.GB20561@ovro.caltech.edu> Sender: linux-can-owner@vger.kernel.org List-ID: To: "Ira W. Snyder" Cc: Oliver Hartkopp , linux-can@vger.kernel.org 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" >>> >>> 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 >>> --- >>> 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.