[PATCH] platform/x86/amd: pmc: Use release_mem_region() to undo request_mem_region_muxed()

[PATCH] platform/x86/amd: pmc: Use release_mem_region() to undo request_mem_region_muxed()

[Hans de Goede]

Muxed (mem) regions will wait in request_mem_region_muxed() if the region
is busy (in use by another consumer) during the call.

In order to wake-up possibly waiting other consumers of the region,
it must be released by a release_mem_region() call, which will actually
wake up any waiters.

release_mem_region() also frees the resource created by
request_mem_region_muxed(), avoiding the need for the unmatched kfree().

Cc: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Mario, can you ask one of the reporters with a machine which needs the quirk
to test this ?
---
 drivers/platform/x86/amd/pmc-quirks.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/amd/pmc-quirks.c b/drivers/platform/x86/amd/pmc-quirks.c
index 362e7c0097d7..ad702463a65d 100644
--- a/drivers/platform/x86/amd/pmc-quirks.c
+++ b/drivers/platform/x86/amd/pmc-quirks.c
@@ -11,7 +11,6 @@
 #include <linux/dmi.h>
 #include <linux/io.h>
 #include <linux/ioport.h>
-#include <linux/slab.h>
 
 #include "pmc.h"
 
@@ -135,12 +134,10 @@ static const struct dmi_system_id fwbug_list[] = {
  */
 static void amd_pmc_skip_nvme_smi_handler(u32 s2idle_bug_mmio)
 {
-	struct resource *res;
 	void __iomem *addr;
 	u8 val;
 
-	res = request_mem_region_muxed(s2idle_bug_mmio, 1, "amd_pmc_pm80");
-	if (!res)
+	if (!request_mem_region_muxed(s2idle_bug_mmio, 1, "amd_pmc_pm80"))
 		return;
 
 	addr = ioremap(s2idle_bug_mmio, 1);
@@ -152,8 +149,7 @@ static void amd_pmc_skip_nvme_smi_handler(u32 s2idle_bug_mmio)
 
 	iounmap(addr);
 cleanup_resource:
-	release_resource(res);
-	kfree(res);
+	release_mem_region(s2idle_bug_mmio, 1);
 }
 
 void amd_pmc_process_restore_quirks(struct amd_pmc_dev *dev)
-- 
2.41.0

Re: [PATCH] platform/x86/amd: pmc: Use release_mem_region() to undo request_mem_region_muxed()

[Andy Shevchenko]

On Tue, Jul 11, 2023 at 11:59:20AM +0200, Hans de Goede wrote:
> Muxed (mem) regions will wait in request_mem_region_muxed() if the region
> is busy (in use by another consumer) during the call.
> 
> In order to wake-up possibly waiting other consumers of the region,
> it must be released by a release_mem_region() call, which will actually
> wake up any waiters.
> 
> release_mem_region() also frees the resource created by
> request_mem_region_muxed(), avoiding the need for the unmatched kfree().

Seems reasonable to me.
Do we need to have a Fixes tag?
Reviewed-by: Andy Shevchenko <andy@kernel.org>

> Cc: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Mario, can you ask one of the reporters with a machine which needs the quirk
> to test this ?
> ---
>  drivers/platform/x86/amd/pmc-quirks.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc-quirks.c b/drivers/platform/x86/amd/pmc-quirks.c
> index 362e7c0097d7..ad702463a65d 100644
> --- a/drivers/platform/x86/amd/pmc-quirks.c
> +++ b/drivers/platform/x86/amd/pmc-quirks.c
> @@ -11,7 +11,6 @@
>  #include <linux/dmi.h>
>  #include <linux/io.h>
>  #include <linux/ioport.h>
> -#include <linux/slab.h>
>  
>  #include "pmc.h"
>  
> @@ -135,12 +134,10 @@ static const struct dmi_system_id fwbug_list[] = {
>   */
>  static void amd_pmc_skip_nvme_smi_handler(u32 s2idle_bug_mmio)
>  {
> -	struct resource *res;
>  	void __iomem *addr;
>  	u8 val;
>  
> -	res = request_mem_region_muxed(s2idle_bug_mmio, 1, "amd_pmc_pm80");
> -	if (!res)
> +	if (!request_mem_region_muxed(s2idle_bug_mmio, 1, "amd_pmc_pm80"))
>  		return;
>  
>  	addr = ioremap(s2idle_bug_mmio, 1);
> @@ -152,8 +149,7 @@ static void amd_pmc_skip_nvme_smi_handler(u32 s2idle_bug_mmio)
>  
>  	iounmap(addr);
>  cleanup_resource:
> -	release_resource(res);
> -	kfree(res);
> +	release_mem_region(s2idle_bug_mmio, 1);
>  }
>  
>  void amd_pmc_process_restore_quirks(struct amd_pmc_dev *dev)
> -- 
> 2.41.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] platform/x86/amd: pmc: Use release_mem_region() to undo request_mem_region_muxed()

[Mario Limonciello]

On 7/11/23 06:13, Andy Shevchenko wrote:
> On Tue, Jul 11, 2023 at 11:59:20AM +0200, Hans de Goede wrote:
>> Muxed (mem) regions will wait in request_mem_region_muxed() if the region
>> is busy (in use by another consumer) during the call.
>>
>> In order to wake-up possibly waiting other consumers of the region,
>> it must be released by a release_mem_region() call, which will actually
>> wake up any waiters.
>>
>> release_mem_region() also frees the resource created by
>> request_mem_region_muxed(), avoiding the need for the unmatched kfree().
> 
> Seems reasonable to me.
> Do we need to have a Fixes tag?
> Reviewed-by: Andy Shevchenko <andy@kernel.org>

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

I've gotten a positive test report with this as well, thanks!

> 
>> Cc: Mario Limonciello <mario.limonciello@amd.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Mario, can you ask one of the reporters with a machine which needs the quirk
>> to test this ?
>> ---
>>   drivers/platform/x86/amd/pmc-quirks.c | 8 ++------
>>   1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd/pmc-quirks.c b/drivers/platform/x86/amd/pmc-quirks.c
>> index 362e7c0097d7..ad702463a65d 100644
>> --- a/drivers/platform/x86/amd/pmc-quirks.c
>> +++ b/drivers/platform/x86/amd/pmc-quirks.c
>> @@ -11,7 +11,6 @@
>>   #include <linux/dmi.h>
>>   #include <linux/io.h>
>>   #include <linux/ioport.h>
>> -#include <linux/slab.h>
>>   
>>   #include "pmc.h"
>>   
>> @@ -135,12 +134,10 @@ static const struct dmi_system_id fwbug_list[] = {
>>    */
>>   static void amd_pmc_skip_nvme_smi_handler(u32 s2idle_bug_mmio)
>>   {
>> -	struct resource *res;
>>   	void __iomem *addr;
>>   	u8 val;
>>   
>> -	res = request_mem_region_muxed(s2idle_bug_mmio, 1, "amd_pmc_pm80");
>> -	if (!res)
>> +	if (!request_mem_region_muxed(s2idle_bug_mmio, 1, "amd_pmc_pm80"))
>>   		return;
>>   
>>   	addr = ioremap(s2idle_bug_mmio, 1);
>> @@ -152,8 +149,7 @@ static void amd_pmc_skip_nvme_smi_handler(u32 s2idle_bug_mmio)
>>   
>>   	iounmap(addr);
>>   cleanup_resource:
>> -	release_resource(res);
>> -	kfree(res);
>> +	release_mem_region(s2idle_bug_mmio, 1);
>>   }
>>   
>>   void amd_pmc_process_restore_quirks(struct amd_pmc_dev *dev)
>> -- 
>> 2.41.0
>>
> 

Re: [PATCH] platform/x86/amd: pmc: Use release_mem_region() to undo request_mem_region_muxed()

[Hans de Goede]

Hi,

On 7/11/23 11:59, Hans de Goede wrote:
> Muxed (mem) regions will wait in request_mem_region_muxed() if the region
> is busy (in use by another consumer) during the call.
> 
> In order to wake-up possibly waiting other consumers of the region,
> it must be released by a release_mem_region() call, which will actually
> wake up any waiters.
> 
> release_mem_region() also frees the resource created by
> request_mem_region_muxed(), avoiding the need for the unmatched kfree().
> 
> Cc: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Mario, Andy, thank you for the reviews, I've applied this patch
to my fixes branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=fixes

I will include this patch in my next fixes pull-req to Linus
for the current kernel development cycle.

Regards,

Hans




> ---
> Mario, can you ask one of the reporters with a machine which needs the quirk
> to test this ?
> ---
>  drivers/platform/x86/amd/pmc-quirks.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc-quirks.c b/drivers/platform/x86/amd/pmc-quirks.c
> index 362e7c0097d7..ad702463a65d 100644
> --- a/drivers/platform/x86/amd/pmc-quirks.c
> +++ b/drivers/platform/x86/amd/pmc-quirks.c
> @@ -11,7 +11,6 @@
>  #include <linux/dmi.h>
>  #include <linux/io.h>
>  #include <linux/ioport.h>
> -#include <linux/slab.h>
>  
>  #include "pmc.h"
>  
> @@ -135,12 +134,10 @@ static const struct dmi_system_id fwbug_list[] = {
>   */
>  static void amd_pmc_skip_nvme_smi_handler(u32 s2idle_bug_mmio)
>  {
> -	struct resource *res;
>  	void __iomem *addr;
>  	u8 val;
>  
> -	res = request_mem_region_muxed(s2idle_bug_mmio, 1, "amd_pmc_pm80");
> -	if (!res)
> +	if (!request_mem_region_muxed(s2idle_bug_mmio, 1, "amd_pmc_pm80"))
>  		return;
>  
>  	addr = ioremap(s2idle_bug_mmio, 1);
> @@ -152,8 +149,7 @@ static void amd_pmc_skip_nvme_smi_handler(u32 s2idle_bug_mmio)
>  
>  	iounmap(addr);
>  cleanup_resource:
> -	release_resource(res);
> -	kfree(res);
> +	release_mem_region(s2idle_bug_mmio, 1);
>  }
>  
>  void amd_pmc_process_restore_quirks(struct amd_pmc_dev *dev)