netdev - Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKWjMd4+CFZsdKvHXk201x_EKY-fQR7YUVtvhfZ8DpsimHDPAg@mail.gmail.com>
Date:	Mon, 24 Aug 2015 00:35:09 -0500
From:	Andy Fleming <afleming@...il.com>
To:	David Miller <davem@...emloft.net>
Cc:	shh.xie@...il.com,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Shaohui Xie <Shaohui.Xie@...escale.com>
Subject: Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine

On Mon, Aug 17, 2015 at 2:18 PM, David Miller <davem@...emloft.net> wrote:
>
> From: <shh.xie@...il.com>
> Date: Fri, 14 Aug 2015 12:23:40 +0800
>
> > From: Shaohui Xie <Shaohui.Xie@...escale.com>
> >
> > Currently, if phy state is PHY_RUNNING, we always register a CHANGE
> > when phy works in polling or interrupt ignored, this will make the
> > adjust_link being called even the phy link did Not changed.
> >
> > checking the phy link to make sure the link did changed before we
> > register a CHANGE, if link did not changed, we do nothing.
> >
> > Signed-off-by: Shaohui Xie <Shaohui.Xie@...escale.com>
>
> Applied, thank you.

My 3rd attempt to send this, so I apologize. Hopefully this final
attempt is finally in plain text. Clearly, it's been a while...

Florian had some good objections to this patch. Doing a PHY status
read is a very time-consuming operation to do every iteration (it's
probably thousands of cycles), and the idea is supposed to be that the
read only happens in the PHY_CHANGELINK state. Every cycle of the
state machine will read the status, and a change will require that the
status be read twice. Worst of all, if you were to change between two
links quickly, the state machine will NEVER notice the change, because
it is only looking for the link up/down state to change.

One solution might be to modify genphy_read_status() so that it
doesn't do the dummy read up front to clear the sticky low bit. This
*might* make it so that PHY_RUNNING always catches a link change. What
I'm not 100% sure of is whether you can guarantee that any link change
will require the link to first go down. We'd have to read the spec,
though I suspect this would work. The other concern with this solution
is that something might be depending on the old behavior. Certainly
phy_change() would need to make sure to do that "dummy" read sometime
after interrupts are enabled.

Another solution is to revert this patch, and modify adjust_link()
functions to only make changes if the state has changed (They all
should be doing this, anyway, but I can imagine that some
drivers/devices might have difficulty determining what's changed).

Anyone have other ideas? I'm happy to implement the first solution,
but I should note I don't have a lot of test hardware these days.

Andy
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html