[PATCH net-next 0/1] Add genphy_c45_config_aneg() function to phy-c45.c

[PATCH net-next 0/1] Add genphy_c45_config_aneg() function to phy-c45.c

[Marco Hartmann]

Currently, using a Clause 45 Phy with the "Generic Clause 45 PHY"
driver leads to a warning, similar to the one below, as soon as the interface
is brought up.


  ------------[ cut here ]------------
  WARNING: CPU: 2 PID: 146 at drivers/net/phy/phy.c:736 phy_error+0x2c/0x64
  Modules linked in: fec
  CPU: 2 PID: 146 Comm: kworker/2:1 Not tainted 5.3.0-rc3-NETNEXT-00816-g48e924c73178 #20
  Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
  Workqueue: events_power_efficient phy_state_machine
  Backtrace: 
  ...

This happens, because the Genphy driver does not provide a config_aneg() func,
so that phy_start_aneg() ultimately fails such that phy_error() is called,
producing the above warning.

This patch adds the function genphy_c45_config_aneg(), which allows
phy_start_aneg() to work correctly for C45 phys.


Marco Hartmann (1):
  Add genphy_c45_config_aneg() function to phy-c45.c

 drivers/net/phy/phy-c45.c | 26 ++++++++++++++++++++++++++
 drivers/net/phy/phy.c     |  2 +-
 include/linux/phy.h       |  1 +
 3 files changed, 28 insertions(+), 1 deletion(-)

-- 
2.7.4

[PATCH net-next 1/1] Add genphy_c45_config_aneg() function to phy-c45.c

[Marco Hartmann]

and call it from phy_config_aneg().

commit 34786005eca3 ("net: phy: prevent PHYs w/o Clause 22 regs from
calling genphy_config_aneg") introduced a check that aborts
phy_config_aneg() if the phy is a C45 phy.
This causes phy_state_machine() to call phy_error() so that the phy
ends up in PHY_HALTED state.

Instead of returning -EOPNOTSUPP, call genphy_c45_config_aneg()
(analogous to the C22 case) so that the state machine can run
correctly.

genphy_c45_config_aneg() closely resembles mv3310_config_aneg()
in drivers/net/phy/marvell10g.c, excluding vendor specific
configurations for 1000BaseT.

Fixes: 34786005eca3 ("net: phy: prevent PHYs w/o Clause 22 regs from
calling genphy_config_aneg")

Signed-off-by: Marco Hartmann <marco.hartmann@nxp.com>
---
 drivers/net/phy/phy-c45.c | 26 ++++++++++++++++++++++++++
 drivers/net/phy/phy.c     |  2 +-
 include/linux/phy.h       |  1 +
 3 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index b9d4145781ca..fa9062fd9122 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -509,6 +509,32 @@ int genphy_c45_read_status(struct phy_device *phydev)
 }
 EXPORT_SYMBOL_GPL(genphy_c45_read_status);
 
+/**
+ * genphy_c45_config_aneg - restart auto-negotiation or forced setup
+ * @phydev: target phy_device struct
+ *
+ * Description: If auto-negotiation is enabled, we configure the
+ *   advertising, and then restart auto-negotiation.  If it is not
+ *   enabled, then we force a configuration.
+ */
+int genphy_c45_config_aneg(struct phy_device *phydev)
+{
+	int ret;
+	bool changed = false;
+
+	if (phydev->autoneg == AUTONEG_DISABLE)
+		return genphy_c45_pma_setup_forced(phydev);
+
+	ret = genphy_c45_an_config_aneg(phydev);
+	if (ret < 0)
+		return ret;
+	if (ret > 0)
+		changed = true;
+
+	return genphy_c45_check_and_restart_aneg(phydev, changed);
+}
+EXPORT_SYMBOL_GPL(genphy_c45_config_aneg);
+
 /* The gen10g_* functions are the old Clause 45 stub */
 
 int gen10g_config_aneg(struct phy_device *phydev)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index f3adea9ef400..74c4e15ebe52 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -507,7 +507,7 @@ static int phy_config_aneg(struct phy_device *phydev)
 	 * allowed to call genphy_config_aneg()
 	 */
 	if (phydev->is_c45 && !(phydev->c45_ids.devices_in_package & BIT(0)))
-		return -EOPNOTSUPP;
+		return genphy_c45_config_aneg(phydev);
 
 	return genphy_config_aneg(phydev);
 }
diff --git a/include/linux/phy.h b/include/linux/phy.h
index d26779f1fb6b..a7ecbe0e55aa 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1117,6 +1117,7 @@ int genphy_c45_an_disable_aneg(struct phy_device *phydev);
 int genphy_c45_read_mdix(struct phy_device *phydev);
 int genphy_c45_pma_read_abilities(struct phy_device *phydev);
 int genphy_c45_read_status(struct phy_device *phydev);
+int genphy_c45_config_aneg(struct phy_device *phydev);
 
 /* The gen10g_* functions are the old Clause 45 stub */
 int gen10g_config_aneg(struct phy_device *phydev);
-- 
2.7.4

Re: [PATCH net-next 1/1] Add genphy_c45_config_aneg() function to phy-c45.c

[Heiner Kallweit]

On 19.08.2019 19:52, Marco Hartmann wrote:
> and call it from phy_config_aneg().
> 
Here something went wrong.

> commit 34786005eca3 ("net: phy: prevent PHYs w/o Clause 22 regs from
> calling genphy_config_aneg") introduced a check that aborts
> phy_config_aneg() if the phy is a C45 phy.
> This causes phy_state_machine() to call phy_error() so that the phy
> ends up in PHY_HALTED state.
> 
> Instead of returning -EOPNOTSUPP, call genphy_c45_config_aneg()
> (analogous to the C22 case) so that the state machine can run
> correctly.
> 
> genphy_c45_config_aneg() closely resembles mv3310_config_aneg()
> in drivers/net/phy/marvell10g.c, excluding vendor specific
> configurations for 1000BaseT.
> 
> Fixes: 34786005eca3 ("net: phy: prevent PHYs w/o Clause 22 regs from
> calling genphy_config_aneg")
> 
This tag seems to be the wrong one. This change was done before
genphy_c45_driver was added. Most likely tag should be:
22b56e827093 ("net: phy: replace genphy_10g_driver with genphy_c45_driver")
And because it's a fix applying to previous kernel versions it should
be annotated "net", not "net-next".

> Signed-off-by: Marco Hartmann <marco.hartmann@nxp.com>
> ---
>  drivers/net/phy/phy-c45.c | 26 ++++++++++++++++++++++++++
>  drivers/net/phy/phy.c     |  2 +-
>  include/linux/phy.h       |  1 +
>  3 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> index b9d4145781ca..fa9062fd9122 100644
> --- a/drivers/net/phy/phy-c45.c
> +++ b/drivers/net/phy/phy-c45.c
> @@ -509,6 +509,32 @@ int genphy_c45_read_status(struct phy_device *phydev)
>  }
>  EXPORT_SYMBOL_GPL(genphy_c45_read_status);
>  
> +/**
> + * genphy_c45_config_aneg - restart auto-negotiation or forced setup
> + * @phydev: target phy_device struct
> + *
> + * Description: If auto-negotiation is enabled, we configure the
> + *   advertising, and then restart auto-negotiation.  If it is not
> + *   enabled, then we force a configuration.
> + */
> +int genphy_c45_config_aneg(struct phy_device *phydev)
> +{
> +	int ret;
> +	bool changed = false;

Reverse xmas tree please.

> [...]

Overall looks good to me. For a single patch you don't have to provide
a cover letter.

Re: [PATCH net-next 1/1] Add genphy_c45_config_aneg() function to phy-c45.c

[Marco Hartmann]

On 19.08.19 21:51, Heiner Kallweit wrote:
> On 19.08.2019 19:52, Marco Hartmann wrote:
>> and call it from phy_config_aneg().
>>
> Here something went wrong.
> 
>> commit 34786005eca3 ("net: phy: prevent PHYs w/o Clause 22 regs from
>> calling genphy_config_aneg") introduced a check that aborts
>> phy_config_aneg() if the phy is a C45 phy.
>> This causes phy_state_machine() to call phy_error() so that the phy
>> ends up in PHY_HALTED state.
>>
>> Instead of returning -EOPNOTSUPP, call genphy_c45_config_aneg()
>> (analogous to the C22 case) so that the state machine can run
>> correctly.
>>
>> genphy_c45_config_aneg() closely resembles mv3310_config_aneg()
>> in drivers/net/phy/marvell10g.c, excluding vendor specific
>> configurations for 1000BaseT.
>>
>> Fixes: 34786005eca3 ("net: phy: prevent PHYs w/o Clause 22 regs from
>> calling genphy_config_aneg")
>>
> This tag seems to be the wrong one. This change was done before
> genphy_c45_driver was added. Most likely tag should be:
> 22b56e827093 ("net: phy: replace genphy_10g_driver with genphy_c45_driver")
> And because it's a fix applying to previous kernel versions it should
> be annotated "net", not "net-next".
> 
You are correct, I fixed the tag and annotation.

>> Signed-off-by: Marco Hartmann <marco.hartmann@nxp.com>
>> ---
>>   drivers/net/phy/phy-c45.c | 26 ++++++++++++++++++++++++++
>>   drivers/net/phy/phy.c     |  2 +-
>>   include/linux/phy.h       |  1 +
>>   3 files changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
>> index b9d4145781ca..fa9062fd9122 100644
>> --- a/drivers/net/phy/phy-c45.c
>> +++ b/drivers/net/phy/phy-c45.c
>> @@ -509,6 +509,32 @@ int genphy_c45_read_status(struct phy_device *phydev)
>>   }
>>   EXPORT_SYMBOL_GPL(genphy_c45_read_status);
>>   
>> +/**
>> + * genphy_c45_config_aneg - restart auto-negotiation or forced setup
>> + * @phydev: target phy_device struct
>> + *
>> + * Description: If auto-negotiation is enabled, we configure the
>> + *   advertising, and then restart auto-negotiation.  If it is not
>> + *   enabled, then we force a configuration.
>> + */
>> +int genphy_c45_config_aneg(struct phy_device *phydev)
>> +{
>> +	int ret;
>> +	bool changed = false;
> 
> Reverse xmas tree please.
> 
ok.

>> [...]
> 
> Overall looks good to me. For a single patch you don't have to provide
> a cover letter.
> 

Thank you for your feedback,
I will provide a v2 of the patch with the above fixes.

Regards,
Marco