netdev - Re: [PATCH v3 net-next 1/3] lan78xx: Lan7801 Support for Fixed PHY

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]

Message-ID: <df80f061-0787-d4c8-dc61-b54a38d78927@gmail.com>
Date:   Thu, 26 Apr 2018 10:24:39 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Raghuram Chary J <raghuramchary.jallipalli@...rochip.com>,
        davem@...emloft.net
Cc:     netdev@...r.kernel.org, unglinuxdriver@...rochip.com,
        woojung.huh@...rochip.com
Subject: Re: [PATCH v3 net-next 1/3] lan78xx: Lan7801 Support for Fixed PHY

On 04/26/2018 10:11 AM, Raghuram Chary J wrote:
> Adding Fixed PHY support to the lan78xx driver.
> 
> Signed-off-by: Raghuram Chary J <raghuramchary.jallipalli@...rochip.com>
> ---
> v0->v1:
>    * Remove driver version #define
>    * Modify netdev_info to netdev_dbg
>    * Move lan7801 specific to new routine and add switch case
>    * Minor cleanup
> 
> v1->v2:
>    * Removed fixedphy variable and used phy_is_pseudo_fixed_link() check.
> v2->v3:
>    * Revert driver version, debug statment changes for separate patch.
>    * Modify lan7801 specific routine with return type struct phy_device.
> ---
>  drivers/net/usb/Kconfig   |   1 +
>  drivers/net/usb/lan78xx.c | 103 ++++++++++++++++++++++++++++++++--------------
>  2 files changed, 74 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
> index f28bd74ac275..418b0904cecb 100644
> --- a/drivers/net/usb/Kconfig
> +++ b/drivers/net/usb/Kconfig
> @@ -111,6 +111,7 @@ config USB_LAN78XX
>  	select MII
>  	select PHYLIB
>  	select MICROCHIP_PHY
> +	select FIXED_PHY
>  	help
>  	  This option adds support for Microchip LAN78XX based USB 2
>  	  & USB 3 10/100/1000 Ethernet adapters.
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index 0867f7275852..ef169d73fadc 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -36,7 +36,7 @@
>  #include <linux/irq.h>
>  #include <linux/irqchip/chained_irq.h>
>  #include <linux/microchipphy.h>
> -#include <linux/phy.h>
> +#include <linux/phy_fixed.h>
>  #include "lan78xx.h"
>  
>  #define DRIVER_AUTHOR	"WOOJUNG HUH <woojung.huh@...rochip.com>"
> @@ -2003,52 +2003,90 @@ static int ksz9031rnx_fixup(struct phy_device *phydev)
>  	return 1;
>  }
>  
> -static int lan78xx_phy_init(struct lan78xx_net *dev)
> +static struct phy_device *lan7801_phy_init(struct phy_device *phydev,
> +					   struct lan78xx_net *dev)

This is confusing, why cannot you just pass a struct lan78xx_net
reference and return a phy_device reference in turn if either:

- phy_find_first() succeeds, or
- you registered a fixed PHY device?

To get there, just restructure how lan78xx_phy_init() does things by
moving the phy_find_first() under the switch()/case for
ID_REV_CHIP_ID_7800_ and ID_REV_CHIP_ID_7850_ and then you can have
lan7801_phy_init() do the phy_find_first().

Or better just merge lan78xx_phy_init() with lan7801_phy_init() maybe?
-- 
Florian