From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Ahmed S. Darwish" Subject: Re: [PATCH v4 2/4] can: kvaser_usb: Update error counters before exiting on OOM Date: Sun, 18 Jan 2015 15:33:47 -0500 Message-ID: <20150118203347.GB15143@linux> References: <20141223154654.GB6460@vivalin-002> <20150111200544.GA8855@linux> <20150111201116.GB8855@linux> <20150111201519.GC8855@linux> <54B3AB6C.8020900@pengutronix.de> <20150112203650.GA11355@linux> <20150116155040.20513.13990@shannon> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from mail-we0-f175.google.com ([74.125.82.175]:33839 "EHLO mail-we0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751817AbbARUdw (ORCPT ); Sun, 18 Jan 2015 15:33:52 -0500 Received: by mail-we0-f175.google.com with SMTP id k11so28242390wes.6 for ; Sun, 18 Jan 2015 12:33:50 -0800 (PST) Content-Disposition: inline In-Reply-To: <20150116155040.20513.13990@shannon> Sender: linux-can-owner@vger.kernel.org List-ID: To: Andri Yngvason Cc: Marc Kleine-Budde , Linux-CAN On Fri, Jan 16, 2015 at 03:50:40PM +0000, Andri Yngvason wrote: > See comments below. > > Quoting Ahmed S. Darwish (2015-01-12 20:36:50) > ... > > > > [ Patch is build-tested, but not _fully_ run-time tested. > > > > It's based on linux-can/testing commit d642b49f6d84b94bd > > "can: kvaser_usb: Don't dereference skb after a netif_rx" ] > > > > Subject: [PATCH] can: kvaser_usb: Update net interface state > > before exiting on OOM > > > > From: Ahmed S. Darwish > > > > Let the network interface can bus state and error counters be > > more accurate in case of Out of Memory conditions. > > > > Suggested-by: Marc Kleine-Budde > > Signed-off-by: Ahmed S. Darwish > > --- > > drivers/net/can/usb/kvaser_usb.c | 182 +++++++++++++++++++++++---------------- > > 1 file changed, 106 insertions(+), 76 deletions(-) > > > > diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c > > index c32cd61..2d0ef76 100644 > > --- a/drivers/net/can/usb/kvaser_usb.c > > +++ b/drivers/net/can/usb/kvaser_usb.c > > @@ -273,6 +273,10 @@ struct kvaser_msg { > > } u; > > } __packed; > > > > +struct kvaser_usb_error_summary { > > + u8 channel, status, txerr, rxerr, error_factor; > > +}; > > + > > struct kvaser_usb_tx_urb_context { > > struct kvaser_usb_net_priv *priv; > > u32 echo_index; > > @@ -615,6 +619,57 @@ static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv) > > priv->tx_contexts[i].echo_index = MAX_TX_URBS; > > } > > > > This function is doing some things that are now be handled by > can_change_state(). You can make your state handling code a lot shorter by > using it. > > > +static void kvaser_usb_rx_error_update_state(struct kvaser_usb_net_priv *priv, > > + const struct kvaser_usb_error_summary *es) > > +{ > > + struct net_device_stats *stats; > > + unsigned int new_state; > > + > > + stats = &priv->netdev->stats; > > + new_state = priv->can.state; > > + > > + netdev_dbg(priv->netdev, "Error status: 0x%02x\n", es->status); > > + > > + if (es->status & M16C_STATE_BUS_OFF) { > can_change_state() updates the state stats. > > + priv->can.can_stats.bus_off++; > > + new_state = CAN_STATE_BUS_OFF; > > + } else if (es->status & M16C_STATE_BUS_PASSIVE) { > > + if (priv->can.state != CAN_STATE_ERROR_PASSIVE) { > See last comment. > > + priv->can.can_stats.error_passive++; > > + } > > + new_state = CAN_STATE_ERROR_PASSIVE; > > + } > > + > > + if (es->status == M16C_STATE_BUS_ERROR) { > > + if ((priv->can.state < CAN_STATE_ERROR_WARNING) && > > + ((es->txerr >= 96) || (es->rxerr >= 96))) { > Same as above. > > + priv->can.can_stats.error_warning++; > > + new_state = CAN_STATE_ERROR_WARNING; > > + } else if (priv->can.state > CAN_STATE_ERROR_ACTIVE) { > > + new_state = CAN_STATE_ERROR_ACTIVE; > > + } > > + } > > + > > + if (!es->status) { > > + new_state = CAN_STATE_ERROR_ACTIVE; > > + } > > + > > + if (priv->can.restart_ms && > > + (priv->can.state >= CAN_STATE_BUS_OFF) && > > + (new_state < CAN_STATE_BUS_OFF)) { > This is NOT in can_change_state(). Keep it. > > + priv->can.can_stats.restarts++; > > + } > > + > > + if (es->error_factor) { > > + priv->can.can_stats.bus_error++; > > + stats->rx_errors++; > > + } > > + > > + priv->bec.txerr = es->txerr; > > + priv->bec.rxerr = es->rxerr; > can_change_state() does this too. > > + priv->can.state = new_state; > > +} > > + > > static void kvaser_usb_rx_error(const struct kvaser_usb *dev, > > const struct kvaser_msg *msg) > ... > > - if (status & M16C_STATE_BUS_RESET) { > > + if (es.status & M16C_STATE_BUS_RESET) { > > kvaser_usb_unlink_tx_urbs(priv); > > return; > > } > > > OK, maybe we need to move "priv->can.state = new_state" out of > can_change_state(). See explanation below. > > + old_state = priv->can.state; > > + kvaser_usb_rx_error_update_state(priv, &es); > > + new_state = priv->can.state; > > + > > skb = alloc_can_err_skb(priv->netdev, &cf); > > if (!skb) { > > stats->rx_dropped++; > > return; > > } > > > You should be able to replace this: > > - new_state = priv->can.state; > > - > > - netdev_dbg(priv->netdev, "Error status: 0x%02x\n", status); > > - > > - if (status & M16C_STATE_BUS_OFF) { > > - cf->can_id |= CAN_ERR_BUSOFF; > > - > > - priv->can.can_stats.bus_off++; > > + if (es.status & M16C_STATE_BUS_OFF) { > > if (!priv->can.restart_ms) > > kvaser_usb_simple_msg_async(priv, CMD_STOP_CHIP); > > - > > netif_carrier_off(priv->netdev); > > > > - new_state = CAN_STATE_BUS_OFF; > > - } else if (status & M16C_STATE_BUS_PASSIVE) { > > - if (priv->can.state != CAN_STATE_ERROR_PASSIVE) { > > + cf->can_id |= CAN_ERR_BUSOFF; > > + } else if (es.status & M16C_STATE_BUS_PASSIVE) { > > + if (old_state != CAN_STATE_ERROR_PASSIVE) { > > cf->can_id |= CAN_ERR_CRTL; > > > > - if (txerr || rxerr) > > - cf->data[1] = (txerr > rxerr) > > + if (es.txerr || es.rxerr) > > + cf->data[1] = (es.txerr > es.rxerr) > > ? CAN_ERR_CRTL_TX_PASSIVE > > : CAN_ERR_CRTL_RX_PASSIVE; > > else > > cf->data[1] = CAN_ERR_CRTL_TX_PASSIVE | > > CAN_ERR_CRTL_RX_PASSIVE; > > - > > - priv->can.can_stats.error_passive++; > > } > > - > > - new_state = CAN_STATE_ERROR_PASSIVE; > > } > > > > - if (status == M16C_STATE_BUS_ERROR) { > > - if ((priv->can.state < CAN_STATE_ERROR_WARNING) && > > - ((txerr >= 96) || (rxerr >= 96))) { > > + if (es.status == M16C_STATE_BUS_ERROR) { > > + if ((old_state < CAN_STATE_ERROR_WARNING) && > > + ((es.txerr >= 96) || (es.rxerr >= 96))) { > > cf->can_id |= CAN_ERR_CRTL; > > - cf->data[1] = (txerr > rxerr) > > + cf->data[1] = (es.txerr > es.rxerr) > > ? CAN_ERR_CRTL_TX_WARNING > > : CAN_ERR_CRTL_RX_WARNING; > > - > > - priv->can.can_stats.error_warning++; > > - new_state = CAN_STATE_ERROR_WARNING; > > - } else if (priv->can.state > CAN_STATE_ERROR_ACTIVE) { > > + } else if (old_state > CAN_STATE_ERROR_ACTIVE) { > > cf->can_id |= CAN_ERR_PROT; > > cf->data[2] = CAN_ERR_PROT_ACTIVE; > > - > > - new_state = CAN_STATE_ERROR_ACTIVE; > > } > With something like: > if(new_state != old_state) > { > tx_state = get_tx_state(...); > rx_state = get_rx_state(...); > can_change_state(priv, cf, tx_state, rx_state); > if(new_state == CAN_STATE_BUS_OFF) > bus_off(...); > } > > } > > > > - if (!status) { > > + if (!es.status) { > > cf->can_id |= CAN_ERR_PROT; > > cf->data[2] = CAN_ERR_PROT_ACTIVE; > > - > > - new_state = CAN_STATE_ERROR_ACTIVE; > > } > > > ... > > I suggest that you create functions named kvaser_get_tx_state and > kvaser_get_rx_state (or one function that returns to argument if that suits you > better) based on kvaser_usb_rx_error_update_state. > > Because you want to assign the netlink state before you do the skb alloc, you > need to make a copy of the netlink state before you change it and reassign the > old state to the netlink structure after the alloc succeeds. This is because > can_change_state() relies on priv->can.state for the "current state". This is a > hack. Perhaps we need to split up can_change_state(). > Thanks a lot Andri for your detailed review! I've included changes, similar in spirit to the above, in the next patch series. Regards, Darwish > See: > http://article.gmane.org/gmane.linux.can/7164 > http://article.gmane.org/gmane.linux.can/7162 > http://article.gmane.org/gmane.linux.can/7163 > http://article.gmane.org/gmane.linux.can/7167 > > Best regards, > Andri Yngvason