netdev - Re: [PATCH V3] can: usb: f81604: add Fintek F81604 support

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] [day] [month] [year] [list]
Date:   Mon, 10 Apr 2023 15:26:44 +0900
From:   Vincent MAILHOL <mailhol.vincent@...adoo.fr>
To:     Peter Hong <peter_hong@...tek.com.tw>
Cc:     wg@...ndegger.com, mkl@...gutronix.de,
        michal.swiatkowski@...ux.intel.com, Steen.Hegelund@...rochip.com,
        davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, frank.jungclaus@....eu,
        linux-kernel@...r.kernel.org, linux-can@...r.kernel.org,
        netdev@...r.kernel.org, hpeter+linux_kernel@...il.com
Subject: Re: [PATCH V3] can: usb: f81604: add Fintek F81604 support

Hi Peter,

On Mon. 10 Apr 2023 at 14:52, Peter Hong <peter_hong@...tek.com.tw> wrote:
> Hi Vincent,
>
> Vincent MAILHOL 於 2023/3/30 下午 09:11 寫道:
> > Hmm, I am still not a fan of setting a mutex for a single concurrency
> > issue which can only happen during probing.
> >
> > What about this:
> >
> >    static int __f81604_set_termination(struct net_device *netdev, u16 term)
> >    {
> >            struct f81604_port_priv *port_priv = netdev_priv(netdev);
> >            u8 mask, data = 0;
> >
> >            if (netdev->dev_id == 0)
> >                    mask = F81604_CAN0_TERM;
> >            else
> >                    mask = F81604_CAN1_TERM;
> >
> >            if (term == F81604_TERMINATION_ENABLED)
> >                    data = mask;
> >
> >            return f81604_mask_set_register(port_priv->dev, F81604_TERMINATOR_REG,
> >                                            mask, data);
> >    }
> >
> >    static int f81604_set_termination(struct net_device *netdev, u16 term)
> >    {
> >            ASSERT_RTNL();
> >
> >            return __f81604_set_termination(struct net_device *netdev, u16 term);
> >    }
> >
> >    static int f81604_init_termination(struct f81604_priv *priv)
> >    {
> >            int i, ret;
> >
> >            for (i = 0; i < ARRAY_SIZE(f81604_priv->netdev); i++) {
> >                    ret = __f81604_set_termination(f81604_priv->netdev[i],
> >                                                   F81604_TERMINATION_DISABLED);
> >                    if (ret)
> >                            return ret;
> >            }
> >    }
> >
> >    static int f81604_probe(struct usb_interface *intf,
> >                            const struct usb_device_id *id)
> >    {
> >            /* ... */
> >
> >            err = f81604_init_termination(priv);
> >            if (err)
> >                    goto failure_cleanup;
> >
> >            for (i = 0; i < ARRAY_SIZE(f81604_priv->netdev); i++) {
> >                    /* ... */
> >            }
> >
> >            /* ... */
> >    }
> >
> > Initialise all resistors with __f81604_set_termination() in probe()
> > before registering any network device. Use f81604_set_termination()
> > which has the lock assert elsewhere.
>
> The f81604_set_termination() will transform into the following code:
>
> static int f81604_write(struct usb_device *dev, u16 reg, u8 data);
> static int f81604_read(struct usb_device *dev, u16 reg, u8 *data);
> static int f81604_update_bits(struct usb_device *dev, u16 reg, u8 mask,
>                                                 u8 data);
>
> static int __f81604_set_termination(struct usb_device *dev, int idx, u16
> term)
> {
>      u8 mask, data = 0;
>
>      if (idx == 0)
>          mask = F81604_CAN0_TERM;
>      else
>          mask = F81604_CAN1_TERM;
>
>      if (term)
>          data = mask;
>
>      return f81604_update_bits(dev, F81604_TERMINATOR_REG, mask, data);
> }
>
> static int f81604_set_termination(struct net_device *netdev, u16 term)
> {
>      struct f81604_port_priv *port_priv = netdev_priv(netdev);
>      struct f81604_priv *priv;
>
>      ASSERT_RTNL();
>
>      priv = usb_get_intfdata(port_priv->intf);
       ^^^^
Why do you need priv here? I do not see it used in the next line.

>      return __f81604_set_termination(port_priv->dev, netdev->dev_id, term);
> }
>
> and also due to f81604_write() / f81604_read() / f81604_update_bits()
> may use
> in f81604_probe() without port private data, so we'll change their first
> parameter
> from "struct f81604_port_priv *priv" to "struct usb_device *dev". Is it OK ?

Right now, it is hard to visualize the final result. Please send what
you think is the best and we will review.

> > Also, looking at your probe() function, in label clean_candev:, if the
> > second can channel fails its initialization, you do not clean the
> > first can channel. I suggest adding a f81604_init_netdev() and
> > handling the netdev issue and cleanup in that function.
>
> When the second can channel failed its initialization, the label
> "clean_candev" will
> clear second "netdev" object and the first "netdev" will cleanup in
> f81604_disconnect().
>
> Could I remain this section of code ?

Oh! I was not aware that disconnect() would be called on a failed
probe. Overall, I prefer the use of subfunctions because it makes it
easier to understand the logic, especially for the cleanup after
failure. Let's say that it is acceptable as-is. OK to keep.