Re: [EXT] Re: [net-next PATCH 1/2] octeontx2-pf: Add devlink param to init and de-init serdes

On Sat, Oct 30, 2021 at 12:55:47PM +0530, sundeep subbaraya wrote:

Hi Ido,

On Thu, Oct 28, 2021 at 7:21 PM Ido Schimmel [off-list ref] wrote:

quoted

On Thu, Oct 28, 2021 at 05:48:02PM +0530, sundeep subbaraya wrote:

quoted

Actually we also need a case where debugging is required when the
logical link is
up (so that packets flow from kernel to SerDes continuously) but the
physical link
is down.

Can you explain the motivation for that? In the past we discussed use
cases for forcing the operational state to down while the administrative
state is up and couldn't find any.

To be honest we got this request from a customer to provide a command to modify
physical link without tying it to a logical link. I have asked for
more details on how
they use it.

Thanks

quoted

quoted

We will change the commit description since it is giving the
wrong impression.
A command to change physical link up/down with no relation to ifconfig
is needed.

So it is obvious that some drivers default to not shutting down the
physical link upon admin down, but that some users want to change that.
In addition, we have your use case to control the physical link without
relation to the logical link. I wonder if it can all be solved with a
new ethtool attribute (part of LINKINFO_{SET,GET} ?) that describes the
physical link policy and has the following values:

* auto: Physical link state is derived from logical link state
* up: Physical link state is always up
* down: Physical link state is always down

IIUC, it should solve your problem and that of the "link-down-on-close"
private flag. It also has the added benefit of allowing user space to
query the default policy. The expectation is that it would be "auto",
but in some scenarios it is "up".

This looks good. Please note that we need the behavior such that after changing
the flag a subsequent ifconfig command is not needed by the user.

auto : in ndo_open, ndo_close check the physical link flag is auto and
send command
          to firmware for bringing physical link up/down.
up: send command to firmware instantaneously for physical link UP
down: send command to firmware instantaneously for physical link DOWN

TBH, I'm not that happy with my ethtool suggestion. It is not very clear
which hardware entities the attribute controls. Maybe it's better to
implement it as a rtnetlink attribute that controls the carrier (e.g.,
"carrier_policy")? Note that we already have ndo_change_carrier(), but
the kdoc comment explicitly mentions that it shouldn't be used by
physical devices:

 * 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.