Re: [PATCH net v4 02/13] net/8390: Fix msg_enable patch snafu

[Finn Thain]

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

[Finn Thain]

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

[David Miller]

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

[David Miller]

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

[Finn Thain]

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

[Finn Thain]

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

[David Miller]

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

[David Miller]

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

[Finn Thain]

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

[Finn Thain]

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

[David Miller]

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.

Re: [PATCH net v4 02/13] net/8390: Fix msg_enable patch snafu

[David Miller]

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.