| Message ID | 20220612213927.3004444-14-dario.binacchi@amarulasolutions.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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? > + > + 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
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 |
On 14.06.2022 08:29:57, Dario Binacchi wrote: > > > + 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 for the explanation. regards, Marc
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; + + 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; + 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; + + 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); + + 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; }
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(+)