| Message ID | 1615294733-22761-9-git-send-email-aisheng.dong@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PM / devfreq: a few small fixes and improvements | expand |
On 21. 3. 9. 오후 9:58, Dong Aisheng wrote: > Check .get_dev_status() in devfreq_update_stats in case it's abused > when a device does not provide it. > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > --- > drivers/devfreq/governor.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h > index 31af6d072a10..67a6dbdd5d23 100644 > --- a/drivers/devfreq/governor.h > +++ b/drivers/devfreq/governor.h > @@ -89,6 +89,9 @@ int devfreq_update_target(struct devfreq *devfreq, unsigned long freq); > > static inline int devfreq_update_stats(struct devfreq *df) > { > + if (!df->profile->get_dev_status) > + return -EINVAL; > + > return df->profile->get_dev_status(df->dev.parent, &df->last_status); > } > #endif /* _GOVERNOR_H */ > Applied it. Thanks.
On 21. 3. 9. 오후 9:58, Dong Aisheng wrote: > Check .get_dev_status() in devfreq_update_stats in case it's abused > when a device does not provide it. > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > --- > drivers/devfreq/governor.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h > index 31af6d072a10..67a6dbdd5d23 100644 > --- a/drivers/devfreq/governor.h > +++ b/drivers/devfreq/governor.h > @@ -89,6 +89,9 @@ int devfreq_update_target(struct devfreq *devfreq, unsigned long freq); > > static inline int devfreq_update_stats(struct devfreq *df) > { > + if (!df->profile->get_dev_status) > + return -EINVAL; > + I'm considering the following method instead of returning the error when .get_dev_status is NULL. if (!df->profile->get_dev_status) { df->last_status.total_time = 0; df->last_status.busy_time = 0; df->last_status.current_frequency = 0; return 0; } > return df->profile->get_dev_status(df->dev.parent, &df->last_status); > } > #endif /* _GOVERNOR_H */ >
On Wed, Mar 10, 2021 at 12:20 AM Chanwoo Choi <cwchoi00@gmail.com> wrote: > > On 21. 3. 9. 오후 9:58, Dong Aisheng wrote: > > Check .get_dev_status() in devfreq_update_stats in case it's abused > > when a device does not provide it. > > > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > > --- > > drivers/devfreq/governor.h | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h > > index 31af6d072a10..67a6dbdd5d23 100644 > > --- a/drivers/devfreq/governor.h > > +++ b/drivers/devfreq/governor.h > > @@ -89,6 +89,9 @@ int devfreq_update_target(struct devfreq *devfreq, unsigned long freq); > > > > static inline int devfreq_update_stats(struct devfreq *df) > > { > > + if (!df->profile->get_dev_status) > > + return -EINVAL; > > + > > I'm considering the following method instead of returning the error > when .get_dev_status is NULL. > > if (!df->profile->get_dev_status) { > df->last_status.total_time = 0; > df->last_status.busy_time = 0; > df->last_status.current_frequency = 0; > return 0; > } I might suggest not cause it's meaningless for ondemand governor but introducing confusing. Simply return error could make the life a bit easier. does it make sense to you? Regards Aisheng > > > return df->profile->get_dev_status(df->dev.parent, &df->last_status); > > } > > #endif /* _GOVERNOR_H */ > > > > > -- > Best Regards, > Samsung Electronics > Chanwoo Choi
On 3/10/21 12:00 PM, Dong Aisheng wrote: > On Wed, Mar 10, 2021 at 12:20 AM Chanwoo Choi <cwchoi00@gmail.com> wrote: >> >> On 21. 3. 9. 오후 9:58, Dong Aisheng wrote: >>> Check .get_dev_status() in devfreq_update_stats in case it's abused >>> when a device does not provide it. >>> >>> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> >>> --- >>> drivers/devfreq/governor.h | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h >>> index 31af6d072a10..67a6dbdd5d23 100644 >>> --- a/drivers/devfreq/governor.h >>> +++ b/drivers/devfreq/governor.h >>> @@ -89,6 +89,9 @@ int devfreq_update_target(struct devfreq *devfreq, unsigned long freq); >>> >>> static inline int devfreq_update_stats(struct devfreq *df) >>> { >>> + if (!df->profile->get_dev_status) >>> + return -EINVAL; >>> + >> >> I'm considering the following method instead of returning the error >> when .get_dev_status is NULL. >> >> if (!df->profile->get_dev_status) { >> df->last_status.total_time = 0; >> df->last_status.busy_time = 0; >> df->last_status.current_frequency = 0; >> return 0; >> } > > I might suggest not cause it's meaningless for ondemand governor but > introducing confusing. Simply return error could make the life a bit easier. > does it make sense to you? Actually, I considered the some corner case as following: We can see the simple_ondemand governor through available_governors even if the devfreq driver doesn't implement the .get_dev_status. In this corner case, My intention tried to prevent the error on this case. But, actually, it is different issue. I'll fix this issue when get_dev_status is NULL, don't show the simple_ondemand governor name through available_governors on other patch. And I applied it. Thanks. > > Regards > Aisheng > >> >>> return df->profile->get_dev_status(df->dev.parent, &df->last_status); >>> } >>> #endif /* _GOVERNOR_H */ >>> >> >> >> -- >> Best Regards, >> Samsung Electronics >> Chanwoo Choi > >
diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h index 31af6d072a10..67a6dbdd5d23 100644 --- a/drivers/devfreq/governor.h +++ b/drivers/devfreq/governor.h @@ -89,6 +89,9 @@ int devfreq_update_target(struct devfreq *devfreq, unsigned long freq); static inline int devfreq_update_stats(struct devfreq *df) { + if (!df->profile->get_dev_status) + return -EINVAL; + return df->profile->get_dev_status(df->dev.parent, &df->last_status); } #endif /* _GOVERNOR_H */
Check .get_dev_status() in devfreq_update_stats in case it's abused when a device does not provide it. Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> --- drivers/devfreq/governor.h | 3 +++ 1 file changed, 3 insertions(+)