Re: consequences of setting net_device_ops ndo_change_carrier()?

[Stephen Hemminger <stephen@networkplumber.org>]

On Sat, 4 Aug 2018 07:57:56 -0400 (EDT)
"Robert P. J. Day" <rpjday@crashcourse.ca> wrote:

> On Sat, 4 Aug 2018, Jiri Pirko wrote:
> 
> > Sat, Aug 04, 2018 at 01:06:58PM CEST, rpjday@crashcourse.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

ndo_change_carrier is not the droid your looking for.

The purpose of ndo_change_carrier was for testing network devices
(ie dummy), and also for cases like network tunnels where the
sofrware carrier state may be controlled by a userspace daemon.

Real network devices  call netif_carrier_on and netif_carrier_off
when they notice change in carrier state in hardware. Typically,
this is when an interrupt happens.