Message-ID: <dleftjd01l99jv.fsf%l.stelmach@samsung.com>
Date:   Tue, 13 Oct 2020 22:04:52 +0200
From:   Lukasz Stelmach <l.stelmach@...sung.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Kukjin Kim <kgene@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Russell King <linux@...linux.org.uk>, jim.cromie@...il.com,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-samsung-soc@...r.kernel.org, netdev@...r.kernel.org,
        devicetree@...r.kernel.org,
        Bartłomiej Żolnierkiewicz 
        <b.zolnierkie@...sung.com>,
        Marek Szyprowski <m.szyprowski@...sung.com>
Subject: Re: [PATCH v2 2/4] net: ax88796c: ASIX AX88796C SPI Ethernet
 Adapter Driver

It was <2020-10-02 pią 22:36>, when Andrew Lunn wrote:
>> +static u32 ax88796c_get_link(struct net_device *ndev)
>> +{
>> +	struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
>> +
>> +	mutex_lock(&ax_local->spi_lock);
>> +
>> +	phy_read_status(ndev->phydev);
>> +
>> +	mutex_unlock(&ax_local->spi_lock);
>
> Why do you take this mutux before calling phy_read_status()? The
> phylib core will not be taking this mutex when it calls into the PHY
> driver. This applies to all the calls you have with phy_
>

I need to review the use of this mutex. Thanks for spotting.

> There should not be any need to call phy_read_status(). phylib will do
> this once per second, or after any interrupt from the PHY. so just use
>
>      phydev->link
>

Using ethtool_op_get_link()

>> +static void
>> +ax88796c_get_regs(struct net_device *ndev, struct ethtool_regs *regs, void *_p)
>> +{
>> +	struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
>> +	u16 *p = _p;
>> +	int offset, i;
>> +
>> +	memset(p, 0, AX88796C_REGDUMP_LEN);
>> +
>> +	for (offset = 0; offset < AX88796C_REGDUMP_LEN; offset += 2) {
>> +		if (!test_bit(offset / 2, ax88796c_no_regs_mask))
>> +			*p = AX_READ(&ax_local->ax_spi, offset);
>> +		p++;
>> +	}
>> +
>> +	for (i = 0; i < AX88796C_PHY_REGDUMP_LEN / 2; i++) {
>> +		*p = phy_read(ax_local->phydev, i);
>> +		p++;
>
> Depending on the PHY, that can be dangerous.

This is a built-in generic PHY. The chip has no lines to attach any
other external one.

> phylib could be busy doing things with the PHY. It could be looking at

How does phylib prevent concurrent access to a PHY? 

> a different page for example.

Different page? 

> miitool(1) can give you the same functionally without the MAC driver
> doing anything, other than forwarding the IOCTL call on.

No, I am afraid mii-tool is not able to dump registers. I am not insisting
on dumping PHY registeres but I think it is nice to have them. Intel
drivers do it.

>> +int ax88796c_mdio_read(struct mii_bus *mdiobus, int phy_id, int loc)
>> +{
>> +	struct ax88796c_device *ax_local = mdiobus->priv;
>> +	int ret;
>> +
>> +	AX_WRITE(&ax_local->ax_spi, MDIOCR_RADDR(loc)
>> +			| MDIOCR_FADDR(phy_id) | MDIOCR_READ, P2_MDIOCR);
>> +
>> +	ret = read_poll_timeout(AX_READ, ret,
>> +				(ret != 0),
>> +				0, jiffies_to_usecs(HZ / 100), false,
>> +				&ax_local->ax_spi, P2_MDIOCR);
>> +	if (ret)
>> +		return -EBUSY;
>
> Return whatever read_poll_timeout() returned. It is probably
> -ETIMEDOUT, but it could also be -EIO for example.

Indeed it is -ETIMEDOUT. Returning ret.

>> +ax88796c_mdio_write(struct mii_bus *mdiobus, int phy_id, int loc, u16 val)
>> +{
>> +	struct ax88796c_device *ax_local = mdiobus->priv;
>> +	int ret;
>> +
>> +	AX_WRITE(&ax_local->ax_spi, val, P2_MDIODR);
>> +
>> +	AX_WRITE(&ax_local->ax_spi,
>> +		 MDIOCR_RADDR(loc) | MDIOCR_FADDR(phy_id)
>> +		 | MDIOCR_WRITE, P2_MDIOCR);
>> +
>> +	ret = read_poll_timeout(AX_READ, ret,
>> +				((ret & MDIOCR_VALID) != 0), 0,
>> +				jiffies_to_usecs(HZ / 100), false,
>> +				&ax_local->ax_spi, P2_MDIOCR);
>> +	if (ret)
>> +		return -EIO;
>> +
>> +	if (loc == MII_ADVERTISE) {
>> +		AX_WRITE(&ax_local->ax_spi, (BMCR_FULLDPLX | BMCR_ANRESTART |
>> +			  BMCR_ANENABLE | BMCR_SPEED100), P2_MDIODR);
>> +		AX_WRITE(&ax_local->ax_spi, (MDIOCR_RADDR(MII_BMCR) |
>> +			  MDIOCR_FADDR(phy_id) | MDIOCR_WRITE),
>> +			  P2_MDIOCR);
>>
>
> What is this doing?
>

Well… it turns autonegotiation when changing advertised link modes. But
this is obvious. As to why this code is here, I will honestly say — I am
not sure (Reminder: this is a vendor driver I am porting, I am more than
happy to receive any comments, thank you). Apparently it is not required
and I am willing to remove it.  It could be of some use when the driver
didn't use phylib.

>> +		ret = read_poll_timeout(AX_READ, ret,
>> +					((ret & MDIOCR_VALID) != 0), 0,
>> +					jiffies_to_usecs(HZ / 100), false,
>> +					&ax_local->ax_spi, P2_MDIOCR);
>> +		if (ret)
>> +			return -EIO;
>> +	}
>> +
>> +	return 0;
>> +}
>
>> +static char *no_regs_list = "80018001,e1918001,8001a001,fc0d0000";
>> +unsigned long ax88796c_no_regs_mask[AX88796C_REGDUMP_LEN / (sizeof(unsigned long) * 8)];
>> +
>> +module_param(comp, int, 0444);
>> +MODULE_PARM_DESC(comp, "0=Non-Compression Mode, 1=Compression Mode");
>> +
>> +module_param(msg_enable, int, 0444);
>> +MODULE_PARM_DESC(msg_enable, "Message mask (see linux/netdevice.h for bitmap)");
>
> No module parameters allowed, not in netdev.
>
>> +static int ax88796c_reload_eeprom(struct ax88796c_device *ax_local)
>> +{
>> +	int ret;
>> +
>> +	AX_WRITE(&ax_local->ax_spi, EECR_RELOAD, P3_EECR);
>> +
>> +	ret = read_poll_timeout(AX_READ, ret,
>> +				(ret & PSR_DEV_READY),
>> +				0, jiffies_to_usecs(2 * HZ / 1000), false,
>> +				&ax_local->ax_spi, P0_PSR);
>> +	if (ret) {
>> +		dev_err(&ax_local->spi->dev,
>> +			"timeout waiting for reload eeprom\n");
>> +		return -1;
>
> return ret not EINVAL which is -1
>

Done.

>> +static int ax88796c_set_mac_address(struct net_device *ndev, void *p)
>> +{
>> +	struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
>> +	struct sockaddr *addr = p;
>> +
>> +	if (!is_valid_ether_addr(addr->sa_data))
>> +		return -EADDRNOTAVAIL;
>
> It would be better to just use eth_mac_addr().
>

Done.

>> +static int
>> +ax88796c_check_free_pages(struct ax88796c_device *ax_local, u8 need_pages)
>> +{
>> +	u8 free_pages;
>> +	u16 tmp;
>> +
>> +	free_pages = AX_READ(&ax_local->ax_spi, P0_TFBFCR) & TX_FREEBUF_MASK;
>> +	if (free_pages < need_pages) {
>> +		/* schedule free page interrupt */
>> +		tmp = AX_READ(&ax_local->ax_spi, P0_TFBFCR)
>> +				& TFBFCR_SCHE_FREE_PAGE;
>> +		AX_WRITE(&ax_local->ax_spi, tmp | TFBFCR_TX_PAGE_SET |
>> +				TFBFCR_SET_FREE_PAGE(need_pages),
>> +				P0_TFBFCR);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static struct sk_buff *
>> +ax88796c_tx_fixup(struct net_device *ndev, struct sk_buff_head *q)
>> +{
>> +	if (netif_msg_pktdata(ax_local)) {
>> +		char pfx[IFNAMSIZ + 7];
>> +
>> +		snprintf(pfx, sizeof(pfx), "%s:     ", ndev->name);
>> +
>> +		netdev_info(ndev, "TX packet len %d, total len %d, seq %d\n",
>> +			    pkt_len, tx_skb->len, seq_num);
>> +
>> +		netdev_info(ndev, "  SPI Header:\n");
>> +		print_hex_dump(KERN_INFO, pfx, DUMP_PREFIX_OFFSET, 16, 1,
>> +			       tx_skb->data, 4, 0);
>> +
>> +		netdev_info(ndev, "  TX SOP:\n");
>> +		print_hex_dump(KERN_INFO, pfx, DUMP_PREFIX_OFFSET, 16, 1,
>> +			       tx_skb->data + 4, TX_OVERHEAD, 0);
>> +
>> +		netdev_info(ndev, "  TX packet:\n");
>> +		print_hex_dump(KERN_INFO, pfx, DUMP_PREFIX_OFFSET, 16, 1,
>> +			       tx_skb->data + 4 + TX_OVERHEAD,
>> +			       tx_skb->len - TX_EOP_SIZE - 4 - TX_OVERHEAD, 0);
>> +
>> +		netdev_info(ndev, "  TX EOP:\n");
>> +		print_hex_dump(KERN_INFO, pfx, DUMP_PREFIX_OFFSET, 16, 1,
>> +			       tx_skb->data + tx_skb->len - 4, 4, 0);
>> +	}
>
> I expect others are going to ask you to remove this.
>

You mean dumping packets? I will if they do. What is pktdata flag for then?

>> +static void ax88796c_handle_link_change(struct net_device *ndev)
>> +{
>> +	if (net_ratelimit())
>> +		phy_print_status(ndev->phydev);
>> +}
>> +
>> +void ax88796c_phy_init(struct ax88796c_device *ax_local)
>> +{
>> +	/* Enable PHY auto-polling */
>> +	AX_WRITE(&ax_local->ax_spi,
>> +		 PCR_PHYID(0x10) | PCR_POLL_EN |
>> +		 PCR_POLL_FLOWCTRL | PCR_POLL_BMCR, P2_PCR);
>
> Auto-polling of the PHY is generally a bad idea. The hardware is not
> going to respect the phydev->lock mutex, for example. Disable this,
> and add a proper ax88796c_handle_link_change().
>

Done.

>> +static int
>> +ax88796c_open(struct net_device *ndev)
>> +{
>> +	struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
>> +	int ret;
>> +	unsigned long irq_flag = IRQF_SHARED;
>> +
>> +	mutex_lock(&ax_local->spi_lock);
>> +
>> +	ret = ax88796c_soft_reset(ax_local);
>> +	if (ret < 0)
>> +		return -ENODEV;
>> +
>> +	ret = request_irq(ndev->irq, ax88796c_interrupt,
>> +			  irq_flag, ndev->name, ndev);
>
> Maybe look at using request_threaded_irq(). You can then remove your
> work queue, and do the work in the thread_fn.
>

There is other work beeing done in the work queue too.

>> +	if (ret) {
>> +		netdev_err(ndev, "unable to get IRQ %d (errno=%d).\n",
>> +			   ndev->irq, ret);
>> +		return -ENXIO;
>
> return ret;
>
> In general, never change a return code unless you have a really good
> reason why. And if you do have a reason, document it.
>

OK, Done.

>> +static int
>> +ax88796c_close(struct net_device *ndev)
>> +{
>> +	struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
>> +
>> +	netif_stop_queue(ndev);
>> +
>> +	free_irq(ndev->irq, ndev);
>> +
>> +	phy_stop(ndev->phydev);
>> +
>> +	mutex_lock(&ax_local->spi_lock);
>> +
>> +	AX_WRITE(&ax_local->ax_spi, IMR_MASKALL, P0_IMR);
>> +	ax88796c_free_skb_queue(&ax_local->tx_wait_q);
>> +
>> +	ax88796c_soft_reset(ax_local);
>> +
>> +	mutex_unlock(&ax_local->spi_lock);
>> +	netif_carrier_off(ndev);
>
> phy_stop() will do that for you.
>

Removed.

>> +static int ax88796c_probe(struct spi_device *spi)
>> +{
>
>> +	ax_local->mdiobus->priv = ax_local;
>> +	ax_local->mdiobus->read = ax88796c_mdio_read;
>> +	ax_local->mdiobus->write = ax88796c_mdio_write;
>> +	ax_local->mdiobus->name = "ax88976c-mdiobus";
>> +	ax_local->mdiobus->phy_mask = ~(1 << 0x10);
>
> BIT(0x10);
>

Done.

>> +
>> +	ret = devm_register_netdev(&spi->dev, ndev);
>> +	if (ret) {
>> +		dev_err(&spi->dev, "failed to register a network device\n");
>> +		destroy_workqueue(ax_local->ax_work_queue);
>> +		goto err;
>> +	}
>
> The device is not live. If this is being used for NFS root, the kernel
> will start using it. So what sort of mess will it get into, if there
> is no PHY yet? Nothing important should happen after register_netdev().
>

But, with an unregistered network device ndev_owner in
phy_attach_direct() is NULL. Thus, phy_connect_direct() below fails.

--8<---------------cut here---------------start------------->8---
   1332         if (dev)
   1333                 ndev_owner = dev->dev.parent->driver->owner;
   1334         if (ndev_owner != bus->owner &&  !try_module_get(bus->owner)) {
   1335                 phydev_err(phydev, "failed to get the bus  module\n");
   1336                 return -EIO;
   1337         }
--8<---------------cut here---------------end--------------->8---


>> +
>> +	ax_local->phydev = phy_find_first(ax_local->mdiobus);
>> +	if (!ax_local->phydev) {
>> +		dev_err(&spi->dev, "no PHY found\n");
>> +		ret = -ENODEV;
>> +		goto err;
>> +	}
>> +
>> +	ax_local->phydev->irq = PHY_IGNORE_INTERRUPT;
>> +	phy_connect_direct(ax_local->ndev, ax_local->phydev,
>> +			   ax88796c_handle_link_change,
>> +			   PHY_INTERFACE_MODE_MII);
>> +
>> +	netif_info(ax_local, probe, ndev, "%s %s registered\n",
>> +		   dev_driver_string(&spi->dev),
>> +		   dev_name(&spi->dev));
>> +	phy_attached_info(ax_local->phydev);
>> +
>> +	ret = 0;
>> +err:
>> +	return ret;
>> +}
>> +
>> +static int ax88796c_remove(struct spi_device *spi)
>> +{
>> +	struct ax88796c_device *ax_local = dev_get_drvdata(&spi->dev);
>> +	struct net_device *ndev = ax_local->ndev;
>
> You might want to disconnect the PHY.
>

I do (-; Done.

>> +
>> +	netif_info(ax_local, probe, ndev, "removing network device %s %s\n",
>> +		   dev_driver_string(&spi->dev),
>> +		   dev_name(&spi->dev));
>> +
>> +	destroy_workqueue(ax_local->ax_work_queue);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id ax88796c_dt_ids[] = {
>> +	{ .compatible = "asix,ax88796c" },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, ax88796c_dt_ids);
>> +
>> +static const struct spi_device_id asix_id[] = {
>> +	{ "ax88796c", 0 },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(spi, asix_id);
>> +
>> +static struct spi_driver ax88796c_spi_driver = {
>> +	.driver = {
>> +		.name = DRV_NAME,
>> +#ifdef CONFIG_USE_OF
>> +		.of_match_table = of_match_ptr(ax88796c_dt_ids),
>> +#endif
>
> I don't think you need the #ifdef.
>

Indeed, it appears to be an uncommon practice to use it in this
context. Done.

>> +#ifndef _AX88796C_MAIN_H
>> +#define _AX88796C_MAIN_H
>> +
>> +#include <linux/netdevice.h>
>> +#include <linux/mii.h>
>> +
>> +#include "ax88796c_spi.h"
>> +
>> +/* These identify the driver base version and may not be removed. */
>> +#define DRV_NAME	"ax88796c"
>> +#define ADP_NAME	"ASIX AX88796C SPI Ethernet Adapter"
>> +#define DRV_VERSION	"1.2.0"
>
> DRV_VERSION are pretty pointless. Not sure you use it anyway. Please
> remove.
>

Done.

>> +	unsigned long		capabilities;
>> +		#define AX_CAP_DMA		1
>> +		#define AX_CAP_COMP		2
>> +		#define AX_CAP_BIDIR		4
>
> BIT(0), BIT(1), BIT(2)...
>

No problem. Do you have any recommendation how to express this

 #define PSR_RESET  (0 << 15)

I know it equals 0, but shows explicitly the bit number.

>> +struct skb_data;
>> +
>> +struct skb_data {
>> +	enum skb_state state;
>> +	struct net_device *ndev;
>> +	struct sk_buff *skb;
>> +	size_t len;
>> +	dma_addr_t phy_addr;
>> +};
>
> A forward definition, followed by the real definition?
>

There must have been something in between. Done.

>> +	#define FER_IPALM		(1 << 0)
>> +	#define FER_DCRC		(1 << 1)
>> +	#define FER_RH3M		(1 << 2)
>> +	#define FER_HEADERSWAP		(1 << 7)
>> +	#define FER_WSWAP		(1 << 8)
>> +	#define FER_BSWAP		(1 << 9)
>> +	#define FER_INTHI		(1 << 10)
>> +	#define FER_INTLO		(0 << 10)
>> +	#define FER_IRQ_PULL		(1 << 11)
>> +	#define FER_RXEN		(1 << 14)
>> +	#define FER_TXEN		(1 << 15)
>
> Isn't checkpatch giving warnings and suggesting BIT?

Not exactly. It gives green CHECK messages, which I decided to
ignore. Apparently a wrong move.

Thanks for the feedback.
-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

Download attachment "signature.asc" of type "application/pgp-signature" (488 bytes)

Powered by blists - more mailing lists