On Fri. 2 Apr 2021 at 19:47, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote:
> On 02.04.2021 18:56:33, Vincent MAILHOL wrote:
> > > > +int es58x_rx_err_msg(struct net_device *netdev, enum es58x_err error,
> > > > + enum es58x_event event, u64 timestamp)
> > > > +{
> > > > + struct es58x_priv *priv = es58x_priv(netdev);
> > > > + struct can_priv *can = netdev_priv(netdev);
> > > > + struct can_device_stats *can_stats = &can->can_stats;
> > > > + struct can_frame *cf;
> > >
> > > = NULL;
> > >
> > > So that the if (cf) properly works...
> >
> > I actually expected cfto be set to NULL when an error
> > occurred (same as skb).
> >
> > But indeed, cf is not initialised if the allocation fails. I was
> > wondering if it would not be better to make both alloc_can_skb()
> > and and alloc_canfd_skb() set cf to NULL. That would remove a
> > pitfall.
>
> Makes sense.
>
> > If you like the idea, I can submit a patch.
>
> Sorry creating this drive by patch. The patch description took longer
> than the actual patch :)
This is often the case for any patches where the diff is one or two lines!
[...]
>
> > > > +/**
> > > > + * es58x_open() - Open and start network device.
> > > > + * @netdev: CAN network device.
> > > > + *
> > > > + * Called when the network transitions to the up state.
> > > > + *
> > > > + * Return: zero on success, errno when any error occurs.
> > > > + */
> > > > +static int es58x_open(struct net_device *netdev)
> > > > +{
> > > > + int ret;
> > > > +
> > > > + ret = open_candev(netdev);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + ret = es58x_enable_channel(netdev);
> > > > + if (ret)
> > > > + return ret;
> > >
> > > Please do an as complete as possible reset and configuration during
> > > open(). If there is any error a ifdown/ifup should fix it. Here on a USB
> > > device with multiple interfaces it's not as easy as on devices with only
> > > one CAN interface.
> >
> > ACK.
> >
> > I will use a function as below to check if all interfaces are
> > down.
> >
> > static bool es58x_are_all_channels_closed(struct es58x_device *es58x_dev)
> > {
> > int i;
> >
> > for (i = 0; i < es58x_dev->num_can_ch; i++) {
> > struct can_priv *can_priv = netdev_priv(es58x_dev->netdev[i]);
> > if (can_priv->state != CAN_STATE_STOPPED)
> > return false;
> > }
> > return true;
> > }
> >
> > I will modify both es58x_open() and es58x_close().
>
> Have a look at
> https://elixir.bootlin.com/linux/v5.11/source/include/linux/kref.h
>
> I'm not sure if kref is overkill here :)
There is indeed a race condition in which the above function
might return true when it is not supposed to.
I checked the struct kref and the refcount_t, but here, I think
that a simple atomic_t is enough because there is no risk of
saturation.
Something like:
| if (atomic_inc_return(opened_channel_cnt) == 1)
| /* configure */
and
| if (atomic_dec_and_test(opened_channel_cnt))
| /* release */
[...]
Yours sincerely,
Vincent
