* [PATCH v2 0/2] Fix usage counter leak by adding a general sync ops @ 2020-11-09 15:04 Zhang Qilong 2020-11-09 15:04 ` [PATCH v2 1/2] PM: runtime: Add a general runtime get sync operation to deal with usage counter Zhang Qilong 2020-11-09 15:04 ` [PATCH v2 2/2] net: fec: Fix reference count leak in fec series ops Zhang Qilong 0 siblings, 2 replies; 11+ messages in thread From: Zhang Qilong @ 2020-11-09 15:04 UTC (permalink / raw) To: rjw, fugang.duan, davem, kuba; +Cc: linux-pm, netdev In many case, we need to check return value of pm_runtime_get_sync, but it brings a trouble to the usage counter processing. Many callers forget to decrease the usage counter when it failed. It has been discussed a lot[0][1]. So we add a function to deal with the usage counter for better coding and view. Then, we replace pm_runtime_get_sync with it in fec_main.c Zhang Qilong (2): PM: runtime: Add a general runtime get sync operation to deal with usage counter net: fec: Fix reference count leak in fec series ops drivers/net/ethernet/freescale/fec_main.c | 12 ++++----- include/linux/pm_runtime.h | 30 +++++++++++++++++++++++ 2 files changed, 35 insertions(+), 7 deletions(-) -- 2.25.4 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/2] PM: runtime: Add a general runtime get sync operation to deal with usage counter 2020-11-09 15:04 [PATCH v2 0/2] Fix usage counter leak by adding a general sync ops Zhang Qilong @ 2020-11-09 15:04 ` Zhang Qilong 2020-11-09 15:20 ` Rafael J. Wysocki 2020-11-09 15:04 ` [PATCH v2 2/2] net: fec: Fix reference count leak in fec series ops Zhang Qilong 1 sibling, 1 reply; 11+ messages in thread From: Zhang Qilong @ 2020-11-09 15:04 UTC (permalink / raw) To: rjw, fugang.duan, davem, kuba; +Cc: linux-pm, netdev In many case, we need to check return value of pm_runtime_get_sync, but it brings a trouble to the usage counter processing. Many callers forget to decrease the usage counter when it failed. It has been discussed a lot[0][1]. So we add a function to deal with the usage counter for better coding. [0]https://lkml.org/lkml/2020/6/14/88 [1]https://patchwork.ozlabs.org/project/linux-tegra/patch/20200520095148.10995-1-dinghao.liu@zju.edu.cn/ Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com> --- include/linux/pm_runtime.h | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h index 4b708f4e8eed..6549ce764400 100644 --- a/include/linux/pm_runtime.h +++ b/include/linux/pm_runtime.h @@ -386,6 +386,36 @@ static inline int pm_runtime_get_sync(struct device *dev) return __pm_runtime_resume(dev, RPM_GET_PUT); } +/** + * pm_runtime_general_get - Bump up usage counter of a device and resume it. + * @dev: Target device. + * + * Increase runtime PM usage counter of @dev first, and carry out runtime-resume + * of it synchronously. If __pm_runtime_resume return negative value(device is in + * error state), we to need decrease the usage counter before it return. If + * __pm_runtime_resume return positive value, it means the runtime of device has + * already been in active state, and we let the new wrapper return zero instead. + * + * The possible return values of this function is zero or negative value. + * zero: + * - it means resume succeeed or runtime of device has already been active, the + * runtime PM usage counter of @dev remains incremented. + * negative: + * - it means failure and the runtime PM usage counter of @dev has been balanced. + */ +static inline int pm_runtime_general_get(struct device *dev) +{ + int ret = 0; + + ret = __pm_runtime_resume(dev, RPM_GET_PUT); + if (ret < 0) { + pm_runtime_put_noidle(dev); + return ret; + } + + return 0; +} + /** * pm_runtime_put - Drop device usage counter and queue up "idle check" if 0. * @dev: Target device. -- 2.25.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] PM: runtime: Add a general runtime get sync operation to deal with usage counter 2020-11-09 15:04 ` [PATCH v2 1/2] PM: runtime: Add a general runtime get sync operation to deal with usage counter Zhang Qilong @ 2020-11-09 15:20 ` Rafael J. Wysocki 2020-11-09 15:49 ` Ulf Hansson 2020-11-09 15:50 ` 答复: " zhangqilong 0 siblings, 2 replies; 11+ messages in thread From: Rafael J. Wysocki @ 2020-11-09 15:20 UTC (permalink / raw) To: Zhang Qilong Cc: Rafael J. Wysocki, fugang.duan, David Miller, Jakub Kicinski, Linux PM, netdev On Mon, Nov 9, 2020 at 4:00 PM Zhang Qilong <zhangqilong3@huawei.com> wrote: > > In many case, we need to check return value of pm_runtime_get_sync, but > it brings a trouble to the usage counter processing. Many callers forget > to decrease the usage counter when it failed. It has been discussed a > lot[0][1]. So we add a function to deal with the usage counter for better > coding. > > [0]https://lkml.org/lkml/2020/6/14/88 > [1]https://patchwork.ozlabs.org/project/linux-tegra/patch/20200520095148.10995-1-dinghao.liu@zju.edu.cn/ > Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com> > --- > include/linux/pm_runtime.h | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h > index 4b708f4e8eed..6549ce764400 100644 > --- a/include/linux/pm_runtime.h > +++ b/include/linux/pm_runtime.h > @@ -386,6 +386,36 @@ static inline int pm_runtime_get_sync(struct device *dev) > return __pm_runtime_resume(dev, RPM_GET_PUT); > } > > +/** > + * pm_runtime_general_get - Bump up usage counter of a device and resume it. > + * @dev: Target device. > + * > + * Increase runtime PM usage counter of @dev first, and carry out runtime-resume > + * of it synchronously. If __pm_runtime_resume return negative value(device is in > + * error state), we to need decrease the usage counter before it return. If > + * __pm_runtime_resume return positive value, it means the runtime of device has > + * already been in active state, and we let the new wrapper return zero instead. > + * > + * The possible return values of this function is zero or negative value. > + * zero: > + * - it means resume succeeed or runtime of device has already been active, the > + * runtime PM usage counter of @dev remains incremented. > + * negative: > + * - it means failure and the runtime PM usage counter of @dev has been balanced. The kerneldoc above is kind of noisy and it is hard to figure out what the helper really does from it. You could basically say something like "Resume @dev synchronously and if that is successful, increment its runtime PM usage counter. Return 0 if the runtime PM usage counter of @dev has been incremented or a negative error code otherwise." > + */ > +static inline int pm_runtime_general_get(struct device *dev) What about pm_runtime_resume_and_get()? > +{ > + int ret = 0; This extra initialization is not necessary. You can initialize ret to the __pm_runtime_resume() return value right away. > + > + ret = __pm_runtime_resume(dev, RPM_GET_PUT); > + if (ret < 0) { > + pm_runtime_put_noidle(dev); > + return ret; > + } > + > + return 0; > +} > + > /** > * pm_runtime_put - Drop device usage counter and queue up "idle check" if 0. > * @dev: Target device. > -- ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] PM: runtime: Add a general runtime get sync operation to deal with usage counter 2020-11-09 15:20 ` Rafael J. Wysocki @ 2020-11-09 15:49 ` Ulf Hansson 2020-11-09 15:53 ` Rafael J. Wysocki 2020-11-09 15:50 ` 答复: " zhangqilong 1 sibling, 1 reply; 11+ messages in thread From: Ulf Hansson @ 2020-11-09 15:49 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Zhang Qilong, Rafael J. Wysocki, Fugang Duan, David Miller, Jakub Kicinski, Linux PM, netdev On Mon, 9 Nov 2020 at 16:20, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Mon, Nov 9, 2020 at 4:00 PM Zhang Qilong <zhangqilong3@huawei.com> wrote: > > > > In many case, we need to check return value of pm_runtime_get_sync, but > > it brings a trouble to the usage counter processing. Many callers forget > > to decrease the usage counter when it failed. It has been discussed a > > lot[0][1]. So we add a function to deal with the usage counter for better > > coding. > > > > [0]https://lkml.org/lkml/2020/6/14/88 > > [1]https://patchwork.ozlabs.org/project/linux-tegra/patch/20200520095148.10995-1-dinghao.liu@zju.edu.cn/ > > Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com> > > --- > > include/linux/pm_runtime.h | 30 ++++++++++++++++++++++++++++++ > > 1 file changed, 30 insertions(+) > > > > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h > > index 4b708f4e8eed..6549ce764400 100644 > > --- a/include/linux/pm_runtime.h > > +++ b/include/linux/pm_runtime.h > > @@ -386,6 +386,36 @@ static inline int pm_runtime_get_sync(struct device *dev) > > return __pm_runtime_resume(dev, RPM_GET_PUT); > > } > > > > +/** > > + * pm_runtime_general_get - Bump up usage counter of a device and resume it. > > + * @dev: Target device. > > + * > > + * Increase runtime PM usage counter of @dev first, and carry out runtime-resume > > + * of it synchronously. If __pm_runtime_resume return negative value(device is in > > + * error state), we to need decrease the usage counter before it return. If > > + * __pm_runtime_resume return positive value, it means the runtime of device has > > + * already been in active state, and we let the new wrapper return zero instead. > > + * > > + * The possible return values of this function is zero or negative value. > > + * zero: > > + * - it means resume succeeed or runtime of device has already been active, the > > + * runtime PM usage counter of @dev remains incremented. > > + * negative: > > + * - it means failure and the runtime PM usage counter of @dev has been balanced. > > The kerneldoc above is kind of noisy and it is hard to figure out what > the helper really does from it. > > You could basically say something like "Resume @dev synchronously and > if that is successful, increment its runtime PM usage counter. Return > 0 if the runtime PM usage counter of @dev has been incremented or a > negative error code otherwise." > > > + */ > > +static inline int pm_runtime_general_get(struct device *dev) > > What about pm_runtime_resume_and_get()? We already have pm_runtime_get_if_active() - so perhaps pm_runtime_get_if_suspended() could be an option as well? > > > +{ > > + int ret = 0; > > This extra initialization is not necessary. > > You can initialize ret to the __pm_runtime_resume() return value right away. > > > + > > + ret = __pm_runtime_resume(dev, RPM_GET_PUT); > > + if (ret < 0) { > > + pm_runtime_put_noidle(dev); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > /** > > * pm_runtime_put - Drop device usage counter and queue up "idle check" if 0. > > * @dev: Target device. > > -- Kind regards Uffe ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] PM: runtime: Add a general runtime get sync operation to deal with usage counter 2020-11-09 15:49 ` Ulf Hansson @ 2020-11-09 15:53 ` Rafael J. Wysocki 2020-11-09 16:05 ` Ulf Hansson 0 siblings, 1 reply; 11+ messages in thread From: Rafael J. Wysocki @ 2020-11-09 15:53 UTC (permalink / raw) To: Ulf Hansson Cc: Rafael J. Wysocki, Zhang Qilong, Rafael J. Wysocki, Fugang Duan, David Miller, Jakub Kicinski, Linux PM, netdev On Mon, Nov 9, 2020 at 4:50 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Mon, 9 Nov 2020 at 16:20, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Mon, Nov 9, 2020 at 4:00 PM Zhang Qilong <zhangqilong3@huawei.com> wrote: > > > > > > In many case, we need to check return value of pm_runtime_get_sync, but > > > it brings a trouble to the usage counter processing. Many callers forget > > > to decrease the usage counter when it failed. It has been discussed a > > > lot[0][1]. So we add a function to deal with the usage counter for better > > > coding. > > > > > > [0]https://lkml.org/lkml/2020/6/14/88 > > > [1]https://patchwork.ozlabs.org/project/linux-tegra/patch/20200520095148.10995-1-dinghao.liu@zju.edu.cn/ > > > Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com> > > > --- > > > include/linux/pm_runtime.h | 30 ++++++++++++++++++++++++++++++ > > > 1 file changed, 30 insertions(+) > > > > > > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h > > > index 4b708f4e8eed..6549ce764400 100644 > > > --- a/include/linux/pm_runtime.h > > > +++ b/include/linux/pm_runtime.h > > > @@ -386,6 +386,36 @@ static inline int pm_runtime_get_sync(struct device *dev) > > > return __pm_runtime_resume(dev, RPM_GET_PUT); > > > } > > > > > > +/** > > > + * pm_runtime_general_get - Bump up usage counter of a device and resume it. > > > + * @dev: Target device. > > > + * > > > + * Increase runtime PM usage counter of @dev first, and carry out runtime-resume > > > + * of it synchronously. If __pm_runtime_resume return negative value(device is in > > > + * error state), we to need decrease the usage counter before it return. If > > > + * __pm_runtime_resume return positive value, it means the runtime of device has > > > + * already been in active state, and we let the new wrapper return zero instead. > > > + * > > > + * The possible return values of this function is zero or negative value. > > > + * zero: > > > + * - it means resume succeeed or runtime of device has already been active, the > > > + * runtime PM usage counter of @dev remains incremented. > > > + * negative: > > > + * - it means failure and the runtime PM usage counter of @dev has been balanced. > > > > The kerneldoc above is kind of noisy and it is hard to figure out what > > the helper really does from it. > > > > You could basically say something like "Resume @dev synchronously and > > if that is successful, increment its runtime PM usage counter. Return > > 0 if the runtime PM usage counter of @dev has been incremented or a > > negative error code otherwise." > > > > > + */ > > > +static inline int pm_runtime_general_get(struct device *dev) > > > > What about pm_runtime_resume_and_get()? > > We already have pm_runtime_get_if_active() - so perhaps > pm_runtime_get_if_suspended() could be an option as well? It doesn't work this way, though. The "get" happens even if the device has not been suspended. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] PM: runtime: Add a general runtime get sync operation to deal with usage counter 2020-11-09 15:53 ` Rafael J. Wysocki @ 2020-11-09 16:05 ` Ulf Hansson 0 siblings, 0 replies; 11+ messages in thread From: Ulf Hansson @ 2020-11-09 16:05 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Zhang Qilong, Rafael J. Wysocki, Fugang Duan, David Miller, Jakub Kicinski, Linux PM, netdev On Mon, 9 Nov 2020 at 16:54, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Mon, Nov 9, 2020 at 4:50 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Mon, 9 Nov 2020 at 16:20, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > On Mon, Nov 9, 2020 at 4:00 PM Zhang Qilong <zhangqilong3@huawei.com> wrote: > > > > > > > > In many case, we need to check return value of pm_runtime_get_sync, but > > > > it brings a trouble to the usage counter processing. Many callers forget > > > > to decrease the usage counter when it failed. It has been discussed a > > > > lot[0][1]. So we add a function to deal with the usage counter for better > > > > coding. > > > > > > > > [0]https://lkml.org/lkml/2020/6/14/88 > > > > [1]https://patchwork.ozlabs.org/project/linux-tegra/patch/20200520095148.10995-1-dinghao.liu@zju.edu.cn/ > > > > Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com> > > > > --- > > > > include/linux/pm_runtime.h | 30 ++++++++++++++++++++++++++++++ > > > > 1 file changed, 30 insertions(+) > > > > > > > > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h > > > > index 4b708f4e8eed..6549ce764400 100644 > > > > --- a/include/linux/pm_runtime.h > > > > +++ b/include/linux/pm_runtime.h > > > > @@ -386,6 +386,36 @@ static inline int pm_runtime_get_sync(struct device *dev) > > > > return __pm_runtime_resume(dev, RPM_GET_PUT); > > > > } > > > > > > > > +/** > > > > + * pm_runtime_general_get - Bump up usage counter of a device and resume it. > > > > + * @dev: Target device. > > > > + * > > > > + * Increase runtime PM usage counter of @dev first, and carry out runtime-resume > > > > + * of it synchronously. If __pm_runtime_resume return negative value(device is in > > > > + * error state), we to need decrease the usage counter before it return. If > > > > + * __pm_runtime_resume return positive value, it means the runtime of device has > > > > + * already been in active state, and we let the new wrapper return zero instead. > > > > + * > > > > + * The possible return values of this function is zero or negative value. > > > > + * zero: > > > > + * - it means resume succeeed or runtime of device has already been active, the > > > > + * runtime PM usage counter of @dev remains incremented. > > > > + * negative: > > > > + * - it means failure and the runtime PM usage counter of @dev has been balanced. > > > > > > The kerneldoc above is kind of noisy and it is hard to figure out what > > > the helper really does from it. > > > > > > You could basically say something like "Resume @dev synchronously and > > > if that is successful, increment its runtime PM usage counter. Return > > > 0 if the runtime PM usage counter of @dev has been incremented or a > > > negative error code otherwise." > > > > > > > + */ > > > > +static inline int pm_runtime_general_get(struct device *dev) > > > > > > What about pm_runtime_resume_and_get()? > > > > We already have pm_runtime_get_if_active() - so perhaps > > pm_runtime_get_if_suspended() could be an option as well? > > It doesn't work this way, though. > > The "get" happens even if the device has not been suspended. Yes, that's right - so pm_runtime_resume_and_get() is probably the best we can pick then. Kind regards Uffe ^ permalink raw reply [flat|nested] 11+ messages in thread
* 答复: [PATCH v2 1/2] PM: runtime: Add a general runtime get sync operation to deal with usage counter 2020-11-09 15:20 ` Rafael J. Wysocki 2020-11-09 15:49 ` Ulf Hansson @ 2020-11-09 15:50 ` zhangqilong 2020-11-09 16:00 ` Rafael J. Wysocki 1 sibling, 1 reply; 11+ messages in thread From: zhangqilong @ 2020-11-09 15:50 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Rafael J. Wysocki, fugang.duan, David Miller, Jakub Kicinski, Linux PM, netdev > operation to deal with usage counter > > On Mon, Nov 9, 2020 at 4:00 PM Zhang Qilong <zhangqilong3@huawei.com> > wrote: > > > > In many case, we need to check return value of pm_runtime_get_sync, > > but it brings a trouble to the usage counter processing. Many callers > > forget to decrease the usage counter when it failed. It has been > > discussed a lot[0][1]. So we add a function to deal with the usage > > counter for better coding. > > > > [0]https://lkml.org/lkml/2020/6/14/88 > > [1]https://patchwork.ozlabs.org/project/linux-tegra/patch/202005200951 > > 48.10995-1-dinghao.liu@zju.edu.cn/ > > Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com> > > --- > > include/linux/pm_runtime.h | 30 ++++++++++++++++++++++++++++++ > > 1 file changed, 30 insertions(+) > > > > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h > > index 4b708f4e8eed..6549ce764400 100644 > > --- a/include/linux/pm_runtime.h > > +++ b/include/linux/pm_runtime.h > > @@ -386,6 +386,36 @@ static inline int pm_runtime_get_sync(struct device > *dev) > > return __pm_runtime_resume(dev, RPM_GET_PUT); } > > > > +/** > > + * pm_runtime_general_get - Bump up usage counter of a device and > resume it. > > + * @dev: Target device. > > + * > > + * Increase runtime PM usage counter of @dev first, and carry out > > +runtime-resume > > + * of it synchronously. If __pm_runtime_resume return negative > > +value(device is in > > + * error state), we to need decrease the usage counter before it > > +return. If > > + * __pm_runtime_resume return positive value, it means the runtime of > > +device has > > + * already been in active state, and we let the new wrapper return zero > instead. > > + * > > + * The possible return values of this function is zero or negative value. > > + * zero: > > + * - it means resume succeeed or runtime of device has already been > active, the > > + * runtime PM usage counter of @dev remains incremented. > > + * negative: > > + * - it means failure and the runtime PM usage counter of @dev has > been balanced. > > The kerneldoc above is kind of noisy and it is hard to figure out what the helper > really does from it. > > You could basically say something like "Resume @dev synchronously and if that > is successful, increment its runtime PM usage counter. Return > 0 if the runtime PM usage counter of @dev has been incremented or a negative > error code otherwise." > How about the following description. /** 390 * pm_runtime_general_get - Bump up usage counter of a device and resume it. 391 * @dev: Target device. 392 * 393 * Increase runtime PM usage counter of @dev first, and carry out runtime-resume 394 * of it synchronously. If __pm_runtime_resume return negative value(device is in 395 * error state), we to need decrease the usage counter before it return. If 396 * __pm_runtime_resume return positive value, it means the runtime of device has 397 * already been in active state, and we let the new wrapper return zero instead. 398 * 399 * Resume @dev synchronously and if that is successful, and increment its runtime 400 * PM usage counter if it turn out to equal to 0. The runtime PM usage counter of 401 * @dev has been incremented or a negative error code otherwise. 402 */ Thanks, Zhang > > + */ > > +static inline int pm_runtime_general_get(struct device *dev) > > What about pm_runtime_resume_and_get()? > I think it's OK. > > +{ > > + int ret = 0; > > This extra initialization is not necessary. > > You can initialize ret to the __pm_runtime_resume() return value right away. > OK, good idea. > > + > > + ret = __pm_runtime_resume(dev, RPM_GET_PUT); > > + if (ret < 0) { > > + pm_runtime_put_noidle(dev); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > /** > > * pm_runtime_put - Drop device usage counter and queue up "idle check" > if 0. > > * @dev: Target device. > > -- ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] PM: runtime: Add a general runtime get sync operation to deal with usage counter 2020-11-09 15:50 ` 答复: " zhangqilong @ 2020-11-09 16:00 ` Rafael J. Wysocki 2020-11-09 16:15 ` 答复: " zhangqilong 0 siblings, 1 reply; 11+ messages in thread From: Rafael J. Wysocki @ 2020-11-09 16:00 UTC (permalink / raw) To: zhangqilong Cc: Rafael J. Wysocki, Rafael J. Wysocki, fugang.duan, David Miller, Jakub Kicinski, Linux PM, netdev On Mon, Nov 9, 2020 at 4:50 PM zhangqilong <zhangqilong3@huawei.com> wrote: > > > operation to deal with usage counter > > > > On Mon, Nov 9, 2020 at 4:00 PM Zhang Qilong <zhangqilong3@huawei.com> > > wrote: > > > > > > In many case, we need to check return value of pm_runtime_get_sync, > > > but it brings a trouble to the usage counter processing. Many callers > > > forget to decrease the usage counter when it failed. It has been > > > discussed a lot[0][1]. So we add a function to deal with the usage > > > counter for better coding. > > > > > > [0]https://lkml.org/lkml/2020/6/14/88 > > > [1]https://patchwork.ozlabs.org/project/linux-tegra/patch/202005200951 > > > 48.10995-1-dinghao.liu@zju.edu.cn/ > > > Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com> > > > --- > > > include/linux/pm_runtime.h | 30 ++++++++++++++++++++++++++++++ > > > 1 file changed, 30 insertions(+) > > > > > > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h > > > index 4b708f4e8eed..6549ce764400 100644 > > > --- a/include/linux/pm_runtime.h > > > +++ b/include/linux/pm_runtime.h > > > @@ -386,6 +386,36 @@ static inline int pm_runtime_get_sync(struct device > > *dev) > > > return __pm_runtime_resume(dev, RPM_GET_PUT); } > > > > > > +/** > > > + * pm_runtime_general_get - Bump up usage counter of a device and > > resume it. > > > + * @dev: Target device. > > > + * > > > + * Increase runtime PM usage counter of @dev first, and carry out > > > +runtime-resume > > > + * of it synchronously. If __pm_runtime_resume return negative > > > +value(device is in > > > + * error state), we to need decrease the usage counter before it > > > +return. If > > > + * __pm_runtime_resume return positive value, it means the runtime of > > > +device has > > > + * already been in active state, and we let the new wrapper return zero > > instead. > > > + * > > > + * The possible return values of this function is zero or negative value. > > > + * zero: > > > + * - it means resume succeeed or runtime of device has already been > > active, the > > > + * runtime PM usage counter of @dev remains incremented. > > > + * negative: > > > + * - it means failure and the runtime PM usage counter of @dev has > > been balanced. > > > > The kerneldoc above is kind of noisy and it is hard to figure out what the helper > > really does from it. > > > > You could basically say something like "Resume @dev synchronously and if that > > is successful, increment its runtime PM usage counter. Return > > 0 if the runtime PM usage counter of @dev has been incremented or a negative > > error code otherwise." > > > > How about the following description. > /** > 390 * pm_runtime_general_get - Bump up usage counter of a device and resume it. > 391 * @dev: Target device. > 392 * > 393 * Increase runtime PM usage counter of @dev first, and carry out runtime-resume > 394 * of it synchronously. If __pm_runtime_resume return negative value(device is in > 395 * error state), we to need decrease the usage counter before it return. If > 396 * __pm_runtime_resume return positive value, it means the runtime of device has > 397 * already been in active state, and we let the new wrapper return zero instead. > 398 * If you add the paragraph below, the one above becomes redundant IMV. > 399 * Resume @dev synchronously and if that is successful, and increment its runtime "Resume @dev synchronously and if that is successful, increment its runtime" (drop the extra "and"). > 400 * PM usage counter if it turn out to equal to 0. The runtime PM usage counter of The "if it turn out to equal to 0" phrase is redundant (and the grammar in it is incorrect). > 401 * @dev has been incremented or a negative error code otherwise. > 402 */ Why don't you use what I said verbatim? ^ permalink raw reply [flat|nested] 11+ messages in thread
* 答复: [PATCH v2 1/2] PM: runtime: Add a general runtime get sync operation to deal with usage counter 2020-11-09 16:00 ` Rafael J. Wysocki @ 2020-11-09 16:15 ` zhangqilong 2020-11-09 16:40 ` Rafael J. Wysocki 0 siblings, 1 reply; 11+ messages in thread From: zhangqilong @ 2020-11-09 16:15 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Rafael J. Wysocki, fugang.duan, David Miller, Jakub Kicinski, Linux PM, netdev Hi > > On Mon, Nov 9, 2020 at 4:50 PM zhangqilong <zhangqilong3@huawei.com> > wrote: > > > > > operation to deal with usage counter > > > > > > On Mon, Nov 9, 2020 at 4:00 PM Zhang Qilong > > > <zhangqilong3@huawei.com> > > > wrote: > > > > > > > > In many case, we need to check return value of > > > > pm_runtime_get_sync, but it brings a trouble to the usage counter > > > > processing. Many callers forget to decrease the usage counter when > > > > it failed. It has been discussed a lot[0][1]. So we add a function > > > > to deal with the usage counter for better coding. > > > > > > > > [0]https://lkml.org/lkml/2020/6/14/88 > > > > [1]https://patchwork.ozlabs.org/project/linux-tegra/patch/20200520 > > > > 0951 48.10995-1-dinghao.liu@zju.edu.cn/ > > > > Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com> > > > > --- > > > > include/linux/pm_runtime.h | 30 ++++++++++++++++++++++++++++++ > > > > 1 file changed, 30 insertions(+) > > > > > > > > diff --git a/include/linux/pm_runtime.h > > > > b/include/linux/pm_runtime.h index 4b708f4e8eed..6549ce764400 > > > > 100644 > > > > --- a/include/linux/pm_runtime.h > > > > +++ b/include/linux/pm_runtime.h > > > > @@ -386,6 +386,36 @@ static inline int pm_runtime_get_sync(struct > > > > device > > > *dev) > > > > return __pm_runtime_resume(dev, RPM_GET_PUT); } > > > > > > > > +/** > > > > + * pm_runtime_general_get - Bump up usage counter of a device and > > > resume it. > > > > + * @dev: Target device. > > > > + * > > > > + * Increase runtime PM usage counter of @dev first, and carry out > > > > +runtime-resume > > > > + * of it synchronously. If __pm_runtime_resume return negative > > > > +value(device is in > > > > + * error state), we to need decrease the usage counter before it > > > > +return. If > > > > + * __pm_runtime_resume return positive value, it means the > > > > +runtime of device has > > > > + * already been in active state, and we let the new wrapper > > > > +return zero > > > instead. > > > > + * > > > > + * The possible return values of this function is zero or negative value. > > > > + * zero: > > > > + * - it means resume succeeed or runtime of device has already been > > > active, the > > > > + * runtime PM usage counter of @dev remains incremented. > > > > + * negative: > > > > + * - it means failure and the runtime PM usage counter of @dev has > > > been balanced. > > > > > > The kerneldoc above is kind of noisy and it is hard to figure out > > > what the helper really does from it. > > > > > > You could basically say something like "Resume @dev synchronously > > > and if that is successful, increment its runtime PM usage counter. > > > Return > > > 0 if the runtime PM usage counter of @dev has been incremented or a > > > negative error code otherwise." > > > > > > > How about the following description. > > /** > > 390 * pm_runtime_general_get - Bump up usage counter of a device and > resume it. > > 391 * @dev: Target device. > > 392 * > > 393 * Increase runtime PM usage counter of @dev first, and carry out > > runtime-resume > > 394 * of it synchronously. If __pm_runtime_resume return negative > > value(device is in > > 395 * error state), we to need decrease the usage counter before it > > return. If > > 396 * __pm_runtime_resume return positive value, it means the runtime > > of device has > > 397 * already been in active state, and we let the new wrapper return zero > instead. > > 398 * > > If you add the paragraph below, the one above becomes redundant IMV. > > > 399 * Resume @dev synchronously and if that is successful, and > > increment its runtime > > "Resume @dev synchronously and if that is successful, increment its runtime" > > (drop the extra "and"). > > > 400 * PM usage counter if it turn out to equal to 0. The runtime PM > > usage counter of > > The "if it turn out to equal to 0" phrase is redundant (and the grammar in it is > incorrect). > > > 401 * @dev has been incremented or a negative error code otherwise. > > 402 */ > > Why don't you use what I said verbatim? I had misunderstand just now, sorry for that. The description is as follows: 389 /** 390 * pm_runtime_resume_and_get - Bump up usage counter of a device and resume it. 391 * @dev: Target device. 392 * 393 * Resume @dev synchronously if that is successful, increment its runtime PM 394 * usage counter. Return 0 if the runtime PM usage counter of @dev has been 395 * incremented or a negative error code otherwise. 396 */ Do you think it's OK? Thanks, Zhang ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] PM: runtime: Add a general runtime get sync operation to deal with usage counter 2020-11-09 16:15 ` 答复: " zhangqilong @ 2020-11-09 16:40 ` Rafael J. Wysocki 0 siblings, 0 replies; 11+ messages in thread From: Rafael J. Wysocki @ 2020-11-09 16:40 UTC (permalink / raw) To: zhangqilong Cc: Rafael J. Wysocki, Rafael J. Wysocki, fugang.duan, David Miller, Jakub Kicinski, Linux PM, netdev On Mon, Nov 9, 2020 at 5:15 PM zhangqilong <zhangqilong3@huawei.com> wrote: > > Hi > > > > > On Mon, Nov 9, 2020 at 4:50 PM zhangqilong <zhangqilong3@huawei.com> > > wrote: > > > > > > > operation to deal with usage counter > > > > > > > > On Mon, Nov 9, 2020 at 4:00 PM Zhang Qilong > > > > <zhangqilong3@huawei.com> > > > > wrote: > > > > > > > > > > In many case, we need to check return value of > > > > > pm_runtime_get_sync, but it brings a trouble to the usage counter > > > > > processing. Many callers forget to decrease the usage counter when > > > > > it failed. It has been discussed a lot[0][1]. So we add a function > > > > > to deal with the usage counter for better coding. > > > > > > > > > > [0]https://lkml.org/lkml/2020/6/14/88 > > > > > [1]https://patchwork.ozlabs.org/project/linux-tegra/patch/20200520 > > > > > 0951 48.10995-1-dinghao.liu@zju.edu.cn/ > > > > > Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com> > > > > > --- > > > > > include/linux/pm_runtime.h | 30 ++++++++++++++++++++++++++++++ > > > > > 1 file changed, 30 insertions(+) > > > > > > > > > > diff --git a/include/linux/pm_runtime.h > > > > > b/include/linux/pm_runtime.h index 4b708f4e8eed..6549ce764400 > > > > > 100644 > > > > > --- a/include/linux/pm_runtime.h > > > > > +++ b/include/linux/pm_runtime.h > > > > > @@ -386,6 +386,36 @@ static inline int pm_runtime_get_sync(struct > > > > > device > > > > *dev) > > > > > return __pm_runtime_resume(dev, RPM_GET_PUT); } > > > > > > > > > > +/** > > > > > + * pm_runtime_general_get - Bump up usage counter of a device and > > > > resume it. > > > > > + * @dev: Target device. > > > > > + * > > > > > + * Increase runtime PM usage counter of @dev first, and carry out > > > > > +runtime-resume > > > > > + * of it synchronously. If __pm_runtime_resume return negative > > > > > +value(device is in > > > > > + * error state), we to need decrease the usage counter before it > > > > > +return. If > > > > > + * __pm_runtime_resume return positive value, it means the > > > > > +runtime of device has > > > > > + * already been in active state, and we let the new wrapper > > > > > +return zero > > > > instead. > > > > > + * > > > > > + * The possible return values of this function is zero or negative value. > > > > > + * zero: > > > > > + * - it means resume succeeed or runtime of device has already been > > > > active, the > > > > > + * runtime PM usage counter of @dev remains incremented. > > > > > + * negative: > > > > > + * - it means failure and the runtime PM usage counter of @dev has > > > > been balanced. > > > > > > > > The kerneldoc above is kind of noisy and it is hard to figure out > > > > what the helper really does from it. > > > > > > > > You could basically say something like "Resume @dev synchronously > > > > and if that is successful, increment its runtime PM usage counter. > > > > Return > > > > 0 if the runtime PM usage counter of @dev has been incremented or a > > > > negative error code otherwise." > > > > > > > > > > How about the following description. > > > /** > > > 390 * pm_runtime_general_get - Bump up usage counter of a device and > > resume it. > > > 391 * @dev: Target device. > > > 392 * > > > 393 * Increase runtime PM usage counter of @dev first, and carry out > > > runtime-resume > > > 394 * of it synchronously. If __pm_runtime_resume return negative > > > value(device is in > > > 395 * error state), we to need decrease the usage counter before it > > > return. If > > > 396 * __pm_runtime_resume return positive value, it means the runtime > > > of device has > > > 397 * already been in active state, and we let the new wrapper return zero > > instead. > > > 398 * > > > > If you add the paragraph below, the one above becomes redundant IMV. > > > > > 399 * Resume @dev synchronously and if that is successful, and > > > increment its runtime > > > > "Resume @dev synchronously and if that is successful, increment its runtime" > > > > (drop the extra "and"). > > > > > 400 * PM usage counter if it turn out to equal to 0. The runtime PM > > > usage counter of > > > > The "if it turn out to equal to 0" phrase is redundant (and the grammar in it is > > incorrect). > > > > > 401 * @dev has been incremented or a negative error code otherwise. > > > 402 */ > > > > Why don't you use what I said verbatim? > > I had misunderstand just now, sorry for that. The description is as follows: > 389 /** > 390 * pm_runtime_resume_and_get - Bump up usage counter of a device and resume it. > 391 * @dev: Target device. > 392 * > 393 * Resume @dev synchronously if that is successful, increment its runtime PM "Resume @dev synchronously and if that is successful, increment its runtime PM" (missing "and"). > 394 * usage counter. Return 0 if the runtime PM usage counter of @dev has been > 395 * incremented or a negative error code otherwise. > 396 */ > > Do you think it's OK? Apart from the above typo, yes it is. Thanks! ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 2/2] net: fec: Fix reference count leak in fec series ops 2020-11-09 15:04 [PATCH v2 0/2] Fix usage counter leak by adding a general sync ops Zhang Qilong 2020-11-09 15:04 ` [PATCH v2 1/2] PM: runtime: Add a general runtime get sync operation to deal with usage counter Zhang Qilong @ 2020-11-09 15:04 ` Zhang Qilong 1 sibling, 0 replies; 11+ messages in thread From: Zhang Qilong @ 2020-11-09 15:04 UTC (permalink / raw) To: rjw, fugang.duan, davem, kuba; +Cc: linux-pm, netdev 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 by replacing it with pm_runtime_general_get. 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 | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index d7919555250d..695720f8263f 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -1808,7 +1808,7 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum) int ret = 0, frame_start, frame_addr, frame_op; bool is_c45 = !!(regnum & MII_ADDR_C45); - ret = pm_runtime_get_sync(dev); + ret = pm_runtime_general_get(dev); if (ret < 0) return ret; @@ -1867,11 +1867,9 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum, int ret, frame_start, frame_addr; bool is_c45 = !!(regnum & MII_ADDR_C45); - ret = pm_runtime_get_sync(dev); + ret = pm_runtime_general_get(dev); if (ret < 0) return ret; - else - ret = 0; if (is_c45) { frame_start = FEC_MMFR_ST_C45; @@ -2275,7 +2273,7 @@ static void fec_enet_get_regs(struct net_device *ndev, u32 i, off; int ret; - ret = pm_runtime_get_sync(dev); + ret = pm_runtime_general_get(dev); if (ret < 0) return; @@ -2976,7 +2974,7 @@ fec_enet_open(struct net_device *ndev) int ret; bool reset_again; - ret = pm_runtime_get_sync(&fep->pdev->dev); + ret = pm_runtime_general_get(&fep->pdev->dev); if (ret < 0) return ret; @@ -3770,7 +3768,7 @@ fec_drv_remove(struct platform_device *pdev) struct device_node *np = pdev->dev.of_node; int ret; - ret = pm_runtime_get_sync(&pdev->dev); + ret = pm_runtime_general_get(&pdev->dev); if (ret < 0) return ret; -- 2.25.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-11-09 16:40 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-09 15:04 [PATCH v2 0/2] Fix usage counter leak by adding a general sync ops Zhang Qilong 2020-11-09 15:04 ` [PATCH v2 1/2] PM: runtime: Add a general runtime get sync operation to deal with usage counter Zhang Qilong 2020-11-09 15:20 ` Rafael J. Wysocki 2020-11-09 15:49 ` Ulf Hansson 2020-11-09 15:53 ` Rafael J. Wysocki 2020-11-09 16:05 ` Ulf Hansson 2020-11-09 15:50 ` 答复: " zhangqilong 2020-11-09 16:00 ` Rafael J. Wysocki 2020-11-09 16:15 ` 答复: " zhangqilong 2020-11-09 16:40 ` Rafael J. Wysocki 2020-11-09 15:04 ` [PATCH v2 2/2] net: fec: Fix reference count leak in fec series ops Zhang Qilong
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).