From: "Lazar, Lijo" <lijo.lazar@amd.com>
To: "Ma, Jun" <majun@amd.com>, Ma Jun <Jun.Ma2@amd.com>,
amd-gfx@lists.freedesktop.org
Cc: Kenneth.Feng@amd.com, Alexander.Deucher@amd.com,
kevinyang.wang@amd.com, Yin Zhenguo <zhenguo.yin@amd.com>
Subject: Re: [PATCH] drm/amdgpu/pm: Fix NULL pointer dereference when set/get power limit
Date: Thu, 7 Mar 2024 09:02:19 +0530 [thread overview]
Message-ID: <20dfca86-fe0a-45fa-b1b0-a9ace5d7090f@amd.com> (raw)
In-Reply-To: <097c4133-47d0-40c9-b441-cdd38b746c77@amd.com>
On 3/7/2024 7:42 AM, Ma, Jun wrote:
> Hi Lijo,
>
> On 3/6/2024 7:16 PM, Lazar, Lijo wrote:
>>
>>
>> On 3/6/2024 3:56 PM, Ma Jun wrote:
>>> Because powerplay_table initialization is skipped under
>>> sriov case, We set default lower and upper OD value to
>>> avoid NULL pointer issue.
>>
>> pp_od_clk_voltage is not enabled in SRIOV (except for GC 9.4.3 one VF
>> mode). Since the interface is not available for SRIOV, a better fix may
>> be to set smu->od_enabled itself to false for SRIOV.
>>
> This is not about pp_od_clk_voltage and od_enabled. This problem only occurs
> when getting power1_cap_* value, because we need to read the od_percent_lower
> value from poweplay_table.
od_enabled = false means there is no OD capability and then OD
lower/higher percentage should be zero. For SRIOV, this will be the case.
Instead you are putting some default values which doesn't necessarily
work for all SKUs (that's the reason they are part of powerplay table).
Thanks,
Lijo
>
> Regards,
> Ma Jun
>> Thanks,
>> Lijo
>>
>>>
>>> Fixes: 7968e9748fbb ("drm/amdgpu/pm: Fix the power1_min_cap value")
>>> Signed-off-by: Ma Jun <Jun.Ma2@amd.com>
>>> Reported-by: Yin Zhenguo <zhenguo.yin@amd.com>
>>> ---
>>> .../gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c | 15 ++++++++++-----
>>> drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 16 ++++++++++------
>>> .../drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c | 15 ++++++++++-----
>>> .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_0_ppt.c | 15 ++++++++++-----
>>> .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c | 15 ++++++++++-----
>>> 5 files changed, 50 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
>>> index 1d96eb274d72..42c5e6b103e8 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
>>> @@ -1303,12 +1303,17 @@ static int arcturus_get_power_limit(struct smu_context *smu,
>>> if (default_power_limit)
>>> *default_power_limit = power_limit;
>>>
>>> - if (smu->od_enabled)
>>> - od_percent_upper = le32_to_cpu(powerplay_table->overdrive_table.max[SMU_11_0_ODSETTING_POWERPERCENTAGE]);
>>> - else
>>> - od_percent_upper = 0;
>>> + if (powerplay_table) {
>>> + if (smu->od_enabled)
>>> + od_percent_upper = le32_to_cpu(powerplay_table->overdrive_table.max[SMU_11_0_ODSETTING_POWERPERCENTAGE]);
>>> + else
>>> + od_percent_upper = 0;
>>>
>>> - od_percent_lower = le32_to_cpu(powerplay_table->overdrive_table.min[SMU_11_0_ODSETTING_POWERPERCENTAGE]);
>>> + od_percent_lower = le32_to_cpu(powerplay_table->overdrive_table.min[SMU_11_0_ODSETTING_POWERPERCENTAGE]);
>>> + } else {
>>> + od_percent_upper = 0;
>>> + od_percent_lower = 10;
>>> + }
>>>
>>> dev_dbg(smu->adev->dev, "od percent upper:%d, od percent lower:%d (default power: %d)\n",
>>> od_percent_upper, od_percent_lower, power_limit);
>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>>> index ed189a3878eb..6cc2e2d27a0d 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>>> @@ -2356,13 +2356,17 @@ static int navi10_get_power_limit(struct smu_context *smu,
>>> if (default_power_limit)
>>> *default_power_limit = power_limit;
>>>
>>> - if (smu->od_enabled &&
>>> - navi10_od_feature_is_supported(od_settings, SMU_11_0_ODCAP_POWER_LIMIT))
>>> - od_percent_upper = le32_to_cpu(powerplay_table->overdrive_table.max[SMU_11_0_ODSETTING_POWERPERCENTAGE]);
>>> - else
>>> - od_percent_upper = 0;
>>> + if (powerplay_table) {
>>> + if (smu->od_enabled && navi10_od_feature_is_supported(od_settings, SMU_11_0_ODCAP_POWER_LIMIT))
>>> + od_percent_upper = le32_to_cpu(powerplay_table->overdrive_table.max[SMU_11_0_ODSETTING_POWERPERCENTAGE]);
>>> + else
>>> + od_percent_upper = 0;
>>>
>>> - od_percent_lower = le32_to_cpu(powerplay_table->overdrive_table.min[SMU_11_0_ODSETTING_POWERPERCENTAGE]);
>>> + od_percent_lower = le32_to_cpu(powerplay_table->overdrive_table.min[SMU_11_0_ODSETTING_POWERPERCENTAGE]);
>>> + } else {
>>> + od_percent_upper = 0;
>>> + od_percent_lower = 10;
>>> + }
>>>
>>> dev_dbg(smu->adev->dev, "od percent upper:%d, od percent lower:%d (default power: %d)\n",
>>> od_percent_upper, od_percent_lower, power_limit);
>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
>>> index e2ad2b972ab0..5daeac2e6239 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
>>> @@ -640,12 +640,17 @@ static int sienna_cichlid_get_power_limit(struct smu_context *smu,
>>> if (default_power_limit)
>>> *default_power_limit = power_limit;
>>>
>>> - if (smu->od_enabled)
>>> - od_percent_upper = le32_to_cpu(powerplay_table->overdrive_table.max[SMU_11_0_7_ODSETTING_POWERPERCENTAGE]);
>>> - else
>>> - od_percent_upper = 0;
>>> + if (powerplay_table) {
>>> + if (smu->od_enabled)
>>> + od_percent_upper = le32_to_cpu(powerplay_table->overdrive_table.max[SMU_11_0_7_ODSETTING_POWERPERCENTAGE]);
>>> + else
>>> + od_percent_upper = 0;
>>>
>>> - od_percent_lower = le32_to_cpu(powerplay_table->overdrive_table.min[SMU_11_0_7_ODSETTING_POWERPERCENTAGE]);
>>> + od_percent_lower = le32_to_cpu(powerplay_table->overdrive_table.min[SMU_11_0_7_ODSETTING_POWERPERCENTAGE]);
>>> + } else {
>>> + od_percent_upper = 0;
>>> + od_percent_lower = 10;
>>> + }
>>>
>>> dev_dbg(smu->adev->dev, "od percent upper:%d, od percent lower:%d (default power: %d)\n",
>>> od_percent_upper, od_percent_lower, power_limit);
>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_0_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_0_ppt.c
>>> index 215f7c91ca73..271151c518e1 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_0_ppt.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_0_ppt.c
>>> @@ -2369,12 +2369,17 @@ static int smu_v13_0_0_get_power_limit(struct smu_context *smu,
>>> if (default_power_limit)
>>> *default_power_limit = power_limit;
>>>
>>> - if (smu->od_enabled)
>>> - od_percent_upper = le32_to_cpu(powerplay_table->overdrive_table.max[SMU_13_0_0_ODSETTING_POWERPERCENTAGE]);
>>> - else
>>> - od_percent_upper = 0;
>>> + if (powerplay_table) {
>>> + if (smu->od_enabled)
>>> + od_percent_upper = le32_to_cpu(powerplay_table->overdrive_table.max[SMU_13_0_0_ODSETTING_POWERPERCENTAGE]);
>>> + else
>>> + od_percent_upper = 0;
>>>
>>> - od_percent_lower = le32_to_cpu(powerplay_table->overdrive_table.min[SMU_13_0_0_ODSETTING_POWERPERCENTAGE]);
>>> + od_percent_lower = le32_to_cpu(powerplay_table->overdrive_table.min[SMU_13_0_0_ODSETTING_POWERPERCENTAGE]);
>>> + } else {
>>> + od_percent_upper = 0;
>>> + od_percent_lower = 10;
>>> + }
>>>
>>> dev_dbg(smu->adev->dev, "od percent upper:%d, od percent lower:%d (default power: %d)\n",
>>> od_percent_upper, od_percent_lower, power_limit);
>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c
>>> index 3dc7b60cb075..533a3c1ba41e 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c
>>> @@ -2333,12 +2333,17 @@ static int smu_v13_0_7_get_power_limit(struct smu_context *smu,
>>> if (default_power_limit)
>>> *default_power_limit = power_limit;
>>>
>>> - if (smu->od_enabled)
>>> - od_percent_upper = le32_to_cpu(powerplay_table->overdrive_table.max[SMU_13_0_7_ODSETTING_POWERPERCENTAGE]);
>>> - else
>>> - od_percent_upper = 0;
>>> + if (powerplay_table) {
>>> + if (smu->od_enabled)
>>> + od_percent_upper = le32_to_cpu(powerplay_table->overdrive_table.max[SMU_13_0_7_ODSETTING_POWERPERCENTAGE]);
>>> + else
>>> + od_percent_upper = 0;
>>>
>>> - od_percent_lower = le32_to_cpu(powerplay_table->overdrive_table.min[SMU_13_0_7_ODSETTING_POWERPERCENTAGE]);
>>> + od_percent_lower = le32_to_cpu(powerplay_table->overdrive_table.min[SMU_13_0_7_ODSETTING_POWERPERCENTAGE]);
>>> + } else {
>>> + od_percent_upper = 0;
>>> + od_percent_lower = 10;
>>> + }
>>>
>>> dev_dbg(smu->adev->dev, "od percent upper:%d, od percent lower:%d (default power: %d)\n",
>>> od_percent_upper, od_percent_lower, power_limit);
prev parent reply other threads:[~2024-03-07 3:32 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-06 10:26 [PATCH] drm/amdgpu/pm: Fix NULL pointer dereference when set/get power limit Ma Jun
2024-03-06 11:16 ` Lazar, Lijo
2024-03-07 2:12 ` Ma, Jun
2024-03-07 3:32 ` Lazar, Lijo [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20dfca86-fe0a-45fa-b1b0-a9ace5d7090f@amd.com \
--to=lijo.lazar@amd.com \
--cc=Alexander.Deucher@amd.com \
--cc=Jun.Ma2@amd.com \
--cc=Kenneth.Feng@amd.com \
--cc=amd-gfx@lists.freedesktop.org \
--cc=kevinyang.wang@amd.com \
--cc=majun@amd.com \
--cc=zhenguo.yin@amd.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox