Re: [PATCH net-next 4/4] bonding: allow carrier and link status to determine link state

[Jay Vosburgh <jay.vosburgh@canonical.com>]

Debabrata Banerjee <dbanerje@akamai.com> wrote:

>In a mixed environment it may be difficult to tell if your hardware
>support carrier, if it does not it can always report true. With a new
>use_carrier option of 2, we can check both carrier and link status
>sequentially, instead of one or the other

	What do you mean by "mixed environment," and under what
circumstances are you seeing an actual benefit from doing the MII /
ethtool test in addition to the standard netif_carrier_ok test?

	The use_carrier option was meant for backwards compatibility
with old-in-2005 device drivers, so this seem counterintuitive to me.  I
don't recall seeing any devices lacking netif_carrier support for some
time.  At this point, I would tend to argue that a new device driver
that does not implement netif_carrier support should be fixed, and not
have another hack added to bonding to work around it.

	-J

>Signed-off-by: Debabrata Banerjee <dbanerje@akamai.com>
>---
> Documentation/networking/bonding.txt |  4 ++--
> drivers/net/bonding/bond_main.c      | 12 ++++++++----
> drivers/net/bonding/bond_options.c   |  7 ++++---
> 3 files changed, 14 insertions(+), 9 deletions(-)
>
>diff --git a/Documentation/networking/bonding.txt b/Documentation/networking/bonding.txt
>index 9ba04c0bab8d..f063730e7e73 100644
>--- a/Documentation/networking/bonding.txt
>+++ b/Documentation/networking/bonding.txt
>@@ -828,8 +828,8 @@ use_carrier
> 	MII / ETHTOOL ioctl method to determine the link state.
> 
> 	A value of 1 enables the use of netif_carrier_ok(), a value of
>-	0 will use the deprecated MII / ETHTOOL ioctls.  The default
>-	value is 1.
>+	0 will use the deprecated MII / ETHTOOL ioctls. A value of 2
>+	will check both.  The default value is 1.
> 
> xmit_hash_policy
> 
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index f7f8a49cb32b..7e9652c4b35c 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -132,7 +132,7 @@ MODULE_PARM_DESC(downdelay, "Delay before considering link down, "
> 			    "in milliseconds");
> module_param(use_carrier, int, 0);
> MODULE_PARM_DESC(use_carrier, "Use netif_carrier_ok (vs MII ioctls) in miimon; "
>-			      "0 for off, 1 for on (default)");
>+			      "0 for off, 1 for on (default), 2 for carrier then legacy checks");
> module_param(mode, charp, 0);
> MODULE_PARM_DESC(mode, "Mode of operation; 0 for balance-rr, "
> 		       "1 for active-backup, 2 for balance-xor, "
>@@ -434,12 +434,16 @@ static int bond_check_dev_link(struct bonding *bond,
> 	int (*ioctl)(struct net_device *, struct ifreq *, int);
> 	struct ifreq ifr;
> 	struct mii_ioctl_data *mii;
>+	bool carrier = true;
> 
> 	if (!reporting && !netif_running(slave_dev))
> 		return 0;
> 
> 	if (bond->params.use_carrier)
>-		return netif_carrier_ok(slave_dev) ? BMSR_LSTATUS : 0;
>+		carrier = netif_carrier_ok(slave_dev) ? BMSR_LSTATUS : 0;
>+
>+	if (!carrier)
>+		return carrier;
> 
> 	/* Try to get link status using Ethtool first. */
> 	if (slave_dev->ethtool_ops->get_link)
>@@ -4399,8 +4403,8 @@ static int bond_check_params(struct bond_params *params)
> 		downdelay = 0;
> 	}
> 
>-	if ((use_carrier != 0) && (use_carrier != 1)) {
>-		pr_warn("Warning: use_carrier module parameter (%d), not of valid value (0/1), so it was set to 1\n",
>+	if (use_carrier < 0 || use_carrier > 2) {
>+		pr_warn("Warning: use_carrier module parameter (%d), not of valid value (0-2), so it was set to 1\n",
> 			use_carrier);
> 		use_carrier = 1;
> 	}
>diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>index 8a945c9341d6..dba6cef05134 100644
>--- a/drivers/net/bonding/bond_options.c
>+++ b/drivers/net/bonding/bond_options.c
>@@ -164,9 +164,10 @@ static const struct bond_opt_value bond_primary_reselect_tbl[] = {
> };
> 
> static const struct bond_opt_value bond_use_carrier_tbl[] = {
>-	{ "off", 0,  0},
>-	{ "on",  1,  BOND_VALFLAG_DEFAULT},
>-	{ NULL,  -1, 0}
>+	{ "off",  0,  0},
>+	{ "on",   1,  BOND_VALFLAG_DEFAULT},
>+	{ "both", 2,  0},
>+	{ NULL,  -1,  0}
> };
> 
> static const struct bond_opt_value bond_all_slaves_active_tbl[] = {
>-- 
>2.17.0
>

---
	-Jay Vosburgh, jay.vosburgh@canonical.com