Re: [PATCH v2 2/4] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver

[Lukasz Stelmach]

It was <2020-10-16 pią 20:01>, when Andrew Lunn wrote:
>> >> +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? 
>
> phydev->lock. All access to the PHY should go through the phylib,
> which will take the lock before calling into the driver.
>
>> > a different page for example.
>> 
>> Different page? 
>
> I was talking about the general case. A number of PHYs have more than
> 32 registers. So they implement pages to give access to more
> registers. For that to work, you need to ensure you don't have
> concurrent access.
>

It is not the case, this one has got only 7 and one only needs to make
sure the transaction in ax88796c_mdio_read() is not messed up.

>> > 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.
>
> It should be able to.
>
> sudo mii-tool -vv eth0
> Using SIOCGMIIPHY=0x8947
> eth0: negotiated 1000baseT-FD flow-control, link ok
>   registers for MII PHY 0: 
>     1040 79ed 001c c800 0de1 c5e1 006d 0000
>     0000 0200 0800 0000 0000 0000 0000 2000
>     0000 0000 ffff 0000 0000 0400 0f00 0f00
>     318b 0053 31ec 8012 bf1f 0000 0000 0000
>   product info: vendor 00:07:32, model 0 rev 0
>   basic mode:   autonegotiation enabled
>   basic status: autonegotiation complete, link ok
>   capabilities: 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD
>   advertising:  1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD flow-control
>   link partner: 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD flow-control

Indeed. However, mii-tool(1) simply calls ioctl(SIOCGMIIREG) number of
times. Is looping in the userspace any better than in kernel? Anyway,
thanks for pointing out possible problems, I will make sure to avoid them.

>> >> +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.
>
> Please do remove it.
>

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---
>
> Which is probably why most drivers actually attach the PHY in open()
> and detach it in close().
>
> It can be done in probe, just look around for a driver which does and
> copy it.
>

I will. Thanks for the info.

>> 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.
>
> Yes, that is useful for documentation. How about:
>
> #define PSR_NOT_RESET BIT(15)
>
> And then turn the logic around.

OK. This is an idea. I'll look around some more.

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

Attachment: signature.asc
Description: PGP signature