netdev - Re: [PATCH V5 18/19] net: ks8851: Implement Parallel bus operations

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]

Date:   Thu, 14 May 2020 16:00:18 +0200
From:   Marek Vasut <marex@...x.de>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>,
        Lukas Wunner <lukas@...ner.de>, Petr Stetiar <ynezz@...e.cz>,
        YueHaibing <yuehaibing@...wei.com>
Subject: Re: [PATCH V5 18/19] net: ks8851: Implement Parallel bus operations

On 5/14/20 3:15 PM, Andrew Lunn wrote:
> On Thu, May 14, 2020 at 04:26:30AM +0200, Marek Vasut wrote:
>> On 5/14/20 3:57 AM, Andrew Lunn wrote:
>>>> diff --git a/drivers/net/ethernet/micrel/ks8851_par.c b/drivers/net/ethernet/micrel/ks8851_par.c
>>>> new file mode 100644
>>>> index 000000000000..90fffacb1695
>>>> --- /dev/null
>>>> +++ b/drivers/net/ethernet/micrel/ks8851_par.c
>>>> @@ -0,0 +1,348 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/* drivers/net/ethernet/micrel/ks8851.c
>>>> + *
>>>> + * Copyright 2009 Simtec Electronics
>>>> + *	http://www.simtec.co.uk/
>>>> + *	Ben Dooks <ben@...tec.co.uk>
>>>> + */
>>>> +
>>>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>>> +
>>>> +#define DEBUG
>>>
>>> I don't think you wanted that left in.
>>
>> This actually was in the original ks8851.c since forever, so I wonder.
>> Maybe a separate patch would be better ?
> 
> Yes, please add another patch.

OK

>>>> +		ks8851_done_tx(ks, skb);
>>>> +	} else {
>>>> +		ret = NETDEV_TX_BUSY;
>>>> +	}
>>>> +
>>>> +	ks8851_unlock_par(ks, &flags);
>>>> +
>>>> +	return ret;
>>>> +}
>>>
>>>> +module_param_named(message, msg_enable, int, 0);
>>>> +MODULE_PARM_DESC(message, "Message verbosity level (0=none, 31=all)");
>>>
>>> Module parameters are bad. A new driver should not have one, if
>>> possible. Please implement the ethtool .get_msglevel and .set_msglevel
>>> instead.
>>
>> This was in the original ks8851.c , so I need to retain it , no ?
> 
> Ah. Err.
> 
> This patch looks like a new driver. It has probe, remove
> module_platform_driver(), etc. So as a new driver, it should not have
> module parameters.
> 
> But then your next patch removes the mll driver. Your intention is
> that this driver replaces the mll driver. So for backwards
> compatibility, yes you do need the module parameter.

All right

btw is jiffies-based timeout OK? Like this:

diff --git a/drivers/net/ethernet/micrel/ks8851_par.c
b/drivers/net/ethernet/micrel/ks8851_par.c
index 5163d10971f4..1d9e7a33ffcf 100644
--- a/drivers/net/ethernet/micrel/ks8851_par.c
+++ b/drivers/net/ethernet/micrel/ks8851_par.c
@@ -238,6 +238,7 @@ static netdev_tx_t ks8851_start_xmit_par(struct
sk_buff *skb,
                                         struct net_device *dev)
 {
        struct ks8851_net *ks = netdev_priv(dev);
+       unsigned long txpoll_start_time;
        netdev_tx_t ret = NETDEV_TX_OK;
        unsigned long flags;
        u16 txmir;
@@ -254,8 +255,20 @@ static netdev_tx_t ks8851_start_xmit_par(struct
sk_buff *skb,
                ks8851_wrfifo_par(ks, skb, false);
                ks8851_wrreg16_par(ks, KS_RXQCR, ks->rc_rxqcr);
                ks8851_wrreg16_par(ks, KS_TXQCR, TXQCR_METFE);
-               while (ks8851_rdreg16_par(ks, KS_TXQCR) & TXQCR_METFE)
-                       ;
+
+               txpoll_start_time = jiffies;
+               while (true) {
+                       if (!(ks8851_rdreg16_par(ks, KS_TXQCR) &
TXQCR_METFE))
+                               break;
+
+                       if (time_after(jiffies, txpoll_start_time + HZ)) {
+                               netdev_warn(dev, "%s: did not complete.\n",
+                                           __func__);
+                               ret = NETDEV_TX_BUSY;
+                               break;
+                       }
+               }
+
                ks8851_done_tx(ks, skb);
        } else {
                ret = NETDEV_TX_BUSY;