| Message ID | 20220612213927.3004444-13-dario.binacchi@amarulasolutions.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
On 12.06.2022 23:39:26, Dario Binacchi wrote: > It extends the protocol to receive the adapter CAN communication errors > and forward them to the netdev upper levels. > > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> > > --- > > (no changes since v2) > > Changes in v2: > - Protect decoding against the case the len value is longer than the > received data. Where is that check? > - Continue error handling even if no skb can be allocated. > > drivers/net/can/slcan/slcan-core.c | 130 ++++++++++++++++++++++++++++- > 1 file changed, 129 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c > index 3df35ae8f040..48077edb9497 100644 > --- a/drivers/net/can/slcan/slcan-core.c > +++ b/drivers/net/can/slcan/slcan-core.c > @@ -175,8 +175,118 @@ int slcan_enable_err_rst_on_open(struct net_device *ndev, bool on) > * STANDARD SLCAN DECAPSULATION * > ************************************************************************/ > > +static void slc_bump_err(struct slcan *sl) > +{ > + struct net_device *dev = sl->dev; > + struct sk_buff *skb; > + struct can_frame *cf; > + char *cmd = sl->rbuff; > + bool rx_errors = false, tx_errors = false; > + int i, len; > + > + if (*cmd != 'e') > + return; This has already been checked in the caller, right? > + > + cmd += SLC_CMD_LEN; > + /* get len from sanitized ASCII value */ > + len = *cmd++; > + if (len >= '0' && len < '9') > + len -= '0'; > + else > + return; > + > + skb = alloc_can_err_skb(dev, &cf); > + > + if (skb) > + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; > + > + for (i = 0; i < len; i++, cmd++) { > + switch (*cmd) { > + case 'a': > + netdev_dbg(dev, "ACK error\n"); > + tx_errors = true; Nitpick: Please decide if you want to set tx/tx_errors here and increment at the end of the function....or..... > + if (skb) { > + cf->can_id |= CAN_ERR_ACK; > + cf->data[3] = CAN_ERR_PROT_LOC_ACK; > + } > + > + break; > + case 'b': > + netdev_dbg(dev, "Bit0 error\n"); > + tx_errors = true; > + if (skb) > + cf->data[2] |= CAN_ERR_PROT_BIT0; > + > + break; > + case 'B': > + netdev_dbg(dev, "Bit1 error\n"); > + tx_errors = true; > + if (skb) > + cf->data[2] |= CAN_ERR_PROT_BIT1; > + > + break; > + case 'c': > + netdev_dbg(dev, "CRC error\n"); > + rx_errors = true; > + if (skb) { > + cf->data[2] |= CAN_ERR_PROT_BIT; > + cf->data[3] = CAN_ERR_PROT_LOC_CRC_SEQ; > + } > + > + break; > + case 'f': > + netdev_dbg(dev, "Form Error\n"); > + rx_errors = true; > + if (skb) > + cf->data[2] |= CAN_ERR_PROT_FORM; > + > + break; > + case 'o': > + netdev_dbg(dev, "Rx overrun error\n"); > + dev->stats.rx_over_errors++; > + dev->stats.rx_errors++; ....if you want to increment in the case. > + if (skb) { > + cf->can_id |= CAN_ERR_CRTL; > + cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW; > + } > + > + break; > + case 'O': > + netdev_dbg(dev, "Tx overrun error\n"); > + dev->stats.tx_errors++; > + if (skb) { > + cf->can_id |= CAN_ERR_CRTL; > + cf->data[1] = CAN_ERR_CRTL_TX_OVERFLOW; > + } > + > + break; > + case 's': > + netdev_dbg(dev, "Stuff error\n"); > + rx_errors = true; > + if (skb) > + cf->data[2] |= CAN_ERR_PROT_STUFF; > + > + break; > + default: > + if (skb) > + dev_kfree_skb(skb); > + > + return; > + } > + } > + > + if (rx_errors) > + dev->stats.rx_errors++; > + > + if (tx_errors) > + dev->stats.tx_errors++; > + > + if (skb) > + netif_rx(skb); > +} > + > /* Send one completely decapsulated can_frame to the network layer */ > -static void slc_bump(struct slcan *sl) > +static void slc_bump_frame(struct slcan *sl) > { > struct sk_buff *skb; > struct can_frame *cf; > @@ -255,6 +365,24 @@ static void slc_bump(struct slcan *sl) > dev_kfree_skb(skb); > } > > +static void slc_bump(struct slcan *sl) > +{ > + switch (sl->rbuff[0]) { > + case 'r': > + fallthrough; > + case 't': > + fallthrough; > + case 'R': > + fallthrough; > + case 'T': > + return slc_bump_frame(sl); > + case 'e': > + return slc_bump_err(sl); > + default: > + return; > + } > +} > + > /* parse tty input stream */ > static void slcan_unesc(struct slcan *sl, unsigned char s) > { > -- > 2.32.0 > > Marc
On Mon, Jun 13, 2022 at 9:32 AM Marc Kleine-Budde <mkl@pengutronix.de> wrote: > > On 12.06.2022 23:39:26, Dario Binacchi wrote: > > It extends the protocol to receive the adapter CAN communication errors > > and forward them to the netdev upper levels. > > > > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> > > > > --- > > > > (no changes since v2) > > > > Changes in v2: > > - Protect decoding against the case the len value is longer than the > > received data. > > Where is that check? I added the default case in the switch statement. Each line that is processed by slc_bump_err() is terminated by a '\r' or '\a' character. If len value is longer than the received characters, it will enter the default case for the eol character and the function will return. But I realize now that I am wrong, the terminator is not placed in the buffer passed to slc_bump_err() !!!!!! :) I will add the right check in v4. Thanks and regards, Dario > > > - Continue error handling even if no skb can be allocated. > > > > drivers/net/can/slcan/slcan-core.c | 130 ++++++++++++++++++++++++++++- > > 1 file changed, 129 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c > > index 3df35ae8f040..48077edb9497 100644 > > --- a/drivers/net/can/slcan/slcan-core.c > > +++ b/drivers/net/can/slcan/slcan-core.c > > @@ -175,8 +175,118 @@ int slcan_enable_err_rst_on_open(struct net_device *ndev, bool on) > > * STANDARD SLCAN DECAPSULATION * > > ************************************************************************/ > > > > +static void slc_bump_err(struct slcan *sl) > > +{ > > + struct net_device *dev = sl->dev; > > + struct sk_buff *skb; > > + struct can_frame *cf; > > + char *cmd = sl->rbuff; > > + bool rx_errors = false, tx_errors = false; > > + int i, len; > > + > > + if (*cmd != 'e') > > + return; > > This has already been checked in the caller, right? > > > + > > + cmd += SLC_CMD_LEN; > > + /* get len from sanitized ASCII value */ > > + len = *cmd++; > > + if (len >= '0' && len < '9') > > + len -= '0'; > > + else > > + return; > > + > > + skb = alloc_can_err_skb(dev, &cf); > > + > > + if (skb) > > + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; > > + > > + for (i = 0; i < len; i++, cmd++) { > > + switch (*cmd) { > > + case 'a': > > + netdev_dbg(dev, "ACK error\n"); > > + tx_errors = true; > > Nitpick: > Please decide if you want to set tx/tx_errors here and increment at the > end of the function....or..... > > > + if (skb) { > > + cf->can_id |= CAN_ERR_ACK; > > + cf->data[3] = CAN_ERR_PROT_LOC_ACK; > > + } > > + > > + break; > > + case 'b': > > + netdev_dbg(dev, "Bit0 error\n"); > > + tx_errors = true; > > + if (skb) > > + cf->data[2] |= CAN_ERR_PROT_BIT0; > > + > > + break; > > + case 'B': > > + netdev_dbg(dev, "Bit1 error\n"); > > + tx_errors = true; > > + if (skb) > > + cf->data[2] |= CAN_ERR_PROT_BIT1; > > + > > + break; > > + case 'c': > > + netdev_dbg(dev, "CRC error\n"); > > + rx_errors = true; > > + if (skb) { > > + cf->data[2] |= CAN_ERR_PROT_BIT; > > + cf->data[3] = CAN_ERR_PROT_LOC_CRC_SEQ; > > + } > > + > > + break; > > + case 'f': > > + netdev_dbg(dev, "Form Error\n"); > > + rx_errors = true; > > + if (skb) > > + cf->data[2] |= CAN_ERR_PROT_FORM; > > + > > + break; > > + case 'o': > > + netdev_dbg(dev, "Rx overrun error\n"); > > + dev->stats.rx_over_errors++; > > + dev->stats.rx_errors++; > > ....if you want to increment in the case. > > > + if (skb) { > > + cf->can_id |= CAN_ERR_CRTL; > > + cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW; > > + } > > + > > + break; > > + case 'O': > > + netdev_dbg(dev, "Tx overrun error\n"); > > + dev->stats.tx_errors++; > > + if (skb) { > > + cf->can_id |= CAN_ERR_CRTL; > > + cf->data[1] = CAN_ERR_CRTL_TX_OVERFLOW; > > + } > > + > > + break; > > + case 's': > > + netdev_dbg(dev, "Stuff error\n"); > > + rx_errors = true; > > + if (skb) > > + cf->data[2] |= CAN_ERR_PROT_STUFF; > > + > > + break; > > + default: > > + if (skb) > > + dev_kfree_skb(skb); > > + > > + return; > > + } > > + } > > + > > + if (rx_errors) > > + dev->stats.rx_errors++; > > + > > + if (tx_errors) > > + dev->stats.tx_errors++; > > + > > + if (skb) > > + netif_rx(skb); > > +} > > + > > /* Send one completely decapsulated can_frame to the network layer */ > > -static void slc_bump(struct slcan *sl) > > +static void slc_bump_frame(struct slcan *sl) > > { > > struct sk_buff *skb; > > struct can_frame *cf; > > @@ -255,6 +365,24 @@ static void slc_bump(struct slcan *sl) > > dev_kfree_skb(skb); > > } > > > > +static void slc_bump(struct slcan *sl) > > +{ > > + switch (sl->rbuff[0]) { > > + case 'r': > > + fallthrough; > > + case 't': > > + fallthrough; > > + case 'R': > > + fallthrough; > > + case 'T': > > + return slc_bump_frame(sl); > > + case 'e': > > + return slc_bump_err(sl); > > + default: > > + return; > > + } > > +} > > + > > /* parse tty input stream */ > > static void slcan_unesc(struct slcan *sl, unsigned char s) > > { > > -- > > 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 |
diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c index 3df35ae8f040..48077edb9497 100644 --- a/drivers/net/can/slcan/slcan-core.c +++ b/drivers/net/can/slcan/slcan-core.c @@ -175,8 +175,118 @@ int slcan_enable_err_rst_on_open(struct net_device *ndev, bool on) * STANDARD SLCAN DECAPSULATION * ************************************************************************/ +static void slc_bump_err(struct slcan *sl) +{ + struct net_device *dev = sl->dev; + struct sk_buff *skb; + struct can_frame *cf; + char *cmd = sl->rbuff; + bool rx_errors = false, tx_errors = false; + int i, len; + + if (*cmd != 'e') + return; + + cmd += SLC_CMD_LEN; + /* get len from sanitized ASCII value */ + len = *cmd++; + if (len >= '0' && len < '9') + len -= '0'; + else + return; + + skb = alloc_can_err_skb(dev, &cf); + + if (skb) + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; + + for (i = 0; i < len; i++, cmd++) { + switch (*cmd) { + case 'a': + netdev_dbg(dev, "ACK error\n"); + tx_errors = true; + if (skb) { + cf->can_id |= CAN_ERR_ACK; + cf->data[3] = CAN_ERR_PROT_LOC_ACK; + } + + break; + case 'b': + netdev_dbg(dev, "Bit0 error\n"); + tx_errors = true; + if (skb) + cf->data[2] |= CAN_ERR_PROT_BIT0; + + break; + case 'B': + netdev_dbg(dev, "Bit1 error\n"); + tx_errors = true; + if (skb) + cf->data[2] |= CAN_ERR_PROT_BIT1; + + break; + case 'c': + netdev_dbg(dev, "CRC error\n"); + rx_errors = true; + if (skb) { + cf->data[2] |= CAN_ERR_PROT_BIT; + cf->data[3] = CAN_ERR_PROT_LOC_CRC_SEQ; + } + + break; + case 'f': + netdev_dbg(dev, "Form Error\n"); + rx_errors = true; + if (skb) + cf->data[2] |= CAN_ERR_PROT_FORM; + + break; + case 'o': + netdev_dbg(dev, "Rx overrun error\n"); + dev->stats.rx_over_errors++; + dev->stats.rx_errors++; + if (skb) { + cf->can_id |= CAN_ERR_CRTL; + cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW; + } + + break; + case 'O': + netdev_dbg(dev, "Tx overrun error\n"); + dev->stats.tx_errors++; + if (skb) { + cf->can_id |= CAN_ERR_CRTL; + cf->data[1] = CAN_ERR_CRTL_TX_OVERFLOW; + } + + break; + case 's': + netdev_dbg(dev, "Stuff error\n"); + rx_errors = true; + if (skb) + cf->data[2] |= CAN_ERR_PROT_STUFF; + + break; + default: + if (skb) + dev_kfree_skb(skb); + + return; + } + } + + if (rx_errors) + dev->stats.rx_errors++; + + if (tx_errors) + dev->stats.tx_errors++; + + if (skb) + netif_rx(skb); +} + /* Send one completely decapsulated can_frame to the network layer */ -static void slc_bump(struct slcan *sl) +static void slc_bump_frame(struct slcan *sl) { struct sk_buff *skb; struct can_frame *cf; @@ -255,6 +365,24 @@ static void slc_bump(struct slcan *sl) dev_kfree_skb(skb); } +static void slc_bump(struct slcan *sl) +{ + switch (sl->rbuff[0]) { + case 'r': + fallthrough; + case 't': + fallthrough; + case 'R': + fallthrough; + case 'T': + return slc_bump_frame(sl); + case 'e': + return slc_bump_err(sl); + default: + return; + } +} + /* parse tty input stream */ static void slcan_unesc(struct slcan *sl, unsigned char s) {
It extends the protocol to receive the adapter CAN communication errors and forward them to the netdev upper levels. Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> --- (no changes since v2) Changes in v2: - Protect decoding against the case the len value is longer than the received data. - Continue error handling even if no skb can be allocated. drivers/net/can/slcan/slcan-core.c | 130 ++++++++++++++++++++++++++++- 1 file changed, 129 insertions(+), 1 deletion(-)