| Message ID | 20220803144015.52946-2-wintera@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | s390/qeth: cache link_info for ethtool | expand |
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
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?
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
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.
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.
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?
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...
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);