netdev - RE: [PATCH 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]

Message-ID: <0573C9D4B793EF43BF95221F2F4CC85153DA07@CHN-SV-EXMX06.mchp-main.com>
Date:   Wed, 25 Apr 2018 05:21:32 +0000
From:   <RaghuramChary.Jallipalli@...rochip.com>
To:     <andrew@...n.ch>
CC:     <davem@...emloft.net>, <netdev@...r.kernel.org>,
        <UNGLinuxDriver@...rochip.com>, <Woojung.Huh@...rochip.com>
Subject: RE: [PATCH net-next] lan78xx: Lan7801 Support for Fixed PHY

Hi Andrew,

> > +#define DRIVER_VERSION	"1.0.7"
> 
> Hi Raghuram
> 
> Driver version strings a pretty pointless. You might want to remove it.
>
OK, will remove it.
 
> > +			netdev_info(dev->net, "Registered FIXED PHY\n");
> 
> There are too many detdev_info() messages here. Maybe make them both
> netdev_dbg().
>

OK. Will modify to netdev_dbg()
 
> > +			dev->interface = PHY_INTERFACE_MODE_RGMII;
> > +			dev->fixedphy = phydev;
> 
> You can use
> 
> if (!phy_is_pseudo_fixed_link(phydev))
> 
> to determine is a PHY is a fixed phy. I think you can then do without
> dev->fixedphy.
> 
dev->fixedphy stores the fixed phydev, which will be passed to the 
fixed_phy_unregister routine , so I think phy_is_pseudo_fixed_link check is not necessary.

> > +			ret = lan78xx_write_reg(dev, HW_CFG, buf);
> > +			goto phyinit;
> 
> Please don't use a goto like this. Maybe turn this into a switch statement?
>
OK. Will modify.
 
> > static int lan78xx_phy_init(struct lan78xx_net *dev)
> >  		goto error;
> 
> Please take a look at what happens at error: It does not look correct.
> Probably now is a good time to refactor the whole of lan78xx_phy_init()
> 

OK. Will take care.

Thanks,
-Raghu