From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Ira W. Snyder" Subject: Re: [PATCH 3/6] can: janz-ican3: fix error and byte counters Date: Thu, 19 Jul 2012 08:50:26 -0700 Message-ID: <20120719155026.GG25905@ovro.caltech.edu> References: <1342650798-2991-1-git-send-email-iws@ovro.caltech.edu> <1342650798-2991-4-git-send-email-iws@ovro.caltech.edu> <5007C13A.3070809@pengutronix.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from ovro.ovro.caltech.edu ([192.100.16.2]:53857 "EHLO ovro.ovro.caltech.edu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750806Ab2GSPu3 (ORCPT ); Thu, 19 Jul 2012 11:50:29 -0400 Content-Disposition: inline In-Reply-To: <5007C13A.3070809@pengutronix.de> Sender: linux-can-owner@vger.kernel.org List-ID: To: Marc Kleine-Budde Cc: linux-can@vger.kernel.org On Thu, Jul 19, 2012 at 10:11:38AM +0200, Marc Kleine-Budde wrote: > On 07/19/2012 12:33 AM, Ira W. Snyder wrote: > > From: "Ira W. Snyder" > > > > The error and byte counter statistics were being incremented > > incorrectly. For example, a TX error would be counted both in tx_errors > > and rx_errors. > > > > This corrects the problem so that tx_errors and rx_errors are only > > incremented for errors caused by packets sent to the bus. Error packets > > generated by the driver are not counted. > > > > The byte counters are only increased for packets which are actually > > transmitted or received from the bus. Error packets generated by the > > driver are not counted. > > > > Signed-off-by: Ira W. Snyder > > Looking at the patch (and not the complete driver) I have some > questions. See inline. > Thanks for the review. A new version of the patch which has addressed these comments will be posted in reply. Ira > > --- > > drivers/net/can/janz-ican3.c | 13 ++++++------- > > 1 files changed, 6 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c > > index ea1919e..d9d4ed1 100644 > > --- a/drivers/net/can/janz-ican3.c > > +++ b/drivers/net/can/janz-ican3.c > > @@ -907,8 +907,7 @@ static void ican3_handle_msglost(struct ican3_dev *mod, struct ican3_msg *msg) > > if (skb) { > > cf->can_id |= CAN_ERR_CRTL; > > cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW; > > - stats->rx_errors++; > ^^^^^^^^^^^^^^^^^^^ > > - stats->rx_bytes += cf->can_dlc; > > + stats->rx_over_errors++; > > I've checked the sja1000 and the mscan driver, on an rx overflow error > they increment both rx_errors and rx_over_errors. > You're right, that was my mistake. I added the rx_errors++ back. > > netif_rx(skb); > > } > > } > > @@ -956,7 +955,6 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg) > > cf->can_id |= CAN_ERR_CRTL; > > cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW; > > stats->rx_over_errors++; > > - stats->rx_errors++; > > dito > ditto :) > > } > > > > /* error warning + passive interrupt */ > > @@ -982,7 +980,6 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg) > > > > dev_dbg(mod->dev, "bus error interrupt\n"); > > mod->can.can_stats.bus_error++; > > - stats->rx_errors++; > > this rx_errors probably moved to the one below ... > > cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; > > > > switch (ecc & ECC_MASK) { > > @@ -1001,8 +998,12 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg) > > break; > > } > > > > - if ((ecc & ECC_DIR) == 0) > > + if (!(ecc & ECC_DIR)) { > > cf->data[2] |= CAN_ERR_PROT_TX; > > + stats->tx_errors++; > > + } else { > > + stats->rx_errors++; > > ...here. right? > Yep, it moved. > > + } > > > > cf->data[6] = txerr; > > cf->data[7] = rxerr; > > @@ -1028,8 +1029,6 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg) > > } > > > > mod->can.state = state; > > - stats->rx_errors++; > > - stats->rx_bytes += cf->can_dlc; > > netif_rx(skb); > > return 0; > > } > > Marc > -- > Pengutronix e.K. | Marc Kleine-Budde | > Industrial Linux Solutions | Phone: +49-231-2826-924 | > Vertretung West/Dortmund | Fax: +49-5121-206917-5555 | > Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | >