linux-kernel - 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <7e9c01da-74be-3d8d-bb0c-d90935d82081@fintek.com.tw>
Date:   Mon, 10 Apr 2023 13:50:30 +0800
From:   Peter Hong <peter_hong@...tek.com.tw>
To:     Vincent MAILHOL <mailhol.vincent@...adoo.fr>
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 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);

     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 ?


> 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 ?

Thanks