| Message ID | 20241025051141.924760-1-bugfood-ml@fatooh.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86/amd/pmc: Detect when STB is not available | expand |
On Thu, 24 Oct 2024, Corey Hickey wrote: > From: Corey Hickey <bugfood-c@fatooh.org> > > Loading the amd_pmc module as: > > amd_pmc enable_stb=1 > > ...can result in the following messages in the kernel ring buffer: > > amd_pmc AMDI0009:00: SMU cmd failed. err: 0xff > ioremap on RAM at 0x0000000000000000 - 0x0000000000ffffff > WARNING: CPU: 10 PID: 2151 at arch/x86/mm/ioremap.c:217 __ioremap_caller+0x2cd/0x340 > > Additional debug shows that this happens when the calls to obtain > S2D_PHYS_ADDR_LOW and S2D_PHYS_ADDR_HIGH return 0. > > Per discussion on platform-driver-x86@vger.kernel.org, this condition > indicates that the STB is not available. If you want to refer to discussion, add it into a Link: tag. Only write the conclusion into the commit message (the part you have there after comma). > In order to avoid the ioremap warning, and to help the user understand > the situation, catch the invalid address and print an error. > > Signed-off-by: Corey Hickey <bugfood-c@fatooh.org> Isn't Fixes tag appropriate for this change? > --- > drivers/platform/x86/amd/pmc/pmc.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c > index bbb8edb62e00..72b1dfc64bf1 100644 > --- a/drivers/platform/x86/amd/pmc/pmc.c > +++ b/drivers/platform/x86/amd/pmc/pmc.c > @@ -998,6 +998,11 @@ static int amd_pmc_s2d_init(struct amd_pmc_dev *dev) > amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low, dev->s2d_msg_id, true); > amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi, dev->s2d_msg_id, true); > > + if (!phys_addr_hi && !phys_addr_low) { > + dev_err(dev->dev, "amd_pmc: STB is not enabled on the system; disable enable_stb or contact system vendor\n"); Won't that end up duplicating the prefix if you put one into the string? The prefix is handled for you by pr_fmt() which is already provided in this file.
On 2024-10-25 07:55, Ilpo Järvinen wrote: >> Per discussion on platform-driver-x86@vger.kernel.org, this condition >> indicates that the STB is not available. > > If you want to refer to discussion, add it into a Link: tag. Only write > the conclusion into the commit message (the part you have there after > comma). Thank you, I have added a Link tag to the mailing list archives. I wanted to avoid taking credit for the claim, though--I don't have the background to claim that on my own. I have changed the wording to refer to Shyam Sundar S K, hoping that's an ok thing to do. >> In order to avoid the ioremap warning, and to help the user understand >> the situation, catch the invalid address and print an error. >> >> Signed-off-by: Corey Hickey <bugfood-c@fatooh.org> > > Isn't Fixes tag appropriate for this change? Hmm... I didn't think so at first, but I guess so? I didn't do a git bisect, but I can identify that the code in question was added by 3d7d407dfb05, so I added that as a Fixes tag. >> --- >> drivers/platform/x86/amd/pmc/pmc.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c >> index bbb8edb62e00..72b1dfc64bf1 100644 >> --- a/drivers/platform/x86/amd/pmc/pmc.c >> +++ b/drivers/platform/x86/amd/pmc/pmc.c >> @@ -998,6 +998,11 @@ static int amd_pmc_s2d_init(struct amd_pmc_dev *dev) >> amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low, dev->s2d_msg_id, true); >> amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi, dev->s2d_msg_id, true); >> >> + if (!phys_addr_hi && !phys_addr_low) { >> + dev_err(dev->dev, "amd_pmc: STB is not enabled on the system; disable enable_stb or contact system vendor\n"); > > Won't that end up duplicating the prefix if you put one into the string? > The prefix is handled for you by pr_fmt() which is already provided in > this file. Ah, yes, I missed that. Thank you for your review. I will submit a patch v2 momentarily. -Corey
diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c index bbb8edb62e00..72b1dfc64bf1 100644 --- a/drivers/platform/x86/amd/pmc/pmc.c +++ b/drivers/platform/x86/amd/pmc/pmc.c @@ -998,6 +998,11 @@ static int amd_pmc_s2d_init(struct amd_pmc_dev *dev) amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low, dev->s2d_msg_id, true); amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi, dev->s2d_msg_id, true); + if (!phys_addr_hi && !phys_addr_low) { + dev_err(dev->dev, "amd_pmc: STB is not enabled on the system; disable enable_stb or contact system vendor\n"); + return -EINVAL; + } + stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low); /* Clear msg_port for other SMU operation */