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]

Message-ID: <20200514131527.GN527401@lunn.ch>
Date:   Thu, 14 May 2020 15:15:27 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Marek Vasut <marex@...x.de>
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 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.

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

	Andrew