Message-ID: <alpine.LFD.2.21.1808040751170.704@localhost.localdomain>
Date:   Sat, 4 Aug 2018 07:57:56 -0400 (EDT)
From:   "Robert P. J. Day" <rpjday@...shcourse.ca>
To:     Jiri Pirko <jiri@...nulli.us>
cc:     Linux kernel ntedev mailing list <netdev@...r.kernel.org>
Subject: Re: consequences of setting net_device_ops ndo_change_carrier()?

On Sat, 4 Aug 2018, Jiri Pirko wrote:

> Sat, Aug 04, 2018 at 01:06:58PM CEST, rpjday@...shcourse.ca wrote:
> >
> >  i'll try to keep this (relatively) short as there may be a simple
> >answer to this, or it could just be a stupid question -- sort of
> >related to previous question (thank you, florian).
> >
> >  currently messing with networking device involving FPGA and some
> >quad-port transceivers, and noticed that, when one unplugs or plugs
> >a device into one of the ports, there is no change in the contents
> >of the corresponding sysfs files /sys/class/net/<ifname>/carrier
> >(or operstate, for that matter, which might be related to this as
> >well). doing this with a "regular" port on my linux laptop
> >certainly confirmed that the carrier file would switch between 0
> >and 1, and operstate would switch between up and down, so i know
> >what behaviour i was *expecting* if things were ostensibly working
> >properly.
> >
> >  long story short, i pawed through the driver code only to stumble
>
> What driver? Has to be out of tree as I don't see any in the
> existing kernel using .ndo_change_carrier (aside of team and dummy)

  yes, currently proprietary and in-house under development, so i have
to be a little vague about certain details.

> >over this in the ethernet driver for the device:
> >
> >  static const struct net_device_ops netdev_netdev_ops = {
> >  ... snip ...
> >        .ndo_change_carrier     = netdev_change_carrier,
> >  ... snip ...
> >  };
> >
> >and
> >
> >  static int
> >  netdev_change_carrier(struct net_device *dev, bool new_carrier)
> >  {
> >        if (new_carrier)
> >                netif_carrier_on(dev);
> >        else
> >                netif_carrier_off(dev);
> >        return 0;
> >  }
> >
> >as i mentioned before, i am really new to kernel networking code,
> >so i did a quick search and found this in netdevice.h:
> >
> >* int (*ndo_change_carrier)(struct net_device *dev, bool new_carrier);
> > *      Called to change device carrier. Soft-devices (like dummy, team, etc)
> > *      which do not represent real hardware may define this to allow their
> > *      userspace components to manage their virtual carrier state. Devices
> > *      that determine carrier state from physical hardware properties (eg
> > *      network cables) or protocol-dependent mechanisms (eg
> > *      USB_CDC_NOTIFY_NETWORK_CONNECTION) should NOT implement this function.
> > *
> >
> >although i still don't fully understand the purpose of that field,
> >it makes me *very* nervous to read that that routine is for "soft"
> >devices, and ***not*** for devices that attempt to determine
> >carrier state from physical hardware properties. i searched the
> >kernel code base for other drivers that set that field, and found
> >only what is mentioned in that comment -- dummy.c, of_dummy_mac.c
> >and team.c.
> >
> >  the testers for this unit are complaining that they are somehow
> >not being notified when they plug and unplug devices from the ports
> >-- is this why? what would be the purpose of assigning a routine to
> >that field? as i read it (and i could be wrong), my impression is
> >that you can have the driver *either* determine the carrier state
> >from physical properties, *or* allow userspace control, but not
> >both, is that correct?
>
> Correct. Your device is physical device, it knows how to get the
> state of the carrier itself.

  that's what i *thought*, good to have confirmation.

> >
> >  i'm about to ask the original authors why they did the above, but
>
> I guess that the reason is that they had no clue what they are doing
> :)

  given that i've been immersed in networking code for only a few
days, i was not about to draw any conclusion like that. :-) i'm going
to continue perusing the code just to feel more confident about my
eventual conclusion, but it would seem that there is no compelling
reason for setting ndo_change_carrier() for actual physical devices,
and that is quite possibly the cause of the weird behaviour the
testers are seeing.  thanks muchly.

rday

-- 

========================================================================
Robert P. J. Day                                 Ottawa, Ontario, CANADA
                  http://crashcourse.ca/dokuwiki

Twitter:                                       http://twitter.com/rpjday
LinkedIn:                               http://ca.linkedin.com/in/rpjday
========================================================================

Powered by blists - more mailing lists