On Fri. 2 Apr 2021 at 19:47, Marc Kleine-Budde [off-list ref] wrote:

On 02.04.2021 18:56:33, Vincent MAILHOL wrote:

quoted

quoted

quoted

+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.

quoted

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!

[...]

quoted

quoted

quoted

+/**
+ * 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