lkml.org 
[lkml]   [2015]   [Jan]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v5 2/5] can: kvaser_usb: Consolidate and unify state change handling
    Date
    Quoting Ahmed S. Darwish (2015-01-20 21:45:37)
    > From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
    >
    > Replace most of the can interface's state and error counters
    > handling with the new can-dev can_change_state() mechanism.
    >
    > Suggested-by: Andri Yngvason <andri.yngvason@marel.com>
    > Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
    > ---
    > drivers/net/can/usb/kvaser_usb.c | 114 +++++++++++++++++++--------------------
    > 1 file changed, 55 insertions(+), 59 deletions(-)
    >
    > diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
    > index 971c5f9..0386d3f 100644
    > --- a/drivers/net/can/usb/kvaser_usb.c
    > +++ b/drivers/net/can/usb/kvaser_usb.c
    > @@ -620,40 +620,43 @@ static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)
    > }
    >
    > static void kvaser_usb_rx_error_update_can_state(struct kvaser_usb_net_priv *priv,
    > - const struct kvaser_usb_error_summary *es)
    > + const struct kvaser_usb_error_summary *es,
    > + struct can_frame *cf)
    > {
    > struct net_device_stats *stats;
    > - enum can_state new_state;
    > -
    > - stats = &priv->netdev->stats;
    > - new_state = priv->can.state;
    > + enum can_state cur_state, new_state, tx_state, rx_state;
    >
    > netdev_dbg(priv->netdev, "Error status: 0x%02x\n", es->status);
    >
    > - if (es->status & M16C_STATE_BUS_OFF) {
    > - priv->can.can_stats.bus_off++;
    > + stats = &priv->netdev->stats;
    > + new_state = cur_state = priv->can.state;
    > +
    > + if (es->status & M16C_STATE_BUS_OFF)
    > new_state = CAN_STATE_BUS_OFF;
    > - } else if (es->status & M16C_STATE_BUS_PASSIVE) {
    > - if (priv->can.state != CAN_STATE_ERROR_PASSIVE)
    > - priv->can.can_stats.error_passive++;
    > + else if (es->status & M16C_STATE_BUS_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))) {
    > - priv->can.can_stats.error_warning++;
    > + if ((cur_state < CAN_STATE_ERROR_WARNING) &&
    > + ((es->txerr >= 96) || (es->rxerr >= 96)))
    > new_state = CAN_STATE_ERROR_WARNING;
    > - } else if (priv->can.state > CAN_STATE_ERROR_ACTIVE) {
    > + else if (cur_state > CAN_STATE_ERROR_ACTIVE)
    > new_state = CAN_STATE_ERROR_ACTIVE;
    > - }
    > }
    >
    > if (!es->status)
    > new_state = CAN_STATE_ERROR_ACTIVE;
    >
    > + if (new_state != cur_state) {
    > + tx_state = (es->txerr >= es->rxerr) ? new_state : 0;
    > + rx_state = (es->txerr <= es->rxerr) ? new_state : 0;
    > +
    > + can_change_state(priv->netdev, cf, tx_state, rx_state);
    > + new_state = priv->can.state;
    > + }
    > +
    > if (priv->can.restart_ms &&
    > - (priv->can.state >= CAN_STATE_BUS_OFF) &&
    > + (cur_state >= CAN_STATE_BUS_OFF) &&
    > (new_state < CAN_STATE_BUS_OFF)) {
    > priv->can.can_stats.restarts++;
    > }
    > @@ -665,18 +668,17 @@ static void kvaser_usb_rx_error_update_can_state(struct kvaser_usb_net_priv *pri
    >
    > priv->bec.txerr = es->txerr;
    > priv->bec.rxerr = es->rxerr;
    > - priv->can.state = new_state;
    > }
    >
    > static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
    > const struct kvaser_msg *msg)
    > {
    > - struct can_frame *cf;
    > + struct can_frame *cf, tmp_cf = { .can_id = CAN_ERR_FLAG, .can_dlc = CAN_ERR_DLC };
    > struct sk_buff *skb;
    > struct net_device_stats *stats;
    > struct kvaser_usb_net_priv *priv;
    > struct kvaser_usb_error_summary es = { };
    > - enum can_state old_state;
    > + enum can_state old_state, new_state;
    >
    > switch (msg->id) {
    > case CMD_CAN_ERROR_EVENT:
    > @@ -721,60 +723,54 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
    > }
    >
    > /* Update all of the can interface's state and error counters before
    > - * trying any skb allocation that can actually fail with -ENOMEM.
    > + * trying any memory allocation that can actually fail with -ENOMEM.
    > + *
    > + * We send a temporary stack-allocated error can frame to
    > + * can_change_state() for the very same reason.
    > + *
    > + * TODO: Split can_change_state() responsibility between updating the
    > + * can interface's state and counters, and the setting up of can error
    > + * frame ID and data to userspace. Remove stack allocation afterwards.
    > */
    > old_state = priv->can.state;
    > - kvaser_usb_rx_error_update_can_state(priv, &es);
    > + kvaser_usb_rx_error_update_can_state(priv, &es, &tmp_cf);
    > + new_state = priv->can.state;
    >
    > skb = alloc_can_err_skb(priv->netdev, &cf);
    > if (!skb) {
    > stats->rx_dropped++;
    > return;
    > }
    > + memcpy(cf, &tmp_cf, sizeof(*cf));
    >
    > - if (es.status & M16C_STATE_BUS_OFF) {
    > - cf->can_id |= CAN_ERR_BUSOFF;
    > -
    > - if (!priv->can.restart_ms)
    > - kvaser_usb_simple_msg_async(priv, CMD_STOP_CHIP);
    > - netif_carrier_off(priv->netdev);
    > - } else if (es.status & M16C_STATE_BUS_PASSIVE) {
    > - if (old_state != CAN_STATE_ERROR_PASSIVE) {
    > - cf->can_id |= CAN_ERR_CRTL;
    > -
    > - 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;
    > + if (new_state != old_state) {
    > + 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);
    > + }
    > +
    > + if (es.status == M16C_STATE_BUS_ERROR) {
    > + if ((old_state >= CAN_STATE_ERROR_WARNING) ||
    > + (es.txerr < 96 && es.rxerr < 96)) {
    > + if (old_state > CAN_STATE_ERROR_ACTIVE) {
    > + cf->can_id |= CAN_ERR_PROT;
    > + cf->data[2] = CAN_ERR_PROT_ACTIVE;
    > + }
    > + }
    > }
    > - }
    >
    > - 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] = (es.txerr > es.rxerr)
    > - ? CAN_ERR_CRTL_TX_WARNING
    > - : CAN_ERR_CRTL_RX_WARNING;
    > - } else if (old_state > CAN_STATE_ERROR_ACTIVE) {
    > + if (!es.status) {
    > cf->can_id |= CAN_ERR_PROT;
    > cf->data[2] = CAN_ERR_PROT_ACTIVE;
    > }
    > - }
    >
    > - if (!es.status) {
    > - cf->can_id |= CAN_ERR_PROT;
    > - cf->data[2] = CAN_ERR_PROT_ACTIVE;
    > - }
    > -
    > - if (priv->can.restart_ms &&
    > - (old_state >= CAN_STATE_BUS_OFF) &&
    > - (priv->can.state < CAN_STATE_BUS_OFF)) {
    > - cf->can_id |= CAN_ERR_RESTARTED;
    > - netif_carrier_on(priv->netdev);
    > + if (priv->can.restart_ms &&
    > + (old_state >= CAN_STATE_BUS_OFF) &&
    > + (new_state < CAN_STATE_BUS_OFF)) {
    > + cf->can_id |= CAN_ERR_RESTARTED;
    > + netif_carrier_on(priv->netdev);
    > + }
    > }
    >
    > if (es.error_factor) {
    > --
    > 1.9.1

    Looks good.

    --
    Andri


    \
     
     \ /
      Last update: 2015-01-21 12:01    [W:2.533 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site