Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220716170007.2020037-1-dario.binacchi@amarulasolutions.com>
 <20220716170007.2020037-3-dario.binacchi@amarulasolutions.com>
 <20220717233842.1451e349.max@enpas.org> <CABGWkvrgX+9J-rOb-EO1wXVAZQ5phwKKpbc-iD491rD9zn5UpQ@mail.gmail.com>
 <20220725150920.63ac3a77.max@enpas.org>
In-Reply-To: <20220725150920.63ac3a77.max@enpas.org>
From:   Dario Binacchi <dario.binacchi@amarulasolutions.com>
Date:   Tue, 26 Jul 2022 16:59:11 +0200
Message-ID: <CABGWkvqghRsP2w+yw6LXVCnJy4ju10jmQnofS858Vh+VVZzMMQ@mail.gmail.com>
Subject: Re: [RFC PATCH 2/5] can: slcan: remove legacy infrastructure
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
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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