| Message ID | 20231120084606.4083194-3-claudiu.beznea.uj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ravb: Add suspend to RAM and runtime PM support for RZ/G3S | expand |
On 11/20/23 11:45 AM, Claudiu wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > pm_runtime_get_sync() may return with error. In case it returns with error > dev->power.usage_count needs to be decremented. pm_runtime_resume_and_get() > takes care of this. Thus use it. > > Along with this pm_runtime_resume_and_get() and reset_control_deassert() > were moved before alloc_etherdev_mqs() to simplify the error path. I don't see how it simplifies the error path... Re-ordering the statements at the end of the error path seems cheaper than what you do. > Also, in case pm_runtime_resume_and_get() returns error the reset signal > is deasserted and runtime PM is disabled (by jumping to the proper > error handling label). > > Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper") > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> [...] MBR, Sergey
On 20.11.2023 21:23, Sergey Shtylyov wrote: > On 11/20/23 11:45 AM, Claudiu wrote: > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> pm_runtime_get_sync() may return with error. In case it returns with error >> dev->power.usage_count needs to be decremented. pm_runtime_resume_and_get() >> takes care of this. Thus use it. >> >> Along with this pm_runtime_resume_and_get() and reset_control_deassert() >> were moved before alloc_etherdev_mqs() to simplify the error path. > > I don't see how it simplifies the error path... By not changing it... Actually, I took the other approach: you suggested in patch 1 to re-arrange the error path, I did it the other way around: changed the initialization path... > Re-ordering the statements at the end of the error path seems cheaper than > what you do. > >> Also, in case pm_runtime_resume_and_get() returns error the reset signal >> is deasserted and runtime PM is disabled (by jumping to the proper >> error handling label). >> >> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper") >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > [...] > > MBR, Sergey
On 11/21/23 8:57 AM, claudiu beznea wrote: [...] >>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>> >>> pm_runtime_get_sync() may return with error. In case it returns with error >>> dev->power.usage_count needs to be decremented. pm_runtime_resume_and_get() >>> takes care of this. Thus use it. >>> >>> Along with this pm_runtime_resume_and_get() and reset_control_deassert() >>> were moved before alloc_etherdev_mqs() to simplify the error path. >> >> I don't see how it simplifies the error path... > > By not changing it... Actually, I took the other approach: you suggested in But it does need to be changed! It's not currently in the reverse order compared to the buildup path... > patch 1 to re-arrange the error path, I did it the other way around: > changed the initialization path... That way you needlessly obfuscate (by moving the code around) the core change you do in this patch: switching from calling pm_runtime_get_sync() to calling pm_runtime_resume_and_get(). :-/ [...] >>> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper") >>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> [...] MBR, Sergey
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index 342978bdbd7e..0486add302b3 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -2635,25 +2635,26 @@ static int ravb_probe(struct platform_device *pdev) return dev_err_probe(&pdev->dev, PTR_ERR(rstc), "failed to get cpg reset\n"); + error = reset_control_deassert(rstc); + if (error) + return error; + + pm_runtime_enable(&pdev->dev); + error = pm_runtime_resume_and_get(&pdev->dev); + if (error < 0) + goto pm_runtime_disable; + ndev = alloc_etherdev_mqs(sizeof(struct ravb_private), NUM_TX_QUEUE, NUM_RX_QUEUE); - if (!ndev) - return -ENOMEM; - + if (!ndev) { + error = -ENOMEM; + goto pm_runtime_put; + } info = of_device_get_match_data(&pdev->dev); ndev->features = info->net_features; ndev->hw_features = info->net_hw_features; - error = reset_control_deassert(rstc); - if (error) { - free_netdev(ndev); - return error; - } - - pm_runtime_enable(&pdev->dev); - pm_runtime_get_sync(&pdev->dev); - if (info->multi_irqs) { if (info->err_mgmt_irqs) irq = platform_get_irq_byname(pdev, "dia"); @@ -2878,8 +2879,9 @@ static int ravb_probe(struct platform_device *pdev) clk_disable_unprepare(priv->refclk); out_release: free_netdev(ndev); - +pm_runtime_put: pm_runtime_put(&pdev->dev); +pm_runtime_disable: pm_runtime_disable(&pdev->dev); reset_control_assert(rstc); return error;