Date:   Wed, 11 Aug 2021 08:09:29 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     ljakku77@...il.com
Cc:     Leon Romanovsky <leon@...nel.org>, netdev@...r.kernel.org,
        nic_swsd@...ltek.com
Subject: Re: NET: r8168/r8169 identifying fix

On 10.08.2021 23:50, Late @ Gmail wrote:
> Hi,
> 
> Now I solved the reloading issue, and r8169 driver works line charm.
> 

This patch is a complete hack and and in parts simply wrong.
In addition it misses the basics of how to submit a patch.
Quality hasn't improved since your first attempt, so better stop
trying to submit this to mainline.

> Patch:
> 
> From: Lauri Jakku <lja@....fi>
> Date: Mon, 9 Aug 2021 21:44:53 +0300
> Subject: [PATCH] net:realtek:r8169 driver load fix
> 
>    net:realtek:r8169 driver load fix
> 
>      Problem:
> 
>        The problem is that (1st load) fails, but there is valid
>        HW found (the ID is known) and this patch is applied, the second
>        time of loading module works ok, and network is connected ok
>        etc.
> 
>      Solution:
> 
>        The driver will trust the HW that reports valid ID and then make
>        re-loading of the module as it would being reloaded manually.
> 
>        I do check that if the HW id is read ok from the HW, then pass
>        -EAGAIN ja try to load 5 times, sleeping 250ms in between.
> 
> Signed-off-by: Lauri Jakku <lja@....fi>
> diff --git a/linux-5.14-rc4/drivers/net/ethernet/realtek/r8169_main.c b/linux-5.14-rc4/drivers/net/ethernet/realtek/r8169_main.c
> index c7af5bc3b..d8e602527 100644
> --- a/linux-5.14-rc4/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/linux-5.14-rc4/drivers/net/ethernet/realtek/r8169_main.c
> @@ -634,6 +634,8 @@ struct rtl8169_private {
>      struct rtl_fw *rtl_fw;
>  
>      u32 ocp_base;
> +
> +    int retry_probeing;
>  };
>  
>  typedef void (*rtl_generic_fct)(struct rtl8169_private *tp);
> @@ -5097,13 +5099,16 @@ static int r8169_mdio_register(struct rtl8169_private *tp)
>      tp->phydev = mdiobus_get_phy(new_bus, 0);
>      if (!tp->phydev) {
>          return -ENODEV;
> -    } else if (!tp->phydev->drv) {
> -        /* Most chip versions fail with the genphy driver.
> -         * Therefore ensure that the dedicated PHY driver is loaded.
> +    } else if (tp->phydev->phy_id != RTL_GIGA_MAC_NONE) {

You compare two completely different things here. The phy_id has nothing
to do with the chip version enum.

> +        /* Most chip versions fail with the genphy driver, BUT do rerport valid IW
> +         * ID. Re-starting the module seem to fix the issue of non-functional driver.
>           */
> -        dev_err(&pdev->dev, "no dedicated PHY driver found for PHY ID 0x%08x, maybe realtek.ko needs to be added to initramfs?\n",
> +        dev_err(&pdev->dev,
> +            "no dedicated driver, but HW found: PHY PHY ID 0x%08x\n",
>              tp->phydev->phy_id);
> -        return -EUNATCH;
> +
> +        dev_err(&pdev->dev, "trying re-probe few times..\n");
> +
>      }
>  
>      tp->phydev->mac_managed_pm = 1;
> @@ -5250,6 +5255,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>      enum mac_version chipset;
>      struct net_device *dev;
>      u16 xid;
> +    int savederr = 0;
>  
>      dev = devm_alloc_etherdev(&pdev->dev, sizeof (*tp));
>      if (!dev)
> @@ -5261,6 +5267,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>      tp->dev = dev;
>      tp->pci_dev = pdev;
>      tp->supports_gmii = ent->driver_data == RTL_CFG_NO_GBIT ? 0 : 1;
> +    tp->retry_probeing = 0;
>      tp->eee_adv = -1;
>      tp->ocp_base = OCP_STD_PHY_BASE;
>  
> @@ -5410,7 +5417,15 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  
>      pci_set_drvdata(pdev, tp);
>  
> -    rc = r8169_mdio_register(tp);
> +    savederr = r8169_mdio_register(tp);
> +
> +    if (
> +        (tp->retry_probeing > 0) &&
> +        (savederr == -EAGAIN)
> +       ) {
> +        netdev_info(dev, " retry of probe requested..............");
> +    }
> +
>      if (rc)
>          return rc;
>  
> @@ -5435,6 +5450,14 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>      if (pci_dev_run_wake(pdev))
>          pm_runtime_put_sync(&pdev->dev);
>  
> +    if (
> +        (tp->retry_probeing > 0) &&
> +        (savederr == -EAGAIN)
> +       ) {
> +        netdev_info(dev, " retry of probe requested..............");
> +        return savederr;

You can not simply return here. You have to clean up.

> +    }
> +
>      return 0;
>  }
>  
> diff --git a/linux-5.14-rc4/drivers/net/phy/phy_device.c b/linux-5.14-rc4/drivers/net/phy/phy_device.c
> index 5d5f9a9ee..59c6ac031 100644

No mixing of changes in phylib and drivers.

> --- a/linux-5.14-rc4/drivers/net/phy/phy_device.c
> +++ b/linux-5.14-rc4/drivers/net/phy/phy_device.c
> @@ -2980,6 +2980,9 @@ struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
>  }
>  EXPORT_SYMBOL_GPL(fwnode_get_phy_node);
>  
> +
> +static int phy_remove(struct device *dev);
> +

No forward declarations.

>  /**
>   * phy_probe - probe and init a PHY device
>   * @dev: device to probe and init
> @@ -2988,13 +2991,22 @@ EXPORT_SYMBOL_GPL(fwnode_get_phy_node);
>   *   set the state to READY (the driver's init function should
>   *   set it to STARTING if needed).
>   */
> +#define REDO_PROBE_TIMES    5
>  static int phy_probe(struct device *dev)
>  {
>      struct phy_device *phydev = to_phy_device(dev);
>      struct device_driver *drv = phydev->mdio.dev.driver;
>      struct phy_driver *phydrv = to_phy_driver(drv);
> +    int again = 0;
> +    int savederr = 0;
> +again_retry:
>      int err = 0;
>  
> +    if (again > 0) {
> +        pr_err("%s: Re-probe %d of driver.....\n",
> +               phydrv->name, again);
> +    }
> +
>      phydev->drv = phydrv;
>  
>      /* Disable the interrupt if the PHY doesn't support it
> @@ -3013,6 +3025,17 @@ static int phy_probe(struct device *dev)
>  
>      if (phydev->drv->probe) {
>          err = phydev->drv->probe(phydev);
> +
> +        /* If again requested. */
> +        if (err == -EAGAIN) {

This doesn't make sense. You check the PHY driver probe return code,
mixing it up with the MAC driver return code.

> +            again++;
> +            savederr = err;
> +            err = 0;
> +
> +            pr_info("%s: EAGAIN: %d ...\n",
> +                phydrv->name, again);
> +        }
> +
>          if (err)
>              goto out;
>      }
> @@ -3081,6 +3104,20 @@ static int phy_probe(struct device *dev)
>  
>      mutex_unlock(&phydev->lock);
>  
> +    if ((savederr == -EAGAIN) &&
> +        ((again > 0) && (again < REDO_PROBE_TIMES))
> +       ) {
> +        pr_err("%s: Retry removal driver..\n",
> +            phydrv->name);
> +
> +        phy_remove(dev);
> +
> +        pr_err("%s: Re-probe driver..\n",
> +            phydrv->name);
> +        savederr = 0;
> +        goto again_retry;
> +    }
> +
>      return err;
>  }
>  
> @@ -3108,6 +3145,7 @@ static int phy_remove(struct device *dev)
>      return 0;
>  }
>  
> +
>  static void phy_shutdown(struct device *dev)
>  {
>      struct phy_device *phydev = to_phy_device(dev);
> 
> 
> 
> On 11.3.2021 18.43, gmail wrote:
>>
>> Heiner Kallweit kirjoitti 11.3.2021 klo 18.23:
>>> On 11.03.2021 17:00, gmail wrote:
>>>> 15. huhtik. 2020, 19.18, Heiner Kallweit <hkallweit1@...il.com <mailto:hkallweit1@...il.com>> kirjoitti:
>>>>
>>>>     On 15.04.2020 16:39, Lauri Jakku wrote:
>>>>
>>>>         Hi, There seems to he Something odd problem, maybe timing
>>>>         related. Stripped version not workingas expected. I get back to
>>>>         you, when  i have it working.
>>>>
>>>>     There's no point in working on your patch. W/o proper justification it
>>>>     isn't acceptable anyway. And so far we still don't know which problem
>>>>     you actually have.
>>>>     FIRST please provide the requested logs and explain the actual problem
>>>>     (incl. the commit that caused the regression).
>>>>
>>>>
>>>>              13. huhtik. 2020, 14.46, Lauri Jakku <ljakku77@...il.com
>>>>         <mailto:ljakku77@...il.com>> kirjoitti: Hi, Fair enough, i'll
>>>>         strip them. -lja On 2020-04-13 14:34, Leon Romanovsky wrote: On
>>>>         Mon, Apr 13, 2020 at 02:02:01PM +0300, Lauri Jakku wrote: Hi,
>>>>         Comments inline. On 2020-04-13 13:58, Leon Romanovsky wrote: On
>>>>         Mon, Apr 13, 2020 at 01:30:13PM +0300, Lauri Jakku wrote: From
>>>>         2d41edd4e6455187094f3a13d58c46eeee35aa31 Mon Sep 17 00:00:00
>>>>         2001 From: Lauri Jakku <lja@....fi> Date: Mon, 13 Apr 2020
>>>>         13:18:35 +0300 Subject: [PATCH] NET: r8168/r8169 identifying fix
>>>>         The driver installation determination made properly by checking
>>>>         PHY vs DRIVER id's. ---
>>>>         drivers/net/ethernet/realtek/r8169_main.c | 70
>>>>         ++++++++++++++++++++--- drivers/net/phy/mdio_bus.c | 11 +++- 2
>>>>         files changed, 72 insertions(+), 9 deletions(-) I would say that
>>>>         most of the code is debug prints. I tought that they are helpful
>>>>         to keep, they are using the debug calls, so they are not visible
>>>>         if user does not like those. You are missing the point of who
>>>>         are your users. Users want to have working device and the code.
>>>>         They don't need or like to debug their kernel. Thanks
>>>>
>>>>     Hi, now i got time to tackle with this again :) .. I know the proposed fix is quite hack, BUT it does give a clue what is wrong.
>>>>
>>>>     Something in subsystem is not working at the first time, but it needs to be reloaded to work ok (second time). So what I will do
>>>>     is that I try out re-do the module load within the module, if there is known HW id available but driver is not available, that
>>>>     would be much nicer and user friendly way.
>>>>
>>>>
>>>>     When the module setup it self nicely on first load, then can be the hunt for late-init of subsystem be checked out. Is the HW
>>>>     not brought up correct way during first time, or does the HW need time to brough up, or what is the cause.
>>>>
>>>>     The justification is the same as all HW driver bugs, the improvement is always better to take in. Or do this patch have some-
>>>>     thing what other patches do not?
>>>>
>>>>     Is there legit reason why NOT to improve something, that is clearly issue for others also than just me ? I will take on the
>>>>     task to fiddle with the module to get it more-less hacky and fully working version. Without the need for user to do something
>>>>     for the module to work.
>>>>
>>>>         --Lauri J.
>>>>
>>>>
>>> I have no clue what you're trying to say. The last patch wasn't acceptable at all.
>>> If you want to submit a patch:
>>>
>>> - Follow kernel code style
>>> - Explain what the exact problem is, what the root cause is, and how your patch fixes it
>>> - Explain why you're sure that it doesn't break processing on other chip versions
>>>    and systems.
>>>
>> Ok, i'll make nice patch that has in comment what is the problem and how does the patch help the case at hand.
>>
>> I don't know the rootcause, but something in subsystem that possibly is initializing bit slowly, cause the reloading
>>
>> of the module provides working network connection, when done via insmod cycle. I'm not sure is it just a timing
>>
>> issue or what. I'd like to check where is the driver pointer populated, and put some debugs to see if the issue is just
>>
>> timing, let's see.
>>
>>
>> The problem is that (1st load) fails, but there is valid HW found (the ID is known), when the hacky patch of mine
>>
>> is applied, the second time of loading module works ok, and network is connected ok etc.
>>
>>
>> I make the change so that when the current HEAD code is going to return failure, i do check that if the HW id is read ok
>>
>> from the HW, then pass -EAGAIN ja try to load 5 times, sleeping 250ms in between.
>>
>>
>> --Lauri J.
>>
>>
>>
>>

Powered by blists - more mailing lists