linux-kernel - Re: [PATCH v4 05/12] can: slcan: use CAN network device driver API

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CABGWkvrdw27T+g==HrknM+52mhvgEDS_4P9__7tsc+aV-oAvCw@mail.gmail.com>
Date:   Tue, 28 Jun 2022 11:48:48 +0200
From:   Dario Binacchi <dario.binacchi@...rulasolutions.com>
To:     Marc Kleine-Budde <mkl@...gutronix.de>
Cc:     linux-kernel@...r.kernel.org,
        Amarula patchwork <linux-amarula@...rulasolutions.com>,
        Oliver Hartkopp <socketcan@...tkopp.net>,
        michael@...rulasolutions.com,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Wolfgang Grandegger <wg@...ndegger.com>,
        linux-can@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH v4 05/12] can: slcan: use CAN network device driver API

Hi Marc,

On Tue, Jun 28, 2022 at 11:28 AM Marc Kleine-Budde <mkl@...gutronix.de> wrote:
>
> On 14.06.2022 14:28:14, Dario Binacchi wrote:
> > As suggested by commit [1], now the driver uses the functions and the
> > data structures provided by the CAN network device driver interface.
> >
> > Currently the driver doesn't implement a way to set bitrate for SLCAN
> > based devices via ip tool, so you'll have to do this by slcand or
> > slcan_attach invocation through the -sX parameter:
> >
> > - slcan_attach -f -s6 -o /dev/ttyACM0
> > - slcand -f -s8 -o /dev/ttyUSB0
> >
> > where -s6 in will set adapter's bitrate to 500 Kbit/s and -s8 to
> > 1Mbit/s.
> > See the table below for further CAN bitrates:
> > - s0 ->   10 Kbit/s
> > - s1 ->   20 Kbit/s
> > - s2 ->   50 Kbit/s
> > - s3 ->  100 Kbit/s
> > - s4 ->  125 Kbit/s
> > - s5 ->  250 Kbit/s
> > - s6 ->  500 Kbit/s
> > - s7 ->  800 Kbit/s
> > - s8 -> 1000 Kbit/s
> >
> > In doing so, the struct can_priv::bittiming.bitrate of the driver is not
> > set and since the open_candev() checks that the bitrate has been set, it
> > must be a non-zero value, the bitrate is set to a fake value (-1U)
> > before it is called.
> >
> > The patch also changes the slcan_devs locking from rtnl to spin_lock. The
> > change was tested with a kernel with the CONFIG_PROVE_LOCKING option
> > enabled that did not show any errors.
>
> You're not allowed to call alloc_candev() with a spin_lock held. See
> today's kernel test robot mail:
>
> | https://lore.kernel.org/all/YrpqO5jepAvv4zkf@xsang-OptiPlex-9020
>
> I think it's best to keep the rtnl for now.

The rtnl_lock() uses a mutex while I used a spin_lock.

static DEFINE_MUTEX(rtnl_mutex);

void rtnl_lock(void)
{
mutex_lock(&rtnl_mutex);
}
EXPORT_SYMBOL(rtnl_lock);

So might it be worth trying with a mutex instead of rtnl_lock(), or do
you think it is
safer to return to rtn_lock () anyway?

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

dario.binacchi@...rulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

T. +39 042 243 5310
info@...rulasolutions.com

www.amarulasolutions.com