[PATCH 1/2] stmmac: intel: Add a missing clk_disable_unprepare() call in intel_eth_pci_remove()

[PATCH 1/2] stmmac: intel: Add a missing clk_disable_unprepare() call in intel_eth_pci_remove()

[Christophe JAILLET]

Commit 09f012e64e4b ("stmmac: intel: Fix clock handling on error and remove
paths") removed this clk_disable_unprepare()

This was partly revert by commit ac322f86b56c ("net: stmmac: Fix clock
handling on remove path") which removed this clk_disable_unprepare()
because:
"
   While unloading the dwmac-intel driver, clk_disable_unprepare() is
   being called twice in stmmac_dvr_remove() and
   intel_eth_pci_remove(). This causes kernel panic on the second call.
"

However later on, commit 5ec55823438e8 ("net: stmmac: add clocks management
for gmac driver") has updated stmmac_dvr_remove() which do not call
clk_disable_unprepare() anymore.

So this call should now be called from intel_eth_pci_remove().

Fixes: 5ec55823438e8 ("net: stmmac: add clocks management for gmac driver")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
/!\     This patch is HIGHLY speculative.     /!\

The corresponding clk_disable_unprepare() is still called within the pm
related stmmac_bus_clks_config() function.

However, with my limited understanding of the pm API, I think it that the
patch is valid.
(in other word, does the pm_runtime_put() and/or pm_runtime_disable()
and/or stmmac_dvr_remove() can end up calling .runtime_suspend())

So please review with care, as I'm not able to test the change by myself.


If I'm wrong, maybe a comment explaining why it is safe to have this
call in the error handling path of the probe and not in the remove function
would avoid erroneous patches generated from static code analyzer to be
sent.
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
index 52f9ed8db9c9..9f38642f86ce 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
@@ -1134,6 +1134,7 @@ static void intel_eth_pci_remove(struct pci_dev *pdev)
 
 	stmmac_dvr_remove(&pdev->dev);
 
+	clk_disable_unprepare(plat->stmmac_clk);
 	clk_unregister_fixed_rate(priv->plat->stmmac_clk);
 
 	pcim_iounmap_regions(pdev, BIT(0));
-- 
2.34.1

[PATCH 2/2] stmmac: intel: Simplify intel_eth_pci_remove()

[Christophe JAILLET]

There is no point to call pcim_iounmap_regions() in the remove function,
this frees a managed resource that would be release by the framework
anyway.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
This patch is speculative.
Sometimes the order of releasing managed resources is tricky.

Just a few drivers have this pattern, while many call pcim_iomap_regions().
If I'm right and this patch is reviewed and merged, I'll look at the
other files if they also can be simplified a bit.
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
index 9f38642f86ce..f68d23051557 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
@@ -1136,8 +1136,6 @@ static void intel_eth_pci_remove(struct pci_dev *pdev)
 
 	clk_disable_unprepare(plat->stmmac_clk);
 	clk_unregister_fixed_rate(priv->plat->stmmac_clk);
-
-	pcim_iounmap_regions(pdev, BIT(0));
 }
 
 static int __maybe_unused intel_eth_pci_suspend(struct device *dev)
-- 
2.34.1

Re: [PATCH 2/2] stmmac: intel: Simplify intel_eth_pci_remove()

[Andy Shevchenko]

On Sat, Jul 30, 2022 at 08:20:02PM +0200, Christophe JAILLET wrote:
> There is no point to call pcim_iounmap_regions() in the remove function,
> this frees a managed resource that would be release by the framework
> anyway.

The patch is fully correct in my opinion. The iounmap() is called exactly in
the same order as if it's done implicitly by managed resources handlers, hence
no need to explicitly call it.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> This patch is speculative.
> Sometimes the order of releasing managed resources is tricky.
> 
> Just a few drivers have this pattern, while many call pcim_iomap_regions().
> If I'm right and this patch is reviewed and merged, I'll look at the
> other files if they also can be simplified a bit.
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> index 9f38642f86ce..f68d23051557 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> @@ -1136,8 +1136,6 @@ static void intel_eth_pci_remove(struct pci_dev *pdev)
>  
>  	clk_disable_unprepare(plat->stmmac_clk);
>  	clk_unregister_fixed_rate(priv->plat->stmmac_clk);
> -
> -	pcim_iounmap_regions(pdev, BIT(0));
>  }
>  
>  static int __maybe_unused intel_eth_pci_suspend(struct device *dev)
> -- 
> 2.34.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] stmmac: intel: Add a missing clk_disable_unprepare() call in intel_eth_pci_remove()

[Andy Shevchenko]

On Sat, Jul 30, 2022 at 08:19:47PM +0200, Christophe JAILLET wrote:
> Commit 09f012e64e4b ("stmmac: intel: Fix clock handling on error and remove
> paths") removed this clk_disable_unprepare()
> 
> This was partly revert by commit ac322f86b56c ("net: stmmac: Fix clock
> handling on remove path") which removed this clk_disable_unprepare()
> because:
> "
>    While unloading the dwmac-intel driver, clk_disable_unprepare() is
>    being called twice in stmmac_dvr_remove() and
>    intel_eth_pci_remove(). This causes kernel panic on the second call.
> "
> 
> However later on, commit 5ec55823438e8 ("net: stmmac: add clocks management
> for gmac driver") has updated stmmac_dvr_remove() which do not call
> clk_disable_unprepare() anymore.
> 
> So this call should now be called from intel_eth_pci_remove().

The correct way of fixing it (which might be very well end up functionally
the same as this patch), is to introduce ->quit() in struct stmmac_pci_info
and assign it correctly, because not all platforms enable clocks.

Perhaps, we may leave this patch as is (for the sake of easy backporting) and
apply another one as I explained above to avoid similar mistakes in the future.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Fixes: 5ec55823438e8 ("net: stmmac: add clocks management for gmac driver")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> /!\     This patch is HIGHLY speculative.     /!\
> 
> The corresponding clk_disable_unprepare() is still called within the pm
> related stmmac_bus_clks_config() function.
> 
> However, with my limited understanding of the pm API, I think it that the
> patch is valid.
> (in other word, does the pm_runtime_put() and/or pm_runtime_disable()
> and/or stmmac_dvr_remove() can end up calling .runtime_suspend())
> 
> So please review with care, as I'm not able to test the change by myself.
> 
> 
> If I'm wrong, maybe a comment explaining why it is safe to have this
> call in the error handling path of the probe and not in the remove function
> would avoid erroneous patches generated from static code analyzer to be
> sent.
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> index 52f9ed8db9c9..9f38642f86ce 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> @@ -1134,6 +1134,7 @@ static void intel_eth_pci_remove(struct pci_dev *pdev)
>  
>  	stmmac_dvr_remove(&pdev->dev);
>  
> +	clk_disable_unprepare(plat->stmmac_clk);
>  	clk_unregister_fixed_rate(priv->plat->stmmac_clk);
>  
>  	pcim_iounmap_regions(pdev, BIT(0));
> -- 
> 2.34.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] stmmac: intel: Add a missing clk_disable_unprepare() call in intel_eth_pci_remove()

[Christophe JAILLET]

Le 30/07/2022 à 22:17, Andy Shevchenko a écrit :
> On Sat, Jul 30, 2022 at 08:19:47PM +0200, Christophe JAILLET wrote:
>> Commit 09f012e64e4b ("stmmac: intel: Fix clock handling on error and remove
>> paths") removed this clk_disable_unprepare()
>>
>> This was partly revert by commit ac322f86b56c ("net: stmmac: Fix clock
>> handling on remove path") which removed this clk_disable_unprepare()
>> because:
>> "
>>     While unloading the dwmac-intel driver, clk_disable_unprepare() is
>>     being called twice in stmmac_dvr_remove() and
>>     intel_eth_pci_remove(). This causes kernel panic on the second call.
>> "
>>
>> However later on, commit 5ec55823438e8 ("net: stmmac: add clocks management
>> for gmac driver") has updated stmmac_dvr_remove() which do not call
>> clk_disable_unprepare() anymore.
>>
>> So this call should now be called from intel_eth_pci_remove().
> 
> The correct way of fixing it (which might be very well end up functionally
> the same as this patch), is to introduce ->quit() in struct stmmac_pci_info
> and assign it correctly, because not all platforms enable clocks.

I won't be able to propose anything like that.

By the way, in the first sentence of my log, s/removed/added/.
(I hope that it can be fixed when/if the patch is applied)


Thanks for the review.

CJ

> 
> Perhaps, we may leave this patch as is (for the sake of easy backporting) and
> apply another one as I explained above to avoid similar mistakes in the future.
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
>> Fixes: 5ec55823438e8 ("net: stmmac: add clocks management for gmac driver")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> /!\     This patch is HIGHLY speculative.     /!\
>>
>> The corresponding clk_disable_unprepare() is still called within the pm
>> related stmmac_bus_clks_config() function.
>>
>> However, with my limited understanding of the pm API, I think it that the
>> patch is valid.
>> (in other word, does the pm_runtime_put() and/or pm_runtime_disable()
>> and/or stmmac_dvr_remove() can end up calling .runtime_suspend())
>>
>> So please review with care, as I'm not able to test the change by myself.
>>
>>
>> If I'm wrong, maybe a comment explaining why it is safe to have this
>> call in the error handling path of the probe and not in the remove function
>> would avoid erroneous patches generated from static code analyzer to be
>> sent.
>> ---
>>   drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
>> index 52f9ed8db9c9..9f38642f86ce 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
>> @@ -1134,6 +1134,7 @@ static void intel_eth_pci_remove(struct pci_dev *pdev)
>>   
>>   	stmmac_dvr_remove(&pdev->dev);
>>   
>> +	clk_disable_unprepare(plat->stmmac_clk);
>>   	clk_unregister_fixed_rate(priv->plat->stmmac_clk);
>>   
>>   	pcim_iounmap_regions(pdev, BIT(0));
>> -- 
>> 2.34.1
>>
> 

Re: [PATCH 1/2] stmmac: intel: Add a missing clk_disable_unprepare() call in intel_eth_pci_remove()

[kernel test robot]

Hi Christophe,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.19-rc8 next-20220728]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Christophe-JAILLET/stmmac-intel-Add-a-missing-clk_disable_unprepare-call-in-intel_eth_pci_remove/20220731-022139
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 620725263f4222b3c94d4ee19846835feec0ad69
config: x86_64-randconfig-a003 (https://download.01.org/0day-ci/archive/20220731/202207310531.48IGPx8Z-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 52cd00cabf479aa7eb6dbb063b7ba41ea57bce9e)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/2d1d09034cc62ee19f799b92bb67640ba86ca557
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Christophe-JAILLET/stmmac-intel-Add-a-missing-clk_disable_unprepare-call-in-intel_eth_pci_remove/20220731-022139
        git checkout 2d1d09034cc62ee19f799b92bb67640ba86ca557
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/net/ethernet/stmicro/stmmac/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c:1107:24: error: use of undeclared identifier 'plat'
           clk_disable_unprepare(plat->stmmac_clk);
                                 ^
   1 error generated.


vim +/plat +1107 drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c

  1092	
  1093	/**
  1094	 * intel_eth_pci_remove
  1095	 *
  1096	 * @pdev: pci device pointer
  1097	 * Description: this function calls the main to free the net resources
  1098	 * and releases the PCI resources.
  1099	 */
  1100	static void intel_eth_pci_remove(struct pci_dev *pdev)
  1101	{
  1102		struct net_device *ndev = dev_get_drvdata(&pdev->dev);
  1103		struct stmmac_priv *priv = netdev_priv(ndev);
  1104	
  1105		stmmac_dvr_remove(&pdev->dev);
  1106	
> 1107		clk_disable_unprepare(plat->stmmac_clk);
  1108		clk_unregister_fixed_rate(priv->plat->stmmac_clk);
  1109	
  1110		pcim_iounmap_regions(pdev, BIT(0));
  1111	}
  1112	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 1/2] stmmac: intel: Add a missing clk_disable_unprepare() call in intel_eth_pci_remove()

[kernel test robot]

Hi Christophe,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.19-rc8 next-20220728]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Christophe-JAILLET/stmmac-intel-Add-a-missing-clk_disable_unprepare-call-in-intel_eth_pci_remove/20220731-022139
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 620725263f4222b3c94d4ee19846835feec0ad69
config: x86_64-randconfig-a016 (https://download.01.org/0day-ci/archive/20220731/202207310545.hHU5SagS-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 52cd00cabf479aa7eb6dbb063b7ba41ea57bce9e)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/2d1d09034cc62ee19f799b92bb67640ba86ca557
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Christophe-JAILLET/stmmac-intel-Add-a-missing-clk_disable_unprepare-call-in-intel_eth_pci_remove/20220731-022139
        git checkout 2d1d09034cc62ee19f799b92bb67640ba86ca557
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c:1107:24: error: use of undeclared identifier 'plat'
           clk_disable_unprepare(plat->stmmac_clk);
                                 ^
   1 error generated.


vim +/plat +1107 drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c

  1092	
  1093	/**
  1094	 * intel_eth_pci_remove
  1095	 *
  1096	 * @pdev: pci device pointer
  1097	 * Description: this function calls the main to free the net resources
  1098	 * and releases the PCI resources.
  1099	 */
  1100	static void intel_eth_pci_remove(struct pci_dev *pdev)
  1101	{
  1102		struct net_device *ndev = dev_get_drvdata(&pdev->dev);
  1103		struct stmmac_priv *priv = netdev_priv(ndev);
  1104	
  1105		stmmac_dvr_remove(&pdev->dev);
  1106	
> 1107		clk_disable_unprepare(plat->stmmac_clk);
  1108		clk_unregister_fixed_rate(priv->plat->stmmac_clk);
  1109	
  1110		pcim_iounmap_regions(pdev, BIT(0));
  1111	}
  1112	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp