| Message ID | 20201108095310.2892555-1-zhangqilong3@huawei.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: fec: Fix reference count leak in fec series ops | expand |
From: Zhang Qilong <zhangqilong3@huawei.com> Sent: Sunday, November 8, 2020 5:53 PM > pm_runtime_get_sync() will increment pm usage at first and it will resume the > device later. If runtime of the device has error or device is in inaccessible > state(or other error state), resume operation will fail. If we do not call put > operation to decrease the reference, it will result in reference count leak. > Moreover, this device cannot enter the idle state and always stay busy or other > non-idle state later. So we fixed it through adding pm_runtime_put_noidle. > > Fixes: 8fff755e9f8d0 ("net: fec: Ensure clocks are enabled while using mdio bus") > Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com> From early discussion for the topic, Wolfram Sang wonder if such de-reference can be better handled by pm runtime core code. https://lkml.org/lkml/2020/6/14/76 Regards, Andy > --- > drivers/net/ethernet/freescale/fec_main.c | 22 ++++++++++++++++------ > 1 file changed, 16 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec_main.c > b/drivers/net/ethernet/freescale/fec_main.c > index d7919555250d..6c02f885c67e 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -1809,8 +1809,10 @@ static int fec_enet_mdio_read(struct mii_bus *bus, > int mii_id, int regnum) > bool is_c45 = !!(regnum & MII_ADDR_C45); > > ret = pm_runtime_get_sync(dev); > - if (ret < 0) > + if (ret < 0) { > + pm_runtime_put_noidle(dev); > return ret; > + } > > if (is_c45) { > frame_start = FEC_MMFR_ST_C45; > @@ -1868,10 +1870,12 @@ static int fec_enet_mdio_write(struct mii_bus > *bus, int mii_id, int regnum, > bool is_c45 = !!(regnum & MII_ADDR_C45); > > ret = pm_runtime_get_sync(dev); > - if (ret < 0) > + if (ret < 0) { > + pm_runtime_put_noidle(dev); > return ret; > - else > + } else { > ret = 0; > + } > > if (is_c45) { > frame_start = FEC_MMFR_ST_C45; > @@ -2276,8 +2280,10 @@ static void fec_enet_get_regs(struct net_device > *ndev, > int ret; > > ret = pm_runtime_get_sync(dev); > - if (ret < 0) > + if (ret < 0) { > + pm_runtime_put_noidle(dev); > return; > + } > > regs->version = fec_enet_register_version; > > @@ -2977,8 +2983,10 @@ fec_enet_open(struct net_device *ndev) > bool reset_again; > > ret = pm_runtime_get_sync(&fep->pdev->dev); > - if (ret < 0) > + if (ret < 0) { > + pm_runtime_put_noidle(&fep->pdev->dev); > return ret; > + } > > pinctrl_pm_select_default_state(&fep->pdev->dev); > ret = fec_enet_clk_enable(ndev, true); @@ -3771,8 +3779,10 @@ > fec_drv_remove(struct platform_device *pdev) > int ret; > > ret = pm_runtime_get_sync(&pdev->dev); > - if (ret < 0) > + if (ret < 0) { > + pm_runtime_put_noidle(&pdev->dev); > return ret; > + } > > cancel_work_sync(&fep->tx_timeout_work); > fec_ptp_stop(pdev); > -- > 2.25.4
> From: Zhang Qilong <zhangqilong3@huawei.com> Sent: Sunday, November 8, > 2020 5:53 PM > > pm_runtime_get_sync() will increment pm usage at first and it will > > resume the device later. If runtime of the device has error or device > > is in inaccessible state(or other error state), resume operation will > > fail. If we do not call put operation to decrease the reference, it will result in > reference count leak. > > Moreover, this device cannot enter the idle state and always stay busy > > or other non-idle state later. So we fixed it through adding > pm_runtime_put_noidle. > > > > Fixes: 8fff755e9f8d0 ("net: fec: Ensure clocks are enabled while using > > mdio bus") > > Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com> > > From early discussion for the topic, Wolfram Sang wonder if such de-reference > can be better handled by pm runtime core code. > I have read the discussion just now, They didn't give a definite result. I agreed with introducing a new or help function to replace the pm_runtime_get_sync gradually. How do you think so ? Regards, Zhang > https://lkml.org/lkml/2020/6/14/76 > > Regards, > Andy > > --- > > drivers/net/ethernet/freescale/fec_main.c | 22 ++++++++++++++++------ > > 1 file changed, 16 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/net/ethernet/freescale/fec_main.c > > b/drivers/net/ethernet/freescale/fec_main.c > > index d7919555250d..6c02f885c67e 100644 > > --- a/drivers/net/ethernet/freescale/fec_main.c > > +++ b/drivers/net/ethernet/freescale/fec_main.c > > @@ -1809,8 +1809,10 @@ static int fec_enet_mdio_read(struct mii_bus > > *bus, int mii_id, int regnum) > > bool is_c45 = !!(regnum & MII_ADDR_C45); > > > > ret = pm_runtime_get_sync(dev); > > - if (ret < 0) > > + if (ret < 0) { > > + pm_runtime_put_noidle(dev); > > return ret; > > + } > > > > if (is_c45) { > > frame_start = FEC_MMFR_ST_C45; > > @@ -1868,10 +1870,12 @@ static int fec_enet_mdio_write(struct mii_bus > > *bus, int mii_id, int regnum, > > bool is_c45 = !!(regnum & MII_ADDR_C45); > > > > ret = pm_runtime_get_sync(dev); > > - if (ret < 0) > > + if (ret < 0) { > > + pm_runtime_put_noidle(dev); > > return ret; > > - else > > + } else { > > ret = 0; > > + } > > > > if (is_c45) { > > frame_start = FEC_MMFR_ST_C45; > > @@ -2276,8 +2280,10 @@ static void fec_enet_get_regs(struct net_device > > *ndev, > > int ret; > > > > ret = pm_runtime_get_sync(dev); > > - if (ret < 0) > > + if (ret < 0) { > > + pm_runtime_put_noidle(dev); > > return; > > + } > > > > regs->version = fec_enet_register_version; > > > > @@ -2977,8 +2983,10 @@ fec_enet_open(struct net_device *ndev) > > bool reset_again; > > > > ret = pm_runtime_get_sync(&fep->pdev->dev); > > - if (ret < 0) > > + if (ret < 0) { > > + pm_runtime_put_noidle(&fep->pdev->dev); > > return ret; > > + } > > > > pinctrl_pm_select_default_state(&fep->pdev->dev); > > ret = fec_enet_clk_enable(ndev, true); @@ -3771,8 +3779,10 @@ > > fec_drv_remove(struct platform_device *pdev) > > int ret; > > > > ret = pm_runtime_get_sync(&pdev->dev); > > - if (ret < 0) > > + if (ret < 0) { > > + pm_runtime_put_noidle(&pdev->dev); > > return ret; > > + } > > > > cancel_work_sync(&fep->tx_timeout_work); > > fec_ptp_stop(pdev); > > -- > > 2.25.4
From: zhangqilong <zhangqilong3@huawei.com>ent: Monday, November 9, 2020 10:52 AM > > From: Zhang Qilong <zhangqilong3@huawei.com> Sent: Sunday, November 8, > > 2020 5:53 PM > > > pm_runtime_get_sync() will increment pm usage at first and it will > > > resume the device later. If runtime of the device has error or > > > device is in inaccessible state(or other error state), resume > > > operation will fail. If we do not call put operation to decrease the > > > reference, it will result in > > reference count leak. > > > Moreover, this device cannot enter the idle state and always stay > > > busy or other non-idle state later. So we fixed it through adding > > pm_runtime_put_noidle. > > > > > > Fixes: 8fff755e9f8d0 ("net: fec: Ensure clocks are enabled while > > > using mdio bus") > > > Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com> > > > > From early discussion for the topic, Wolfram Sang wonder if such > > de-reference can be better handled by pm runtime core code. > > > I have read the discussion just now, They didn't give a definite result. I agreed > with introducing a new or help function to replace the pm_runtime_get_sync > gradually. How do you think so ? > > Regards, > Zhang I also think so to avoid so much duplication code in function driver. > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml > > .org%2Flkml%2F2020%2F6%2F14%2F76&data=04%7C01%7Cfugang.dua > n%40nxp. > > > com%7Ceaef5020d1c143dfda5208d8845a6391%7C686ea1d3bc2b4c6fa92cd99 > c5c301 > > > 635%7C0%7C0%7C637404871050135426%7CUnknown%7CTWFpbGZsb3d8eyJ > WIjoiMC4wL > > > jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&s > data > > =GLi34HRLzzTnTjT54dpNXkMdvuPuyVhcSuAGqc1rdvw%3D&reserved=0 > > > > Regards, > > Andy > > > --- > > > drivers/net/ethernet/freescale/fec_main.c | 22 > > > ++++++++++++++++------ > > > 1 file changed, 16 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/freescale/fec_main.c > > > b/drivers/net/ethernet/freescale/fec_main.c > > > index d7919555250d..6c02f885c67e 100644 > > > --- a/drivers/net/ethernet/freescale/fec_main.c > > > +++ b/drivers/net/ethernet/freescale/fec_main.c > > > @@ -1809,8 +1809,10 @@ static int fec_enet_mdio_read(struct mii_bus > > > *bus, int mii_id, int regnum) > > > bool is_c45 = !!(regnum & MII_ADDR_C45); > > > > > > ret = pm_runtime_get_sync(dev); > > > - if (ret < 0) > > > + if (ret < 0) { > > > + pm_runtime_put_noidle(dev); > > > return ret; > > > + } > > > > > > if (is_c45) { > > > frame_start = FEC_MMFR_ST_C45; > > > @@ -1868,10 +1870,12 @@ static int fec_enet_mdio_write(struct > > > mii_bus *bus, int mii_id, int regnum, > > > bool is_c45 = !!(regnum & MII_ADDR_C45); > > > > > > ret = pm_runtime_get_sync(dev); > > > - if (ret < 0) > > > + if (ret < 0) { > > > + pm_runtime_put_noidle(dev); > > > return ret; > > > - else > > > + } else { > > > ret = 0; > > > + } > > > > > > if (is_c45) { > > > frame_start = FEC_MMFR_ST_C45; > > > @@ -2276,8 +2280,10 @@ static void fec_enet_get_regs(struct > > > net_device *ndev, > > > int ret; > > > > > > ret = pm_runtime_get_sync(dev); > > > - if (ret < 0) > > > + if (ret < 0) { > > > + pm_runtime_put_noidle(dev); > > > return; > > > + } > > > > > > regs->version = fec_enet_register_version; > > > > > > @@ -2977,8 +2983,10 @@ fec_enet_open(struct net_device *ndev) > > > bool reset_again; > > > > > > ret = pm_runtime_get_sync(&fep->pdev->dev); > > > - if (ret < 0) > > > + if (ret < 0) { > > > + pm_runtime_put_noidle(&fep->pdev->dev); > > > return ret; > > > + } > > > > > > pinctrl_pm_select_default_state(&fep->pdev->dev); > > > ret = fec_enet_clk_enable(ndev, true); @@ -3771,8 +3779,10 @@ > > > fec_drv_remove(struct platform_device *pdev) > > > int ret; > > > > > > ret = pm_runtime_get_sync(&pdev->dev); > > > - if (ret < 0) > > > + if (ret < 0) { > > > + pm_runtime_put_noidle(&pdev->dev); > > > return ret; > > > + } > > > > > > cancel_work_sync(&fep->tx_timeout_work); > > > fec_ptp_stop(pdev); > > > -- > > > 2.25.4
> From: zhangqilong <zhangqilong3@huawei.com>ent: Monday, November 9, > 2020 10:52 AM > > > From: Zhang Qilong <zhangqilong3@huawei.com> Sent: Sunday, November > > > 8, > > > 2020 5:53 PM > > > > pm_runtime_get_sync() will increment pm usage at first and it will > > > > resume the device later. If runtime of the device has error or > > > > device is in inaccessible state(or other error state), resume > > > > operation will fail. If we do not call put operation to decrease > > > > the reference, it will result in > > > reference count leak. > > > > Moreover, this device cannot enter the idle state and always stay > > > > busy or other non-idle state later. So we fixed it through adding > > > pm_runtime_put_noidle. > > > > > > > > Fixes: 8fff755e9f8d0 ("net: fec: Ensure clocks are enabled while > > > > using mdio bus") > > > > Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com> > > > > > > From early discussion for the topic, Wolfram Sang wonder if such > > > de-reference can be better handled by pm runtime core code. > > > > > I have read the discussion just now, They didn't give a definite > > result. I agreed with introducing a new or help function to replace > > the pm_runtime_get_sync gradually. How do you think so ? > > > > Regards, > > Zhang > > I also think so to avoid so much duplication code in function driver. Hi, I have added a more general operation in flowing patch https://lore.kernel.org/netdev/20201109080938.4174745-1-zhangqilong3@huawei.com/T/#t to solve this problem. Comments are welcome if you are free. Regards, Zhang > > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flk > > > > ml .org%2Flkml%2F2020%2F6%2F14%2F76&data=04%7C01%7Cfugang.du > a > > n%40nxp. > > > > > > com%7Ceaef5020d1c143dfda5208d8845a6391%7C686ea1d3bc2b4c6fa92cd99 > > c5c301 > > > > > > 635%7C0%7C0%7C637404871050135426%7CUnknown%7CTWFpbGZsb3d8eyJ > > WIjoiMC4wL > > > > > > jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000& > s > > data > > > > =GLi34HRLzzTnTjT54dpNXkMdvuPuyVhcSuAGqc1rdvw%3D&reserved=0 > > > > > > Regards, > > > Andy > > > > --- > > > > drivers/net/ethernet/freescale/fec_main.c | 22 > > > > ++++++++++++++++------ > > > > 1 file changed, 16 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/net/ethernet/freescale/fec_main.c > > > > b/drivers/net/ethernet/freescale/fec_main.c > > > > index d7919555250d..6c02f885c67e 100644 > > > > --- a/drivers/net/ethernet/freescale/fec_main.c > > > > +++ b/drivers/net/ethernet/freescale/fec_main.c > > > > @@ -1809,8 +1809,10 @@ static int fec_enet_mdio_read(struct > > > > mii_bus *bus, int mii_id, int regnum) > > > > bool is_c45 = !!(regnum & MII_ADDR_C45); > > > > > > > > ret = pm_runtime_get_sync(dev); > > > > - if (ret < 0) > > > > + if (ret < 0) { > > > > + pm_runtime_put_noidle(dev); > > > > return ret; > > > > + } > > > > > > > > if (is_c45) { > > > > frame_start = FEC_MMFR_ST_C45; > > > > @@ -1868,10 +1870,12 @@ static int fec_enet_mdio_write(struct > > > > mii_bus *bus, int mii_id, int regnum, > > > > bool is_c45 = !!(regnum & MII_ADDR_C45); > > > > > > > > ret = pm_runtime_get_sync(dev); > > > > - if (ret < 0) > > > > + if (ret < 0) { > > > > + pm_runtime_put_noidle(dev); > > > > return ret; > > > > - else > > > > + } else { > > > > ret = 0; > > > > + } > > > > > > > > if (is_c45) { > > > > frame_start = FEC_MMFR_ST_C45; > > > > @@ -2276,8 +2280,10 @@ static void fec_enet_get_regs(struct > > > > net_device *ndev, > > > > int ret; > > > > > > > > ret = pm_runtime_get_sync(dev); > > > > - if (ret < 0) > > > > + if (ret < 0) { > > > > + pm_runtime_put_noidle(dev); > > > > return; > > > > + } > > > > > > > > regs->version = fec_enet_register_version; > > > > > > > > @@ -2977,8 +2983,10 @@ fec_enet_open(struct net_device *ndev) > > > > bool reset_again; > > > > > > > > ret = pm_runtime_get_sync(&fep->pdev->dev); > > > > - if (ret < 0) > > > > + if (ret < 0) { > > > > + pm_runtime_put_noidle(&fep->pdev->dev); > > > > return ret; > > > > + } > > > > > > > > pinctrl_pm_select_default_state(&fep->pdev->dev); > > > > ret = fec_enet_clk_enable(ndev, true); @@ -3771,8 +3779,10 @@ > > > > fec_drv_remove(struct platform_device *pdev) > > > > int ret; > > > > > > > > ret = pm_runtime_get_sync(&pdev->dev); > > > > - if (ret < 0) > > > > + if (ret < 0) { > > > > + pm_runtime_put_noidle(&pdev->dev); > > > > return ret; > > > > + } > > > > > > > > cancel_work_sync(&fep->tx_timeout_work); > > > > fec_ptp_stop(pdev); > > > > -- > > > > 2.25.4
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index d7919555250d..6c02f885c67e 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -1809,8 +1809,10 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum) bool is_c45 = !!(regnum & MII_ADDR_C45); ret = pm_runtime_get_sync(dev); - if (ret < 0) + if (ret < 0) { + pm_runtime_put_noidle(dev); return ret; + } if (is_c45) { frame_start = FEC_MMFR_ST_C45; @@ -1868,10 +1870,12 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum, bool is_c45 = !!(regnum & MII_ADDR_C45); ret = pm_runtime_get_sync(dev); - if (ret < 0) + if (ret < 0) { + pm_runtime_put_noidle(dev); return ret; - else + } else { ret = 0; + } if (is_c45) { frame_start = FEC_MMFR_ST_C45; @@ -2276,8 +2280,10 @@ static void fec_enet_get_regs(struct net_device *ndev, int ret; ret = pm_runtime_get_sync(dev); - if (ret < 0) + if (ret < 0) { + pm_runtime_put_noidle(dev); return; + } regs->version = fec_enet_register_version; @@ -2977,8 +2983,10 @@ fec_enet_open(struct net_device *ndev) bool reset_again; ret = pm_runtime_get_sync(&fep->pdev->dev); - if (ret < 0) + if (ret < 0) { + pm_runtime_put_noidle(&fep->pdev->dev); return ret; + } pinctrl_pm_select_default_state(&fep->pdev->dev); ret = fec_enet_clk_enable(ndev, true); @@ -3771,8 +3779,10 @@ fec_drv_remove(struct platform_device *pdev) int ret; ret = pm_runtime_get_sync(&pdev->dev); - if (ret < 0) + if (ret < 0) { + pm_runtime_put_noidle(&pdev->dev); return ret; + } cancel_work_sync(&fep->tx_timeout_work); fec_ptp_stop(pdev);
pm_runtime_get_sync() will increment pm usage at first and it will resume the device later. If runtime of the device has error or device is in inaccessible state(or other error state), resume operation will fail. If we do not call put operation to decrease the reference, it will result in reference count leak. Moreover, this device cannot enter the idle state and always stay busy or other non-idle state later. So we fixed it through adding pm_runtime_put_noidle. Fixes: 8fff755e9f8d0 ("net: fec: Ensure clocks are enabled while using mdio bus") Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com> --- drivers/net/ethernet/freescale/fec_main.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)