From: Oliver Hartkopp <socketcan@hartkopp.net>
To: Max Staudt <max@enpas.org>,
Dario Binacchi <dario.binacchi@amarulasolutions.com>
Cc: linux-kernel@vger.kernel.org,
Jeroen Hofstee <jhofstee@victronenergy.com>,
michael@amarulasolutions.com,
Amarula patchwork <linux-amarula@amarulasolutions.com>,
"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>,
Marc Kleine-Budde <mkl@pengutronix.de>,
Paolo Abeni <pabeni@redhat.com>,
Vincent Mailhol <mailhol.vincent@wanadoo.fr>,
Wolfgang Grandegger <wg@grandegger.com>,
linux-can@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [RFC PATCH 2/5] can: slcan: remove legacy infrastructure
Date: Mon, 18 Jul 2022 08:57:21 +0200 [thread overview]
Message-ID: <6faf29c7-3e9d-bc21-9eac-710f901085d8@hartkopp.net> (raw)
In-Reply-To: <20220717233842.1451e349.max@enpas.org>
On 17.07.22 23:38, Max Staudt wrote:
> Hi Dario,
>
> This looks good, thank you for continuing to look after slcan!
>
> A few comments below.
>
>
>
> On Sat, 16 Jul 2022 19:00:04 +0200
> Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote:
>
> [...]
>
>
>> @@ -68,7 +62,6 @@ MODULE_PARM_DESC(maxdev, "Maximum number of slcan interfaces");
>> SLC_STATE_BE_TXCNT_LEN)
>> struct slcan {
>> struct can_priv can;
>> - int magic;
>>
>> /* Various fields. */
>> struct tty_struct *tty; /* ptr to TTY structure */
>> @@ -84,17 +77,14 @@ struct slcan {
>> int xleft; /* bytes left in XMIT queue */
>>
>> 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 */
>> +#define SLF_ERROR 0 /* Parity, etc. error */
>> +#define SLF_XCMD 1 /* 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 */
>
> I assume xcmd_wait() came in as part of the previous patch series?
>
>
> [...]
>
>
>> /* Send a can_frame to a TTY queue. */
>> @@ -652,25 +637,21 @@ 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 != CAN_BITRATE_UNKNOWN) {
>> - 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);
>> + if (sl->can.bittiming.bitrate &&
>> + sl->can.bittiming.bitrate != CAN_BITRATE_UNKNOWN) {
>> + err = slcan_transmit_cmd(sl, "C\r");
>> + 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);
>> + flush_work(&sl->tx_work);
>> +
>> netif_stop_queue(dev);
>> sl->rcount = 0;
>> sl->xleft = 0;
>
> I suggest moving these two assignments to slc_open() - see below.
>
>
> [...]
>
>
>> @@ -883,72 +786,50 @@ static int slcan_open(struct tty_struct *tty)
>> if (!tty->ops->write)
>> return -EOPNOTSUPP;
>>
>> - /* RTnetlink lock is misused here to serialize concurrent
>> - * opens of slcan channels. There are better ways, but it is
>> - * the simplest one.
>> - */
>> - rtnl_lock();
>> + dev = alloc_candev(sizeof(*sl), 1);
>> + if (!dev)
>> + return -ENFILE;
>>
>> - /* Collect hanged up channels. */
>> - slc_sync();
>> + sl = netdev_priv(dev);
>>
>> - sl = tty->disc_data;
>> + /* Configure TTY interface */
>> + tty->receive_room = 65536; /* We don't flow control */
>> + sl->rcount = 0;
>> + sl->xleft = 0;
>
> I suggest moving the zeroing to slc_open() - i.e. to the netdev open
> function. As a bonus, you can then remove the same two assignments from
> slc_close() (see above). They are only used when netif_running(), with
> appropiate guards already in place as far as I can see.
>
>
>> + spin_lock_init(&sl->lock);
>> + INIT_WORK(&sl->tx_work, slcan_transmit);
>> + init_waitqueue_head(&sl->xcmd_wait);
>>
>> - err = -EEXIST;
>> - /* First make sure we're not already connected. */
>> - if (sl && sl->magic == SLCAN_MAGIC)
>> - goto err_exit;
>> + /* Configure CAN metadata */
>> + sl->can.bitrate_const = slcan_bitrate_const;
>> + sl->can.bitrate_const_cnt = ARRAY_SIZE(slcan_bitrate_const);
>>
>> - /* OK. Find a free SLCAN channel to use. */
>> - err = -ENFILE;
>> - sl = slc_alloc();
>> - if (!sl)
>> - goto err_exit;
>> + /* Configure netdev interface */
>> + sl->dev = dev;
>> + strscpy(dev->name, "slcan%d", sizeof(dev->name));
>
> The third parameter looks... unintentional :)
>
> What do the maintainers think of dropping the old "slcan" name, and
> just allowing this to be a normal canX device? These patches do bring
> it closer to that, after all. In this case, this name string magic
> could be dropped altogether.
>
I'm fine with it in general. But we have to take into account that there
might be existing setups that still might use the slcan_attach or slcand
mechanic which will likely break after the kernel update.
But in the end the slcan0 shows up everywhere - even in log files, etc.
So we really should name it canX. When people really get in trouble with
it, they can rename the network interface name with the 'ip' tool ...
Best regards,
Oliver
>
> [...]
>
>
>
> This looks good to me overall.
>
> Thanks Dario!
>
>
> Max
next prev parent reply other threads:[~2022-07-18 6:58 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-16 17:00 [RFC PATCH 0/5] can: slcan: extend supported features (step 2) Dario Binacchi
2022-07-16 17:00 ` [RFC PATCH 1/5] can: slcan: remove useless header inclusions Dario Binacchi
2022-07-16 17:00 ` [RFC PATCH 2/5] can: slcan: remove legacy infrastructure Dario Binacchi
2022-07-17 21:38 ` Max Staudt
2022-07-18 6:57 ` Oliver Hartkopp [this message]
2022-07-18 10:15 ` Marc Kleine-Budde
2022-07-18 10:23 ` Oliver Hartkopp
2022-07-19 1:03 ` Max Staudt
2022-07-19 19:59 ` Marc Kleine-Budde
2022-07-25 6:40 ` Dario Binacchi
2022-07-25 13:09 ` Max Staudt
2022-07-26 14:59 ` Dario Binacchi
2022-07-16 17:00 ` [RFC PATCH 3/5] can: slcan: change every `slc' occurrence in `slcan' Dario Binacchi
2022-07-16 17:00 ` [RFC PATCH 4/5] can: slcan: use the generic can_change_mtu() Dario Binacchi
2022-07-16 17:00 ` [RFC PATCH 5/5] can: slcan: send the listen-only command to the adapter Dario Binacchi
2022-07-18 10:22 ` Marc Kleine-Budde
2022-07-21 8:17 ` Dario Binacchi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6faf29c7-3e9d-bc21-9eac-710f901085d8@hartkopp.net \
--to=socketcan@hartkopp.net \
--cc=dario.binacchi@amarulasolutions.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=gregkh@linuxfoundation.org \
--cc=jhofstee@victronenergy.com \
--cc=jirislaby@kernel.org \
--cc=kuba@kernel.org \
--cc=linux-amarula@amarulasolutions.com \
--cc=linux-can@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mailhol.vincent@wanadoo.fr \
--cc=max@enpas.org \
--cc=michael@amarulasolutions.com \
--cc=mkl@pengutronix.de \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=wg@grandegger.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).