netdev - RE: [PATCH v2 net-next] 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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 26 Apr 2018 06:27:57 +0000
From:   <RaghuramChary.Jallipalli@...rochip.com>
To:     <f.fainelli@...il.com>, <davem@...emloft.net>
CC:     <netdev@...r.kernel.org>, <UNGLinuxDriver@...rochip.com>,
        <Woojung.Huh@...rochip.com>
Subject: RE: [PATCH v2 net-next] lan78xx: Lan7801 Support for Fixed PHY

Hi Florian,

> > v0->v1:
> >    * Remove driver version #define
> 
> This should be a separate patch of its own, more comment below.
> 
OK. Patch should be for net, correct?
 
> > -static int lan78xx_phy_init(struct lan78xx_net *dev)
> > +static int lan7801_phy_init(struct phy_device **phydev,
> > +			    struct lan78xx_net *dev)
> 
> Passing a **phydev looks really unnecessary here, why don't just return a
> struct phy_device * as a return argument here? If you need to communicate
> a specific return value, you can use ERR_PTR()/PTR_ERR() for that purpose.
> 
Done.

> >  		if (ret < 0) {
> > -			netdev_err(dev->net, "fail to register fixup\n");
> > +			netdev_err(dev->net, "fail to register fixup
> PHY_KSZ9031RNX\n");
> 
> Unrelated message change, that should be a separate commit.

OK.

> > +		phydev->is_internal = true;
> 
> Uggh you are not really supposed to set that, the matching PHY driver should
> have PHY_IS_INTERNAL in the .flags member to indicate that to the core PHY
> library.
>
OK. Will handle this too in separate patch/commit for net. 

Thanks,
Raghu