| 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: <01f6cf2f-9d5f-010c-b3f3-194350d01cf0@samsung.com>
Date: Mon, 28 Jun 2021 10:27:47 +0200
From: Marek Szyprowski <m.szyprowski@...sung.com>
To: Oleksij Rempel <o.rempel@...gutronix.de>
Cc: Heiner Kallweit <hkallweit1@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>,
Russell King <linux@...linux.org.uk>, kernel@...gutronix.de,
linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2 4/8] net: usb: asix: ax88772: add phylib
support
Hi Oleksij,
On 23.06.2021 09:06, Oleksij Rempel wrote:
> On Mon, Jun 21, 2021 at 08:05:49AM +0200, Marek Szyprowski wrote:
>> On 18.06.2021 15:20, Oleksij Rempel wrote:
>>> On Fri, Jun 18, 2021 at 01:11:41PM +0200, Marek Szyprowski wrote:
>>>> On 18.06.2021 13:04, Heiner Kallweit wrote:
>>>>> On 18.06.2021 12:13, Oleksij Rempel wrote:
>>>>>> thank you for your feedback.
>>>>>>
>>>>>> On Fri, Jun 18, 2021 at 10:39:12AM +0200, Marek Szyprowski wrote:
>>>>>>> On 07.06.2021 10:27, Oleksij Rempel wrote:
>>>>>>>> To be able to use ax88772 with external PHYs and use advantage of
>>>>>>>> existing PHY drivers, we need to port at least ax88772 part of asix
>>>>>>>> driver to the phylib framework.
>>>>>>>>
>>>>>>>> Signed-off-by: Oleksij Rempel <o.rempel@...gutronix.de>
>>>>>>> I found one more issue with this patch. On one of my test boards
>>>>>>> (Samsung Exynos5250 SoC based Arndale) system fails to establish network
>>>>>>> connection just after starting the kernel when the driver is build-in.
>>>>>>>
>>>>> If you build in the MAC driver, do you also build in the PHY driver?
>>>>> If the PHY driver is still a module this could explain why genphy
>>>>> driver is used.
>>>>> And your dmesg filtering suppresses the phy_attached_info() output
>>>>> that would tell us the truth.
>>>> Here is a bit more complete log:
>>>>
>>>> # dmesg | grep -i Asix
>>>> [ 2.412966] usbcore: registered new interface driver asix
>>>> [ 4.620094] usb 1-3.2.4: Manufacturer: ASIX Elec. Corp.
>>>> [ 4.641797] asix 1-3.2.4:1.0 (unnamed net_device) (uninitialized):
>>>> invalid hw address, using random
>>>> [ 5.657009] libphy: Asix MDIO Bus: probed
>>>> [ 5.750584] Asix Electronics AX88772A usb-001:004:10: attached PHY
>>>> driver (mii_bus:phy_addr=usb-001:004:10, irq=POLL)
>>>> [ 5.763908] asix 1-3.2.4:1.0 eth0: register 'asix' at
>>>> usb-12110000.usb-3.2.4, ASIX AX88772 USB 2.0 Ethernet, fe:a5:29:e2:97:3e
>>>> [ 9.090270] asix 1-3.2.4:1.0 eth0: Link is Up - 100Mbps/Full - flow
>>>> control off
>>>>
>>>> This seems to be something different than missing PHY driver.
>>> Can you please test it:
>>>
>>> diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
>>> index aec97b021a73..7897108a1a42 100644
>>> --- a/drivers/net/usb/asix_devices.c
>>> +++ b/drivers/net/usb/asix_devices.c
>>> @@ -453,6 +453,7 @@ static int ax88772a_hw_reset(struct usbnet *dev, int in_pm)
>>> u16 rx_ctl, phy14h, phy15h, phy16h;
>>> u8 chipcode = 0;
>>>
>>> + netdev_info(dev->net, "ax88772a_hw_reset\n");
>>> ret = asix_write_gpio(dev, AX_GPIO_RSE, 5, in_pm);
>>> if (ret < 0)
>>> goto out;
>>> @@ -509,31 +510,7 @@ static int ax88772a_hw_reset(struct usbnet *dev, int in_pm)
>>> goto out;
>>> }
>>> } else if ((chipcode & AX_CHIPCODE_MASK) == AX_AX88772A_CHIPCODE) {
>>> - /* Check if the PHY registers have default settings */
>>> - phy14h = asix_mdio_read_nopm(dev->net, dev->mii.phy_id,
>>> - AX88772A_PHY14H);
>>> - phy15h = asix_mdio_read_nopm(dev->net, dev->mii.phy_id,
>>> - AX88772A_PHY15H);
>>> - phy16h = asix_mdio_read_nopm(dev->net, dev->mii.phy_id,
>>> - AX88772A_PHY16H);
>>> -
>>> - netdev_dbg(dev->net,
>>> - "772a_hw_reset: MR20=0x%x MR21=0x%x MR22=0x%x\n",
>>> - phy14h, phy15h, phy16h);
>>> -
>>> - /* Restore PHY registers default setting if not */
>>> - if (phy14h != AX88772A_PHY14H_DEFAULT)
>>> - asix_mdio_write_nopm(dev->net, dev->mii.phy_id,
>>> - AX88772A_PHY14H,
>>> - AX88772A_PHY14H_DEFAULT);
>>> - if (phy15h != AX88772A_PHY15H_DEFAULT)
>>> - asix_mdio_write_nopm(dev->net, dev->mii.phy_id,
>>> - AX88772A_PHY15H,
>>> - AX88772A_PHY15H_DEFAULT);
>>> - if (phy16h != AX88772A_PHY16H_DEFAULT)
>>> - asix_mdio_write_nopm(dev->net, dev->mii.phy_id,
>>> - AX88772A_PHY16H,
>>> - AX88772A_PHY16H_DEFAULT);
>>> + netdev_info(dev->net, "do not touch PHY regs\n");
>>> }
>>>
>>> ret = asix_write_cmd(dev, AX_CMD_WRITE_IPG0,
>> This doesn't help for this issue.
> Ok.
> So far I was not able to see obvious differences between:
> probe -> ip link set dev eth1 up
>
> and
>
> probe -> ip link set dev eth1 up;
> ip link set dev eth1 down;
> ip link set dev eth1 up
>
>
> Except of PHY sate. By default the PHY is in resumed state after probe
> and is able to negotiate the link even if the MAC is down.
> After ip link set dev eth1 down, the PHY is in suspend state, as
> expected.
>
> Can you please test this change?
>
> diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
> index aec97b021a73..2c115216420a 100644
> --- a/drivers/net/usb/asix_devices.c
> +++ b/drivers/net/usb/asix_devices.c
> @@ -701,6 +701,7 @@ static int ax88772_init_phy(struct usbnet *dev)
> return ret;
> }
>
> + phy_suspend(priv->phydev);
> priv->phydev->mac_managed_pm = 1;
>
> phy_attached_info(priv->phydev);
>
I'm sorry for the late reply, I've just got back from vacations. The
above change fixes the issue.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Powered by blists - more mailing lists