* [PATCH net] stmmac: dwmac-intel-plat: fix call balance of tx_clk handling routines
@ 2024-09-30 18:37 Vitalii Mordan
2024-10-03 11:18 ` Simon Horman
0 siblings, 1 reply; 4+ messages in thread
From: Vitalii Mordan @ 2024-09-30 18:37 UTC (permalink / raw)
To: Alexandre Torgue
Cc: Vitalii Mordan, Jose Abreu, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Maxime Coquelin, netdev, linux-stm32,
linux-arm-kernel, linux-kernel, lvc-project, Fedor Pchelkin,
Alexey Khoroshilov, Vadim Mutilin
If the clock dwmac->tx_clk was not enabled in intel_eth_plat_probe,
it should not be disabled in any path.
Conversely, if it was enabled in intel_eth_plat_probe, it must be disabled
in all error paths to ensure proper cleanup.
Found by Linux Verification Center (linuxtesting.org) with Klever.
Fixes: 9efc9b2b04c7 ("net: stmmac: Add dwmac-intel-plat for GBE driver")
Signed-off-by: Vitalii Mordan <mordan@ispras.ru>
---
.../ethernet/stmicro/stmmac/dwmac-intel-plat.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
index d68f0c4e7835..2a2893f2f2a8 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
@@ -108,7 +108,12 @@ static int intel_eth_plat_probe(struct platform_device *pdev)
if (IS_ERR(dwmac->tx_clk))
return PTR_ERR(dwmac->tx_clk);
- clk_prepare_enable(dwmac->tx_clk);
+ ret = clk_prepare_enable(dwmac->tx_clk);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "Failed to enable tx_clk\n");
+ return ret;
+ }
/* Check and configure TX clock rate */
rate = clk_get_rate(dwmac->tx_clk);
@@ -117,6 +122,7 @@ static int intel_eth_plat_probe(struct platform_device *pdev)
rate = dwmac->data->tx_clk_rate;
ret = clk_set_rate(dwmac->tx_clk, rate);
if (ret) {
+ clk_disable_unprepare(dwmac->tx_clk);
dev_err(&pdev->dev,
"Failed to set tx_clk\n");
return ret;
@@ -131,6 +137,8 @@ static int intel_eth_plat_probe(struct platform_device *pdev)
rate = dwmac->data->ptp_ref_clk_rate;
ret = clk_set_rate(plat_dat->clk_ptp_ref, rate);
if (ret) {
+ if (dwmac->data->tx_clk_en)
+ clk_disable_unprepare(dwmac->tx_clk);
dev_err(&pdev->dev,
"Failed to set clk_ptp_ref\n");
return ret;
@@ -150,7 +158,8 @@ static int intel_eth_plat_probe(struct platform_device *pdev)
ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
if (ret) {
- clk_disable_unprepare(dwmac->tx_clk);
+ if (dwmac->data->tx_clk_en)
+ clk_disable_unprepare(dwmac->tx_clk);
return ret;
}
@@ -162,7 +171,8 @@ static void intel_eth_plat_remove(struct platform_device *pdev)
struct intel_dwmac *dwmac = get_stmmac_bsp_priv(&pdev->dev);
stmmac_pltfr_remove(pdev);
- clk_disable_unprepare(dwmac->tx_clk);
+ if (dwmac->data->tx_clk_en)
+ clk_disable_unprepare(dwmac->tx_clk);
}
static struct platform_driver intel_eth_plat_driver = {
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net] stmmac: dwmac-intel-plat: fix call balance of tx_clk handling routines
2024-09-30 18:37 [PATCH net] stmmac: dwmac-intel-plat: fix call balance of tx_clk handling routines Vitalii Mordan
@ 2024-10-03 11:18 ` Simon Horman
2024-10-03 12:55 ` Fedor Pchelkin
0 siblings, 1 reply; 4+ messages in thread
From: Simon Horman @ 2024-10-03 11:18 UTC (permalink / raw)
To: Vitalii Mordan
Cc: Alexandre Torgue, Jose Abreu, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Maxime Coquelin, netdev, linux-stm32,
linux-arm-kernel, linux-kernel, lvc-project, Fedor Pchelkin,
Alexey Khoroshilov, Vadim Mutilin
On Mon, Sep 30, 2024 at 09:37:15PM +0300, Vitalii Mordan wrote:
> If the clock dwmac->tx_clk was not enabled in intel_eth_plat_probe,
> it should not be disabled in any path.
>
> Conversely, if it was enabled in intel_eth_plat_probe, it must be disabled
> in all error paths to ensure proper cleanup.
>
> Found by Linux Verification Center (linuxtesting.org) with Klever.
>
> Fixes: 9efc9b2b04c7 ("net: stmmac: Add dwmac-intel-plat for GBE driver")
> Signed-off-by: Vitalii Mordan <mordan@ispras.ru>
> ---
> .../ethernet/stmicro/stmmac/dwmac-intel-plat.c | 16 +++++++++++++---
> 1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
> index d68f0c4e7835..2a2893f2f2a8 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
> @@ -108,7 +108,12 @@ static int intel_eth_plat_probe(struct platform_device *pdev)
> if (IS_ERR(dwmac->tx_clk))
> return PTR_ERR(dwmac->tx_clk);
>
> - clk_prepare_enable(dwmac->tx_clk);
> + ret = clk_prepare_enable(dwmac->tx_clk);
> + if (ret) {
> + dev_err(&pdev->dev,
> + "Failed to enable tx_clk\n");
> + return ret;
> + }
>
> /* Check and configure TX clock rate */
> rate = clk_get_rate(dwmac->tx_clk);
> @@ -117,6 +122,7 @@ static int intel_eth_plat_probe(struct platform_device *pdev)
> rate = dwmac->data->tx_clk_rate;
> ret = clk_set_rate(dwmac->tx_clk, rate);
> if (ret) {
> + clk_disable_unprepare(dwmac->tx_clk);
> dev_err(&pdev->dev,
> "Failed to set tx_clk\n");
> return ret;
Hi Vitalii,
I think that unwinding using a goto label would be more idiomatic here
and in the following changes to intel_eth_plat_probe().
> @@ -131,6 +137,8 @@ static int intel_eth_plat_probe(struct platform_device *pdev)
> rate = dwmac->data->ptp_ref_clk_rate;
> ret = clk_set_rate(plat_dat->clk_ptp_ref, rate);
> if (ret) {
> + if (dwmac->data->tx_clk_en)
> + clk_disable_unprepare(dwmac->tx_clk);
> dev_err(&pdev->dev,
> "Failed to set clk_ptp_ref\n");
> return ret;
> @@ -150,7 +158,8 @@ static int intel_eth_plat_probe(struct platform_device *pdev)
>
> ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
> if (ret) {
> - clk_disable_unprepare(dwmac->tx_clk);
> + if (dwmac->data->tx_clk_en)
> + clk_disable_unprepare(dwmac->tx_clk);
Smatch warns that dwmac->data may be NULL here.
> return ret;
> }
>
> @@ -162,7 +171,8 @@ static void intel_eth_plat_remove(struct platform_device *pdev)
> struct intel_dwmac *dwmac = get_stmmac_bsp_priv(&pdev->dev);
>
> stmmac_pltfr_remove(pdev);
> - clk_disable_unprepare(dwmac->tx_clk);
> + if (dwmac->data->tx_clk_en)
And I wonder if it can be NULL here too.
> + clk_disable_unprepare(dwmac->tx_clk);
> }
>
> static struct platform_driver intel_eth_plat_driver = {
> --
> 2.25.1
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] stmmac: dwmac-intel-plat: fix call balance of tx_clk handling routines
2024-10-03 11:18 ` Simon Horman
@ 2024-10-03 12:55 ` Fedor Pchelkin
2024-10-03 14:35 ` Simon Horman
0 siblings, 1 reply; 4+ messages in thread
From: Fedor Pchelkin @ 2024-10-03 12:55 UTC (permalink / raw)
To: Simon Horman
Cc: Vitalii Mordan, Alexandre Torgue, Jose Abreu, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Maxime Coquelin,
netdev, linux-stm32, linux-arm-kernel, linux-kernel, lvc-project,
Alexey Khoroshilov, Vadim Mutilin
Hello,
On Thu, 03. Oct 12:18, Simon Horman wrote:
> On Mon, Sep 30, 2024 at 09:37:15PM +0300, Vitalii Mordan wrote:
> > If the clock dwmac->tx_clk was not enabled in intel_eth_plat_probe,
> > it should not be disabled in any path.
> >
> > Conversely, if it was enabled in intel_eth_plat_probe, it must be disabled
> > in all error paths to ensure proper cleanup.
> >
> > Found by Linux Verification Center (linuxtesting.org) with Klever.
> >
> > Fixes: 9efc9b2b04c7 ("net: stmmac: Add dwmac-intel-plat for GBE driver")
> > Signed-off-by: Vitalii Mordan <mordan@ispras.ru>
> > ---
> > .../ethernet/stmicro/stmmac/dwmac-intel-plat.c | 16 +++++++++++++---
> > 1 file changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
> > index d68f0c4e7835..2a2893f2f2a8 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
> > @@ -108,7 +108,12 @@ static int intel_eth_plat_probe(struct platform_device *pdev)
> > if (IS_ERR(dwmac->tx_clk))
> > return PTR_ERR(dwmac->tx_clk);
> >
> > - clk_prepare_enable(dwmac->tx_clk);
> > + ret = clk_prepare_enable(dwmac->tx_clk);
> > + if (ret) {
> > + dev_err(&pdev->dev,
> > + "Failed to enable tx_clk\n");
> > + return ret;
> > + }
> >
> > /* Check and configure TX clock rate */
> > rate = clk_get_rate(dwmac->tx_clk);
> > @@ -117,6 +122,7 @@ static int intel_eth_plat_probe(struct platform_device *pdev)
> > rate = dwmac->data->tx_clk_rate;
> > ret = clk_set_rate(dwmac->tx_clk, rate);
> > if (ret) {
> > + clk_disable_unprepare(dwmac->tx_clk);
> > dev_err(&pdev->dev,
> > "Failed to set tx_clk\n");
> > return ret;
>
> Hi Vitalii,
>
> I think that unwinding using a goto label would be more idiomatic here
> and in the following changes to intel_eth_plat_probe().
>
> > @@ -131,6 +137,8 @@ static int intel_eth_plat_probe(struct platform_device *pdev)
> > rate = dwmac->data->ptp_ref_clk_rate;
> > ret = clk_set_rate(plat_dat->clk_ptp_ref, rate);
> > if (ret) {
> > + if (dwmac->data->tx_clk_en)
> > + clk_disable_unprepare(dwmac->tx_clk);
> > dev_err(&pdev->dev,
> > "Failed to set clk_ptp_ref\n");
> > return ret;
> > @@ -150,7 +158,8 @@ static int intel_eth_plat_probe(struct platform_device *pdev)
> >
> > ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
> > if (ret) {
> > - clk_disable_unprepare(dwmac->tx_clk);
> > + if (dwmac->data->tx_clk_en)
> > + clk_disable_unprepare(dwmac->tx_clk);
>
> Smatch warns that dwmac->data may be NULL here.
FWIW, there is a patch [1] targeted at net-next which removes the seemingly
redundant check for dwmac->data.
[1]: https://lore.kernel.org/netdev/20240930183926.2112546-1-mordan@ispras.ru/
At the moment device_get_match_data() can't return NULL in probe function
of this driver - it gets the data from static const intel_eth_plat_match[]
table where every entry has defined non-NULL .data.
It's not expected (at least currently) that there would be any code changes
to the driver match table so it looks worthwhile to remove the check in
order to reduce additional complexity in error paths and
intel_eth_plat_remove().
That said, maybe it would be more safe now to rearrange the check to fail
at probe stage in case dwmac->data is NULL. Just not to confuse the static
analysis tools :)
Thanks!
>
> > return ret;
> > }
> >
> > @@ -162,7 +171,8 @@ static void intel_eth_plat_remove(struct platform_device *pdev)
> > struct intel_dwmac *dwmac = get_stmmac_bsp_priv(&pdev->dev);
> >
> > stmmac_pltfr_remove(pdev);
> > - clk_disable_unprepare(dwmac->tx_clk);
> > + if (dwmac->data->tx_clk_en)
>
> And I wonder if it can be NULL here too.
>
> > + clk_disable_unprepare(dwmac->tx_clk);
> > }
> >
> > static struct platform_driver intel_eth_plat_driver = {
> > --
> > 2.25.1
> >
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] stmmac: dwmac-intel-plat: fix call balance of tx_clk handling routines
2024-10-03 12:55 ` Fedor Pchelkin
@ 2024-10-03 14:35 ` Simon Horman
0 siblings, 0 replies; 4+ messages in thread
From: Simon Horman @ 2024-10-03 14:35 UTC (permalink / raw)
To: Fedor Pchelkin
Cc: Vitalii Mordan, Alexandre Torgue, Jose Abreu, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Maxime Coquelin,
netdev, linux-stm32, linux-arm-kernel, linux-kernel, lvc-project,
Alexey Khoroshilov, Vadim Mutilin, Andrew Lunn
+ Andrew Lunn
On Thu, Oct 03, 2024 at 03:55:35PM +0300, Fedor Pchelkin wrote:
> Hello,
>
> On Thu, 03. Oct 12:18, Simon Horman wrote:
> > On Mon, Sep 30, 2024 at 09:37:15PM +0300, Vitalii Mordan wrote:
> > > If the clock dwmac->tx_clk was not enabled in intel_eth_plat_probe,
> > > it should not be disabled in any path.
> > >
> > > Conversely, if it was enabled in intel_eth_plat_probe, it must be disabled
> > > in all error paths to ensure proper cleanup.
> > >
> > > Found by Linux Verification Center (linuxtesting.org) with Klever.
> > >
> > > Fixes: 9efc9b2b04c7 ("net: stmmac: Add dwmac-intel-plat for GBE driver")
> > > Signed-off-by: Vitalii Mordan <mordan@ispras.ru>
> > > ---
> > > .../ethernet/stmicro/stmmac/dwmac-intel-plat.c | 16 +++++++++++++---
> > > 1 file changed, 13 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
> > > index d68f0c4e7835..2a2893f2f2a8 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
> > > @@ -108,7 +108,12 @@ static int intel_eth_plat_probe(struct platform_device *pdev)
> > > if (IS_ERR(dwmac->tx_clk))
> > > return PTR_ERR(dwmac->tx_clk);
> > >
> > > - clk_prepare_enable(dwmac->tx_clk);
> > > + ret = clk_prepare_enable(dwmac->tx_clk);
> > > + if (ret) {
> > > + dev_err(&pdev->dev,
> > > + "Failed to enable tx_clk\n");
> > > + return ret;
> > > + }
> > >
> > > /* Check and configure TX clock rate */
> > > rate = clk_get_rate(dwmac->tx_clk);
> > > @@ -117,6 +122,7 @@ static int intel_eth_plat_probe(struct platform_device *pdev)
> > > rate = dwmac->data->tx_clk_rate;
> > > ret = clk_set_rate(dwmac->tx_clk, rate);
> > > if (ret) {
> > > + clk_disable_unprepare(dwmac->tx_clk);
> > > dev_err(&pdev->dev,
> > > "Failed to set tx_clk\n");
> > > return ret;
> >
> > Hi Vitalii,
> >
> > I think that unwinding using a goto label would be more idiomatic here
> > and in the following changes to intel_eth_plat_probe().
> >
> > > @@ -131,6 +137,8 @@ static int intel_eth_plat_probe(struct platform_device *pdev)
> > > rate = dwmac->data->ptp_ref_clk_rate;
> > > ret = clk_set_rate(plat_dat->clk_ptp_ref, rate);
> > > if (ret) {
> > > + if (dwmac->data->tx_clk_en)
> > > + clk_disable_unprepare(dwmac->tx_clk);
> > > dev_err(&pdev->dev,
> > > "Failed to set clk_ptp_ref\n");
> > > return ret;
> > > @@ -150,7 +158,8 @@ static int intel_eth_plat_probe(struct platform_device *pdev)
> > >
> > > ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
> > > if (ret) {
> > > - clk_disable_unprepare(dwmac->tx_clk);
> > > + if (dwmac->data->tx_clk_en)
> > > + clk_disable_unprepare(dwmac->tx_clk);
> >
> > Smatch warns that dwmac->data may be NULL here.
>
> FWIW, there is a patch [1] targeted at net-next which removes the seemingly
> redundant check for dwmac->data.
>
> [1]: https://lore.kernel.org/netdev/20240930183926.2112546-1-mordan@ispras.ru/
>
> At the moment device_get_match_data() can't return NULL in probe function
> of this driver - it gets the data from static const intel_eth_plat_match[]
> table where every entry has defined non-NULL .data.
>
> It's not expected (at least currently) that there would be any code changes
> to the driver match table so it looks worthwhile to remove the check in
> order to reduce additional complexity in error paths and
> intel_eth_plat_remove().
Thanks, I hadn't correlated that patch with this one.
I agree that it address my comment about needing
to check wmac->data here and below.
> That said, maybe it would be more safe now to rearrange the check to fail
> at probe stage in case dwmac->data is NULL. Just not to confuse the static
> analysis tools :)
I see that Andrew Lunn responded that a comment is appropriate,
rather than writing code to please the checkers. And, FWIIW,
I agree with his suggestion.
>
> Thanks!
>
> >
> > > return ret;
> > > }
> > >
> > > @@ -162,7 +171,8 @@ static void intel_eth_plat_remove(struct platform_device *pdev)
> > > struct intel_dwmac *dwmac = get_stmmac_bsp_priv(&pdev->dev);
> > >
> > > stmmac_pltfr_remove(pdev);
> > > - clk_disable_unprepare(dwmac->tx_clk);
> > > + if (dwmac->data->tx_clk_en)
> >
> > And I wonder if it can be NULL here too.
> >
> > > + clk_disable_unprepare(dwmac->tx_clk);
> > > }
> > >
> > > static struct platform_driver intel_eth_plat_driver = {
> > > --
> > > 2.25.1
> > >
> > >
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-10-03 14:37 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-30 18:37 [PATCH net] stmmac: dwmac-intel-plat: fix call balance of tx_clk handling routines Vitalii Mordan
2024-10-03 11:18 ` Simon Horman
2024-10-03 12:55 ` Fedor Pchelkin
2024-10-03 14:35 ` Simon Horman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).