From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 428E8C43334 for ; Mon, 25 Jul 2022 13:09:36 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235480AbiGYNJf (ORCPT ); Mon, 25 Jul 2022 09:09:35 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40432 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235481AbiGYNJc (ORCPT ); Mon, 25 Jul 2022 09:09:32 -0400 Received: from mail.enpas.org (zhong.enpas.org [46.38.239.100]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 22AF013D76; Mon, 25 Jul 2022 06:09:28 -0700 (PDT) Received: from [127.0.0.1] (localhost [127.0.0.1]) by mail.enpas.org (Postfix) with ESMTPSA id 430C0FF9C3; Mon, 25 Jul 2022 13:09:26 +0000 (UTC) Date: Mon, 25 Jul 2022 15:09:20 +0200 From: Max Staudt To: Dario Binacchi Cc: linux-kernel@vger.kernel.org, Jeroen Hofstee , michael@amarulasolutions.com, Amarula patchwork , "David S. Miller" , Eric Dumazet , Greg Kroah-Hartman , Jakub Kicinski , Jiri Slaby , Marc Kleine-Budde , Paolo Abeni , Vincent Mailhol , Wolfgang Grandegger , linux-can@vger.kernel.org, netdev@vger.kernel.org Subject: Re: [RFC PATCH 2/5] can: slcan: remove legacy infrastructure Message-ID: <20220725150920.63ac3a77.max@enpas.org> In-Reply-To: References: <20220716170007.2020037-1-dario.binacchi@amarulasolutions.com> <20220716170007.2020037-3-dario.binacchi@amarulasolutions.com> <20220717233842.1451e349.max@enpas.org> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Mon, 25 Jul 2022 08:40:24 +0200 Dario Binacchi 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 :) Thank you! Max