ethtool 5.7: netlink ENOENT error when setting WOL

ethtool 5.7: netlink ENOENT error when setting WOL

[Heiner Kallweit]

Since ethtool 5.7 following happens (kernel is latest linux-next):

ethtool -s enp3s0 wol g
netlink error: No such file or directory

With ethtool 5.6 this doesn't happen. I also checked the latest ethtool
git version (5.7 + some fixes), error still occurs.

Heiner

Re: ethtool 5.7: netlink ENOENT error when setting WOL

[Heiner Kallweit]

On 10.06.2020 10:26, Heiner Kallweit wrote:
> Since ethtool 5.7 following happens (kernel is latest linux-next):
> 
> ethtool -s enp3s0 wol g
> netlink error: No such file or directory
> 
> With ethtool 5.6 this doesn't happen. I also checked the latest ethtool
> git version (5.7 + some fixes), error still occurs.
> 
> Heiner
> 
Bisecting points to:
netlink: show netlink error even without extack

Re: ethtool 5.7: netlink ENOENT error when setting WOL

[Michal Kubecek]

On Wed, Jun 10, 2020 at 10:52:26AM +0200, Heiner Kallweit wrote:
> On 10.06.2020 10:26, Heiner Kallweit wrote:
> > Since ethtool 5.7 following happens (kernel is latest linux-next):
> > 
> > ethtool -s enp3s0 wol g
> > netlink error: No such file or directory
> > 
> > With ethtool 5.6 this doesn't happen. I also checked the latest ethtool
> > git version (5.7 + some fixes), error still occurs.
> > 
> > Heiner
> > 
> Bisecting points to:
> netlink: show netlink error even without extack

Just to make sure you are hitting the same problem I'm just looking at,
please check if

- your kernel is built with ETHTOOL_NETLINK=n
- the command actually succeeds (i.e. changes the WoL modes)
- output with of "ethtool --debug 0x12 -s enp3s0 wol g" looks like

  sending genetlink packet (32 bytes):
      msg length 32 genl-ctrl
      CTRL_CMD_GETFAMILY
          CTRL_ATTR_FAMILY_NAME = "ethtool"
  received genetlink packet (52 bytes):
      msg length 52 error errno=-2
  netlink error: No such file or directory
  offending message:
      ETHTOOL_MSG_LINKINFO_SET
          ETHTOOL_A_LINKINFO_PORT = 101

If this is the case, than the commit found by bisect only revealed an
issue which was introduced earlier by commit 76bdf9372824 ("netlink: use
pretty printing for ethtool netlink messages"). The patch below should
suppress the message as intended.

Michal

diff --git a/netlink/nlsock.c b/netlink/nlsock.c
index 2c760b770ec5..c3f09b6ee9ab 100644
--- a/netlink/nlsock.c
+++ b/netlink/nlsock.c
@@ -255,12 +255,12 @@ int nlsock_process_reply(struct nl_socket *nlsk, mnl_cb_t reply_cb, void *data)
 
 		nlhdr = (struct nlmsghdr *)buff;
 		if (nlhdr->nlmsg_type == NLMSG_ERROR) {
-			bool silent = nlsk->nlctx->suppress_nlerr;
+			unsigned int suppress = nlsk->nlctx->suppress_nlerr;
 			bool pretty;
 
 			pretty = debug_on(nlsk->nlctx->ctx->debug,
 					  DEBUG_NL_PRETTY_MSG);
-			return nlsock_process_ack(nlhdr, len, silent, pretty);
+			return nlsock_process_ack(nlhdr, len, suppress, pretty);
 		}
 
 		msgbuff->nlhdr = nlhdr;

Re: ethtool 5.7: netlink ENOENT error when setting WOL

[Heiner Kallweit]

On 10.06.2020 11:13, Michal Kubecek wrote:
> On Wed, Jun 10, 2020 at 10:52:26AM +0200, Heiner Kallweit wrote:
>> On 10.06.2020 10:26, Heiner Kallweit wrote:
>>> Since ethtool 5.7 following happens (kernel is latest linux-next):
>>>
>>> ethtool -s enp3s0 wol g
>>> netlink error: No such file or directory
>>>
>>> With ethtool 5.6 this doesn't happen. I also checked the latest ethtool
>>> git version (5.7 + some fixes), error still occurs.
>>>
>>> Heiner
>>>
>> Bisecting points to:
>> netlink: show netlink error even without extack
> 
> Just to make sure you are hitting the same problem I'm just looking at,
> please check if
> 
> - your kernel is built with ETHTOOL_NETLINK=n

No, because I have PHYLIB=m.
Not sure what it would take to allow building ethtool netlink support
as a module. But that's just on a side note.

> - the command actually succeeds (i.e. changes the WoL modes)

Yes, it does.

> - output with of "ethtool --debug 0x12 -s enp3s0 wol g" looks like
> 
>   sending genetlink packet (32 bytes):
>       msg length 32 genl-ctrl
>       CTRL_CMD_GETFAMILY
>           CTRL_ATTR_FAMILY_NAME = "ethtool"
>   received genetlink packet (52 bytes):
>       msg length 52 error errno=-2
>   netlink error: No such file or directory
>   offending message:
>       ETHTOOL_MSG_LINKINFO_SET
>           ETHTOOL_A_LINKINFO_PORT = 101
> 

That's the exact output I get.

> If this is the case, than the commit found by bisect only revealed an
> issue which was introduced earlier by commit 76bdf9372824 ("netlink: use
> pretty printing for ethtool netlink messages"). The patch below should
> suppress the message as intended.
> 
> Michal
> 

Thanks, Heiner

> diff --git a/netlink/nlsock.c b/netlink/nlsock.c
> index 2c760b770ec5..c3f09b6ee9ab 100644
> --- a/netlink/nlsock.c
> +++ b/netlink/nlsock.c
> @@ -255,12 +255,12 @@ int nlsock_process_reply(struct nl_socket *nlsk, mnl_cb_t reply_cb, void *data)
>  
>  		nlhdr = (struct nlmsghdr *)buff;
>  		if (nlhdr->nlmsg_type == NLMSG_ERROR) {
> -			bool silent = nlsk->nlctx->suppress_nlerr;
> +			unsigned int suppress = nlsk->nlctx->suppress_nlerr;
>  			bool pretty;
>  
>  			pretty = debug_on(nlsk->nlctx->ctx->debug,
>  					  DEBUG_NL_PRETTY_MSG);
> -			return nlsock_process_ack(nlhdr, len, silent, pretty);
> +			return nlsock_process_ack(nlhdr, len, suppress, pretty);
>  		}
>  
>  		msgbuff->nlhdr = nlhdr;
> 

Re: ethtool 5.7: netlink ENOENT error when setting WOL

[Michal Kubecek]

On Wed, Jun 10, 2020 at 12:50:30PM +0200, Heiner Kallweit wrote:
> On 10.06.2020 11:13, Michal Kubecek wrote:
> > Just to make sure you are hitting the same problem I'm just looking at,
> > please check if
> > 
> > - your kernel is built with ETHTOOL_NETLINK=n
> 
> No, because I have PHYLIB=m.
> Not sure what it would take to allow building ethtool netlink support
> as a module. But that's just on a side note.

Yes, this is the unfortunate fallout of the new dependency between
ETHTOOL_NETLINK and PHYLIB introduced with the cable diagnostic series.
As "make oldconfig" silently disables ETHTOOL_NETLINK when PHYLIB=m,
many people won't even notice. Even I fell for this when I suddently
noticed my testing merge window snapshot has ETHTOOL_NETLINK disable.
I guess we will have to find some nicer solution.

Michal

Re: ethtool 5.7: netlink ENOENT error when setting WOL

[Heiner Kallweit]

On 10.06.2020 13:53, Michal Kubecek wrote:
> On Wed, Jun 10, 2020 at 12:50:30PM +0200, Heiner Kallweit wrote:
>> On 10.06.2020 11:13, Michal Kubecek wrote:
>>> Just to make sure you are hitting the same problem I'm just looking at,
>>> please check if
>>>
>>> - your kernel is built with ETHTOOL_NETLINK=n
>>
>> No, because I have PHYLIB=m.
>> Not sure what it would take to allow building ethtool netlink support
>> as a module. But that's just on a side note.
> 
> Yes, this is the unfortunate fallout of the new dependency between
> ETHTOOL_NETLINK and PHYLIB introduced with the cable diagnostic series.
> As "make oldconfig" silently disables ETHTOOL_NETLINK when PHYLIB=m,
> many people won't even notice. Even I fell for this when I suddently
> noticed my testing merge window snapshot has ETHTOOL_NETLINK disable.
> I guess we will have to find some nicer solution.
> 

Seems that disabling ETHTOOL_NETLINK for PHYLIB=m has (at least) one
more side effect. I just saw that ifconfig doesn't report LOWER_UP
any longer. Reason seems to be that the ioctl fallback supports
16 bits for the flags only (and IFF_LOWER_UP is bit 16).
See dev_ifsioc_locked().

> Michal
> 
Heiner

Re: ethtool 5.7: netlink ENOENT error when setting WOL

[Michal Kubecek]

On Mon, Jun 15, 2020 at 12:35:30AM +0200, Heiner Kallweit wrote:
> Seems that disabling ETHTOOL_NETLINK for PHYLIB=m has (at least) one
> more side effect. I just saw that ifconfig doesn't report LOWER_UP
> any longer. Reason seems to be that the ioctl fallback supports
> 16 bits for the flags only (and IFF_LOWER_UP is bit 16).
> See dev_ifsioc_locked().

I don't think this is related to CONFIG_ETHTOOL_NETLINK; AFAIK ifconfig
does not use netlink at all and device flags are certainly not passed
via ethtool netlink.

Michal

Re: ethtool 5.7: netlink ENOENT error when setting WOL

[Heiner Kallweit]

On 15.06.2020 01:26, Michal Kubecek wrote:
> On Mon, Jun 15, 2020 at 12:35:30AM +0200, Heiner Kallweit wrote:
>> Seems that disabling ETHTOOL_NETLINK for PHYLIB=m has (at least) one
>> more side effect. I just saw that ifconfig doesn't report LOWER_UP
>> any longer. Reason seems to be that the ioctl fallback supports
>> 16 bits for the flags only (and IFF_LOWER_UP is bit 16).
>> See dev_ifsioc_locked().
> 
> I don't think this is related to CONFIG_ETHTOOL_NETLINK; AFAIK ifconfig
> does not use netlink at all and device flags are certainly not passed
> via ethtool netlink.
> 
Right, my bad. Just mixed up the output of ifconfig and "ip link",
ip link says LOWER_UP whilst ifconfig says RUNNING.

> Michal
> 
Heiner

Re: ethtool 5.7: netlink ENOENT error when setting WOL

[Florian Fainelli]

On 6/10/2020 3:50 AM, Heiner Kallweit wrote:
> On 10.06.2020 11:13, Michal Kubecek wrote:
>> On Wed, Jun 10, 2020 at 10:52:26AM +0200, Heiner Kallweit wrote:
>>> On 10.06.2020 10:26, Heiner Kallweit wrote:
>>>> Since ethtool 5.7 following happens (kernel is latest linux-next):
>>>>
>>>> ethtool -s enp3s0 wol g
>>>> netlink error: No such file or directory
>>>>
>>>> With ethtool 5.6 this doesn't happen. I also checked the latest ethtool
>>>> git version (5.7 + some fixes), error still occurs.
>>>>
>>>> Heiner
>>>>
>>> Bisecting points to:
>>> netlink: show netlink error even without extack
>>
>> Just to make sure you are hitting the same problem I'm just looking at,
>> please check if
>>
>> - your kernel is built with ETHTOOL_NETLINK=n
> 
> No, because I have PHYLIB=m.
> Not sure what it would take to allow building ethtool netlink support
> as a module. But that's just on a side note.

Not sure it makes sense to build ETHTOOL_NETLINK as a module, but at
least ensuring that ETHTOOL_NETLINK is built into the kernel if PHYLIB=y
or PHYLIB=m would make sense, or, better we find a way to decouple the
two by using function pointers from the phy_driver directly that way
there is no symbol dependency (but reference counting has to work).
-- 
Florian

Re: ethtool 5.7: netlink ENOENT error when setting WOL

[Andrew Lunn]

> Not sure it makes sense to build ETHTOOL_NETLINK as a module, but at
> least ensuring that ETHTOOL_NETLINK is built into the kernel if PHYLIB=y
> or PHYLIB=m would make sense, or, better we find a way to decouple the
> two by using function pointers from the phy_driver directly that way
> there is no symbol dependency (but reference counting has to work).

Hi Florian

It is not so easy to make PHYLIB=m work. ethtool netlink needs to call
into the phylib core in order to trigger a cable test, not just PHY
drivers.

Ideas welcome.

      Andrew

Re: ethtool 5.7: netlink ENOENT error when setting WOL

[Heiner Kallweit]

On 10.06.2020 22:05, Andrew Lunn wrote:
>> Not sure it makes sense to build ETHTOOL_NETLINK as a module, but at
>> least ensuring that ETHTOOL_NETLINK is built into the kernel if PHYLIB=y
>> or PHYLIB=m would make sense, or, better we find a way to decouple the
>> two by using function pointers from the phy_driver directly that way
>> there is no symbol dependency (but reference counting has to work).
> 
> Hi Florian
> 
> It is not so easy to make PHYLIB=m work. ethtool netlink needs to call
> into the phylib core in order to trigger a cable test, not just PHY
> drivers.
> 
> Ideas welcome.
> 
When looking at functions like phy_start_cable_test() we could do the
following: Most of it doesn't need phylib and could be moved to
ethtool/cabletest.c. Or maybe into a separate ethtool phylib glue
code source file. The phylib calls (phy_link_down, phy_trigger_machine)
then would have to be moved into the cable_test_start callback.
I see that each callback implementation then would have some
boilerplate code. But maybe we could facilitate this with few helpers,
so that a cable test callback would look like:

phy_cable_test_boiler_start()
actual_cable_test()
phy_cable_test_boiler_end()

>       Andrew
> 
Heiner

Re: ethtool 5.7: netlink ENOENT error when setting WOL

[Florian Fainelli]

On 6/14/2020 9:14 AM, Heiner Kallweit wrote:
> On 10.06.2020 22:05, Andrew Lunn wrote:
>>> Not sure it makes sense to build ETHTOOL_NETLINK as a module, but at
>>> least ensuring that ETHTOOL_NETLINK is built into the kernel if PHYLIB=y
>>> or PHYLIB=m would make sense, or, better we find a way to decouple the
>>> two by using function pointers from the phy_driver directly that way
>>> there is no symbol dependency (but reference counting has to work).
>>
>> Hi Florian
>>
>> It is not so easy to make PHYLIB=m work. ethtool netlink needs to call
>> into the phylib core in order to trigger a cable test, not just PHY
>> drivers.
>>
>> Ideas welcome.
>>
> When looking at functions like phy_start_cable_test() we could do the
> following: Most of it doesn't need phylib and could be moved to
> ethtool/cabletest.c. Or maybe into a separate ethtool phylib glue
> code source file. The phylib calls (phy_link_down, phy_trigger_machine)
> then would have to be moved into the cable_test_start callback.
> I see that each callback implementation then would have some
> boilerplate code. But maybe we could facilitate this with few helpers,
> so that a cable test callback would look like:
> 
> phy_cable_test_boiler_start()
> actual_cable_test()
> phy_cable_test_boiler_end()

Yes, that could work, the other possibility would be to extend
ethtool_ops and add cable_test_start function pointers, and then we just
punt onto the network device driver to call the appropriate PHYLIB
function. That way we already have a mechanism in place for registering
callbacks which is based upon the network device driver lifecycle, and
from the network device driver is guaranteed to load PHYLIB because of
direct symbol dependencies. The caveat with that approach is that each
network device driver needs top opt in for those, as opposed to us
defaulting to using PHYLIB and enabling all drivers that use PHYLIB by
default.
-- 
Florian