netdev - Re: [PATCH 1/2] net: phy: core: use genphy version of callbacks read_status and 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <8aab2425-9738-f54c-dd57-2cf38236ae3f@gmail.com>
Date:   Wed, 29 Nov 2017 21:47:16 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>,
        David Miller <davem@...emloft.net>,
        Andrew Lunn <andrew@...n.ch>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH 1/2] net: phy: core: use genphy version of callbacks
 read_status and config_aneg per default

Am 15.11.2017 um 22:56 schrieb Florian Fainelli:
> On 11/15/2017 01:42 PM, Heiner Kallweit wrote:
>> read_status and config_aneg are the only mandatory callbacks and most
>> of the time the generic implementation is used by drivers.
>> So make the core fall back to the generic version if a driver doesn't
>> implement the respective callback.
>>
>> Also currently the core doesn't seem to verify that drivers implement
>> the mandatory calls. If a driver doesn't do so we'd just get a NPE.
> 
> Right, which is not an unusual way to signal that something is mandatory.
> 
>> With this patch this potential issue doesn't exit any longer.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
> 
> Reviewed-by: Florian Fainelli <f.fainelli@...il.com>
> 
> Note that net-next is closed at the moment, so you will have to resubmit
> this when the tree opens back again.
> 
I see that the two patches have status "deferred" in patchwork.
So do I have to actually resubmit or are they going to be be picked up
from patchwork?

Rgds, Heiner

>> ---
>>  drivers/net/phy/phy.c |  5 ++++-
>>  include/linux/phy.h   | 33 ++++++++++++++++++---------------
>>  2 files changed, 22 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index 2b1e67bc1..a0e7605dc 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -493,7 +493,10 @@ static int phy_start_aneg_priv(struct phy_device *phydev, bool sync)
>>  	/* Invalidate LP advertising flags */
>>  	phydev->lp_advertising = 0;
>>  
>> -	err = phydev->drv->config_aneg(phydev);
>> +	if (phydev->drv->config_aneg)
>> +		err = phydev->drv->config_aneg(phydev);
>> +	else
>> +		err = genphy_config_aneg(phydev);
>>  	if (err < 0)
>>  		goto out_unlock;
>>  
>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>> index dc82a07cb..958b5162a 100644
>> --- a/include/linux/phy.h
>> +++ b/include/linux/phy.h
>> @@ -497,13 +497,13 @@ struct phy_device {
>>   * flags: A bitfield defining certain other features this PHY
>>   *   supports (like interrupts)
>>   *
>> - * The drivers must implement config_aneg and read_status.  All
>> - * other functions are optional. Note that none of these
>> - * functions should be called from interrupt time.  The goal is
>> - * for the bus read/write functions to be able to block when the
>> - * bus transaction is happening, and be freed up by an interrupt
>> - * (The MPC85xx has this ability, though it is not currently
>> - * supported in the driver).
>> + * All functions are optional. If config_aneg or read_status
>> + * are not implemented, the phy core uses the genphy versions.
>> + * Note that none of these functions should be called from
>> + * interrupt time. The goal is for the bus read/write functions
>> + * to be able to block when the bus transaction is happening,
>> + * and be freed up by an interrupt (The MPC85xx has this ability,
>> + * though it is not currently supported in the driver).
>>   */
>>  struct phy_driver {
>>  	struct mdio_driver_common mdiodrv;
>> @@ -841,14 +841,6 @@ int phy_aneg_done(struct phy_device *phydev);
>>  int phy_stop_interrupts(struct phy_device *phydev);
>>  int phy_restart_aneg(struct phy_device *phydev);
>>  
>> -static inline int phy_read_status(struct phy_device *phydev)
>> -{
>> -	if (!phydev->drv)
>> -		return -EIO;
>> -
>> -	return phydev->drv->read_status(phydev);
>> -}
>> -
>>  #define phydev_err(_phydev, format, args...)	\
>>  	dev_err(&_phydev->mdio.dev, format, ##args)
>>  
>> @@ -890,6 +882,17 @@ int genphy_c45_read_pma(struct phy_device *phydev);
>>  int genphy_c45_pma_setup_forced(struct phy_device *phydev);
>>  int genphy_c45_an_disable_aneg(struct phy_device *phydev);
>>  
>> +static inline int phy_read_status(struct phy_device *phydev)
>> +{
>> +	if (!phydev->drv)
>> +		return -EIO;
>> +
>> +	if (phydev->drv->read_status)
>> +		return phydev->drv->read_status(phydev);
>> +	else
>> +		return genphy_read_status(phydev);
>> +}
>> +
>>  void phy_driver_unregister(struct phy_driver *drv);
>>  void phy_drivers_unregister(struct phy_driver *drv, int n);
>>  int phy_driver_register(struct phy_driver *new_driver, struct module *owner);
>>
> 
>