Message-ID: <dleftjwo15qyei.fsf%l.stelmach@samsung.com>
Date:   Mon, 07 Sep 2020 19:39:33 +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>,
        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, b.zolnierkie@...sung.com,
        m.szyprowski@...sung.com
Subject: Re: [PATCH 1/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter
 Driver

It was <2020-08-25 wto 20:01>, when Andrew Lunn wrote:

> Hi Łukasz
>
> It is pretty clear this is a "vendor crap driver".

I can't deny.

> It needs quite a bit more work on it.

I'll be more than happy to take your suggestions.

> On Tue, Aug 25, 2020 at 07:03:09PM +0200, Łukasz Stelmach wrote:
>> +++ b/drivers/net/ethernet/asix/ax88796c_ioctl.c
>
> This is an odd filename. The ioctl code is wrong anyway, but there is
> a lot more than ioctl in here. I suggest you give it a new name.
>

Sure, any suggestions?

>> @@ -0,0 +1,293 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2010 ASIX Electronics Corporation
>> + * Copyright (c) 2020 Samsung Electronics Co., Ltd.
>> + *
>> + * ASIX AX88796C SPI Fast Ethernet Linux driver
>> + */
>> +
>> +#include "ax88796c_main.h"
>> +#include "ax88796c_spi.h"
>> +#include "ax88796c_ioctl.h"
>> +
>> +u8 ax88796c_check_power(struct ax88796c_device *ax_local)
>
> bool ?

OK.

It appears, however, that 0 means OK and 1 !OK. Do you think changing to
TRUE and FALSE (or FALSE and TRUE) is required?

>> +{
>> +	struct spi_status ax_status;
>> +
>> +	/* Check media link status first */
>> +	if (netif_carrier_ok(ax_local->ndev) ||
>> +	    (ax_local->ps_level == AX_PS_D0)  ||
>> +	    (ax_local->ps_level == AX_PS_D1)) {
>> +		return 0;
>> +	}
>> +
>> +	AX_READ_STATUS(&ax_local->ax_spi, &ax_status);
>> +	if (!(ax_status.status & AX_STATUS_READY))
>> +		return 1;
>> +
>> +	return 0;
>> +}
>> +
>> +u8 ax88796c_check_power_and_wake(struct ax88796c_device *ax_local)
>> +{
>> +	struct spi_status ax_status;
>> +	unsigned long start_time;
>> +
>> +	/* Check media link status first */
>> +	if (netif_carrier_ok(ax_local->ndev) ||
>> +	    (ax_local->ps_level == AX_PS_D0) ||
>> +	    (ax_local->ps_level == AX_PS_D1)) {
>> +		return 0;
>> +	}
>> +
>> +	AX_READ_STATUS(&ax_local->ax_spi, &ax_status);
>> +	if (!(ax_status.status & AX_STATUS_READY)) {
>> +
>> +		/* AX88796C in power saving mode */
>> +		AX_WAKEUP(&ax_local->ax_spi);
>> +
>> +		/* Check status */
>> +		start_time = jiffies;
>> +		do {
>> +			if (time_after(jiffies, start_time + HZ/2)) {
>> +				netdev_err(ax_local->ndev,
>> +					"timeout waiting for wakeup"
>> +					" from power saving\n");
>> +				break;
>> +			}
>> +
>> +			AX_READ_STATUS(&ax_local->ax_spi, &ax_status);
>> +
>> +		} while (!(ax_status.status & AX_STATUS_READY));
>
> include/linux/iopoll.h
>

Done. The result seems only slightly more elegant since the generic
read_poll_timeout() needs to be employed.

>
> Can the device itself put itself to sleep? If not, maybe just track
> the power saving state in struct ax88796c_device?
>

Yes, it can. When the cable is unplugged, parts of of the chip enter
power saving mode and the values in registers change.

>> +int ax88796c_mdio_read(struct net_device *ndev, int phy_id, int loc)
>> +{
>> +	struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
>> +	unsigned long start_time;
>> +
>> +	AX_WRITE(&ax_local->ax_spi, MDIOCR_RADDR(loc)
>> +			| MDIOCR_FADDR(phy_id) | MDIOCR_READ, P2_MDIOCR);
>> +
>> +	start_time = jiffies;
>> +	while ((AX_READ(&ax_local->ax_spi, P2_MDIOCR) & MDIOCR_VALID) == 0) {
>> +		if (time_after(jiffies, start_time + HZ/100))
>> +			return -EBUSY;
>> +	}
>
> Another use case of iopoll.h
>

Done.

>> +	return AX_READ(&ax_local->ax_spi, P2_MDIODR);
>> +}
>> +
>> +void
>> +ax88796c_mdio_write(struct net_device *ndev, int phy_id, int loc, int val)
>> +{
>> +	struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
>> +	unsigned long start_time;
>> +
>> +	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);
>> +
>> +	start_time = jiffies;
>> +	while ((AX_READ(&ax_local->ax_spi, P2_MDIOCR) & MDIOCR_VALID) == 0) {
>> +		if (time_after(jiffies, start_time + HZ/100))
>> +			return;
>> +	}
>> +
>> +	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);
>
> Odd. An mdio bus driver should not need to do anything like this.
>
> Humm, please make this is a plain MDIO bus driver, using
> mdiobus_register().
>

The manufacturer says

    The AX88796C integrates on-chip Fast Ethernet MAC and PHY, […]

There is a single integrated PHY in this chip and no possiblity to
connect external one. Do you think it makes sense in such case to
introduce the additional layer of abstraction?

>> +
>> +		start_time = jiffies;
>> +		while ((AX_READ(&ax_local->ax_spi, P2_MDIOCR)
>> +					& MDIOCR_VALID) == 0) {
>> +			if (time_after(jiffies, start_time + HZ/100))
>> +				return;
>> +		}
>> +	}
>> +}
>> +
>
>> +static void ax88796c_get_drvinfo(struct net_device *ndev,
>> +				 struct ethtool_drvinfo *info)
>> +{
>> +	/* Inherit standard device info */
>> +	strncpy(info->driver, DRV_NAME, sizeof(info->driver));
>> +	strncpy(info->version, DRV_VERSION, sizeof(info->version));
>
> verion is pretty much not wanted any more.
>

Removed.

>> +static u32 ax88796c_get_link(struct net_device *ndev)
>> +{
>> +	u32 link;
>> +	struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
>> +	u8 power;
>> +
>> +	down(&ax_local->spi_lock);
>> +	power = ax88796c_check_power_and_wake(ax_local);
>> +
>> +	link = mii_link_ok(&ax_local->mii);
>> +
>> +	if (power)
>> +		ax88796c_set_power_saving(ax_local, ax_local->ps_level);
>> +	up(&ax_local->spi_lock);
>> +
>> +	return link;
>> +
>> +
>> +}
>
> When you convert to phylib, this will go away.
>

See above (single integrated phy).

>> +static int
>> +ax88796c_get_link_ksettings(struct net_device *ndev,
>> +			    struct ethtool_link_ksettings *cmd)
>> +{
>> +	struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
>> +	u8 power;
>> +
>> +	down(&ax_local->spi_lock);
>
> Please use a mutex, not semaphores.
>

Done.

>> +module_param(comp, int, 0);
>> +MODULE_PARM_DESC(comp, "0=Non-Compression Mode, 1=Compression Mode");
>> +
>> +module_param(ps_level, int, 0);
>> +MODULE_PARM_DESC(ps_level,
>> +	"Power Saving Level (0:disable 1:level 1 2:level 2)");
>> +
>> +module_param(msg_enable, int, 0);
>> +MODULE_PARM_DESC(msg_enable, "Message mask (see linux/netdevice.h for bitmap)");
>> +
>> +static char *macaddr;
>> +module_param(macaddr, charp, 0);
>> +MODULE_PARM_DESC(macaddr, "MAC address");
>
> No Module parameters. You can get the MAC address from DT.

What about systems without DT? Not every bootloader is sophisicated
enough to edit DT before starting kernel. AX88786C is a chip that can be
used in a variety of systems and I'd like to avoid too strong
assumptions.

Of course reading MAC address from DT is a good idea and I will add it.

> msg_enable can be controlled by ethtool.

But it does not work during boot.

>> +MODULE_AUTHOR("ASIX");
>
> Do you expect ASIX to support this? 

No.

> You probably want to put your name here.

I don't want to be considered as the only author and as far as I can
tell being mentioned as an author does not imply being a
maintainer. Do you think two MODULE_AUTHOR()s be OK?
 
>> +MODULE_DESCRIPTION("ASIX AX88796C SPI Ethernet driver");
>> +MODULE_LICENSE("GPL");
>> +
>> +static void ax88796c_dump_regs(struct ax88796c_device *ax_local)
>> +{
>> +	struct net_device *ndev = ax_local->ndev;
>> +	u8 i, j;
>> +
>> +	netdev_info(ndev,
>> +		"       Page0   Page1   Page2   Page3   "
>> +				"Page4   Page5   Page6   Page7\n");
>> +	for (i = 0; i < 0x20; i += 2) {
>> +		netdev_info(ndev, "0x%02x   ", i);
>> +		for (j = 0; j < 8; j++) {
>> +			netdev_info(ndev, "0x%04x  ",
>> +				AX_READ(&ax_local->ax_spi, j * 0x20 + i));
>> +		}
>> +		netdev_info(ndev, "\n");
>> +	}
>> +	netdev_info(ndev, "\n");
>
> Please implement ethtool -d, not this.
>

Done.

>> +}
>> +
>> +static void ax88796c_dump_phy_regs(struct ax88796c_device *ax_local)
>> +{
>> +	struct net_device *ndev = ax_local->ndev;
>> +	int i;
>> +
>> +	netdev_info(ndev, "Dump PHY registers:\n");
>> +	for (i = 0; i < 6; i++) {
>> +		netdev_info(ndev, "  MR%d = 0x%04x\n", i,
>> +			ax88796c_mdio_read(ax_local->ndev,
>> +			ax_local->mii.phy_id, i));
>> +	}
>> +}
>> +
>
> Please delete. Let the PHY driver worry about PHY registers.
>

See above (single integrated phy).

>> +static void ax88796c_watchdog(struct ax88796c_device *ax_local)
>> +{
>> +	struct net_device *ndev = ax_local->ndev;
>> +	u16 phy_status;
>> +	unsigned long time_to_chk = AX88796C_WATCHDOG_PERIOD;
>> +
>> +	if (ax88796c_check_power(ax_local)) {
>> +		mod_timer(&ax_local->watchdog, jiffies + time_to_chk);
>> +		return;
>> +	}
>> +
>> +	ax88796c_set_power_saving(ax_local, AX_PS_D0);
>
> You might want to look at runtime PM for all this power management.
>

This one and the one below, are to manage device's PM state during this
function.

>> +
>> +	phy_status = AX_READ(&ax_local->ax_spi, P0_PSCR);
>> +	if (phy_status & PSCR_PHYLINK) {
>> +
>> +		ax_local->w_state = ax_nop;
>> +		time_to_chk = 0;
>> +
>> +	} else if (!(phy_status & PSCR_PHYCOFF)) {
>> +		/* The ethernet cable has been plugged */
>> +		if (ax_local->w_state == chk_cable) {
>> +			if (netif_msg_timer(ax_local))
>> +				netdev_info(ndev, "Cable connected\n");
>> +
>> +			ax_local->w_state = chk_link;
>> +			ax_local->w_ticks = 0;
>> +		} else {
>> +			if (netif_msg_timer(ax_local))
>> +				netdev_info(ndev, "Check media status\n");
>> +
>> +			if (++ax_local->w_ticks == AX88796C_WATCHDOG_RESTART) {
>> +				if (netif_msg_timer(ax_local))
>> +					netdev_info(ndev, "Restart autoneg\n");
>> +				ax88796c_mdio_write(ndev,
>> +					ax_local->mii.phy_id, MII_BMCR,
>> +					(BMCR_SPEED100 | BMCR_ANENABLE |
>> +					BMCR_ANRESTART));
>> +
>> +				if (netif_msg_hw(ax_local))
>> +					ax88796c_dump_phy_regs(ax_local);
>> +				ax_local->w_ticks = 0;
>> +			}
>> +		}
>> +	} else {
>> +		if (netif_msg_timer(ax_local))
>> +			netdev_info(ndev, "Check cable status\n");
>> +
>> +		ax_local->w_state = chk_cable;
>> +	}
>> +
>> +	ax88796c_set_power_saving(ax_local, ax_local->ps_level);
>> +
>> +	if (time_to_chk)
>> +		mod_timer(&ax_local->watchdog, jiffies + time_to_chk);
>> +}
>
> This is not the normal use of a watchdog in network drivers. The
> normal case is the network stack as asked the driver to do something,
> normally a TX, and the driver has not reported the action has
> completed.  The state of the cable should not make any
> difference. This does not actually appear to do anything useful, like
> kick the hardware to bring it back to life.
>

Maybe it's the naming that is a problem. Yes, it is not a watchdog, but
rather a periodic housekeeping and it kicks hw if it can't negotiate
the connection. The question is: should the settings be reset in such case.

>> +static int ax88796c_soft_reset(struct ax88796c_device *ax_local)
>> +{
>> +	unsigned long start;
>> +	u16 temp;
>> +
>> +	AX_WRITE(&ax_local->ax_spi, PSR_RESET, P0_PSR);
>> +	AX_WRITE(&ax_local->ax_spi, PSR_RESET_CLR, P0_PSR);
>> +
>> +	start = jiffies;
>> +	while (!(AX_READ(&ax_local->ax_spi, P0_PSR) & PSR_DEV_READY)) {
>> +		if (time_after(jiffies, start + (160 * HZ / 1000))) {
>> +			dev_err(&ax_local->spi->dev,
>> +				"timeout waiting for reset completion\n");
>> +			return -1;
>> +		}
>> +	}
>
> iopoll.h. 
>

Done

>> +#if 0
>> +static void ax88796c_set_multicast(struct net_device *ndev)
>> +{
>> +	struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
>> +
>> +	set_bit(EVENT_SET_MULTI, &ax_local->flags);
>> +	queue_work(ax_local->ax_work_queue, &ax_local->ax_work);
>> +}
>> +#endif
>
> We don't allow #if 0 code in mainline.
>

Removed.

>> +	if (netif_msg_pktdata(ax_local)) {
>> +		int loop;
>> +		netdev_info(ndev, "TX packet len %d, total len %d, seq %d\n",
>> +				pkt_len, tx_skb->len, seq_num);
>> +
>> +		netdev_info(ndev, "  Dump SPI Header:\n    ");
>> +		for (loop = 0; loop < 4; loop++)
>> +			netdev_info(ndev, "%02x ", *(tx_skb->data + loop));
>> +
>> +		netdev_info(ndev, "\n");
>
> This no longer works as far as i remember. Lines are terminate by
> default even if they don't have a \n.
>
> Please you should not be using netdev_info(). netdev_dbg() please.
>

I changed most netif_msg_*()+netdev_*() to netif_*(), including
netif_dbg() in several places. However, after reading other drivers I
decided to leave this at INFO level. I think this is the way to go,
because this is what user asks for and with dynamic debug enabled users
would have to ask for these in two different places.

>> +static inline void
>> +ax88796c_skb_return(struct ax88796c_device *ax_local, struct sk_buff *skb,
>> +			struct rx_header *rxhdr)
>> +{
>
> No inline functions in C code please.
>

Done.

>> +	struct net_device *ndev = ax_local->ndev;
>> +	int status;
>> +
>> +	do {
>> +		if (!(ax_local->checksum & AX_RX_CHECKSUM))
>> +			break;
>> +
>> +		/* checksum error bit is set */
>> +		if ((rxhdr->flags & RX_HDR3_L3_ERR) ||
>> +		    (rxhdr->flags & RX_HDR3_L4_ERR))
>> +			break;
>> +
>> +		if ((rxhdr->flags & RX_HDR3_L4_TYPE_TCP) ||
>> +		    (rxhdr->flags & RX_HDR3_L4_TYPE_UDP)) {
>> +			skb->ip_summed = CHECKSUM_UNNECESSARY;
>> +		}
>> +	} while (0);
>
>
> ??
>

if() break; Should I use goto?

>> +
>> +	ax_local->stats.rx_packets++;
>> +	ax_local->stats.rx_bytes += skb->len;
>> +	skb->dev = ndev;
>> +
>> +	skb->truesize = skb->len + sizeof(struct sk_buff);
>> +	skb->protocol = eth_type_trans(skb, ax_local->ndev);
>> +
>> +	if (netif_msg_rx_status(ax_local))
>> +		netdev_info(ndev, "< rx, len %zu, type 0x%x\n",
>> +			skb->len + sizeof(struct ethhdr), skb->protocol);
>> +
>> +	status = netif_rx(skb);
>> +	if (status != NET_RX_SUCCESS && netif_msg_rx_err(ax_local))
>> +		netdev_info(ndev, "netif_rx status %d\n", status);
>
> Please go through the driver and use netdev_dbg() where appropriate.
>

Done, partially (see above.)

>> +}
>> +
>> +static void dump_packet(struct net_device *ndev, const char *msg, int len, const char *data)
>> +{
>> +        netdev_printk(KERN_DEBUG, ndev,  DRV_NAME ": %s - packet len:%d\n", msg, len);
>> +        print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET, 16, 1,
>> +                        data, len, true);
>> +}
>> +
>> +static void
>> +ax88796c_rx_fixup(struct ax88796c_device *ax_local, struct sk_buff *rx_skb)
>> +{
>> +	struct rx_header *rxhdr = (struct rx_header *) rx_skb->data;
>> +	struct net_device *ndev = ax_local->ndev;
>> +	u16 len;
>> +
>> +	be16_to_cpus(&rxhdr->flags_len);
>> +	be16_to_cpus(&rxhdr->seq_lenbar);
>> +	be16_to_cpus(&rxhdr->flags);
>> +
>> +	if ((((short)rxhdr->flags_len) & RX_HDR1_PKT_LEN) !=
>> +			 (~((short)rxhdr->seq_lenbar) & 0x7FF)) {
>> +		if (netif_msg_rx_err(ax_local)) {
>> +			int i;
>> +			netdev_err(ndev, "Header error\n");
>> +			//netdev_err(ndev, "Dump received frame\n");
>> +			/* for (i = 0; i < rx_skb->len; i++) { */
>> +			/* 	netdev_err(ndev, "%02x ", */
>> +			/* 			*(rx_skb->data + i)); */
>> +			/* 	if (((i + 1) % 16) == 0) */
>> +			/* 		netdev_err(ndev, "\n"); */
>> +			/* } */
>
> No commented out code.
>

Removed.

>> +			dump_packet(ndev, __func__, rx_skb->len, rx_skb->data);
>
> and this is questionable. I can understand it while writing a driver,
> but once it works, this is the sort of thing you remove.
>

Removed.

>> +		}
>> +		ax_local->stats.rx_frame_errors++;
>> +		kfree_skb(rx_skb);
>> +		return;
>> +	}
>> +
>> +	if ((rxhdr->flags_len & RX_HDR1_MII_ERR) ||
>> +			(rxhdr->flags_len & RX_HDR1_CRC_ERR)) {
>> +		if (netif_msg_rx_err(ax_local))
>> +			netdev_err(ndev, "CRC or MII error\n");
>> +
>> +		ax_local->stats.rx_crc_errors++;
>> +		kfree_skb(rx_skb);
>> +		return;
>> +	}
>> +
>> +	len = rxhdr->flags_len & RX_HDR1_PKT_LEN;
>> +	if (netif_msg_pktdata(ax_local)) {
>> +		int loop;
>> +		netdev_info(ndev, "RX data, total len %d, packet len %d\n",
>> +				rx_skb->len, len);
>> +
>> +		netdev_info(ndev, "  Dump RX packet header:\n    ");
>> +		for (loop = 0; loop < sizeof(*rxhdr); loop++)
>> +			netdev_info(ndev, "%02x ", *(rx_skb->data + loop));
>> +
>> +		netdev_info(ndev, "\n  Dump RX packet:");
>> +		for (loop = 0; loop < len; loop++) {
>> +			if ((loop % 16) == 0)
>> +				netdev_info(ndev, "\n    ");
>> +			netdev_info(ndev, "%02x ",
>> +				*(rx_skb->data + loop + sizeof(*rxhdr)));
>> +		}
>> +		netdev_info(ndev, "\n");
>> +	}
>> +
>> +	skb_pull(rx_skb, sizeof(*rxhdr));
>> +	__pskb_trim(rx_skb, len);
>> +
>> +	return ax88796c_skb_return(ax_local, rx_skb, rxhdr);
>> +}
>
>> +void ax88796c_phy_init(struct ax88796c_device *ax_local)
>> +{
>> +	u16 advertise = ADVERTISE_ALL | ADVERTISE_CSMA | ADVERTISE_PAUSE_CAP;
>> +
>> +	/* Setup LED mode */
>> +	AX_WRITE(&ax_local->ax_spi,
>> +		  (LCR_LED0_EN | LCR_LED0_DUPLEX | LCR_LED1_EN |
>> +		   LCR_LED1_100MODE), P2_LCR0);
>> +	AX_WRITE(&ax_local->ax_spi,
>> +		  (AX_READ(&ax_local->ax_spi, P2_LCR1) & LCR_LED2_MASK) |
>> +		   LCR_LED2_EN | LCR_LED2_LINK, P2_LCR1);
>> +
>> +	/* Enable PHY auto-polling */
>> +	AX_WRITE(&ax_local->ax_spi,
>> +		  POOLCR_PHYID(ax_local->mii.phy_id) | POOLCR_POLL_EN |
>> +		  POOLCR_POLL_FLOWCTRL | POOLCR_POLL_BMCR, P2_POOLCR);
>
> What exactly does PHY polling do? Generally, you don't want the MAC
> touching the PHY, because it can upset the PHY driver.
>

See above (single integrated phy).

>> +
>> +	ax88796c_mdio_write(ax_local->ndev,
>> +			ax_local->mii.phy_id, MII_ADVERTISE, advertise);
>> +
>> +	ax88796c_mdio_write(ax_local->ndev, ax_local->mii.phy_id, MII_BMCR,
>> +			BMCR_SPEED100 | BMCR_ANENABLE | BMCR_ANRESTART);
>> +}
>> +
>
> I stopped reviewing here.

It took a while to work through it all. Thank you very much for your
effort.

-- 
Ł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