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