| 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
| ||
|
Message-ID: <555233A9.1080109@gmail.com>
Date: Tue, 12 May 2015 10:08:57 -0700
From: Florian Fainelli <f.fainelli@...il.com>
To: Bert Vermeulen <bert@...t.com>, netdev@...r.kernel.org,
jogo@...nwrt.org
Subject: Re: [PATCH] mdio-gpio: Optionally ignore TA errors
On 12/05/15 07:56, Bert Vermeulen wrote:
> Some PHYs do not always drive TA low despite returning valid data. Allow
> ignoring the TA value if we know we are connected to one of these.
>
> This is based on a patch by Jonas Gorski.
I think we need a more generic solution to this problem, not localized
only to the mdio-gpio driver as this also affects other systems using
non-bit banged MDIO bus controllers, and something that limits the TA
workaround to particular PHY addresses, not globally.
What I was thinking about is something along these lines:
- add a phy_ignore_ta_mask field to struct mii_bus
- when a MDIO bus is probed via Device Tree, scan for a "ignore-ta"
boolean property and set the phy_ignore_ta_mask bitmask accordingly
- when a MDIO bus is probed via regular platform data it can set the
phy_ignore_ta_mask directly while probing/connecting the PHY devices
- update the relevant MDIO bus drivers to check for this bit if they are
known to be interfaced with pathological PHY devices/switches that do
not release the drive TA low when expected
Does that sound reasonable to you?
>
> Signed-off-by: Bert Vermeulen <bert@...t.com>
> ---
> This fixes MDIO communication with the second AR8316 on the Mikrotik RB493G.
> The board has an AR7161 SoC, which has only one MDIO interface, but there are
> two AR8316 switch chips. The first is handled by the SoC's MDIO pins, and
> enumerates fine. The second is wired into some GPIO pins, and thus uses the
> mdio-gpio driver.
>
> Evidently, the AR8316 doesn't drive TA low when mdiobb_read() expects it, and
> the driver discards the data as a result. OpenWrt includes a simplified
> version of this patch: the check is removed altogether, and things just work.
>
> A few problems with this patch:
>
> - It's overly broad. As it turns out, this missing TA-low only happens once,
> on the first read, which is done by get_phy_device() to read the PHY id.
> Putting a discarded read in there before the real one ALSO fixes the problem:
> the second and subsequent reads are fine. Unfortunately I can't force a
> dummy read on this board without smashing through a bunch of layers.
>
> - I'm not entirely convinced that the PHY's bringing TA low needs checking
> to begin with. The standard clearly says that the PHY "shall" do it, but
> it's equally clear that the MDIO interface on the AR7161 doesn't have a
> problem talking to the AR8316. Presumably there are other SoCs in the wild
> talking to AR8316 MDIO interfaces.
>
> - It could also be a timing problem in mdiobb_read(), but I haven't been
> able to find it if so. Unfortunately my board doesn't really allow me
> to stick a logic analyzer on there and figure out what's different between
> the two MDIO channels.
>
> Advice, anyone?
>
>
> drivers/net/phy/mdio-bitbang.c | 2 +-
> drivers/net/phy/mdio-gpio.c | 1 +
> include/linux/mdio-bitbang.h | 3 +++
> include/linux/mdio-gpio.h | 2 ++
> 4 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/mdio-bitbang.c b/drivers/net/phy/mdio-bitbang.c
> index daec9b0..306fb74 100644
> --- a/drivers/net/phy/mdio-bitbang.c
> +++ b/drivers/net/phy/mdio-bitbang.c
> @@ -166,7 +166,7 @@ static int mdiobb_read(struct mii_bus *bus, int phy, int reg)
> ctrl->ops->set_mdio_dir(ctrl, 0);
>
> /* check the turnaround bit: the PHY should be driving it to zero */
> - if (mdiobb_get_bit(ctrl) != 0) {
> + if (mdiobb_get_bit(ctrl) != 0 && !ctrl->ignore_ta_errors) {
> /* PHY didn't drive TA low -- flush any bits it
> * may be trying to send.
> */
> diff --git a/drivers/net/phy/mdio-gpio.c b/drivers/net/phy/mdio-gpio.c
> index 0a0578a..f5cddf5 100644
> --- a/drivers/net/phy/mdio-gpio.c
> +++ b/drivers/net/phy/mdio-gpio.c
> @@ -138,6 +138,7 @@ static struct mii_bus *mdio_gpio_bus_init(struct device *dev,
> if (!bitbang)
> goto out;
>
> + bitbang->ctrl.ignore_ta_errors = pdata->ignore_ta_errors;
> bitbang->ctrl.ops = &mdio_gpio_ops;
> bitbang->ctrl.reset = pdata->reset;
> bitbang->mdc = pdata->mdc;
> diff --git a/include/linux/mdio-bitbang.h b/include/linux/mdio-bitbang.h
> index 76f52bb..3469bf2 100644
> --- a/include/linux/mdio-bitbang.h
> +++ b/include/linux/mdio-bitbang.h
> @@ -31,6 +31,9 @@ struct mdiobb_ops {
> };
>
> struct mdiobb_ctrl {
> + /* Some PHYs don't drive TA low even though they completed successfully. */
> + unsigned int ignore_ta_errors:1;
> +
> const struct mdiobb_ops *ops;
> /* reset callback */
> int (*reset)(struct mii_bus *bus);
> diff --git a/include/linux/mdio-gpio.h b/include/linux/mdio-gpio.h
> index 66c30a7..cfe6d1b 100644
> --- a/include/linux/mdio-gpio.h
> +++ b/include/linux/mdio-gpio.h
> @@ -19,6 +19,8 @@ struct mdio_gpio_platform_data {
> unsigned int mdio;
> unsigned int mdo;
>
> + unsigned int ignore_ta_errors:1;
> +
> bool mdc_active_low;
> bool mdio_active_low;
> bool mdo_active_low;
>
--
Florian
--
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
Powered by blists - more mailing lists