There is no limit on high "idx" can go. It should be less than ARRAY_SIZE(data.states) which is 16.
The "data" variable wasn't declared in that scope so I shifted the code around a bit to make it work.
Fixes: f3898ea12fc1 ('drm/amd/powerplay: add some sysfs interfaces for powerplay.') Signed-off-by: Dan Carpenter dan.carpenter@oracle.com
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c index 589b36e..ce9e97f 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c @@ -275,25 +275,23 @@ static ssize_t amdgpu_set_pp_force_state(struct device *dev,
if (strlen(buf) == 1) adev->pp_force_state_enabled = false; - else { - ret = kstrtol(buf, 0, &idx); + else if (adev->pp_enabled) { + struct pp_states_info data;
- if (ret) { + ret = kstrtol(buf, 0, &idx); + if (ret || idx >= ARRAY_SIZE(data.states)) { count = -EINVAL; goto fail; }
- if (adev->pp_enabled) { - struct pp_states_info data; - amdgpu_dpm_get_pp_num_states(adev, &data); - state = data.states[idx]; - /* only set user selected power states */ - if (state != POWER_STATE_TYPE_INTERNAL_BOOT && - state != POWER_STATE_TYPE_DEFAULT) { - amdgpu_dpm_dispatch_task(adev, - AMD_PP_EVENT_ENABLE_USER_STATE, &state, NULL); - adev->pp_force_state_enabled = true; - } + amdgpu_dpm_get_pp_num_states(adev, &data); + state = data.states[idx]; + /* only set user selected power states */ + if (state != POWER_STATE_TYPE_INTERNAL_BOOT && + state != POWER_STATE_TYPE_DEFAULT) { + amdgpu_dpm_dispatch_task(adev, + AMD_PP_EVENT_ENABLE_USER_STATE, &state, NULL); + adev->pp_force_state_enabled = true; } } fail:
Am 16.06.2016 08:41, schrieb Dan Carpenter:
There is no limit on high "idx" can go. It should be less than ARRAY_SIZE(data.states) which is 16.
The "data" variable wasn't declared in that scope so I shifted the code around a bit to make it work.
Fixes: f3898ea12fc1 ('drm/amd/powerplay: add some sysfs interfaces for powerplay.') Signed-off-by: Dan Carpenter dan.carpenter@oracle.com
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c index 589b36e..ce9e97f 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c @@ -275,25 +275,23 @@ static ssize_t amdgpu_set_pp_force_state(struct device *dev,
if (strlen(buf) == 1) adev->pp_force_state_enabled = false;
- else {
ret = kstrtol(buf, 0, &idx);
- else if (adev->pp_enabled) {
struct pp_states_info data;
if (ret) {
ret = kstrtol(buf, 0, &idx);if (ret || idx >= ARRAY_SIZE(data.states)) { count = -EINVAL; goto fail;
i would also expect a check idx < 0, does it mean this can not happen ? otherwise maybe kstrtoul is a solution ?
re, wh
if (adev->pp_enabled) {struct pp_states_info data;amdgpu_dpm_get_pp_num_states(adev, &data);state = data.states[idx];/* only set user selected power states */if (state != POWER_STATE_TYPE_INTERNAL_BOOT &&state != POWER_STATE_TYPE_DEFAULT) {amdgpu_dpm_dispatch_task(adev,AMD_PP_EVENT_ENABLE_USER_STATE, &state, NULL);adev->pp_force_state_enabled = true;}
amdgpu_dpm_get_pp_num_states(adev, &data);state = data.states[idx];/* only set user selected power states */if (state != POWER_STATE_TYPE_INTERNAL_BOOT &&state != POWER_STATE_TYPE_DEFAULT) {amdgpu_dpm_dispatch_task(adev,AMD_PP_EVENT_ENABLE_USER_STATE, &state, NULL);adev->pp_force_state_enabled = true;fail:
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 16, 2016 at 09:26:03AM +0200, walter harms wrote:
Am 16.06.2016 08:41, schrieb Dan Carpenter:
There is no limit on high "idx" can go. It should be less than ARRAY_SIZE(data.states) which is 16.
The "data" variable wasn't declared in that scope so I shifted the code around a bit to make it work.
Fixes: f3898ea12fc1 ('drm/amd/powerplay: add some sysfs interfaces for powerplay.') Signed-off-by: Dan Carpenter dan.carpenter@oracle.com
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c index 589b36e..ce9e97f 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c @@ -275,25 +275,23 @@ static ssize_t amdgpu_set_pp_force_state(struct device *dev,
if (strlen(buf) == 1) adev->pp_force_state_enabled = false;
- else {
ret = kstrtol(buf, 0, &idx);
- else if (adev->pp_enabled) {
struct pp_states_info data;
if (ret) {
ret = kstrtol(buf, 0, &idx);if (ret || idx >= ARRAY_SIZE(data.states)) { count = -EINVAL; goto fail;i would also expect a check idx < 0, does it mean this can not happen ? otherwise maybe kstrtoul is a solution ?
The original code could underflow, but my code can't. ARRAY_SIZE() means the comparison is type promoted to size_t which is unsigned long.
regards, dan carpenter
Am 16.06.2016 um 09:54 schrieb Dan Carpenter:
On Thu, Jun 16, 2016 at 09:26:03AM +0200, walter harms wrote:
Am 16.06.2016 08:41, schrieb Dan Carpenter:
There is no limit on high "idx" can go. It should be less than ARRAY_SIZE(data.states) which is 16.
The "data" variable wasn't declared in that scope so I shifted the code around a bit to make it work.
Fixes: f3898ea12fc1 ('drm/amd/powerplay: add some sysfs interfaces for powerplay.') Signed-off-by: Dan Carpenter dan.carpenter@oracle.com
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c index 589b36e..ce9e97f 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c @@ -275,25 +275,23 @@ static ssize_t amdgpu_set_pp_force_state(struct device *dev,
if (strlen(buf) == 1) adev->pp_force_state_enabled = false;
- else {
ret = kstrtol(buf, 0, &idx);
- else if (adev->pp_enabled) {
struct pp_states_info data;
if (ret) {
ret = kstrtol(buf, 0, &idx);if (ret || idx >= ARRAY_SIZE(data.states)) { count = -EINVAL; goto fail;i would also expect a check idx < 0, does it mean this can not happen ? otherwise maybe kstrtoul is a solution ?
The original code could underflow, but my code can't. ARRAY_SIZE() means the comparison is type promoted to size_t which is unsigned long.
That's probably true, but not very obvious (not that I understand much of the power related code anyway).
Using kstrtoul() in the first place would make it a bit less obscure and probably generate a nice error code when somebody really tries to use a negative index here.
Cheers, Christian.
regards, dan carpenter
Sure, I'll resend. Unsigned is a little cleaner. There is no new error code/message or change in behavior though either way.
regards, dan carpenter
There is no limit on high "idx" can go. It should be less than ARRAY_SIZE(data.states) which is 16.
The "data" variable wasn't declared in that scope so I shifted the code around a bit to make it work. Also I made "idx" unsigned.
Fixes: f3898ea12fc1 ('drm/amd/powerplay: add some sysfs interfaces for powerplay.') Signed-off-by: Dan Carpenter dan.carpenter@oracle.com --- v2: make idx unsigned
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c index 589b36e..0e13d80 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c @@ -270,30 +270,28 @@ static ssize_t amdgpu_set_pp_force_state(struct device *dev, struct drm_device *ddev = dev_get_drvdata(dev); struct amdgpu_device *adev = ddev->dev_private; enum amd_pm_state_type state = 0; - long idx; + unsigned long idx; int ret;
if (strlen(buf) == 1) adev->pp_force_state_enabled = false; - else { - ret = kstrtol(buf, 0, &idx); + else if (adev->pp_enabled) { + struct pp_states_info data;
- if (ret) { + ret = kstrtoul(buf, 0, &idx); + if (ret || idx >= ARRAY_SIZE(data.states)) { count = -EINVAL; goto fail; }
- if (adev->pp_enabled) { - struct pp_states_info data; - amdgpu_dpm_get_pp_num_states(adev, &data); - state = data.states[idx]; - /* only set user selected power states */ - if (state != POWER_STATE_TYPE_INTERNAL_BOOT && - state != POWER_STATE_TYPE_DEFAULT) { - amdgpu_dpm_dispatch_task(adev, - AMD_PP_EVENT_ENABLE_USER_STATE, &state, NULL); - adev->pp_force_state_enabled = true; - } + amdgpu_dpm_get_pp_num_states(adev, &data); + state = data.states[idx]; + /* only set user selected power states */ + if (state != POWER_STATE_TYPE_INTERNAL_BOOT && + state != POWER_STATE_TYPE_DEFAULT) { + amdgpu_dpm_dispatch_task(adev, + AMD_PP_EVENT_ENABLE_USER_STATE, &state, NULL); + adev->pp_force_state_enabled = true; } } fail:
Am 16.06.2016 um 10:30 schrieb Dan Carpenter:
There is no limit on high "idx" can go. It should be less than ARRAY_SIZE(data.states) which is 16.
The "data" variable wasn't declared in that scope so I shifted the code around a bit to make it work. Also I made "idx" unsigned.
Fixes: f3898ea12fc1 ('drm/amd/powerplay: add some sysfs interfaces for powerplay.') Signed-off-by: Dan Carpenter dan.carpenter@oracle.com
Acked-by: Christian König christian.koenig@amd.com.
v2: make idx unsigned
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c index 589b36e..0e13d80 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c @@ -270,30 +270,28 @@ static ssize_t amdgpu_set_pp_force_state(struct device *dev, struct drm_device *ddev = dev_get_drvdata(dev); struct amdgpu_device *adev = ddev->dev_private; enum amd_pm_state_type state = 0;
- long idx;
unsigned long idx; int ret;
if (strlen(buf) == 1) adev->pp_force_state_enabled = false;
- else {
ret = kstrtol(buf, 0, &idx);
- else if (adev->pp_enabled) {
struct pp_states_info data;
if (ret) {
ret = kstrtoul(buf, 0, &idx);if (ret || idx >= ARRAY_SIZE(data.states)) { count = -EINVAL; goto fail;
if (adev->pp_enabled) {struct pp_states_info data;amdgpu_dpm_get_pp_num_states(adev, &data);state = data.states[idx];/* only set user selected power states */if (state != POWER_STATE_TYPE_INTERNAL_BOOT &&state != POWER_STATE_TYPE_DEFAULT) {amdgpu_dpm_dispatch_task(adev,AMD_PP_EVENT_ENABLE_USER_STATE, &state, NULL);adev->pp_force_state_enabled = true;}
amdgpu_dpm_get_pp_num_states(adev, &data);state = data.states[idx];/* only set user selected power states */if (state != POWER_STATE_TYPE_INTERNAL_BOOT &&state != POWER_STATE_TYPE_DEFAULT) {amdgpu_dpm_dispatch_task(adev,AMD_PP_EVENT_ENABLE_USER_STATE, &state, NULL);adev->pp_force_state_enabled = true;
On Thu, Jun 16, 2016 at 4:39 AM, Christian König christian.koenig@amd.com wrote:
Am 16.06.2016 um 10:30 schrieb Dan Carpenter:
There is no limit on high "idx" can go. It should be less than ARRAY_SIZE(data.states) which is 16.
The "data" variable wasn't declared in that scope so I shifted the code around a bit to make it work. Also I made "idx" unsigned.
Fixes: f3898ea12fc1 ('drm/amd/powerplay: add some sysfs interfaces for powerplay.') Signed-off-by: Dan Carpenter dan.carpenter@oracle.com
Acked-by: Christian König christian.koenig@amd.com.
Applied. thanks!
Alex
v2: make idx unsigned
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c index 589b36e..0e13d80 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c @@ -270,30 +270,28 @@ static ssize_t amdgpu_set_pp_force_state(struct device *dev, struct drm_device *ddev = dev_get_drvdata(dev); struct amdgpu_device *adev = ddev->dev_private; enum amd_pm_state_type state = 0;
long idx;
unsigned long idx; int ret; if (strlen(buf) == 1) adev->pp_force_state_enabled = false;
else {ret = kstrtol(buf, 0, &idx);
else if (adev->pp_enabled) {struct pp_states_info data;
if (ret) {ret = kstrtoul(buf, 0, &idx);if (ret || idx >= ARRAY_SIZE(data.states)) { count = -EINVAL; goto fail; }
if (adev->pp_enabled) {
struct pp_states_info data;amdgpu_dpm_get_pp_num_states(adev, &data);state = data.states[idx];/* only set user selected power states */if (state != POWER_STATE_TYPE_INTERNAL_BOOT &&state != POWER_STATE_TYPE_DEFAULT) {amdgpu_dpm_dispatch_task(adev,AMD_PP_EVENT_ENABLE_USER_STATE, &state, NULL);
adev->pp_force_state_enabled = true;}
amdgpu_dpm_get_pp_num_states(adev, &data);state = data.states[idx];/* only set user selected power states */if (state != POWER_STATE_TYPE_INTERNAL_BOOT &&state != POWER_STATE_TYPE_DEFAULT) {amdgpu_dpm_dispatch_task(adev,AMD_PP_EVENT_ENABLE_USER_STATE,&state, NULL);
adev->pp_force_state_enabled = true; } }
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Christian König -
Dan Carpenter -
walter harms