| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20201016180100.GF139700@lunn.ch>
Date: Fri, 16 Oct 2020 20:01:00 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Lukasz Stelmach <l.stelmach@...sung.com>
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
> >> +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.
> > 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
> >> +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.
> >> +
> >> + 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.
> 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.
Andrew
Powered by blists - more mailing lists