Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220612213927.3004444-1-dario.binacchi@amarulasolutions.com>
 <20220612213927.3004444-14-dario.binacchi@amarulasolutions.com> <20220613073706.rk3bve57zi2p3nnz@pengutronix.de>
In-Reply-To: <20220613073706.rk3bve57zi2p3nnz@pengutronix.de>
From:   Dario Binacchi <dario.binacchi@amarulasolutions.com>
Date:   Tue, 14 Jun 2022 08:29:57 +0200
Message-ID: <CABGWkvqb3VHEMUaRsxcdL0+85hOSwJAtYWq+JskQ3KG+Hnca5g@mail.gmail.com>
Subject: Re: [PATCH v3 13/13] can: slcan: extend the protocol with CAN state info
To:     Marc Kleine-Budde <mkl@pengutronix.de>
Cc:     linux-kernel@vger.kernel.org, michael@amarulasolutions.com,
        Amarula patchwork <linux-amarula@amarulasolutions.com>,
        Oliver Hartkopp <socketcan@hartkopp.net>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jakub Kicinski <kuba@kernel.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
        Vincent Mailhol <mailhol.vincent@wanadoo.fr>,
        Wolfgang Grandegger <wg@grandegger.com>,
        linux-can@vger.kernel.org, netdev@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Jun 13, 2022 at 9:37 AM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 12.06.2022 23:39:27, Dario Binacchi wrote:
> > It extends the protocol to receive the adapter CAN state changes
> > (warning, busoff, etc.) and forward them to the netdev upper levels.
> >
> > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> >
> > ---
> >
> > Changes in v3:
> > - Drop the patch "can: slcan: simplify the device de-allocation".
> > - Add the patch "can: netlink: dump bitrate 0 if can_priv::bittiming.bitrate is -1U".
> >
> > Changes in v2:
> > - Continue error handling even if no skb can be allocated.
> >
> >  drivers/net/can/slcan/slcan-core.c | 66 ++++++++++++++++++++++++++++++
> >  1 file changed, 66 insertions(+)
> >
> > diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c
> > index 48077edb9497..5ba1c141f942 100644
> > --- a/drivers/net/can/slcan/slcan-core.c
> > +++ b/drivers/net/can/slcan/slcan-core.c
> > @@ -78,6 +78,9 @@ MODULE_PARM_DESC(maxdev, "Maximum number of slcan interfaces");
> >  #define SLC_CMD_LEN 1
> >  #define SLC_SFF_ID_LEN 3
> >  #define SLC_EFF_ID_LEN 8
> > +#define SLC_STATE_LEN 1
> > +#define SLC_STATE_BE_RXCNT_LEN 3
> > +#define SLC_STATE_BE_TXCNT_LEN 3
> >
> >  struct slcan {
> >       struct can_priv         can;
> > @@ -175,6 +178,67 @@ int slcan_enable_err_rst_on_open(struct net_device *ndev, bool on)
> >    *                  STANDARD SLCAN DECAPSULATION                     *
> >    ************************************************************************/
> >
> > +static void slc_bump_state(struct slcan *sl)
> > +{
> > +     struct net_device *dev = sl->dev;
> > +     struct sk_buff *skb;
> > +     struct can_frame *cf;
> > +     char *cmd = sl->rbuff;
> > +     u32 rxerr, txerr;
> > +     enum can_state state, rx_state, tx_state;
> > +
> > +     if (*cmd != 's')
> > +             return;
>
> Checked by the caller?
>
> > +
> > +     cmd += SLC_CMD_LEN;
> > +     switch (*cmd) {
> > +     case 'a':
> > +             state = CAN_STATE_ERROR_ACTIVE;
> > +             break;
> > +     case 'w':
> > +             state = CAN_STATE_ERROR_WARNING;
> > +             break;
> > +     case 'p':
> > +             state = CAN_STATE_ERROR_PASSIVE;
> > +             break;
> > +     case 'f':
> > +             state = CAN_STATE_BUS_OFF;
> > +             break;
> > +     default:
> > +             return;
> > +     }
> > +
> > +     if (state == sl->can.state)
> > +             return;
> > +
> > +     cmd += SLC_STATE_BE_RXCNT_LEN + 1;
>
> Have you checked that you have received that much data?
>
> > +     cmd[SLC_STATE_BE_TXCNT_LEN] = 0;
> > +     if (kstrtou32(cmd, 10, &txerr))
> > +             return;
> > +
> > +     *cmd = 0;
> > +     cmd -= SLC_STATE_BE_RXCNT_LEN;
> > +     if (kstrtou32(cmd, 10, &rxerr))
> > +             return;
>
> Why do you parse TX first and then RX?

Since adding the end-of-string character to the counter to be decoded
invalidates the next one.
If I had started from the rx counter, I would have found the
transmission counter always at 0.

Thanks and regards,
Dario

>
> > +
> > +     skb = alloc_can_err_skb(dev, &cf);
> > +
> > +     if (skb) {
> > +             cf->data[6] = txerr;
> > +             cf->data[7] = rxerr;
> > +     }
> > +
> > +     tx_state = txerr >= rxerr ? state : 0;
> > +     rx_state = txerr <= rxerr ? state : 0;
> > +     can_change_state(dev, skb ? cf : NULL, tx_state, rx_state);
>
> alloc_can_err_skb() set cf to NULL if no skb can be allocated.
>
> > +
> > +     if (state == CAN_STATE_BUS_OFF)
> > +             can_bus_off(dev);
> > +
> > +     if (skb)
> > +             netif_rx(skb);
> > +}
> > +
> >  static void slc_bump_err(struct slcan *sl)
> >  {
> >       struct net_device *dev = sl->dev;
> > @@ -378,6 +442,8 @@ static void slc_bump(struct slcan *sl)
> >               return slc_bump_frame(sl);
> >       case 'e':
> >               return slc_bump_err(sl);
> > +     case 's':
> > +             return slc_bump_state(sl);
> >       default:
> >               return;
> >       }
> > --
> > 2.32.0
> >
> >
>
> Marc
>
> --
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   | https://www.pengutronix.de  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


-- 

Dario Binacchi

Embedded Linux Developer

dario.binacchi@amarulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

T. +39 042 243 5310
info@amarulasolutions.com

www.amarulasolutions.com