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

Powered by blists - more mailing lists