All of lore.kernel.org
 help / color / mirror / Atom feed
From: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
To: andriy.shevchenko@linux.intel.com, vee.khee.wong@intel.com,
	weifeng.voon@intel.com,
	Giuseppe Cavallaro <peppe.cavallaro@st.com>,
	Alexandre Torgue <alexandre.torgue@foss.st.com>,
	Jose Abreu <joabreu@synopsys.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	Joakim Zhang <qiangqing.zhang@nxp.com>,
	Andrew Lunn <andrew@lunn.ch>
Cc: linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org,
	Christophe JAILLET <christophe.jaillet@wanadoo.fr>,
	netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com,
	linux-arm-kernel@lists.infradead.org
Subject: [PATCH 1/2] stmmac: intel: Add a missing clk_disable_unprepare() call in intel_eth_pci_remove()
Date: Sat, 30 Jul 2022 20:19:47 +0200	[thread overview]
Message-ID: <b5b44a0c025d0fdddd9b9d23153261363089a06a.1659204745.git.christophe.jaillet@wanadoo.fr> (raw)

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


WARNING: multiple messages have this Message-ID (diff)
From: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
To: andriy.shevchenko@linux.intel.com, vee.khee.wong@intel.com,
	weifeng.voon@intel.com,
	Giuseppe Cavallaro <peppe.cavallaro@st.com>,
	Alexandre Torgue <alexandre.torgue@foss.st.com>,
	Jose Abreu <joabreu@synopsys.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	Joakim Zhang <qiangqing.zhang@nxp.com>,
	Andrew Lunn <andrew@lunn.ch>
Cc: linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org,
	Christophe JAILLET <christophe.jaillet@wanadoo.fr>,
	netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com,
	linux-arm-kernel@lists.infradead.org
Subject: [PATCH 1/2] stmmac: intel: Add a missing clk_disable_unprepare() call in intel_eth_pci_remove()
Date: Sat, 30 Jul 2022 20:19:47 +0200	[thread overview]
Message-ID: <b5b44a0c025d0fdddd9b9d23153261363089a06a.1659204745.git.christophe.jaillet@wanadoo.fr> (raw)

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

             reply	other threads:[~2022-07-30 18:20 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-07-30 18:19 Christophe JAILLET [this message]
2022-07-30 18:19 ` [PATCH 1/2] stmmac: intel: Add a missing clk_disable_unprepare() call in intel_eth_pci_remove() Christophe JAILLET
2022-07-30 18:20 ` [PATCH 2/2] stmmac: intel: Simplify intel_eth_pci_remove() Christophe JAILLET
2022-07-30 18:20   ` Christophe JAILLET
2022-07-30 20:07   ` Andy Shevchenko
2022-07-30 20:07     ` Andy Shevchenko
2022-07-30 20:17 ` [PATCH 1/2] stmmac: intel: Add a missing clk_disable_unprepare() call in intel_eth_pci_remove() Andy Shevchenko
2022-07-30 20:17   ` Andy Shevchenko
2022-07-30 20:30   ` Christophe JAILLET
2022-07-30 20:30     ` Christophe JAILLET
2022-07-30 20:30     ` Christophe JAILLET
2022-07-30 21:40 ` kernel test robot
2022-07-30 21:40   ` kernel test robot
2022-07-30 21:50 ` kernel test robot
2022-07-30 21:50   ` kernel test robot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b5b44a0c025d0fdddd9b9d23153261363089a06a.1659204745.git.christophe.jaillet@wanadoo.fr \
    --to=christophe.jaillet@wanadoo.fr \
    --cc=alexandre.torgue@foss.st.com \
    --cc=andrew@lunn.ch \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=joabreu@synopsys.com \
    --cc=kernel-janitors@vger.kernel.org \
    --cc=kuba@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-stm32@st-md-mailman.stormreply.com \
    --cc=mcoquelin.stm32@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=peppe.cavallaro@st.com \
    --cc=qiangqing.zhang@nxp.com \
    --cc=vee.khee.wong@intel.com \
    --cc=weifeng.voon@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.