[PATCH net 0/2] s390/qeth: cache link_info for ethtool

[PATCH net 0/2] s390/qeth: cache link_info for ethtool

[Alexandra Winter]

Cache the link info and keep it up to date, so ethtool's 
get_link_ksettings doesn't have to go down to the hardware.
Debug data has shown that this can actually solve a recovery
problem. 

Alexandra Winter (2):
  s390/qeth: update cached link_info for ethtool
  s390/qeth: use cached link_info for ethtool

 drivers/s390/net/qeth_core_main.c | 169 +++++++++---------------------
 drivers/s390/net/qeth_ethtool.c   |  12 +--
 2 files changed, 49 insertions(+), 132 deletions(-)

-- 
2.24.3 (Apple Git-128)

[PATCH net 1/2] s390/qeth: update cached link_info for ethtool

[Alexandra Winter]

Speed, duplex, port type and link mode can change, after the physical link
goes down (STOPLAN) or the card goes offline, so set them to the default
values for the card type. STARTLAN and set_online initiate a hard setup
that will restore the current values.

Signed-off-by: Alexandra Winter <wintera@linux.ibm.com>
Reviewed-by: Thorsten Winkler <twinkler@linux.ibm.com>
---
 drivers/s390/net/qeth_core_main.c | 83 ++++++++++++++++++-------------
 1 file changed, 48 insertions(+), 35 deletions(-)

diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c
index 9e54fe76a9b2..05582a7a55e2 100644
--- a/drivers/s390/net/qeth_core_main.c
+++ b/drivers/s390/net/qeth_core_main.c
@@ -763,6 +763,49 @@ static void qeth_issue_ipa_msg(struct qeth_ipa_cmd *cmd, int rc,
 				 ipa_name, com, CARD_DEVID(card));
 }
 
+static void qeth_default_link_info(struct qeth_card *card)
+{
+	struct qeth_link_info *link_info = &card->info.link_info;
+
+	QETH_CARD_TEXT(card, 2, "dftlinfo");
+	link_info->duplex = DUPLEX_FULL;
+
+	if (IS_IQD(card) || IS_VM_NIC(card)) {
+		link_info->speed = SPEED_10000;
+		link_info->port = PORT_FIBRE;
+		link_info->link_mode = QETH_LINK_MODE_FIBRE_SHORT;
+	} else {
+		switch (card->info.link_type) {
+		case QETH_LINK_TYPE_FAST_ETH:
+		case QETH_LINK_TYPE_LANE_ETH100:
+			link_info->speed = SPEED_100;
+			link_info->port = PORT_TP;
+			break;
+		case QETH_LINK_TYPE_GBIT_ETH:
+		case QETH_LINK_TYPE_LANE_ETH1000:
+			link_info->speed = SPEED_1000;
+			link_info->port = PORT_FIBRE;
+			break;
+		case QETH_LINK_TYPE_10GBIT_ETH:
+			link_info->speed = SPEED_10000;
+			link_info->port = PORT_FIBRE;
+			break;
+		case QETH_LINK_TYPE_25GBIT_ETH:
+			link_info->speed = SPEED_25000;
+			link_info->port = PORT_FIBRE;
+			break;
+		default:
+			dev_info(&card->gdev->dev,
+				 "Unknown link type %x\n",
+				 card->info.link_type);
+			link_info->speed = SPEED_UNKNOWN;
+			link_info->port = PORT_OTHER;
+		}
+
+		link_info->link_mode = QETH_LINK_MODE_UNKNOWN;
+	}
+}
+
 static struct qeth_ipa_cmd *qeth_check_ipa_data(struct qeth_card *card,
 						struct qeth_ipa_cmd *cmd)
 {
@@ -790,6 +833,7 @@ static struct qeth_ipa_cmd *qeth_check_ipa_data(struct qeth_card *card,
 				 netdev_name(card->dev), card->info.chpid);
 			qeth_issue_ipa_msg(cmd, cmd->hdr.return_code, card);
 			netif_carrier_off(card->dev);
+			qeth_default_link_info(card);
 		}
 		return NULL;
 	case IPA_CMD_STARTLAN:
@@ -4839,6 +4883,7 @@ static int qeth_init_link_info_oat_cb(struct qeth_card *card,
 	struct qeth_query_oat_physical_if *phys_if;
 	struct qeth_query_oat_reply *reply;
 
+	QETH_CARD_TEXT(card, 2, "qoatincb");
 	if (qeth_setadpparms_inspect_rc(cmd))
 		return -EIO;
 
@@ -4918,41 +4963,7 @@ static int qeth_init_link_info_oat_cb(struct qeth_card *card,
 
 static void qeth_init_link_info(struct qeth_card *card)
 {
-	card->info.link_info.duplex = DUPLEX_FULL;
-
-	if (IS_IQD(card) || IS_VM_NIC(card)) {
-		card->info.link_info.speed = SPEED_10000;
-		card->info.link_info.port = PORT_FIBRE;
-		card->info.link_info.link_mode = QETH_LINK_MODE_FIBRE_SHORT;
-	} else {
-		switch (card->info.link_type) {
-		case QETH_LINK_TYPE_FAST_ETH:
-		case QETH_LINK_TYPE_LANE_ETH100:
-			card->info.link_info.speed = SPEED_100;
-			card->info.link_info.port = PORT_TP;
-			break;
-		case QETH_LINK_TYPE_GBIT_ETH:
-		case QETH_LINK_TYPE_LANE_ETH1000:
-			card->info.link_info.speed = SPEED_1000;
-			card->info.link_info.port = PORT_FIBRE;
-			break;
-		case QETH_LINK_TYPE_10GBIT_ETH:
-			card->info.link_info.speed = SPEED_10000;
-			card->info.link_info.port = PORT_FIBRE;
-			break;
-		case QETH_LINK_TYPE_25GBIT_ETH:
-			card->info.link_info.speed = SPEED_25000;
-			card->info.link_info.port = PORT_FIBRE;
-			break;
-		default:
-			dev_info(&card->gdev->dev, "Unknown link type %x\n",
-				 card->info.link_type);
-			card->info.link_info.speed = SPEED_UNKNOWN;
-			card->info.link_info.port = PORT_OTHER;
-		}
-
-		card->info.link_info.link_mode = QETH_LINK_MODE_UNKNOWN;
-	}
+	qeth_default_link_info(card);
 
 	/* Get more accurate data via QUERY OAT: */
 	if (qeth_adp_supported(card, IPA_SETADP_QUERY_OAT)) {
@@ -5445,6 +5456,8 @@ int qeth_set_offline(struct qeth_card *card, const struct qeth_discipline *disc,
 	/* cancel any stalled cmd that might block the rtnl: */
 	qeth_clear_ipacmd_list(card);
 
+	qeth_default_link_info(card);
+
 	rtnl_lock();
 	card->info.open_when_online = card->dev->flags & IFF_UP;
 	dev_close(card->dev);
-- 
2.24.3 (Apple Git-128)

Re: [PATCH net 1/2] s390/qeth: update cached link_info for ethtool

[Andrew Lunn]

On Wed, Aug 03, 2022 at 04:40:14PM +0200, Alexandra Winter wrote:
> Speed, duplex, port type and link mode can change, after the physical link
> goes down (STOPLAN) or the card goes offline

If the link is down, speed, and duplex are meaningless. They should be
set to DUPLEX_UNKNOWN, SPEED_UNKNOWN. There is no PORT_UNKNOWN, but
generally, it does not change on link up, so you could set this
depending on the hardware type.

	Andrew

Re: [PATCH net 1/2] s390/qeth: update cached link_info for ethtool

[Alexandra Winter]

On 03.08.22 17:19, Andrew Lunn wrote:
> On Wed, Aug 03, 2022 at 04:40:14PM +0200, Alexandra Winter wrote:
>> Speed, duplex, port type and link mode can change, after the physical link
>> goes down (STOPLAN) or the card goes offline
> 
> If the link is down, speed, and duplex are meaningless. They should be
> set to DUPLEX_UNKNOWN, SPEED_UNKNOWN. There is no PORT_UNKNOWN, but
> generally, it does not change on link up, so you could set this
> depending on the hardware type.
> 
> 	Andrew

Thank you Andrew for the review. I fully understand your point.
I would like to propose that I put that on my ToDo list and fix
that in a follow-on patch to net-next.

The fields in the link_info control blocks are used today to generate
other values (e.g. supported speed) which will not work with *_UNKNOWN,
so the follow-on patch will be more than just 2 lines.
These 2 patches under review are required to solve a recovery problem, 
so I would like them to go to net asap.

Would that be ok for you?

Re: [PATCH net 1/2] s390/qeth: update cached link_info for ethtool

[Andrew Lunn]

On Thu, Aug 04, 2022 at 10:53:33AM +0200, Alexandra Winter wrote:
> 
> 
> On 03.08.22 17:19, Andrew Lunn wrote:
> > On Wed, Aug 03, 2022 at 04:40:14PM +0200, Alexandra Winter wrote:
> >> Speed, duplex, port type and link mode can change, after the physical link
> >> goes down (STOPLAN) or the card goes offline
> > 
> > If the link is down, speed, and duplex are meaningless. They should be
> > set to DUPLEX_UNKNOWN, SPEED_UNKNOWN. There is no PORT_UNKNOWN, but
> > generally, it does not change on link up, so you could set this
> > depending on the hardware type.
> > 
> > 	Andrew
> 
> Thank you Andrew for the review. I fully understand your point.
> I would like to propose that I put that on my ToDo list and fix
> that in a follow-on patch to net-next.
> 
> The fields in the link_info control blocks are used today to generate
> other values (e.g. supported speed) which will not work with *_UNKNOWN,
> so the follow-on patch will be more than just 2 lines.

So it sounds like your code is all backwards around. If you know what
the hardware is, you know the supported link modes are, assuming its
not an SFP and the SFP module is not plugged in. Those link modes
should be independent of if the link is up or not. speed/duplex is
only valid when the link is up and negotiation has finished.

Since this is for net, than yes, maybe it would be best to go with a
minimal patch to make your backwards around code work. But for
net-next, you really should fix this properly. 

	  Andrew

Re: [PATCH net 1/2] s390/qeth: update cached link_info for ethtool

[Alexandra Winter]

On 04.08.22 15:08, Andrew Lunn wrote:
> On Thu, Aug 04, 2022 at 10:53:33AM +0200, Alexandra Winter wrote:
>>
>>
>> On 03.08.22 17:19, Andrew Lunn wrote:
>>> On Wed, Aug 03, 2022 at 04:40:14PM +0200, Alexandra Winter wrote:
>>>> Speed, duplex, port type and link mode can change, after the physical link
>>>> goes down (STOPLAN) or the card goes offline
>>>
>>> If the link is down, speed, and duplex are meaningless. They should be
>>> set to DUPLEX_UNKNOWN, SPEED_UNKNOWN. There is no PORT_UNKNOWN, but
>>> generally, it does not change on link up, so you could set this
>>> depending on the hardware type.
>>>
>>> 	Andrew
>>
>> Thank you Andrew for the review. I fully understand your point.
>> I would like to propose that I put that on my ToDo list and fix
>> that in a follow-on patch to net-next.
>>
>> The fields in the link_info control blocks are used today to generate
>> other values (e.g. supported speed) which will not work with *_UNKNOWN,
>> so the follow-on patch will be more than just 2 lines.
> 
> So it sounds like your code is all backwards around. If you know what
> the hardware is, you know the supported link modes are, assuming its
> not an SFP and the SFP module is not plugged in. Those link modes
> should be independent of if the link is up or not. speed/duplex is
> only valid when the link is up and negotiation has finished.
> 
> Since this is for net, than yes, maybe it would be best to go with a
> minimal patch to make your backwards around code work. But for
> net-next, you really should fix this properly. 
> 
> 	  Andrew

Thank you Andrew. I agree with your analysis.

Re: [PATCH net 1/2] s390/qeth: update cached link_info for ethtool

[Jakub Kicinski]

On Thu, 4 Aug 2022 15:08:11 +0200 Andrew Lunn wrote:
> On Thu, Aug 04, 2022 at 10:53:33AM +0200, Alexandra Winter wrote:
> > Thank you Andrew for the review. I fully understand your point.
> > I would like to propose that I put that on my ToDo list and fix
> > that in a follow-on patch to net-next.
> > 
> > The fields in the link_info control blocks are used today to generate
> > other values (e.g. supported speed) which will not work with *_UNKNOWN,
> > so the follow-on patch will be more than just 2 lines.  
> 
> So it sounds like your code is all backwards around. If you know what
> the hardware is, you know the supported link modes are, assuming its
> not an SFP and the SFP module is not plugged in. Those link modes
> should be independent of if the link is up or not. speed/duplex is
> only valid when the link is up and negotiation has finished.

To make sure I understand - the code depends on the speed and duplex
being set to something specific when the device is _down_? Can this be
spelled out more clearly in the commit message?

> Since this is for net, than yes, maybe it would be best to go with a
> minimal patch to make your backwards around code work. But for
> net-next, you really should fix this properly. 

Then again this patch doesn't look like a regression fix (and does not
have a fixes tag). Channeling my inner Greg I'd say - fix this right and
then worry about backports later. 

Re: [PATCH net 1/2] s390/qeth: update cached link_info for ethtool

[Alexandra Winter]

On 04.08.22 22:27, Jakub Kicinski wrote:
> On Thu, 4 Aug 2022 15:08:11 +0200 Andrew Lunn wrote:
>> On Thu, Aug 04, 2022 at 10:53:33AM +0200, Alexandra Winter wrote:
>>> Thank you Andrew for the review. I fully understand your point.
>>> I would like to propose that I put that on my ToDo list and fix
>>> that in a follow-on patch to net-next.
>>>
>>> The fields in the link_info control blocks are used today to generate
>>> other values (e.g. supported speed) which will not work with *_UNKNOWN,
>>> so the follow-on patch will be more than just 2 lines.  
>>
>> So it sounds like your code is all backwards around. If you know what
>> the hardware is, you know the supported link modes are, assuming its
>> not an SFP and the SFP module is not plugged in. Those link modes
>> should be independent of if the link is up or not. speed/duplex is
>> only valid when the link is up and negotiation has finished.
> 
> To make sure I understand - the code depends on the speed and duplex
> being set to something specific when the device is _down_? Can this be
> spelled out more clearly in the commit message?
This was a discussion about existing code. We display default speed and
duplex even when the device is down. And this patch does not change that
behaviour. Andrew rightfully pointed out that this should (eventually) be
changed.
> 
>> Since this is for net, than yes, maybe it would be best to go with a
>> minimal patch to make your backwards around code work. But for
>> net-next, you really should fix this properly. 
> 
> Then again this patch doesn't look like a regression fix (and does not
> have a fixes tag). Channeling my inner Greg I'd say - fix this right and
> then worry about backports later. 
This patch is a pre-req for [PATCH net 2/2] s390/qeth: use cached link_info for ethtool
2/2 is the regression fix.
Guidance is welcome. Should I merge them into a single commit?
Or clarify in the commit message of 1/1 that this is a preparation for 2/2?

Re: [PATCH net 1/2] s390/qeth: update cached link_info for ethtool

[Jakub Kicinski]

On Fri, 5 Aug 2022 09:05:47 +0200 Alexandra Winter wrote:
> >> Since this is for net, than yes, maybe it would be best to go with a
> >> minimal patch to make your backwards around code work. But for
> >> net-next, you really should fix this properly.   
> > 
> > Then again this patch doesn't look like a regression fix (and does not
> > have a fixes tag). Channeling my inner Greg I'd say - fix this right and
> > then worry about backports later.   
> This patch is a pre-req for [PATCH net 2/2] s390/qeth: use cached link_info for ethtool
> 2/2 is the regression fix.
> Guidance is welcome. Should I merge them into a single commit?
> Or clarify in the commit message of 1/1 that this is a preparation for 2/2?

Ohh, now it makes far more sense, I see. Could you please add a line to
patch 1 saying that it's a pre-req for the next change, separated out
for ease of review? Hopefully the backport does not get confused and
pulls in both of them...

[PATCH net 2/2] s390/qeth: use cached link_info for ethtool

[Alexandra Winter]

As the cached information is kept up to date, it is no longer necessary to
query the card on each call of get_link_ksettings.
Tools like snmpd, prometheus, sadc or nmbd call get_link_ksettings, so
this has the potential to reduce system load.

This eliminates the need for qeth_query_card_info(). Since
commit e6e771b3d897 ("s390/qeth: detach netdevice while card is offline")
there was a timing window during recovery, that qeth_query_card_info could
be sent to the card, even before it was ready for it, leading to a failing
recovery. There is evidence that this window was hit, as not all callers
of get_link_ksettings() check for netif_device_present.

Fixes: e6e771b3d897 ("s390/qeth: detach netdevice while card is offline")
Signed-off-by: Alexandra Winter <wintera@linux.ibm.com>
Reviewed-by: Thorsten Winkler <twinkler@linux.ibm.com>
---
 drivers/s390/net/qeth_core_main.c | 86 -------------------------------
 drivers/s390/net/qeth_ethtool.c   | 12 +----
 2 files changed, 1 insertion(+), 97 deletions(-)

diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c
index 05582a7a55e2..10e38fe54bc9 100644
--- a/drivers/s390/net/qeth_core_main.c
+++ b/drivers/s390/net/qeth_core_main.c
@@ -4788,92 +4788,6 @@ static int qeth_query_oat_command(struct qeth_card *card, char __user *udata)
 	return rc;
 }
 
-static int qeth_query_card_info_cb(struct qeth_card *card,
-				   struct qeth_reply *reply, unsigned long data)
-{
-	struct qeth_ipa_cmd *cmd = (struct qeth_ipa_cmd *)data;
-	struct qeth_link_info *link_info = reply->param;
-	struct qeth_query_card_info *card_info;
-
-	QETH_CARD_TEXT(card, 2, "qcrdincb");
-	if (qeth_setadpparms_inspect_rc(cmd))
-		return -EIO;
-
-	card_info = &cmd->data.setadapterparms.data.card_info;
-	netdev_dbg(card->dev,
-		   "card info: card_type=0x%02x, port_mode=0x%04x, port_speed=0x%08x\n",
-		   card_info->card_type, card_info->port_mode,
-		   card_info->port_speed);
-
-	switch (card_info->port_mode) {
-	case CARD_INFO_PORTM_FULLDUPLEX:
-		link_info->duplex = DUPLEX_FULL;
-		break;
-	case CARD_INFO_PORTM_HALFDUPLEX:
-		link_info->duplex = DUPLEX_HALF;
-		break;
-	default:
-		link_info->duplex = DUPLEX_UNKNOWN;
-	}
-
-	switch (card_info->card_type) {
-	case CARD_INFO_TYPE_1G_COPPER_A:
-	case CARD_INFO_TYPE_1G_COPPER_B:
-		link_info->speed = SPEED_1000;
-		link_info->port = PORT_TP;
-		break;
-	case CARD_INFO_TYPE_1G_FIBRE_A:
-	case CARD_INFO_TYPE_1G_FIBRE_B:
-		link_info->speed = SPEED_1000;
-		link_info->port = PORT_FIBRE;
-		break;
-	case CARD_INFO_TYPE_10G_FIBRE_A:
-	case CARD_INFO_TYPE_10G_FIBRE_B:
-		link_info->speed = SPEED_10000;
-		link_info->port = PORT_FIBRE;
-		break;
-	default:
-		switch (card_info->port_speed) {
-		case CARD_INFO_PORTS_10M:
-			link_info->speed = SPEED_10;
-			break;
-		case CARD_INFO_PORTS_100M:
-			link_info->speed = SPEED_100;
-			break;
-		case CARD_INFO_PORTS_1G:
-			link_info->speed = SPEED_1000;
-			break;
-		case CARD_INFO_PORTS_10G:
-			link_info->speed = SPEED_10000;
-			break;
-		case CARD_INFO_PORTS_25G:
-			link_info->speed = SPEED_25000;
-			break;
-		default:
-			link_info->speed = SPEED_UNKNOWN;
-		}
-
-		link_info->port = PORT_OTHER;
-	}
-
-	return 0;
-}
-
-int qeth_query_card_info(struct qeth_card *card,
-			 struct qeth_link_info *link_info)
-{
-	struct qeth_cmd_buffer *iob;
-
-	QETH_CARD_TEXT(card, 2, "qcrdinfo");
-	if (!qeth_adp_supported(card, IPA_SETADP_QUERY_CARD_INFO))
-		return -EOPNOTSUPP;
-	iob = qeth_get_adapter_cmd(card, IPA_SETADP_QUERY_CARD_INFO, 0);
-	if (!iob)
-		return -ENOMEM;
-
-	return qeth_send_ipa_cmd(card, iob, qeth_query_card_info_cb, link_info);
-}
-
 static int qeth_init_link_info_oat_cb(struct qeth_card *card,
 				      struct qeth_reply *reply_priv,
 				      unsigned long data)
diff --git a/drivers/s390/net/qeth_ethtool.c b/drivers/s390/net/qeth_ethtool.c
index b0b36b2132fe..9eba0a32e9f9 100644
--- a/drivers/s390/net/qeth_ethtool.c
+++ b/drivers/s390/net/qeth_ethtool.c
@@ -428,8 +428,8 @@ static int qeth_get_link_ksettings(struct net_device *netdev,
 				   struct ethtool_link_ksettings *cmd)
 {
 	struct qeth_card *card = netdev->ml_priv;
-	struct qeth_link_info link_info;
 
+	QETH_CARD_TEXT(card, 4, "ethtglks");
 	cmd->base.speed = card->info.link_info.speed;
 	cmd->base.duplex = card->info.link_info.duplex;
 	cmd->base.port = card->info.link_info.port;
@@ -439,16 +439,6 @@ static int qeth_get_link_ksettings(struct net_device *netdev,
 	cmd->base.eth_tp_mdix = ETH_TP_MDI_INVALID;
 	cmd->base.eth_tp_mdix_ctrl = ETH_TP_MDI_INVALID;
 
-	/* Check if we can obtain more accurate information.	 */
-	if (!qeth_query_card_info(card, &link_info)) {
-		if (link_info.speed != SPEED_UNKNOWN)
-			cmd->base.speed = link_info.speed;
-		if (link_info.duplex != DUPLEX_UNKNOWN)
-			cmd->base.duplex = link_info.duplex;
-		if (link_info.port != PORT_OTHER)
-			cmd->base.port = link_info.port;
-	}
-
 	qeth_set_ethtool_link_modes(cmd, card->info.link_info.link_mode);
 
 	return 0;
-- 
2.24.3 (Apple Git-128)