netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Andrew Lunn <andrew@lunn.ch>
To: Alexandra Winter <wintera@linux.ibm.com>
Cc: David Miller <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	netdev@vger.kernel.org, linux-s390@vger.kernel.org,
	Heiko Carstens <hca@linux.ibm.com>,
	Thorsten Winkler <twinkler@linux.ibm.com>
Subject: Re: [PATCH net 1/2] s390/qeth: update cached link_info for ethtool
Date: Thu, 4 Aug 2022 15:08:11 +0200	[thread overview]
Message-ID: <YuvEu9/bzLGU2sTA@lunn.ch> (raw)
In-Reply-To: <dae87dee-67b0-30ce-91c0-a81eae8ec66f@linux.ibm.com>

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

  reply	other threads:[~2022-08-04 13:08 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-08-03 14:40 [PATCH net 0/2] s390/qeth: cache link_info for ethtool Alexandra Winter
2022-08-03 14:40 ` [PATCH net 1/2] s390/qeth: update cached " Alexandra Winter
2022-08-03 15:19   ` Andrew Lunn
2022-08-04  8:53     ` Alexandra Winter
2022-08-04 13:08       ` Andrew Lunn [this message]
2022-08-04 13:44         ` Alexandra Winter
2022-08-04 20:27         ` Jakub Kicinski
2022-08-05  7:05           ` Alexandra Winter
2022-08-05 21:29             ` Jakub Kicinski
2022-08-03 14:40 ` [PATCH net 2/2] s390/qeth: use " Alexandra Winter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YuvEu9/bzLGU2sTA@lunn.ch \
    --to=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=hca@linux.ibm.com \
    --cc=kuba@kernel.org \
    --cc=linux-s390@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=twinkler@linux.ibm.com \
    --cc=wintera@linux.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).