netdev - RE: [PATCH v3 net-next 1/2] lan743x: Add main source files for new lan743x driver

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: <90A7E81AE28BAE4CBDDB3B35F187D2644071EFDF@CHN-SV-EXMX02.mchp-main.com>
Date:   Fri, 2 Mar 2018 05:57:54 +0000
From:   <Bryan.Whitehead@...rochip.com>
To:     <andrew@...n.ch>
CC:     <davem@...emloft.net>, <UNGLinuxDriver@...rochip.com>,
        <netdev@...r.kernel.org>
Subject: RE: [PATCH v3 net-next 1/2] lan743x: Add main source files for new
 lan743x driver

> > +static int lan743x_phy_reset(struct lan743x_adapter *adapter) {
> > +	u32 data;
> > +
> > +	data = lan743x_csr_read(adapter, PMT_CTL);
> > +	data |= PMT_CTL_ETH_PHY_RST_;
> > +	lan743x_csr_write(adapter, PMT_CTL, data);
> > +
> > +	return readx_poll_timeout(LAN743X_CSR_READ_OP, PMT_CTL,
> data,
> > +				  (!(data & PMT_CTL_ETH_PHY_RST_) &&
> > +				  (data & PMT_CTL_READY_)),
> > +				  50000, 1000000);
> > +}
> 
> Hi Bryan
> 
> Could you explain this a bit more. What exactly is it resetting? Do we need to
> tell the phylib that the PHY has been reset and that it needs to re-program it?
> Or by phy do you mean a SERDES interface?

Hi Andrew,

This function resets the Ethernet phy. But it is called only in probe and before mdiobus_register. So I don't believe it is necessary to tell phylib.

[snip]
> > +
> > +	/* PHY interrupt enabled here */
> > +	phy_start(phydev);
> > +	phy_start_aneg(phydev);
> > +	return 0;
> 
> Are phy interrupts really enabled here? I could of missed it, but i don't see
> anywhere PHY interrupts are configured. This is either done via device tree,
> you set phydev->irq, or mdiobus->irq[X].

Sorry that is an obsolete comment, I will remove it. It is not using phy interrupts. It's using polling.

> 
> > +static int lan743x_tx_open(struct lan743x_tx *tx) {
> > +	struct lan743x_adapter *adapter = NULL;
> > +	u32 data = 0;
> > +	int ret;
> > +
> > +	adapter = tx->adapter;
> > +	ret = lan743x_tx_ring_init(tx);
> > +	if (ret)
> > +		goto return_error;
> 
> You could just return here. You don't do anything useful at
> return_error:

Yes, I'll fix it.

[snip]
 
> This is much nicer without all the flags. Thanks.

No problem, thanks for your patients with me.

> 
> > +static struct pci_driver lan743x_pcidev_driver = {
> > +	.name     = DRIVER_NAME,
> > +	.id_table = lan743x_pcidev_tbl,
> > +	.probe    = lan743x_pcidev_probe,
> > +	.remove   = lan743x_pcidev_remove,
> > +	.shutdown = lan743x_pcidev_shutdown, };
> > +
> > +static int __init lan743x_module_init(void) {
> > +	int result = -EINVAL;
> > +
> > +	pr_info(DRIVER_DESC "\n");
> > +	result = pci_register_driver(&lan743x_pcidev_driver);
> > +	if (result)
> > +		pr_warn("pci_register_driver returned error code, %d\n",
> > +			result);
> > +	return result;
> > +}
> > +
> > +module_init(lan743x_module_init);
> > +
> > +static void __exit lan743x_module_exit(void) {
> > +	pci_unregister_driver(&lan743x_pcidev_driver);
> > +}
> >
> > +module_exit(lan743x_module_exit);
> 
> You can replace this boilerplate code with module_pci_driver().
> You don't do anything special here.

OK

Thanks,
Bryan