lkml.org 
[lkml]   [2022]   [Jun]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v4 05/12] can: slcan: use CAN network device driver API
On 28.06.2022 11:48:48, Dario Binacchi wrote:
> Hi Marc,
>
> On Tue, Jun 28, 2022 at 11:28 AM Marc Kleine-Budde <mkl@pengutronix.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?

As Max pointed out the whole static dev array is not needed at all. Just
keep the rtnl, until removing the array altogether.

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 |
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2022-06-28 12:40    [W:0.057 / U:0.356 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site