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
