From: Dario Binacchi <dario.binacchi@amarulasolutions.com>
To: Max Staudt <max@enpas.org>
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: Tue, 26 Jul 2022 16:59:11 +0200 [thread overview]
Message-ID: <CABGWkvqghRsP2w+yw6LXVCnJy4ju10jmQnofS858Vh+VVZzMMQ@mail.gmail.com> (raw)
In-Reply-To: <20220725150920.63ac3a77.max@enpas.org>
Hi Max,
On Mon, Jul 25, 2022 at 3:09 PM Max Staudt <max@enpas.org> wrote:
>
> On Mon, 25 Jul 2022 08:40:24 +0200
> Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote:
>
> > > > @@ -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.
> >
> > I think it is better to keep the code as it is, since at the entry of
> > the netdev
> > open function, netif_running already returns true (it is set to true by the
> > calling function) and therefore it would be less safe to reset the
> > rcount and xleft
> > fields.
>
> Wow, great catch!
>
> I wonder why __LINK_STATE_START is set before ->ndo_open() is called...?
>
>
> Since the drivers are similar, I've checked can327. It is unaffected,
> because the counters are additionally guarded by a spinlock. Same in
> slcan, where netdev_close() takes the spinlock to reset the counters.
>
> So you *could* move them to netdev_open() *if* they are always guarded
> by the slcan lock.
>
> Or, leave it as it is, as it seems to be correct. Your choice :)
If possible I prefer not to use spin_lock. So I prefer to keep the code as is.
Thanks and regards,
Dario
>
>
> Thank you!
>
> Max
--
Dario Binacchi
Embedded Linux Developer
dario.binacchi@amarulasolutions.com
__________________________________
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
info@amarulasolutions.com
www.amarulasolutions.com
next prev parent reply other threads:[~2022-07-26 14:59 UTC|newest]
Thread overview: 20+ 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
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 [this message]
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
2022-07-17 14:12 ` [RFC PATCH 0/5] can: slcan: extend supported features (step 2) Oliver Hartkopp
2022-07-18 6:59 ` Dario Binacchi
2022-07-18 7:15 ` Oliver Hartkopp
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=CABGWkvqghRsP2w+yw6LXVCnJy4ju10jmQnofS858Vh+VVZzMMQ@mail.gmail.com \
--to=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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.