Re: [PATCH net v4 02/13] net/8390: Fix msg_enable patch snafu
On Wed, 14 Feb 2018, David Miller wrote: > > Have you considered that implementing the ethtool hooks in the core > > driver might allow removal of all 8390 driver 'msg_enable' module > > parameters and msglevel ethtool hooks added by c45f812f0280, excepting > > those in the core driver? But even if we did that, it seems to me that > > we still need this patch. > > No, because the module parameter lets you set the default msg level at > the time the driver loads, so you can control messages printed very > early on before it is practical to invoke ethtool and set the msg level. > > This is why most drivers have this module parameter, and implement > such a scheme. Among the 8390 drivers, so far only ne2k-pci implements that scheme. This patch implements that scheme for ax88796 and etherh as well, by making better use of the msg_enable module parameter in lib8390.c. The axnet_cs and pcnet_cs modules lack the msglevel ethtool ops and the msg_enable module parameters. The remaining nine modules lack just the ethtool ops. If you like I will write additional patches to implement the missing ethtool ops or module parameters or both (?) This small patch already addresses the use-case in which the end-user needs to enable (for example) probe messages for some or all 8390 drivers. Thoughts? --
Re: [PATCH net v4 02/13] net/8390: Fix msg_enable patch snafu
From: Finn Thain Date: Thu, 15 Feb 2018 09:11:13 +1100 (AEDT) > On Tue, 13 Feb 2018, David Miller wrote: > >> > I think you have overlooked those modules which offer no way to set >> > p->msg_enable, i.e. ax88796, axnet_cs, etherh, hydra, mac8390, >> > mcf8390, pcnet_cs and zorro8390. >> >> Then that's a bug, we have a very simple easy to implement interface for >> setting this (ethtool). >> >> And by adding the simple hook, you will make these older drivers easier >> to debug for the few people still using them. > > Have you considered that implementing the ethtool hooks in the core driver > might allow removal of all 8390 driver 'msg_enable' module parameters and > msglevel ethtool hooks added by c45f812f0280, excepting those in the core > driver? But even if we did that, it seems to me that we still need this > patch. No, because the module parameter lets you set the default msg level at the time the driver loads, so you can control messages printed very early on before it is practical to invoke ethtool and set the msg level. This is why most drivers have this module parameter, and implement such a scheme.
Re: [PATCH net v4 02/13] net/8390: Fix msg_enable patch snafu
On Tue, 13 Feb 2018, David Miller wrote: > > I think you have overlooked those modules which offer no way to set > > p->msg_enable, i.e. ax88796, axnet_cs, etherh, hydra, mac8390, > > mcf8390, pcnet_cs and zorro8390. > > Then that's a bug, we have a very simple easy to implement interface for > setting this (ethtool). > > And by adding the simple hook, you will make these older drivers easier > to debug for the few people still using them. Have you considered that implementing the ethtool hooks in the core driver might allow removal of all 8390 driver 'msg_enable' module parameters and msglevel ethtool hooks added by c45f812f0280, excepting those in the core driver? But even if we did that, it seems to me that we still need this patch. The missing set_msglevel ethtool hooks and the msg_enable bugs patched here are separate issues inasmuchas the lib8390.c module parameter called 'msg_enable' presently controls only the version message whereas the ethtool hooks cannot control the version message logging at all. I'm not against improving ethtool support. Would you please explain how doing so (one way or another) would alter this patch? --
Re: [PATCH net v4 02/13] net/8390: Fix msg_enable patch snafu
From: Finn Thain Date: Tue, 13 Feb 2018 16:03:09 +1100 (AEDT) > I think you have overlooked those modules which offer no way to set > p->msg_enable, i.e. ax88796, axnet_cs, etherh, hydra, mac8390, mcf8390, > pcnet_cs and zorro8390. Then that's a bug, we have a very simple easy to implement interface for setting this (ethtool). And by adding the simple hook, you will make these older drivers easier to debug for the few people still using them.
Re: [PATCH net v4 02/13] net/8390: Fix msg_enable patch snafu
On Mon, 12 Feb 2018, David Miller wrote: > From: Finn Thain > Date: Sun, 11 Feb 2018 22:08:43 -0500 (EST) > > > The lib8390 module parameter 'msg_enable' doesn't do anything useful: > > it causes an ancient version string to be logged. > > Not true. > > You need to look at the various netif_*() et al. message logging > interfaces, they check "p->msg_enable" to determine which messages to > print. > I think you have overlooked those modules which offer no way to set p->msg_enable, i.e. ax88796, axnet_cs, etherh, hydra, mac8390, mcf8390, pcnet_cs and zorro8390. The apne, ne, ne2k-pci, smc-ultra, stnic, and wd modules are not at issue here because they define their own msg_enable parameters and they also assign p->msg_enable accordingly. > I'm not applying this, sorry. > Please take another look. > Just for the record, I consider these kinds of ancient driver cleanups > painful to review, and unless they allow some ugly global kernel API to > be improved or removed such changes have very little gain. > OK. When there are no users, I'll send you no patches. But even then, I may continue to send patches to other maintainers who may be open to the possibility of unforeseen future uses of the body of contributed code under their care. > In fact, most of them have a good chance to break things. > You saw the tested-by tags, right? Anyway, if you consider my previous work, I believe you'll find that I've fixed more bugs than I've introduced. > It is especially a dubious sequence when you cluster so many of these > things together into a large patch series. > Sorry if that gave the wrong impression. Doing that made my workflow easier. > If you are really serious about fixing real bugs, post these one at a > time, very slowly, for us to review properly and apply. > > Thank you. Please don't feel like I'm trying to pressure you to expedite this. FWIW, this submission was arranged so that you might cherry-pick the first N patches, for some satisfactory value of N. I will split up this series by driver (8390, sonic, etc) so it's more clear which patches are independent and which patches can be reviewed together. Thanks. -- > -- > To unsubscribe from this list: send the line "unsubscribe linux-m68k" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
Re: [PATCH net v4 02/13] net/8390: Fix msg_enable patch snafu
From: Finn Thain Date: Sun, 11 Feb 2018 22:08:43 -0500 (EST) > The lib8390 module parameter 'msg_enable' doesn't do anything useful: > it causes an ancient version string to be logged. Not true. You need to look at the various netif_*() et al. message logging interfaces, they check "p->msg_enable" to determine which messages to print. I'm not applying this, sorry. Just for the record, I consider these kinds of ancient driver cleanups painful to review, and unless they allow some ugly global kernel API to be improved or removed such changes have very little gain. In fact, most of them have a good chance to break things. It is especially a dubious sequence when you cluster so many of these things together into a large patch series. If you are really serious about fixing real bugs, post these one at a time, very slowly, for us to review properly and apply. Thank you.
[PATCH net v4 02/13] net/8390: Fix msg_enable patch snafu
The lib8390 module parameter 'msg_enable' doesn't do anything useful:
it causes an ancient version string to be logged.
Remove redundant code that logs the same string.
In ne.c and wd.c, the value of ei_local->msg_enable is used before
being assigned. Use ne_msg_enable and wd_msg_enable, respectively.
Most of the other 8390 drivers never assign ei_local->msg_enable.
Use the 'msg_enable' module parameter from lib8390 as the default
value.
Eliminate the pointless static and local variables.
Clean up an indentation mistake.
All of these issues originated from the same patch.
Cc: Russell King
Fixes: c45f812f0280 ("8390 : Replace ei_debug with msg_enable/NETIF_MSG_*
feature")
Tested-by: Stan Johnson
Signed-off-by: Finn Thain
---
Only the mac8390.c and lib8390.c changes have been tested. The other
changes are similar but untested.
---
drivers/net/ethernet/8390/ax88796.c | 3 ---
drivers/net/ethernet/8390/axnet_cs.c | 2 --
drivers/net/ethernet/8390/etherh.c| 17 -
drivers/net/ethernet/8390/hydra.c | 4
drivers/net/ethernet/8390/lib8390.c | 2 ++
drivers/net/ethernet/8390/mac8390.c | 8
drivers/net/ethernet/8390/mcf8390.c | 4
drivers/net/ethernet/8390/ne.c| 2 +-
drivers/net/ethernet/8390/pcnet_cs.c | 4
drivers/net/ethernet/8390/wd.c| 2 +-
drivers/net/ethernet/8390/zorro8390.c | 5 -
11 files changed, 4 insertions(+), 49 deletions(-)
diff --git a/drivers/net/ethernet/8390/ax88796.c
b/drivers/net/ethernet/8390/ax88796.c
index 245554707163..da61cf3cb3a9 100644
--- a/drivers/net/ethernet/8390/ax88796.c
+++ b/drivers/net/ethernet/8390/ax88796.c
@@ -77,8 +77,6 @@ static unsigned char version[] = "ax88796.c: Copyright
2005,2007 Simtec Electron
#define AX_GPOC_PPDSET BIT(6)
-static u32 ax_msg_enable;
-
/* device private data */
struct ax_device {
@@ -747,7 +745,6 @@ static int ax_init_dev(struct net_device *dev)
ei_local->block_output = &ax_block_output;
ei_local->get_8390_hdr = &ax_get_8390_hdr;
ei_local->priv = 0;
- ei_local->msg_enable = ax_msg_enable;
dev->netdev_ops = &ax_netdev_ops;
dev->ethtool_ops = &ax_ethtool_ops;
diff --git a/drivers/net/ethernet/8390/axnet_cs.c
b/drivers/net/ethernet/8390/axnet_cs.c
index 7bddb8efb6d5..d422a124cd7c 100644
--- a/drivers/net/ethernet/8390/axnet_cs.c
+++ b/drivers/net/ethernet/8390/axnet_cs.c
@@ -104,7 +104,6 @@ static void AX88190_init(struct net_device *dev, int
startp);
static int ax_open(struct net_device *dev);
static int ax_close(struct net_device *dev);
static irqreturn_t ax_interrupt(int irq, void *dev_id);
-static u32 axnet_msg_enable;
/**/
@@ -151,7 +150,6 @@ static int axnet_probe(struct pcmcia_device *link)
return -ENOMEM;
ei_local = netdev_priv(dev);
-ei_local->msg_enable = axnet_msg_enable;
spin_lock_init(&ei_local->page_lock);
info = PRIV(dev);
diff --git a/drivers/net/ethernet/8390/etherh.c
b/drivers/net/ethernet/8390/etherh.c
index 11cbf22ad201..32e9627e3880 100644
--- a/drivers/net/ethernet/8390/etherh.c
+++ b/drivers/net/ethernet/8390/etherh.c
@@ -64,8 +64,6 @@ static char version[] =
#include "lib8390.c"
-static u32 etherh_msg_enable;
-
struct etherh_priv {
void __iomem*ioc_fast;
void __iomem*memc;
@@ -502,18 +500,6 @@ etherh_close(struct net_device *dev)
}
/*
- * Initialisation
- */
-
-static void __init etherh_banner(void)
-{
- static int version_printed;
-
- if ((etherh_msg_enable & NETIF_MSG_DRV) && (version_printed++ == 0))
- pr_info("%s", version);
-}
-
-/*
* Read the ethernet address string from the on board rom.
* This is an ascii string...
*/
@@ -671,8 +657,6 @@ etherh_probe(struct expansion_card *ec, const struct
ecard_id *id)
struct etherh_priv *eh;
int ret;
- etherh_banner();
-
ret = ecard_request_resources(ec);
if (ret)
goto out;
@@ -757,7 +741,6 @@ etherh_probe(struct expansion_card *ec, const struct
ecard_id *id)
ei_local->block_output = etherh_block_output;
ei_local->get_8390_hdr = etherh_get_header;
ei_local->interface_num = 0;
- ei_local->msg_enable = etherh_msg_enable;
etherh_reset(dev);
__NS8390_init(dev, 0);
diff --git a/drivers/net/ethernet/8390/hydra.c
b/drivers/net/ethernet/8390/hydra.c
index 8ae249195301..941754ea78ec 100644
--- a/drivers/net/ethernet/8390/hydra.c
+++ b/drivers/net/ethernet/8390/hydra.c
@@ -66,7 +66,6 @@ static void hydra_block_input(struct net_device *dev, int
count,
static void hydra_block_output(struct net_device *dev, int count,
const unsigned char *buf, int start_page);
static void hydra_remove_one(struct zorro_dev *z);
-static u32 hydra_msg_enable;
static struct zorro_device_id hydra_zorro_tbl[] = {
{ ZORRO_PROD_HYDRA_SYSTEMS_AMIG
