Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Lukasz Stelmach <l.stelmach@samsung.com>
To:     Andrew Lunn <andrew@lunn.ch>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Krzysztof Kozlowski <krzk@kernel.org>,
        Kukjin Kim <kgene@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Russell King <linux@armlinux.org.uk>, jim.cromie@gmail.com,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-samsung-soc@vger.kernel.org, netdev@vger.kernel.org,
        devicetree@vger.kernel.org,
        =?utf-8?Q?Bart=C5=82omiej_=C5=BBolnierkiewi?= =?utf-8?Q?cz?= 
        <b.zolnierkie@samsung.com>,
        Marek Szyprowski <m.szyprowski@samsung.com>
Subject: Re: [PATCH v2 2/4] net: ax88796c: ASIX AX88796C SPI Ethernet
 Adapter Driver
Date:   Fri, 16 Oct 2020 21:18:54 +0200
In-Reply-To: <20201016180100.GF139700@lunn.ch> (Andrew Lunn's message of
        "Fri, 16 Oct 2020 20:01:00 +0200")
Message-ID: <dleftj5z7a568x.fsf%l.stelmach@samsung.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-="; micalg="pgp-sha256";
        protocol="application/pgp-signature"
CMS-TYPE: 201P
References: <20201016180100.GF139700@lunn.ch>
        <CGME20201016191905eucas1p235fb430a8f330a39e64f4fd6b81decb2@eucas1p2.samsung.com>
Precedence: bulk

--=-=-=
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

It was <2020-10-16 pi=C4=85 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 =3D to_ax88796c_device(ndev);
>> >> +	u16 *p =3D _p;
>> >> +	int offset, i;
>> >> +
>> >> +	memset(p, 0, AX88796C_REGDUMP_LEN);
>> >> +
>> >> +	for (offset =3D 0; offset < AX88796C_REGDUMP_LEN; offset +=3D 2) {
>> >> +		if (!test_bit(offset / 2, ax88796c_no_regs_mask))
>> >> +			*p =3D AX_READ(&ax_local->ax_spi, offset);
>> >> +		p++;
>> >> +	}
>> >> +
>> >> +	for (i =3D 0; i < AX88796C_PHY_REGDUMP_LEN / 2; i++) {
>> >> +		*p =3D phy_read(ax_local->phydev, i);
>> >> +		p++;
>> >
>> > Depending on the PHY, that can be dangerous.
>>=20
>> This is a built-in generic PHY. The chip has no lines to attach any
>> other external one.
>>=20
>> > phylib could be busy doing things with the PHY. It could be looking at
>>=20
>> How does phylib prevent concurrent access to a PHY?=20
>
> 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.
>>=20
>> Different page?=20
>
> 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.
>>=20
>> No, I am afraid mii-tool is not able to dump registers.
>
> It should be able to.
>
> sudo mii-tool -vv eth0
> Using SIOCGMIIPHY=3D0x8947
> eth0: negotiated 1000baseT-FD flow-control, link ok
>   registers for MII PHY 0:=20
>     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, u1=
6 val)
>> >> +{
>> >> +	struct ax88796c_device *ax_local =3D 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 =3D read_poll_timeout(AX_READ, ret,
>> >> +				((ret & MDIOCR_VALID) !=3D 0), 0,
>> >> +				jiffies_to_usecs(HZ / 100), false,
>> >> +				&ax_local->ax_spi, P2_MDIOCR);
>> >> +	if (ret)
>> >> +		return -EIO;
>> >> +
>> >> +	if (loc =3D=3D 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?
>> >
>>=20
>> Well=E2=80=A6 it turns autonegotiation when changing advertised link mod=
es. But
>> this is obvious. As to why this code is here, I will honestly say =E2=80=
=94 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 =3D 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().
>> >
>>=20
>> But, with an unregistered network device ndev_owner in
>> phy_attach_direct() is NULL. Thus, phy_connect_direct() below fails.
>>=20
>> --8<---------------cut here---------------start------------->8---
>>    1332         if (dev)
>>    1333                 ndev_owner =3D dev->dev.parent->driver->owner;
>>    1334         if (ndev_owner !=3D bus->owner &&  !try_module_get(bus->=
owner)) {
>>    1335                 phydev_err(phydev, "failed to get the bus  modul=
e\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
>>=20
>>  #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.

=2D-=20
=C5=81ukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

--=-=-=
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEEXpuyqjq9kGEVr9UQsK4enJilgBAFAl+J8h4ACgkQsK4enJil
gBD8qAf/eYt3jr44Y+Wjm/yUU1Myn0dZY7KbEcVSJo2oZEiYL3nKpTvcFeMPw9dH
o49vEFwADVrjiHXyy1w05jJTyUBv8RiAMEpCovbFL26nW6eyxqNCyUUwRRDgi7fk
uBnUyJWTcryPVGWH2bw84+ZtTvGh9UuHQpAWzhGf0X48kIym5GvgPK4+OaonqoPg
C+9oQxLxJ4b0/+JHBSRcqHA2YVdHVZYCsCXchxkpX8cRxoNeLc6Y0pNXzfGKFYCM
wLpJd+ZZ6bL4f4sEdIfpimB0nIcV19BxtmvbWFR6Bzur6+fTYxHY9sH8CrGS4rrJ
+6ZXm5UZSxjT6/dYpUKiatNQZXTxFg==
=JcIo
-----END PGP SIGNATURE-----
--=-=-=--