Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 11 Jun 2022 18:11:57 +0200
From:   Andrew Lunn <andrew@lunn.ch>
To:     Oleksij Rempel <o.rempel@pengutronix.de>
Cc:     Heiner Kallweit <hkallweit1@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Michal Kubecek <mkubecek@suse.cz>, kernel@pengutronix.de,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH net-next v1 1/1] net: phy: add remote fault support
Message-ID: <YqS+zYHf6eHMWJlD@lunn.ch>
References: <20220608093403.3999446-1-o.rempel@pengutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220608093403.3999446-1-o.rempel@pengutronix.de>
Precedence: bulk

> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> index c67bf3060173..6c55c7f9b680 100644
> --- a/drivers/net/phy/phy-c45.c
> +++ b/drivers/net/phy/phy-c45.c
> @@ -205,7 +205,7 @@ static int genphy_c45_baset1_an_config_aneg(struct phy_device *phydev)
>  		break;
>  	case MASTER_SLAVE_CFG_UNKNOWN:
>  	case MASTER_SLAVE_CFG_UNSUPPORTED:
> -		return 0;
> +		break;
>  	default:
>  		phydev_warn(phydev, "Unsupported Master/Slave mode\n");
>  		return -EOPNOTSUPP;
> @@ -223,11 +223,16 @@ static int genphy_c45_baset1_an_config_aneg(struct phy_device *phydev)
>  		break;
>  	}
>  
> +	if (phydev->remote_fault_set >= REMOTE_FAULT_CFG_ERROR)
> +		adv_l |= MDIO_AN_T1_ADV_L_REMOTE_FAULT;
> +
>  	adv_l |= linkmode_adv_to_mii_t1_adv_l_t(phydev->advertising);
>  
>  	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_T1_ADV_L,
> -				     (MDIO_AN_T1_ADV_L_FORCE_MS | MDIO_AN_T1_ADV_L_PAUSE_CAP
> -				     | MDIO_AN_T1_ADV_L_PAUSE_ASYM), adv_l);
> +				     (MDIO_AN_T1_ADV_L_FORCE_MS |
> +				      MDIO_AN_T1_ADV_L_PAUSE_CAP |
> +				      MDIO_AN_T1_ADV_L_PAUSE_ASYM |
> +				      MDIO_AN_T1_ADV_L_REMOTE_FAULT), adv_l);

Since this is part of config_aneg, i assume you have to trigger an
renegotiation, which will put the link down for a while. Is that
actually required? Can the fault indicator be set at runtime without a
full auto-neg? I suppose for a fault indicator, it probably does not
matter, there is a fault... But i'm wondering about future extensions
which might want to send values when the link is up. I've seen some
PHYs indicate their make/model, etc. What sort of API would be needed
for that?

It might also be useful if we could send an event to userspace when
the receive state changes, so there is no need to poll. I thought
something link a linkstate message was broadcast under some
conditions? That again my suggest ksetting is maybe not the best place
for this?

I see no problem in exposing this information, but i would like to be
sure we get the API correct.

     Andrew