linux-kernel - Re: [PATCH net-next 1/1] Add genphy_c45_config

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] [day] [month] [year] [list]

Date:   Wed, 21 Aug 2019 10:55:34 +0000
From:   Marco Hartmann <marco.hartmann@....com>
To:     Heiner Kallweit <hkallweit1@...il.com>,
        Christian Herber <christian.herber@....com>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next 1/1] Add genphy_c45_config_aneg() function to
 phy-c45.c

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@....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