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 04:26:30 +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: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 ?

[...]

>> +	if (likely(txmir >= skb->len + 12)) {
>> +		ks8851_wrreg16_par(ks, KS_RXQCR, ks->rc_rxqcr | RXQCR_SDA);
>> +		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)
>> +			;
> 
> You should have a timeout/retry limit here, just in case.

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 ?