This series originated as a result of CAN communication tests for an
application using the USBtin adapter (https://www.fischl.de/usbtin/).
The tests showed some errors but for the driver everything was ok.
Also, being the first time I used the slcan driver, I was amazed that
it was not possible to configure the bitrate via the ip tool.
For these two reasons, I started looking at the driver code and realized
that it didn't use the CAN network device driver interface.
Starting from these assumptions, I tried to:
- Use the CAN network device driver interface.
- Set the bitrate via the ip tool.
- Send the open/close command to the adapter from the driver.
- Add ethtool support to reset the adapter errors.
- Extend the protocol to forward the adapter CAN communication
errors and the CAN state changes to the netdev upper layers.
Except for the protocol extension patches (i. e. forward the adapter CAN
communication errors and the CAN state changes to the netdev upper
layers), the whole series has been tested. Testing the extension
protocol patches requires updating the adapter firmware. Before modifying
the firmware I think it makes sense to know if these extensions can be
considered useful.
Before applying the series I used these commands:
slcan_attach -f -s6 -o /dev/ttyACM0
slcand ttyACM0 can0
ip link set can0 up
After applying the series I am using these commands:
slcan_attach /dev/ttyACM0
slcand ttyACM0 can0
ip link set dev can0 down
ip link set can0 type can bitrate 500000
ethtool --set-priv-flags can0 err-rst-on-open on
ip link set dev can0 up
Now there is a clearer separation between serial line and CAN,
but above all, it is possible to use the ip and ethtool commands
as it happens for any CAN device driver. The changes are backward
compatible, you can continue to use the slcand and slcan_attach
command options.
Dario Binacchi (13):
can: slcan: use the BIT() helper
can: slcan: use netdev helpers to print out messages
can: slcan: use the alloc_can_skb() helper
can: slcan: use CAN network device driver API
can: slcan: simplify the device de-allocation
can: slcan: allow to send commands to the adapter
can: slcan: set bitrate by CAN device driver API
can: slcan: send the open command to the adapter
can: slcan: send the close command to the adapter
can: slcan: move driver into separate sub directory
can: slcan: add ethtool support to reset adapter errors
can: slcan: extend the protocol with error info
can: slcan: extend the protocol with CAN state info
drivers/net/can/Makefile | 2 +-
drivers/net/can/slcan/Makefile | 7 +
.../net/can/{slcan.c => slcan/slcan-core.c} | 464 +++++++++++++++---
drivers/net/can/slcan/slcan-ethtool.c | 65 +++
drivers/net/can/slcan/slcan.h | 18 +
5 files changed, 480 insertions(+), 76 deletions(-)
create mode 100644 drivers/net/can/slcan/Makefile
rename drivers/net/can/{slcan.c => slcan/slcan-core.c} (67%)
create mode 100644 drivers/net/can/slcan/slcan-ethtool.c
create mode 100644 drivers/net/can/slcan/slcan.h
--
2.32.0
On Tue. 7 juin 2022 at 18:47, Dario Binacchi
<[email protected]> wrote:
> This series originated as a result of CAN communication tests for an
> application using the USBtin adapter (https://www.fischl.de/usbtin/).
> The tests showed some errors but for the driver everything was ok.
> Also, being the first time I used the slcan driver, I was amazed that
> it was not possible to configure the bitrate via the ip tool.
> For these two reasons, I started looking at the driver code and realized
> that it didn't use the CAN network device driver interface.
That's funny! Yesterday, I sent this comment:
https://lore.kernel.org/linux-can/CAMZ6RqKZwC_OKcgH+WPacY6kbNbj4xR2Gdg2NQtm5Ka5Hfw79A@mail.gmail.com/
And today, you send a full series to remove all the dust from the
slcan driver. Do I have some kind of mystical power to summon people
on the mailing list?
> Starting from these assumptions, I tried to:
> - Use the CAN network device driver interface.
In order to use the CAN network device driver, a.k.a. can-dev module,
drivers/net/can/Kbuild has to be adjusted: move slcan inside CAN_DEV
scope.
@Mark: because I will have to send a new version for my can-dev/Kbuild
cleanup, maybe I can take that change and add it to my series?
> - Set the bitrate via the ip tool.
> - Send the open/close command to the adapter from the driver.
> - Add ethtool support to reset the adapter errors.
> - Extend the protocol to forward the adapter CAN communication
> errors and the CAN state changes to the netdev upper layers.
>
> Except for the protocol extension patches (i. e. forward the adapter CAN
> communication errors and the CAN state changes to the netdev upper
> layers), the whole series has been tested. Testing the extension
> protocol patches requires updating the adapter firmware. Before modifying
> the firmware I think it makes sense to know if these extensions can be
> considered useful.
>
> Before applying the series I used these commands:
>
> slcan_attach -f -s6 -o /dev/ttyACM0
> slcand ttyACM0 can0
> ip link set can0 up
>
> After applying the series I am using these commands:
>
> slcan_attach /dev/ttyACM0
> slcand ttyACM0 can0
> ip link set dev can0 down
> ip link set can0 type can bitrate 500000
> ethtool --set-priv-flags can0 err-rst-on-open on
> ip link set dev can0 up
>
> Now there is a clearer separation between serial line and CAN,
> but above all, it is possible to use the ip and ethtool commands
> as it happens for any CAN device driver. The changes are backward
> compatible, you can continue to use the slcand and slcan_attach
> command options.
>
>
>
> Dario Binacchi (13):
> can: slcan: use the BIT() helper
> can: slcan: use netdev helpers to print out messages
> can: slcan: use the alloc_can_skb() helper
> can: slcan: use CAN network device driver API
> can: slcan: simplify the device de-allocation
> can: slcan: allow to send commands to the adapter
> can: slcan: set bitrate by CAN device driver API
> can: slcan: send the open command to the adapter
> can: slcan: send the close command to the adapter
> can: slcan: move driver into separate sub directory
> can: slcan: add ethtool support to reset adapter errors
> can: slcan: extend the protocol with error info
> can: slcan: extend the protocol with CAN state info
>
> drivers/net/can/Makefile | 2 +-
> drivers/net/can/slcan/Makefile | 7 +
> .../net/can/{slcan.c => slcan/slcan-core.c} | 464 +++++++++++++++---
> drivers/net/can/slcan/slcan-ethtool.c | 65 +++
> drivers/net/can/slcan/slcan.h | 18 +
> 5 files changed, 480 insertions(+), 76 deletions(-)
> create mode 100644 drivers/net/can/slcan/Makefile
> rename drivers/net/can/{slcan.c => slcan/slcan-core.c} (67%)
> create mode 100644 drivers/net/can/slcan/slcan-ethtool.c
> create mode 100644 drivers/net/can/slcan/slcan.h
Since slcan_devs array contains the addresses of the created devices, I
think it is more natural to use its address to remove it from the list.
It is not necessary to store the index of the array that points to the
device in the driver's private data.
Signed-off-by: Dario Binacchi <[email protected]>
---
drivers/net/can/slcan.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index 956b47bd40a7..4df0455e11a2 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -428,11 +428,17 @@ static int slc_open(struct net_device *dev)
static void slc_dealloc(struct slcan *sl)
{
- int i = sl->dev->base_addr;
+ unsigned int i;
- free_candev(sl->dev);
- if (slcan_devs)
- slcan_devs[i] = NULL;
+ for (i = 0; i < maxdev; i++) {
+ if (sl->dev == slcan_devs[i]) {
+ free_candev(sl->dev);
+ slcan_devs[i] = NULL;
+ return;
+ }
+ }
+
+ pr_err("slcan: can't free %s resources\n", sl->dev->name);
}
static int slcan_change_mtu(struct net_device *dev, int new_mtu)
@@ -529,7 +535,6 @@ static struct slcan *slc_alloc(void)
snprintf(dev->name, sizeof(dev->name), "slcan%d", i);
dev->netdev_ops = &slc_netdev_ops;
- dev->base_addr = i;
sl = netdev_priv(dev);
/* Initialize channel control data */
--
2.32.0
This patch moves the slcan driver into a separate directory, a later
patch will add more files.
Signed-off-by: Dario Binacchi <[email protected]>
---
drivers/net/can/Makefile | 2 +-
drivers/net/can/slcan/Makefile | 6 ++++++
drivers/net/can/{slcan.c => slcan/slcan-core.c} | 0
3 files changed, 7 insertions(+), 1 deletion(-)
create mode 100644 drivers/net/can/slcan/Makefile
rename drivers/net/can/{slcan.c => slcan/slcan-core.c} (100%)
diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
index 0af85983634c..210354df273c 100644
--- a/drivers/net/can/Makefile
+++ b/drivers/net/can/Makefile
@@ -5,7 +5,7 @@
obj-$(CONFIG_CAN_VCAN) += vcan.o
obj-$(CONFIG_CAN_VXCAN) += vxcan.o
-obj-$(CONFIG_CAN_SLCAN) += slcan.o
+obj-$(CONFIG_CAN_SLCAN) += slcan/
obj-y += dev/
obj-y += rcar/
diff --git a/drivers/net/can/slcan/Makefile b/drivers/net/can/slcan/Makefile
new file mode 100644
index 000000000000..2e84f7bf7617
--- /dev/null
+++ b/drivers/net/can/slcan/Makefile
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_CAN_SLCAN) += slcan.o
+
+slcan-objs :=
+slcan-objs += slcan-core.o
diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan/slcan-core.c
similarity index 100%
rename from drivers/net/can/slcan.c
rename to drivers/net/can/slcan/slcan-core.c
--
2.32.0
This is a preparation patch for the upcoming support to change the
bitrate via ip tool, reset the adapter error states via the ethtool API
and, more generally, send commands to the adapter.
Since some commands (e. g. setting the bitrate) will be sent before
calling the open_candev(), the netif_running() will return false and so
a new flag bit (i. e. SLF_XCMD) for serial transmission has to be added.
Signed-off-by: Dario Binacchi <[email protected]>
---
drivers/net/can/slcan.c | 46 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 45 insertions(+), 1 deletion(-)
diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index 4df0455e11a2..dbd4ebdfa024 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -97,6 +97,9 @@ struct slcan {
unsigned long flags; /* Flag values/ mode etc */
#define SLF_INUSE 0 /* Channel in use */
#define SLF_ERROR 1 /* Parity, etc. error */
+#define SLF_XCMD 2 /* Command transmission */
+ wait_queue_head_t xcmd_wait; /* Wait queue for commands */
+ /* transmission */
};
static struct net_device **slcan_devs;
@@ -310,12 +313,22 @@ static void slcan_transmit(struct work_struct *work)
spin_lock_bh(&sl->lock);
/* First make sure we're connected. */
- if (!sl->tty || sl->magic != SLCAN_MAGIC || !netif_running(sl->dev)) {
+ if (!sl->tty || sl->magic != SLCAN_MAGIC ||
+ (unlikely(!netif_running(sl->dev)) &&
+ likely(!test_bit(SLF_XCMD, &sl->flags)))) {
spin_unlock_bh(&sl->lock);
return;
}
if (sl->xleft <= 0) {
+ if (unlikely(test_bit(SLF_XCMD, &sl->flags))) {
+ clear_bit(SLF_XCMD, &sl->flags);
+ clear_bit(TTY_DO_WRITE_WAKEUP, &sl->tty->flags);
+ spin_unlock_bh(&sl->lock);
+ wake_up(&sl->xcmd_wait);
+ return;
+ }
+
/* Now serial buffer is almost free & we can start
* transmission of another packet */
sl->dev->stats.tx_packets++;
@@ -379,6 +392,36 @@ static netdev_tx_t slc_xmit(struct sk_buff *skb, struct net_device *dev)
* Routines looking at netdevice side.
******************************************/
+static int slcan_transmit_cmd(struct slcan *sl, const unsigned char *cmd)
+{
+ int ret, actual, n;
+
+ spin_lock(&sl->lock);
+ if (sl->tty == NULL) {
+ spin_unlock(&sl->lock);
+ return -ENODEV;
+ }
+
+ n = snprintf(sl->xbuff, sizeof(sl->xbuff), "%s", cmd);
+ set_bit(TTY_DO_WRITE_WAKEUP, &sl->tty->flags);
+ actual = sl->tty->ops->write(sl->tty, sl->xbuff, n);
+ sl->xleft = n - actual;
+ sl->xhead = sl->xbuff + actual;
+ set_bit(SLF_XCMD, &sl->flags);
+ spin_unlock(&sl->lock);
+ ret = wait_event_interruptible_timeout(sl->xcmd_wait,
+ !test_bit(SLF_XCMD, &sl->flags),
+ HZ);
+ clear_bit(SLF_XCMD, &sl->flags);
+ if (ret == -ERESTARTSYS)
+ return ret;
+
+ if (ret == 0)
+ return -ETIMEDOUT;
+
+ return 0;
+}
+
/* Netdevice UP -> DOWN routine */
static int slc_close(struct net_device *dev)
{
@@ -542,6 +585,7 @@ static struct slcan *slc_alloc(void)
sl->dev = dev;
spin_lock_init(&sl->lock);
INIT_WORK(&sl->tx_work, slcan_transmit);
+ init_waitqueue_head(&sl->xcmd_wait);
slcan_devs[i] = dev;
return sl;
--
2.32.0
In case the bitrate has been set via ip tool, it sends the open command
("O\r") to the adapter.
Signed-off-by: Dario Binacchi <[email protected]>
---
drivers/net/can/slcan.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index f1bf32b70c4d..f18097c62222 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -469,8 +469,15 @@ static int slc_open(struct net_device *dev)
* can.bittiming.bitrate is 0, causing open_candev() to fail.
* So let's set to a fake value.
*/
- if (sl->can.bittiming.bitrate == 0)
+ if (sl->can.bittiming.bitrate == 0) {
sl->can.bittiming.bitrate = -1UL;
+ } else {
+ err = slcan_transmit_cmd(sl, "O\r");
+ if (err) {
+ netdev_err(dev, "failed to send open command 'O\\r'\n");
+ return err;
+ }
+ }
err = open_candev(dev);
if (err) {
--
2.32.0
It allows to set the bitrate via ip tool, as it happens for the other
CAN device drivers. It still remains possible to set the bitrate via
slcand or slcan_attach utilities. In case the ip tool is used, the
driver will send the serial command to the adapter.
The struct can_bittiming_const and struct can_priv::clock.freq has been
set with empirical values that allow you to get a correct bit timing, so
that the slc_do_set_bittiming() can be called.
Signed-off-by: Dario Binacchi <[email protected]>
---
DTS properties could be used to set the can.clock.freq and the
can.bittiming_const variables. This way the parameters could be changed
based on the type of the adapter.
drivers/net/can/slcan.c | 54 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 53 insertions(+), 1 deletion(-)
diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index dbd4ebdfa024..f1bf32b70c4d 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -105,6 +105,18 @@ struct slcan {
static struct net_device **slcan_devs;
static DEFINE_SPINLOCK(slcan_lock);
+static const struct can_bittiming_const slcan_bittiming_const = {
+ .name = KBUILD_MODNAME,
+ .tseg1_min = 2,
+ .tseg1_max = 256,
+ .tseg2_min = 1,
+ .tseg2_max = 128,
+ .sjw_max = 128,
+ .brp_min = 1,
+ .brp_max = 256,
+ .brp_inc = 1,
+};
+
/************************************************************************
* SLCAN ENCAPSULATION FORMAT *
************************************************************************/
@@ -435,6 +447,7 @@ static int slc_close(struct net_device *dev)
netif_stop_queue(dev);
close_candev(dev);
sl->can.state = CAN_STATE_STOPPED;
+ sl->can.bittiming.bitrate = 0;
sl->rcount = 0;
sl->xleft = 0;
spin_unlock_bh(&sl->lock);
@@ -456,7 +469,9 @@ static int slc_open(struct net_device *dev)
* can.bittiming.bitrate is 0, causing open_candev() to fail.
* So let's set to a fake value.
*/
- sl->can.bittiming.bitrate = -1;
+ if (sl->can.bittiming.bitrate == 0)
+ sl->can.bittiming.bitrate = -1UL;
+
err = open_candev(dev);
if (err) {
netdev_err(dev, "failed to open can device\n");
@@ -554,6 +569,40 @@ static void slc_sync(void)
}
}
+static int slc_do_set_bittiming(struct net_device *dev)
+{
+ struct slcan *sl = netdev_priv(dev);
+ unsigned char cmd[SLC_MTU];
+ int i, s = -1, err;
+ unsigned int bitrates[] = {
+ 10000, 20000, 50000, 100000,
+ 125000, 250000, 500000, 800000,
+ 1000000,
+ };
+
+ for (i = 0; i < ARRAY_SIZE(bitrates); i++) {
+ if (sl->can.bittiming.bitrate == bitrates[i]) {
+ s = i;
+ break;
+ }
+ }
+
+ if (s < 0) {
+ netdev_err(dev, "invalid bitrate\n");
+ return -EINVAL;
+ }
+
+ snprintf(cmd, sizeof(cmd), "C\rS%d\r", s);
+ err = slcan_transmit_cmd(sl, cmd);
+ if (err) {
+ sl->can.bittiming.bitrate = 0;
+ netdev_err(sl->dev,
+ "failed to send bitrate command 'C\\rS%d\\r'\n", s);
+ }
+
+ return err;
+}
+
/* Find a free SLCAN channel, and link in this `tty' line. */
static struct slcan *slc_alloc(void)
{
@@ -583,6 +632,9 @@ static struct slcan *slc_alloc(void)
/* Initialize channel control data */
sl->magic = SLCAN_MAGIC;
sl->dev = dev;
+ sl->can.clock.freq = 24 * 1000 * 1000;
+ sl->can.bittiming_const = &slcan_bittiming_const;
+ sl->can.do_set_bittiming = slc_do_set_bittiming;
spin_lock_init(&sl->lock);
INIT_WORK(&sl->tx_work, slcan_transmit);
init_waitqueue_head(&sl->xcmd_wait);
--
2.32.0
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 <[email protected]>
---
drivers/net/can/slcan/slcan-core.c | 65 ++++++++++++++++++++++++++++++
1 file changed, 65 insertions(+)
diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c
index 02e7c14de45c..ab4c08a7dc81 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;
@@ -182,6 +185,66 @@ 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 (unlikely(!skb))
+ return;
+
+ cf->data[6] = txerr;
+ cf->data[7] = rxerr;
+
+ tx_state = txerr >= rxerr ? state : 0;
+ rx_state = txerr <= rxerr ? state : 0;
+ can_change_state(dev, cf, tx_state, rx_state);
+
+ if (state == CAN_STATE_BUS_OFF)
+ can_bus_off(dev);
+
+ netif_rx(skb);
+}
+
static void slc_bump_err(struct slcan *sl)
{
struct net_device *dev = sl->dev;
@@ -354,6 +417,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
Replace printk() calls with corresponding netdev helpers.
Signed-off-by: Dario Binacchi <[email protected]>
---
drivers/net/can/slcan.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index b37d35c2a23a..6162a9c21672 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -365,7 +365,7 @@ static netdev_tx_t slc_xmit(struct sk_buff *skb, struct net_device *dev)
spin_lock(&sl->lock);
if (!netif_running(dev)) {
spin_unlock(&sl->lock);
- printk(KERN_WARNING "%s: xmit: iface is down\n", dev->name);
+ netdev_warn(dev, "xmit: iface is down\n");
goto out;
}
if (sl->tty == NULL) {
@@ -776,8 +776,7 @@ static void __exit slcan_exit(void)
sl = netdev_priv(dev);
if (sl->tty) {
- printk(KERN_ERR "%s: tty discipline still running\n",
- dev->name);
+ netdev_err(dev, "tty discipline still running\n");
}
unregister_netdev(dev);
--
2.32.0
On 07.06.2022 19:27:05, Vincent MAILHOL wrote:
> On Tue. 7 juin 2022 at 18:47, Dario Binacchi
> <[email protected]> wrote:
> > This series originated as a result of CAN communication tests for an
> > application using the USBtin adapter (https://www.fischl.de/usbtin/).
> > The tests showed some errors but for the driver everything was ok.
> > Also, being the first time I used the slcan driver, I was amazed that
> > it was not possible to configure the bitrate via the ip tool.
> > For these two reasons, I started looking at the driver code and realized
> > that it didn't use the CAN network device driver interface.
>
> That's funny! Yesterday, I sent this comment:
> https://lore.kernel.org/linux-can/CAMZ6RqKZwC_OKcgH+WPacY6kbNbj4xR2Gdg2NQtm5Ka5Hfw79A@mail.gmail.com/
>
> And today, you send a full series to remove all the dust from the
> slcan driver. Do I have some kind of mystical power to summon people
> on the mailing list?
That would be very useful and awesome super power, I'm a bit jealous. :D
> > Starting from these assumptions, I tried to:
> > - Use the CAN network device driver interface.
>
> In order to use the CAN network device driver, a.k.a. can-dev module,
> drivers/net/can/Kbuild has to be adjusted: move slcan inside CAN_DEV
> scope.
>
> @Mark: because I will have to send a new version for my can-dev/Kbuild
> cleanup, maybe I can take that change and add it to my series?
Let's get the your Kconfig/Makefile changes into can-next/master first.
Then Dario can then base this series on that branch.
regards,
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 |
Use the BIT() helper instead of an explicit shift.
Signed-off-by: Dario Binacchi <[email protected]>
---
drivers/net/can/slcan.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index 64a3aee8a7da..b37d35c2a23a 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -413,7 +413,7 @@ static int slc_open(struct net_device *dev)
if (sl->tty == NULL)
return -ENODEV;
- sl->flags &= (1 << SLF_INUSE);
+ sl->flags &= BIT(SLF_INUSE);
netif_start_queue(dev);
return 0;
}
--
2.32.0
It extends the protocol to receive the adapter CAN communication errors
and forward them to the netdev upper levels.
Signed-off-by: Dario Binacchi <[email protected]>
---
drivers/net/can/slcan/slcan-core.c | 104 ++++++++++++++++++++++++++++-
1 file changed, 103 insertions(+), 1 deletion(-)
diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c
index b813a59534a3..02e7c14de45c 100644
--- a/drivers/net/can/slcan/slcan-core.c
+++ b/drivers/net/can/slcan/slcan-core.c
@@ -182,8 +182,92 @@ 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 (unlikely(!skb))
+ return;
+
+ 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");
+ cf->can_id |= CAN_ERR_ACK;
+ cf->data[3] = CAN_ERR_PROT_LOC_ACK;
+ tx_errors = true;
+ break;
+ case 'b':
+ netdev_dbg(dev, "Bit0 error\n");
+ cf->data[2] |= CAN_ERR_PROT_BIT0;
+ tx_errors = true;
+ break;
+ case 'B':
+ netdev_dbg(dev, "Bit1 error\n");
+ cf->data[2] |= CAN_ERR_PROT_BIT1;
+ tx_errors = true;
+ break;
+ case 'c':
+ netdev_dbg(dev, "CRC error\n");
+ cf->data[2] |= CAN_ERR_PROT_BIT;
+ cf->data[3] = CAN_ERR_PROT_LOC_CRC_SEQ;
+ rx_errors = true;
+ break;
+ case 'f':
+ netdev_dbg(dev, "Form Error\n");
+ cf->data[2] |= CAN_ERR_PROT_FORM;
+ rx_errors = true;
+ break;
+ case 'o':
+ netdev_dbg(dev, "Rx overrun error\n");
+ cf->can_id |= CAN_ERR_CRTL;
+ cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
+ dev->stats.rx_over_errors++;
+ dev->stats.rx_errors++;
+ break;
+ case 'O':
+ netdev_dbg(dev, "Tx overrun error\n");
+ cf->can_id |= CAN_ERR_CRTL;
+ cf->data[1] = CAN_ERR_CRTL_TX_OVERFLOW;
+ dev->stats.tx_errors++;
+ break;
+ case 's':
+ netdev_dbg(dev, "Stuff error\n");
+ cf->data[2] |= CAN_ERR_PROT_STUFF;
+ rx_errors = true;
+ break;
+ }
+ }
+
+ if (rx_errors)
+ dev->stats.rx_errors++;
+
+ if (tx_errors)
+ dev->stats.tx_errors++;
+
+ 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, *scf;
@@ -257,6 +341,24 @@ static void slc_bump(struct slcan *sl)
netif_rx(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
This patch adds a private flag to the slcan driver to switch the
"err-rst-on-open" setting on and off.
"err-rst-on-open" on - Reset error states on opening command
"err-rst-on-open" off - Don't reset error states on opening command
(default)
The setting can only be changed if the interface is down:
ip link set dev can0 down
ethtool --set-priv-flags can0 err-rst-on-open {off|on}
ip link set dev can0 up
Signed-off-by: Dario Binacchi <[email protected]>
---
drivers/net/can/slcan/Makefile | 1 +
drivers/net/can/slcan/slcan-core.c | 36 +++++++++++++++
drivers/net/can/slcan/slcan-ethtool.c | 65 +++++++++++++++++++++++++++
drivers/net/can/slcan/slcan.h | 18 ++++++++
4 files changed, 120 insertions(+)
create mode 100644 drivers/net/can/slcan/slcan-ethtool.c
create mode 100644 drivers/net/can/slcan/slcan.h
diff --git a/drivers/net/can/slcan/Makefile b/drivers/net/can/slcan/Makefile
index 2e84f7bf7617..8a88e484ee21 100644
--- a/drivers/net/can/slcan/Makefile
+++ b/drivers/net/can/slcan/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_CAN_SLCAN) += slcan.o
slcan-objs :=
slcan-objs += slcan-core.o
+slcan-objs += slcan-ethtool.o
diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c
index d63d270d21da..b813a59534a3 100644
--- a/drivers/net/can/slcan/slcan-core.c
+++ b/drivers/net/can/slcan/slcan-core.c
@@ -57,6 +57,8 @@
#include <linux/can/dev.h>
#include <linux/can/skb.h>
+#include "slcan.h"
+
MODULE_ALIAS_LDISC(N_SLCAN);
MODULE_DESCRIPTION("serial line CAN interface");
MODULE_LICENSE("GPL");
@@ -98,6 +100,8 @@ struct slcan {
#define SLF_INUSE 0 /* Channel in use */
#define SLF_ERROR 1 /* Parity, etc. error */
#define SLF_XCMD 2 /* Command transmission */
+ unsigned long cmd_flags; /* Command flags */
+#define CF_ERR_RST 0 /* Reset errors on open */
wait_queue_head_t xcmd_wait; /* Wait queue for commands */
/* transmission */
};
@@ -117,6 +121,28 @@ static const struct can_bittiming_const slcan_bittiming_const = {
.brp_inc = 1,
};
+bool slcan_err_rst_on_open(struct net_device *ndev)
+{
+ struct slcan *sl = netdev_priv(ndev);
+
+ return !!test_bit(CF_ERR_RST, &sl->cmd_flags);
+}
+
+int slcan_enable_err_rst_on_open(struct net_device *ndev, bool on)
+{
+ struct slcan *sl = netdev_priv(ndev);
+
+ if (netif_running(ndev))
+ return -EBUSY;
+
+ if (on)
+ set_bit(CF_ERR_RST, &sl->cmd_flags);
+ else
+ clear_bit(CF_ERR_RST, &sl->cmd_flags);
+
+ return 0;
+}
+
/************************************************************************
* SLCAN ENCAPSULATION FORMAT *
************************************************************************/
@@ -483,6 +509,15 @@ static int slc_open(struct net_device *dev)
if (sl->can.bittiming.bitrate == 0) {
sl->can.bittiming.bitrate = -1UL;
} else {
+ if (test_bit(CF_ERR_RST, &sl->cmd_flags)) {
+ err = slcan_transmit_cmd(sl, "F\r");
+ if (err) {
+ netdev_err(sl->dev,
+ "failed to send error command 'F\\r'\n");
+ return err;
+ }
+ }
+
err = slcan_transmit_cmd(sl, "O\r");
if (err) {
netdev_err(dev, "failed to send open command 'O\\r'\n");
@@ -645,6 +680,7 @@ static struct slcan *slc_alloc(void)
snprintf(dev->name, sizeof(dev->name), "slcan%d", i);
dev->netdev_ops = &slc_netdev_ops;
+ slcan_set_ethtool_ops(dev);
sl = netdev_priv(dev);
/* Initialize channel control data */
diff --git a/drivers/net/can/slcan/slcan-ethtool.c b/drivers/net/can/slcan/slcan-ethtool.c
new file mode 100644
index 000000000000..bf0afdc4e49d
--- /dev/null
+++ b/drivers/net/can/slcan/slcan-ethtool.c
@@ -0,0 +1,65 @@
+// SPDX-License-Identifier: GPL-2.0+
+/* Copyright (c) 2022 Amarula Solutions, Dario Binacchi <[email protected]>
+ *
+ */
+
+#include <linux/can/dev.h>
+#include <linux/ethtool.h>
+#include <linux/kernel.h>
+#include <linux/netdevice.h>
+#include <linux/platform_device.h>
+
+#include "slcan.h"
+
+static const char slcan_priv_flags_strings[][ETH_GSTRING_LEN] = {
+#define SLCAN_PRIV_FLAGS_ERR_RST_ON_OPEN BIT(0)
+ "err-rst-on-open",
+};
+
+static void slcan_get_strings(struct net_device *ndev, u32 stringset, u8 *data)
+{
+ switch (stringset) {
+ case ETH_SS_PRIV_FLAGS:
+ memcpy(data, slcan_priv_flags_strings,
+ sizeof(slcan_priv_flags_strings));
+ }
+}
+
+static u32 slcan_get_priv_flags(struct net_device *ndev)
+{
+ u32 flags = 0;
+
+ if (slcan_err_rst_on_open(ndev))
+ flags |= SLCAN_PRIV_FLAGS_ERR_RST_ON_OPEN;
+
+ return flags;
+}
+
+static int slcan_set_priv_flags(struct net_device *ndev, u32 flags)
+{
+ bool err_rst_op_open = !!(flags & SLCAN_PRIV_FLAGS_ERR_RST_ON_OPEN);
+
+ return slcan_enable_err_rst_on_open(ndev, err_rst_op_open);
+}
+
+static int slcan_get_sset_count(struct net_device *netdev, int sset)
+{
+ switch (sset) {
+ case ETH_SS_PRIV_FLAGS:
+ return ARRAY_SIZE(slcan_priv_flags_strings);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static const struct ethtool_ops slcan_ethtool_ops = {
+ .get_strings = slcan_get_strings,
+ .get_priv_flags = slcan_get_priv_flags,
+ .set_priv_flags = slcan_set_priv_flags,
+ .get_sset_count = slcan_get_sset_count,
+};
+
+void slcan_set_ethtool_ops(struct net_device *netdev)
+{
+ netdev->ethtool_ops = &slcan_ethtool_ops;
+}
diff --git a/drivers/net/can/slcan/slcan.h b/drivers/net/can/slcan/slcan.h
new file mode 100644
index 000000000000..d463c8d99e22
--- /dev/null
+++ b/drivers/net/can/slcan/slcan.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0
+ * slcan.h - serial line CAN interface driver
+ *
+ * Copyright (C) Laurence Culhane <[email protected]>
+ * Copyright (C) Fred N. van Kempen <[email protected]>
+ * Copyright (C) Oliver Hartkopp <[email protected]>
+ * Copyright (C) 2022 Amarula Solutions, Dario Binacchi <[email protected]>
+ *
+ */
+
+#ifndef _SLCAN_H
+#define _SLCAN_H
+
+bool slcan_err_rst_on_open(struct net_device *ndev);
+int slcan_enable_err_rst_on_open(struct net_device *ndev, bool on);
+void slcan_set_ethtool_ops(struct net_device *ndev);
+
+#endif /* _SLCAN_H */
--
2.32.0
On 07.06.2022 11:47:52, 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 <[email protected]>
>
> ---
>
> drivers/net/can/slcan/slcan-core.c | 65 ++++++++++++++++++++++++++++++
> 1 file changed, 65 insertions(+)
>
> diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c
> index 02e7c14de45c..ab4c08a7dc81 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;
> @@ -182,6 +185,66 @@ 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 (unlikely(!skb))
> + return;
Please continue error handling, even if no skb can be allocated.
> +
> + cf->data[6] = txerr;
> + cf->data[7] = rxerr;
> +
> + tx_state = txerr >= rxerr ? state : 0;
> + rx_state = txerr <= rxerr ? state : 0;
> + can_change_state(dev, cf, tx_state, rx_state);
> +
> + if (state == CAN_STATE_BUS_OFF)
> + can_bus_off(dev);
> +
> + netif_rx(skb);
> +}
> +
> static void slc_bump_err(struct slcan *sl)
> {
> struct net_device *dev = sl->dev;
> @@ -354,6 +417,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;
> }
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 07.06.2022 11:47:50, Dario Binacchi wrote:
> This patch adds a private flag to the slcan driver to switch the
> "err-rst-on-open" setting on and off.
>
> "err-rst-on-open" on - Reset error states on opening command
>
> "err-rst-on-open" off - Don't reset error states on opening command
> (default)
>
> The setting can only be changed if the interface is down:
>
> ip link set dev can0 down
> ethtool --set-priv-flags can0 err-rst-on-open {off|on}
> ip link set dev can0 up
>
> Signed-off-by: Dario Binacchi <[email protected]>
I'm a big fan of bringing the device into a well known good state during
ifup. What would be the reasons/use cases to not reset the device?
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 |
In case the bitrate has been set via ip tool, it sends the close command
("C\r") to the adapter.
Signed-off-by: Dario Binacchi <[email protected]>
---
drivers/net/can/slcan.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index f18097c62222..d63d270d21da 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -438,9 +438,20 @@ static int slcan_transmit_cmd(struct slcan *sl, const unsigned char *cmd)
static int slc_close(struct net_device *dev)
{
struct slcan *sl = netdev_priv(dev);
+ int err;
spin_lock_bh(&sl->lock);
if (sl->tty) {
+ if (sl->can.bittiming.bitrate &&
+ sl->can.bittiming.bitrate != -1) {
+ spin_unlock_bh(&sl->lock);
+ err = slcan_transmit_cmd(sl, "C\r");
+ spin_lock_bh(&sl->lock);
+ if (err)
+ netdev_warn(dev,
+ "failed to send close command 'C\\r'\n");
+ }
+
/* TTY discipline is running. */
clear_bit(TTY_DO_WRITE_WAKEUP, &sl->tty->flags);
}
--
2.32.0
+CC: Max Staudt <[email protected]>
On Tue. 7 Jun. 2022 at 18:47, Dario Binacchi
<[email protected]> wrote:
> This series originated as a result of CAN communication tests for an
> application using the USBtin adapter (https://www.fischl.de/usbtin/).
> The tests showed some errors but for the driver everything was ok.
> Also, being the first time I used the slcan driver, I was amazed that
> it was not possible to configure the bitrate via the ip tool.
> For these two reasons, I started looking at the driver code and realized
> that it didn't use the CAN network device driver interface.
>
> Starting from these assumptions, I tried to:
> - Use the CAN network device driver interface.
> - Set the bitrate via the ip tool.
> - Send the open/close command to the adapter from the driver.
> - Add ethtool support to reset the adapter errors.
> - Extend the protocol to forward the adapter CAN communication
> errors and the CAN state changes to the netdev upper layers.
>
> Except for the protocol extension patches (i. e. forward the adapter CAN
> communication errors and the CAN state changes to the netdev upper
> layers), the whole series has been tested. Testing the extension
> protocol patches requires updating the adapter firmware. Before modifying
> the firmware I think it makes sense to know if these extensions can be
> considered useful.
>
> Before applying the series I used these commands:
>
> slcan_attach -f -s6 -o /dev/ttyACM0
> slcand ttyACM0 can0
> ip link set can0 up
>
> After applying the series I am using these commands:
>
> slcan_attach /dev/ttyACM0
> slcand ttyACM0 can0
> ip link set dev can0 down
> ip link set can0 type can bitrate 500000
> ethtool --set-priv-flags can0 err-rst-on-open on
> ip link set dev can0 up
In his CAN327 driver, Max manages to bring the can0 device without the
need of dedicated user space daemon by using line discipline
(ldattach):
https://lore.kernel.org/linux-can/[email protected]/
Isn't the same feasible with slcan so that we completely remove the
dependency toward slcand?
Max what do you think of this?
> Now there is a clearer separation between serial line and CAN,
> but above all, it is possible to use the ip and ethtool commands
> as it happens for any CAN device driver. The changes are backward
> compatible, you can continue to use the slcand and slcan_attach
> command options.
Yours sincerely,
Vincent Mailhol
On Tue. 7 Jun 2022 at 19:39, Marc Kleine-Budde <[email protected]> wrote:
> On 07.06.2022 19:27:05, Vincent MAILHOL wrote:
> > On Tue. 7 juin 2022 at 18:47, Dario Binacchi
> > <[email protected]> wrote:
> > > This series originated as a result of CAN communication tests for an
> > > application using the USBtin adapter (https://www.fischl.de/usbtin/).
> > > The tests showed some errors but for the driver everything was ok.
> > > Also, being the first time I used the slcan driver, I was amazed that
> > > it was not possible to configure the bitrate via the ip tool.
> > > For these two reasons, I started looking at the driver code and realized
> > > that it didn't use the CAN network device driver interface.
> >
> > That's funny! Yesterday, I sent this comment:
> > https://lore.kernel.org/linux-can/CAMZ6RqKZwC_OKcgH+WPacY6kbNbj4xR2Gdg2NQtm5Ka5Hfw79A@mail.gmail.com/
> >
> > And today, you send a full series to remove all the dust from the
> > slcan driver. Do I have some kind of mystical power to summon people
> > on the mailing list?
>
> That would be very useful and awesome super power, I'm a bit jealous. :D
>
> > > Starting from these assumptions, I tried to:
> > > - Use the CAN network device driver interface.
> >
> > In order to use the CAN network device driver, a.k.a. can-dev module,
> > drivers/net/can/Kbuild has to be adjusted: move slcan inside CAN_DEV
> > scope.
> >
> > @Mark: because I will have to send a new version for my can-dev/Kbuild
> > cleanup, maybe I can take that change and add it to my series?
>
> Let's get the your Kconfig/Makefile changes into can-next/master first.
> Then Dario can then base this series on that branch.
ACK. I'll keep my series as-is.
On 07.06.22 11:47, Dario Binacchi wrote:
> Since slcan_devs array contains the addresses of the created devices, I
> think it is more natural to use its address to remove it from the list.
> It is not necessary to store the index of the array that points to the
> device in the driver's private data.
IMO this patch should not be part of the slcan enhancement series.
I can see the "miss-use" of dev->base_addr but when we change this code
we should also take care of a similar handling in drivers/net/slip/slip.c
Therefore a change like this should be done in slcan.c and slip.c
simultaneously with a single patch.
Best regards,
Oliver
>
> Signed-off-by: Dario Binacchi <[email protected]>
> ---
>
> drivers/net/can/slcan.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
> index 956b47bd40a7..4df0455e11a2 100644
> --- a/drivers/net/can/slcan.c
> +++ b/drivers/net/can/slcan.c
> @@ -428,11 +428,17 @@ static int slc_open(struct net_device *dev)
>
> static void slc_dealloc(struct slcan *sl)
> {
> - int i = sl->dev->base_addr;
> + unsigned int i;
>
> - free_candev(sl->dev);
> - if (slcan_devs)
> - slcan_devs[i] = NULL;
> + for (i = 0; i < maxdev; i++) {
> + if (sl->dev == slcan_devs[i]) {
> + free_candev(sl->dev);
> + slcan_devs[i] = NULL;
> + return;
> + }
> + }
> +
> + pr_err("slcan: can't free %s resources\n", sl->dev->name);
> }
>
> static int slcan_change_mtu(struct net_device *dev, int new_mtu)
> @@ -529,7 +535,6 @@ static struct slcan *slc_alloc(void)
>
> snprintf(dev->name, sizeof(dev->name), "slcan%d", i);
> dev->netdev_ops = &slc_netdev_ops;
> - dev->base_addr = i;
> sl = netdev_priv(dev);
>
> /* Initialize channel control data */
>
On Tue, 7 Jun 2022 21:19:54 +0900
Vincent MAILHOL <[email protected]> wrote:
> In his CAN327 driver, Max manages to bring the can0 device without the
> need of dedicated user space daemon by using line discipline
> (ldattach):
> https://lore.kernel.org/linux-can/[email protected]/
>
> Isn't the same feasible with slcan so that we completely remove the
> dependency toward slcand?
> Max what do you think of this?
I think it is a good idea to move this into the kernel driver. I don't
have a slcan device, but a quick peek at its protocol suggests that
it can be done much easier and cleaner than in can327.
Fun fact: The use of a userspace "ignition" tool is a possible use case
baked into can327's design ;)
There is only one use case I can see for it though: Probing the
ELM327's baud rate, and/or setting it before attaching the ldisc. This
is the single thing that the kernel driver cannot do, since it is
attached to an already running TTY, with a fixed speed. Everything else
is configurable via "ip link".
slcand could be used to auto-probe and set the baud rate as well.
Then again, these stripped-down tools could likely be implemented as
shell scripts calling stty and ldattach...
Max
It is used successfully by most (if not all) CAN device drivers. It
allows to remove replicated code.
Signed-off-by: Dario Binacchi <[email protected]>
---
drivers/net/can/slcan.c | 19 +++++--------------
1 file changed, 5 insertions(+), 14 deletions(-)
diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index 6162a9c21672..964b02f321ab 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -54,6 +54,7 @@
#include <linux/kernel.h>
#include <linux/workqueue.h>
#include <linux/can.h>
+#include <linux/can/dev.h>
#include <linux/can/skb.h>
#include <linux/can/can-ml.h>
@@ -143,7 +144,7 @@ static struct net_device **slcan_devs;
static void slc_bump(struct slcan *sl)
{
struct sk_buff *skb;
- struct can_frame cf;
+ struct can_frame cf, *scf;
int i, tmp;
u32 tmpid;
char *cmd = sl->rbuff;
@@ -201,21 +202,11 @@ static void slc_bump(struct slcan *sl)
}
}
- skb = dev_alloc_skb(sizeof(struct can_frame) +
- sizeof(struct can_skb_priv));
- if (!skb)
+ skb = alloc_can_skb(sl->dev, &scf);
+ if (unlikely(!skb))
return;
- skb->dev = sl->dev;
- skb->protocol = htons(ETH_P_CAN);
- skb->pkt_type = PACKET_BROADCAST;
- skb->ip_summed = CHECKSUM_UNNECESSARY;
-
- can_skb_reserve(skb);
- can_skb_prv(skb)->ifindex = sl->dev->ifindex;
- can_skb_prv(skb)->skbcnt = 0;
-
- skb_put_data(skb, &cf, sizeof(struct can_frame));
+ memcpy(scf, &cf, sizeof(*scf));
sl->dev->stats.rx_packets++;
if (!(cf.can_id & CAN_RTR_FLAG))
--
2.32.0
On 08.06.2022 02:15:37, Max Staudt wrote:
> To speed up the slcan cleanup, may I suggest looking at can327?
>
> It started as a modification of slcan, and over the past few months,
> it has gone through several review rounds in upstreaming. In fact, a
> *ton* of things pointed out during reviews would apply 1:1 to slcan.
>
> What's more, there's legacy stuff that's no longer needed. No
> SLCAN_MAGIC, no slcan_devs, ... it's all gone in can327. May I suggest
> you have a look at it and bring slcan's boilerplate in line with it?
+1
Most of Dario's series looks good. I suggest that we mainline this
first. If there's interest and energy the slcan driver can be reworked
to re-use the more modern concepts of the can327 driver.
> It's certainly not perfect (7 patch series and counting, and that's
> just the public ones), but I'm sure that looking at the two drivers
> side-by-side could serve as a good starting point, to avoid
> re-reviewing the same things all over again.
regards,
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 Wed, 8 Jun 2022 09:19:47 +0200
Marc Kleine-Budde <[email protected]> wrote:
> On 08.06.2022 02:15:37, Max Staudt wrote:
> > To speed up the slcan cleanup, may I suggest looking at can327?
> >
> > It started as a modification of slcan, and over the past few months,
> > it has gone through several review rounds in upstreaming. In fact, a
> > *ton* of things pointed out during reviews would apply 1:1 to slcan.
> >
> > What's more, there's legacy stuff that's no longer needed. No
> > SLCAN_MAGIC, no slcan_devs, ... it's all gone in can327. May I
> > suggest you have a look at it and bring slcan's boilerplate in line
> > with it?
>
> +1
>
> Most of Dario's series looks good. I suggest that we mainline this
> first. If there's interest and energy the slcan driver can be reworked
> to re-use the more modern concepts of the can327 driver.
Agreed. It does look good, and I'm glad to see slcan get some
love.
Thanks Dario!
Max
Hi Marc,
On Tue, Jun 7, 2022 at 12:52 PM Marc Kleine-Budde <[email protected]> wrote:
>
> On 07.06.2022 11:47:50, Dario Binacchi wrote:
> > This patch adds a private flag to the slcan driver to switch the
> > "err-rst-on-open" setting on and off.
> >
> > "err-rst-on-open" on - Reset error states on opening command
> >
> > "err-rst-on-open" off - Don't reset error states on opening command
> > (default)
> >
> > The setting can only be changed if the interface is down:
> >
> > ip link set dev can0 down
> > ethtool --set-priv-flags can0 err-rst-on-open {off|on}
> > ip link set dev can0 up
> >
> > Signed-off-by: Dario Binacchi <[email protected]>
>
> I'm a big fan of bringing the device into a well known good state during
> ifup. What would be the reasons/use cases to not reset the device?
Because by default either slcand and slcan_attach don't reset the error states,
but you must use the `-f' option to do so. So, I followed this use case.
Thanks and regards,
Dario
>
> 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
[email protected]
__________________________________
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
[email protected]
http://www.amarulasolutions.com
On 07.06.2022 11:47:45, Dario Binacchi wrote:
> This is a preparation patch for the upcoming support to change the
> bitrate via ip tool, reset the adapter error states via the ethtool API
> and, more generally, send commands to the adapter.
>
> Since some commands (e. g. setting the bitrate) will be sent before
> calling the open_candev(), the netif_running() will return false and so
> a new flag bit (i. e. SLF_XCMD) for serial transmission has to be added.
>
> Signed-off-by: Dario Binacchi <[email protected]>
I think this patch can be dropped, let me explain:
You don't have to implement the do_set_bittiming callback. It's
perfectly OK to set the bitrate during the ndo_open callback after
open_candev().
regards,
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 08.06.2022 18:33:08, Dario Binacchi wrote:
> Hi Marc,
>
> On Tue, Jun 7, 2022 at 12:52 PM Marc Kleine-Budde <[email protected]> wrote:
> >
> > On 07.06.2022 11:47:50, Dario Binacchi wrote:
> > > This patch adds a private flag to the slcan driver to switch the
> > > "err-rst-on-open" setting on and off.
> > >
> > > "err-rst-on-open" on - Reset error states on opening command
> > >
> > > "err-rst-on-open" off - Don't reset error states on opening command
> > > (default)
> > >
> > > The setting can only be changed if the interface is down:
> > >
> > > ip link set dev can0 down
> > > ethtool --set-priv-flags can0 err-rst-on-open {off|on}
> > > ip link set dev can0 up
> > >
> > > Signed-off-by: Dario Binacchi <[email protected]>
> >
> > I'm a big fan of bringing the device into a well known good state during
> > ifup. What would be the reasons/use cases to not reset the device?
>
> Because by default either slcand and slcan_attach don't reset the
> error states, but you must use the `-f' option to do so. So, I
> followed this use case.
Is this a CAN bus error state, like Bus Off or some controller (i.e. non
CAN related) error?
regards,
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 |
Hi Marc,
On Thu, Jun 9, 2022 at 8:38 AM Marc Kleine-Budde <[email protected]> wrote:
>
> On 08.06.2022 18:33:08, Dario Binacchi wrote:
> > Hi Marc,
> >
> > On Tue, Jun 7, 2022 at 12:52 PM Marc Kleine-Budde <[email protected]> wrote:
> > >
> > > On 07.06.2022 11:47:50, Dario Binacchi wrote:
> > > > This patch adds a private flag to the slcan driver to switch the
> > > > "err-rst-on-open" setting on and off.
> > > >
> > > > "err-rst-on-open" on - Reset error states on opening command
> > > >
> > > > "err-rst-on-open" off - Don't reset error states on opening command
> > > > (default)
> > > >
> > > > The setting can only be changed if the interface is down:
> > > >
> > > > ip link set dev can0 down
> > > > ethtool --set-priv-flags can0 err-rst-on-open {off|on}
> > > > ip link set dev can0 up
> > > >
> > > > Signed-off-by: Dario Binacchi <[email protected]>
> > >
> > > I'm a big fan of bringing the device into a well known good state during
> > > ifup. What would be the reasons/use cases to not reset the device?
> >
> > Because by default either slcand and slcan_attach don't reset the
> > error states, but you must use the `-f' option to do so. So, I
> > followed this use case.
>
> Is this a CAN bus error state, like Bus Off or some controller (i.e. non
> CAN related) error?
The help option of slcan_attach and slcand prints " -f (read status
flags with 'F\\r' to reset error states)\n"
I looked at the sources of the adapter I am using (USBtin, which uses
the mcp2515 controller). The 'F'
command reads the EFLG register (0x2d) without resetting the RX0OVR
and RX1OVR overrun bits.
The error states reset is done by 'f <subcmd>' command, that is not
managed by slcan_attach/slcand.
switch (subcmd) {
case 0x0: // Disable status reporting
mcp2515_write_register(MCP2515_REG_CANINTE, 0x00);
return CR;
case 0x1: // Enable status reporting
mcp2515_write_register(MCP2515_REG_CANINTE, 0x20); //
ERRIE interrupt to INT pin
return CR;
case 0x2: // Clear overrun errors
mcp2515_write_register(MCP2515_REG_EFLG, 0x00);
return CR;
case 0x3: // Reinit/reset MCP2515 to clear all errors
if (state == STATE_CONFIG) {
mcp2515_init();
return CR;
}
break;
}
Thanks and regards,
Dario
>
> regards,
> 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
[email protected]
__________________________________
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
[email protected]
http://www.amarulasolutions.com
On 09.06.2022 09:24:14, Dario Binacchi wrote:
> > > > I'm a big fan of bringing the device into a well known good state during
> > > > ifup. What would be the reasons/use cases to not reset the device?
> > >
> > > Because by default either slcand and slcan_attach don't reset the
> > > error states, but you must use the `-f' option to do so. So, I
> > > followed this use case.
> >
> > Is this a CAN bus error state, like Bus Off or some controller (i.e. non
> > CAN related) error?
>
> The help option of slcan_attach and slcand prints " -f (read status
> flags with 'F\\r' to reset error states)\n" I looked at the sources of
> the adapter I am using (USBtin, which uses the mcp2515 controller).
> The 'F' command reads the EFLG register (0x2d) without resetting the
> RX0OVR and RX1OVR overrun bits.
The Lawicel doc [1] says 'F' is to read the status flags not to clear
it. However commit 7ef581fec029 ("slcan_attach: added '-f' commandline
option to read status flags") [2] suggests that there are some adapters
that the reading of the status flag clears the errors. IMHO the 'F'
command should be send unconditionally during open.
[1] http://www.can232.com/docs/can232_v3.pdf
[2] https://github.com/linux-can/can-utils/commit/7ef581fec0298a228baa71372ea5667874fb4a56
> The error states reset is done by 'f <subcmd>' command, that is not
> managed by slcan_attach/slcand.
Is the 'f' command is non-standard?
> switch (subcmd) {
> case 0x0: // Disable status reporting
> mcp2515_write_register(MCP2515_REG_CANINTE, 0x00);
> return CR;
> case 0x1: // Enable status reporting
> mcp2515_write_register(MCP2515_REG_CANINTE, 0x20); //
> ERRIE interrupt to INT pin
> return CR;
> case 0x2: // Clear overrun errors
> mcp2515_write_register(MCP2515_REG_EFLG, 0x00);
> return CR;
> case 0x3: // Reinit/reset MCP2515 to clear all errors
> if (state == STATE_CONFIG) {
> mcp2515_init();
> return CR;
> }
> break;
> }
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 |
Hi Marc,
On Thu, Jun 9, 2022 at 10:01 AM Marc Kleine-Budde <[email protected]> wrote:
>
> On 09.06.2022 09:24:14, Dario Binacchi wrote:
> > > > > I'm a big fan of bringing the device into a well known good state during
> > > > > ifup. What would be the reasons/use cases to not reset the device?
> > > >
> > > > Because by default either slcand and slcan_attach don't reset the
> > > > error states, but you must use the `-f' option to do so. So, I
> > > > followed this use case.
> > >
> > > Is this a CAN bus error state, like Bus Off or some controller (i.e. non
> > > CAN related) error?
> >
> > The help option of slcan_attach and slcand prints " -f (read status
> > flags with 'F\\r' to reset error states)\n" I looked at the sources of
> > the adapter I am using (USBtin, which uses the mcp2515 controller).
> > The 'F' command reads the EFLG register (0x2d) without resetting the
> > RX0OVR and RX1OVR overrun bits.
>
> The Lawicel doc [1] says 'F' is to read the status flags not to clear
> it. However commit 7ef581fec029 ("slcan_attach: added '-f' commandline
> option to read status flags") [2] suggests that there are some adapters
> that the reading of the status flag clears the errors. IMHO the 'F'
> command should be send unconditionally during open.
When in doubt I would follow the slcand / slcan_attach approach, that don't
send the 'F \ r' command by default. We would be more backward compatible
as regards the sequence of commands to be sent to the controller.
>
> [1] http://www.can232.com/docs/can232_v3.pdf
> [2] https://github.com/linux-can/can-utils/commit/7ef581fec0298a228baa71372ea5667874fb4a56
>
> > The error states reset is done by 'f <subcmd>' command, that is not
> > managed by slcan_attach/slcand.
>
> Is the 'f' command is non-standard?
It's possible.
Thanks and regards,
Dario
>
> > switch (subcmd) {
> > case 0x0: // Disable status reporting
> > mcp2515_write_register(MCP2515_REG_CANINTE, 0x00);
> > return CR;
> > case 0x1: // Enable status reporting
> > mcp2515_write_register(MCP2515_REG_CANINTE, 0x20); //
> > ERRIE interrupt to INT pin
> > return CR;
> > case 0x2: // Clear overrun errors
> > mcp2515_write_register(MCP2515_REG_EFLG, 0x00);
> > return CR;
> > case 0x3: // Reinit/reset MCP2515 to clear all errors
> > if (state == STATE_CONFIG) {
> > mcp2515_init();
> > return CR;
> > }
> > break;
> > }
>
> 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
[email protected]
__________________________________
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
[email protected]
http://www.amarulasolutions.com
On 09.06.2022 10:52:03, Dario Binacchi wrote:
> > > The help option of slcan_attach and slcand prints " -f (read status
> > > flags with 'F\\r' to reset error states)\n" I looked at the sources of
> > > the adapter I am using (USBtin, which uses the mcp2515 controller).
> > > The 'F' command reads the EFLG register (0x2d) without resetting the
> > > RX0OVR and RX1OVR overrun bits.
> >
> > The Lawicel doc [1] says 'F' is to read the status flags not to clear
> > it. However commit 7ef581fec029 ("slcan_attach: added '-f' commandline
> > option to read status flags") [2] suggests that there are some adapters
> > that the reading of the status flag clears the errors. IMHO the 'F'
> > command should be send unconditionally during open.
>
> When in doubt I would follow the slcand / slcan_attach approach, that don't
> send the 'F \ r' command by default. We would be more backward compatible
> as regards the sequence of commands to be sent to the controller.
Ok, keep it this way.
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 |
Hi Marc,
On Thu, Jun 9, 2022 at 9:16 AM Marc Kleine-Budde <[email protected]> wrote:
>
> On 07.06.2022 11:47:45, Dario Binacchi wrote:
> > This is a preparation patch for the upcoming support to change the
> > bitrate via ip tool, reset the adapter error states via the ethtool API
> > and, more generally, send commands to the adapter.
> >
> > Since some commands (e. g. setting the bitrate) will be sent before
> > calling the open_candev(), the netif_running() will return false and so
> > a new flag bit (i. e. SLF_XCMD) for serial transmission has to be added.
> >
> > Signed-off-by: Dario Binacchi <[email protected]>
>
> I think this patch can be dropped, let me explain:
>
> You don't have to implement the do_set_bittiming callback. It's
> perfectly OK to set the bitrate during the ndo_open callback after
> open_candev().
I developed what you suggested (i. e. remove the SLF_XCMD bit and set the
bitrate, as well as send the "F\r" and "O\r" commands, after calling
the open_candev(),
but now I can't send the close command ("C\r") during the ndo_stop() since
netif_running() returns false. The CAN framework clears netif_running() before
calling the ndo_stop(). IMHO the SLF_XCMD bit then becomes necessary to
transmit the commands to the adapter from the ndo_open() and
ndo_stop() routines.
Any suggestions ?
Thanks and regards,
Dario
>
> regards,
> 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
[email protected]
__________________________________
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
[email protected]
http://www.amarulasolutions.com
On 11.06.2022 23:43:47, Dario Binacchi wrote:
> Hi Marc,
>
> On Thu, Jun 9, 2022 at 9:16 AM Marc Kleine-Budde <[email protected]> wrote:
> >
> > On 07.06.2022 11:47:45, Dario Binacchi wrote:
> > > This is a preparation patch for the upcoming support to change the
> > > bitrate via ip tool, reset the adapter error states via the ethtool API
> > > and, more generally, send commands to the adapter.
> > >
> > > Since some commands (e. g. setting the bitrate) will be sent before
> > > calling the open_candev(), the netif_running() will return false and so
> > > a new flag bit (i. e. SLF_XCMD) for serial transmission has to be added.
> > >
> > > Signed-off-by: Dario Binacchi <[email protected]>
> >
> > I think this patch can be dropped, let me explain:
> >
> > You don't have to implement the do_set_bittiming callback. It's
> > perfectly OK to set the bitrate during the ndo_open callback after
> > open_candev().
>
> I developed what you suggested (i. e. remove the SLF_XCMD bit and set the
> bitrate, as well as send the "F\r" and "O\r" commands, after calling
> the open_candev(),
Note:
Max Staudt noticed that you need a do_set_bittiming() callback if you
have only fixed bitrates. I've create a patch that you can have fixed
bit rate only an no do_set_bittiming() callback.
| https://lore.kernel.org/all/[email protected]/
Or use the can-next/master branch as your base.
> but now I can't send the close command ("C\r") during the ndo_stop() since
> netif_running() returns false. The CAN framework clears netif_running() before
> calling the ndo_stop(). IMHO the SLF_XCMD bit then becomes necessary to
> transmit the commands to the adapter from the ndo_open() and
> ndo_stop() routines.
> Any suggestions ?
I see. Keep the setting of the bit rate in the ndo_open(), add the
SLF_XCMD so that you can send messages in ndo_close().
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 |