[PATCH 0/6] powerpc: use lock guards for mutex Set 1

[PATCH 0/6] powerpc: use lock guards for mutex Set 1

[Shrikanth Hegde]

This is an effort to make the code simpler by making use of lock
guards which were introduced in [1], which works by using __cleanup 
attributes. 

[1]: https://lkml.kernel.org/r/20230612093537.614161713%40infradead.org

This series aims mainly at simplifying code around mutex with goto
statements. If it makes sense, there are more code simplification which 
can done for preempt, rcu, spinlock as well. Even for mutex, there is
more which could be done. Even there are usecases for kfree which could
use the new __free infra. 

Copying from the cover-letter of [2], summarising the effort as below.
TL;DR:

	void foo()
	{
		mutex_lock(&lock);
		if (!cond)
			goto unlock;
		...
	unlock:
		mutex_unlock(&lock);
	}

can now be written like:

	void foo()
	{
		guard(mutex)(&lock);
		if (!cond)
			return;
		...
	}

[2]: https://lore.kernel.org/all/20230801204121.929256934@infradead.org/


Please review. Code is compile/boot tested except for powernv. 
Have kept the patches separate for easier bisect. Let me if they should
be combined into one. Commit message is same for all. 

Shrikanth Hegde (6):
  powerpc: eeh: use lock guard for mutex
  powerpc: rtas: use lock guard for mutex
  powerpc: fadump: use lock guard for mutex
  powerpc: book3s: vas: use lock guard for mutex
  powerpc: powenv: oxcl: use lock guard for mutex
  powerpc: sysdev: use lock guard for mutex

 arch/powerpc/kernel/eeh.c                   | 20 +++----
 arch/powerpc/kernel/fadump.c                |  6 +-
 arch/powerpc/kernel/rtas_flash.c            | 64 +++++++--------------
 arch/powerpc/platforms/book3s/vas-api.c     | 19 ++----
 arch/powerpc/platforms/powernv/ocxl.c       | 12 +---
 arch/powerpc/sysdev/fsl_mpic_timer_wakeup.c |  8 +--
 6 files changed, 39 insertions(+), 90 deletions(-)

-- 
2.39.3

[PATCH 1/6] powerpc: eeh: use lock guard for mutex

[Shrikanth Hegde]

use guard(mutex) for scope based resource management of mutex.
This would make the code simpler and easier to maintain.

More details on lock guards can be found at
https://lore.kernel.org/all/20230612093537.614161713@infradead.org/T/#u

Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
---
 arch/powerpc/kernel/eeh.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 83fe99861eb1..929474c0ec77 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -1208,16 +1208,16 @@ int eeh_dev_open(struct pci_dev *pdev)
 	struct eeh_dev *edev;
 	int ret = -ENODEV;
 
-	mutex_lock(&eeh_dev_mutex);
+	guard(mutex)(&eeh_dev_mutex);
 
 	/* No PCI device ? */
 	if (!pdev)
-		goto out;
+		return ret;
 
 	/* No EEH device or PE ? */
 	edev = pci_dev_to_eeh_dev(pdev);
 	if (!edev || !edev->pe)
-		goto out;
+		return ret;
 
 	/*
 	 * The PE might have been put into frozen state, but we
@@ -1227,16 +1227,12 @@ int eeh_dev_open(struct pci_dev *pdev)
 	 */
 	ret = eeh_pe_change_owner(edev->pe);
 	if (ret)
-		goto out;
+		return ret;
 
 	/* Increase PE's pass through count */
 	atomic_inc(&edev->pe->pass_dev_cnt);
-	mutex_unlock(&eeh_dev_mutex);
 
 	return 0;
-out:
-	mutex_unlock(&eeh_dev_mutex);
-	return ret;
 }
 EXPORT_SYMBOL_GPL(eeh_dev_open);
 
@@ -1252,22 +1248,20 @@ void eeh_dev_release(struct pci_dev *pdev)
 {
 	struct eeh_dev *edev;
 
-	mutex_lock(&eeh_dev_mutex);
+	guard(mutex)(&eeh_dev_mutex);
 
 	/* No PCI device ? */
 	if (!pdev)
-		goto out;
+		return;
 
 	/* No EEH device ? */
 	edev = pci_dev_to_eeh_dev(pdev);
 	if (!edev || !edev->pe || !eeh_pe_passed(edev->pe))
-		goto out;
+		return;
 
 	/* Decrease PE's pass through count */
 	WARN_ON(atomic_dec_if_positive(&edev->pe->pass_dev_cnt) < 0);
 	eeh_pe_change_owner(edev->pe);
-out:
-	mutex_unlock(&eeh_dev_mutex);
 }
 EXPORT_SYMBOL(eeh_dev_release);
 
-- 
2.39.3

[PATCH 2/6] powerpc: rtas: use lock guard for mutex

[Shrikanth Hegde]

use guard(mutex) for scope based resource management of mutex.
This would make the code simpler and easier to maintain.

More details on lock guards can be found at
https://lore.kernel.org/all/20230612093537.614161713@infradead.org/T/#u

Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
---
 arch/powerpc/kernel/rtas_flash.c | 64 ++++++++++----------------------
 1 file changed, 20 insertions(+), 44 deletions(-)

diff --git a/arch/powerpc/kernel/rtas_flash.c b/arch/powerpc/kernel/rtas_flash.c
index 5407024881e5..583dc16e9d3c 100644
--- a/arch/powerpc/kernel/rtas_flash.c
+++ b/arch/powerpc/kernel/rtas_flash.c
@@ -312,13 +312,13 @@ static ssize_t rtas_flash_write(struct file *file, const char __user *buffer,
 {
 	struct rtas_update_flash_t *const uf = &rtas_update_flash_data;
 	char *p;
-	int next_free, rc;
+	int next_free;
 	struct flash_block_list *fl;
 
-	mutex_lock(&rtas_update_flash_mutex);
+	guard(mutex)(&rtas_update_flash_mutex);
 
 	if (uf->status == FLASH_AUTH || count == 0)
-		goto out;	/* discard data */
+		return count;	/* discard data */
 
 	/* In the case that the image is not ready for flashing, the memory
 	 * allocated for the block list will be freed upon the release of the 
@@ -327,7 +327,7 @@ static ssize_t rtas_flash_write(struct file *file, const char __user *buffer,
 	if (uf->flist == NULL) {
 		uf->flist = kmem_cache_zalloc(flash_block_cache, GFP_KERNEL);
 		if (!uf->flist)
-			goto nomem;
+			return -ENOMEM;
 	}
 
 	fl = uf->flist;
@@ -338,7 +338,7 @@ static ssize_t rtas_flash_write(struct file *file, const char __user *buffer,
 		/* Need to allocate another block_list */
 		fl->next = kmem_cache_zalloc(flash_block_cache, GFP_KERNEL);
 		if (!fl->next)
-			goto nomem;
+			return -ENOMEM;
 		fl = fl->next;
 		next_free = 0;
 	}
@@ -347,25 +347,17 @@ static ssize_t rtas_flash_write(struct file *file, const char __user *buffer,
 		count = RTAS_BLK_SIZE;
 	p = kmem_cache_zalloc(flash_block_cache, GFP_KERNEL);
 	if (!p)
-		goto nomem;
+		return -ENOMEM;
 	
 	if(copy_from_user(p, buffer, count)) {
 		kmem_cache_free(flash_block_cache, p);
-		rc = -EFAULT;
-		goto error;
+		return -EFAULT;
 	}
 	fl->blocks[next_free].data = p;
 	fl->blocks[next_free].length = count;
 	fl->num_blocks++;
-out:
-	mutex_unlock(&rtas_update_flash_mutex);
-	return count;
 
-nomem:
-	rc = -ENOMEM;
-error:
-	mutex_unlock(&rtas_update_flash_mutex);
-	return rc;
+	return count;
 }
 
 /*
@@ -405,19 +397,18 @@ static ssize_t manage_flash_write(struct file *file, const char __user *buf,
 	static const char reject_str[] = "0";
 	static const char commit_str[] = "1";
 	char stkbuf[10];
-	int op, rc;
+	int op;
 
-	mutex_lock(&rtas_manage_flash_mutex);
+	guard(mutex)(&rtas_manage_flash_mutex);
 
 	if ((args_buf->status == MANAGE_AUTH) || (count == 0))
-		goto out;
+		return count;
 		
 	op = -1;
 	if (buf) {
 		if (count > 9) count = 9;
-		rc = -EFAULT;
 		if (copy_from_user (stkbuf, buf, count))
-			goto error;
+			return -EFAULT;
 		if (strncmp(stkbuf, reject_str, strlen(reject_str)) == 0) 
 			op = RTAS_REJECT_TMP_IMG;
 		else if (strncmp(stkbuf, commit_str, strlen(commit_str)) == 0) 
@@ -425,18 +416,11 @@ static ssize_t manage_flash_write(struct file *file, const char __user *buf,
 	}
 	
 	if (op == -1) {   /* buf is empty, or contains invalid string */
-		rc = -EINVAL;
-		goto error;
+		return -EINVAL;
 	}
 
 	manage_flash(args_buf, op);
-out:
-	mutex_unlock(&rtas_manage_flash_mutex);
 	return count;
-
-error:
-	mutex_unlock(&rtas_manage_flash_mutex);
-	return rc;
 }
 
 /*
@@ -499,16 +483,14 @@ static ssize_t validate_flash_write(struct file *file, const char __user *buf,
 {
 	struct rtas_validate_flash_t *const args_buf =
 		&rtas_validate_flash_data;
-	int rc;
 
-	mutex_lock(&rtas_validate_flash_mutex);
+	guard(mutex)(&rtas_validate_flash_mutex);
 
 	/* We are only interested in the first 4K of the
 	 * candidate image */
 	if ((*off >= VALIDATE_BUF_SIZE) || 
 		(args_buf->status == VALIDATE_AUTH)) {
 		*off += count;
-		mutex_unlock(&rtas_validate_flash_mutex);
 		return count;
 	}
 
@@ -519,20 +501,14 @@ static ssize_t validate_flash_write(struct file *file, const char __user *buf,
 		args_buf->status = VALIDATE_INCOMPLETE;
 	}
 
-	if (!access_ok(buf, count)) {
-		rc = -EFAULT;
-		goto done;
-	}
-	if (copy_from_user(args_buf->buf + *off, buf, count)) {
-		rc = -EFAULT;
-		goto done;
-	}
+	if (!access_ok(buf, count))
+		return -EFAULT;
+
+	if (copy_from_user(args_buf->buf + *off, buf, count))
+		return -EFAULT;
 
 	*off += count;
-	rc = count;
-done:
-	mutex_unlock(&rtas_validate_flash_mutex);
-	return rc;
+	return count;
 }
 
 static int validate_flash_release(struct inode *inode, struct file *file)
-- 
2.39.3

[PATCH 3/6] powerpc: fadump: use lock guard for mutex

[Shrikanth Hegde]

use guard(mutex) for scope based resource management of mutex.
This would make the code simpler and easier to maintain.

More details on lock guards can be found at
https://lore.kernel.org/all/20230612093537.614161713@infradead.org/T/#u

Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
---
 arch/powerpc/kernel/fadump.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
index 4b371c738213..5fd2c546fd8c 100644
--- a/arch/powerpc/kernel/fadump.c
+++ b/arch/powerpc/kernel/fadump.c
@@ -1374,15 +1374,13 @@ static void fadump_free_elfcorehdr_buf(void)
 
 static void fadump_invalidate_release_mem(void)
 {
-	mutex_lock(&fadump_mutex);
+	guard(mutex)(&fadump_mutex);
+
 	if (!fw_dump.dump_active) {
-		mutex_unlock(&fadump_mutex);
 		return;
 	}
 
 	fadump_cleanup();
-	mutex_unlock(&fadump_mutex);
-
 	fadump_free_elfcorehdr_buf();
 	fadump_release_memory(fw_dump.boot_mem_top, memblock_end_of_DRAM());
 	fadump_free_cpu_notes_buf();
-- 
2.39.3

Re: [PATCH 3/6] powerpc: fadump: use lock guard for mutex

[Peter Zijlstra]

On Fri, Mar 14, 2025 at 11:15:41AM +0530, Shrikanth Hegde wrote:
> use guard(mutex) for scope based resource management of mutex.
> This would make the code simpler and easier to maintain.
> 
> More details on lock guards can be found at
> https://lore.kernel.org/all/20230612093537.614161713@infradead.org/T/#u
> 
> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
> ---
>  arch/powerpc/kernel/fadump.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
> index 4b371c738213..5fd2c546fd8c 100644
> --- a/arch/powerpc/kernel/fadump.c
> +++ b/arch/powerpc/kernel/fadump.c
> @@ -1374,15 +1374,13 @@ static void fadump_free_elfcorehdr_buf(void)
>  
>  static void fadump_invalidate_release_mem(void)
>  {
> -	mutex_lock(&fadump_mutex);
> +	guard(mutex)(&fadump_mutex);
> +
>  	if (!fw_dump.dump_active) {
> -		mutex_unlock(&fadump_mutex);
>  		return;
>  	}
>  
>  	fadump_cleanup();
> -	mutex_unlock(&fadump_mutex);
> -

This will result in running the below functions with the mutex held.

>  	fadump_free_elfcorehdr_buf();
>  	fadump_release_memory(fw_dump.boot_mem_top, memblock_end_of_DRAM());
>  	fadump_free_cpu_notes_buf();


The equivalent transformation for the above code would look like:

static void fadump_invalidate_release_mem(void)
{
	scoped_guard (mutex, &fadump_mutex) {
		if (!fw_dump.dump_active)
			return;

		fadump_cleanup();
	}

	fadump_free_elfcorehdr_buf();
	...

Re: [PATCH 3/6] powerpc: fadump: use lock guard for mutex

[Shrikanth Hegde]

On 3/14/25 13:52, Peter Zijlstra wrote:

Thanks Peter for taking a look.

> On Fri, Mar 14, 2025 at 11:15:41AM +0530, Shrikanth Hegde wrote:
>> use guard(mutex) for scope based resource management of mutex.
>> This would make the code simpler and easier to maintain.
>>
>> More details on lock guards can be found at
>> https://lore.kernel.org/all/20230612093537.614161713@infradead.org/T/#u
>>
>> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
>> ---
>>   arch/powerpc/kernel/fadump.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
>> index 4b371c738213..5fd2c546fd8c 100644
>> --- a/arch/powerpc/kernel/fadump.c
>> +++ b/arch/powerpc/kernel/fadump.c
>> @@ -1374,15 +1374,13 @@ static void fadump_free_elfcorehdr_buf(void)
>>   
>>   static void fadump_invalidate_release_mem(void)
>>   {
>> -	mutex_lock(&fadump_mutex);
>> +	guard(mutex)(&fadump_mutex);
>> +
>>   	if (!fw_dump.dump_active) {
>> -		mutex_unlock(&fadump_mutex);
>>   		return;
>>   	}
>>   
>>   	fadump_cleanup();
>> -	mutex_unlock(&fadump_mutex);
>> -
> 
> This will result in running the below functions with the mutex held.
> 
>>   	fadump_free_elfcorehdr_buf();
>>   	fadump_release_memory(fw_dump.boot_mem_top, memblock_end_of_DRAM());
>>   	fadump_free_cpu_notes_buf();
> 

Ok. Got it, since the variable is still in scope unlock wont be called.
So, will use scoped_guard as you suggested below in v2.

> 
> The equivalent transformation for the above code would look like:
> 
> static void fadump_invalidate_release_mem(void)
> {
> 	scoped_guard (mutex, &fadump_mutex) {
> 		if (!fw_dump.dump_active)
> 			return;
> 
> 		fadump_cleanup();
> 	}
> 
> 	fadump_free_elfcorehdr_buf();
> 	...

ok.

[PATCH 4/6] powerpc: book3s: vas: use lock guard for mutex

[Shrikanth Hegde]

use guard(mutex) for scope based resource management of mutex.
This would make the code simpler and easier to maintain.

More details on lock guards can be found at
https://lore.kernel.org/all/20230612093537.614161713@infradead.org/T/#u

There is also an example of using scoped_guard. 

Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
---
 arch/powerpc/platforms/book3s/vas-api.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/platforms/book3s/vas-api.c b/arch/powerpc/platforms/book3s/vas-api.c
index 0b6365d85d11..eb1a97271afb 100644
--- a/arch/powerpc/platforms/book3s/vas-api.c
+++ b/arch/powerpc/platforms/book3s/vas-api.c
@@ -425,7 +425,7 @@ static vm_fault_t vas_mmap_fault(struct vm_fault *vmf)
 		return VM_FAULT_SIGBUS;
 	}
 
-	mutex_lock(&txwin->task_ref.mmap_mutex);
+	guard(mutex)(&txwin->task_ref.mmap_mutex);
 	/*
 	 * The window may be inactive due to lost credit (Ex: core
 	 * removal with DLPAR). If the window is active again when
@@ -437,11 +437,9 @@ static vm_fault_t vas_mmap_fault(struct vm_fault *vmf)
 		if (paste_addr) {
 			fault = vmf_insert_pfn(vma, vma->vm_start,
 					(paste_addr >> PAGE_SHIFT));
-			mutex_unlock(&txwin->task_ref.mmap_mutex);
 			return fault;
 		}
 	}
-	mutex_unlock(&txwin->task_ref.mmap_mutex);
 
 	/*
 	 * Received this fault due to closing the actual window.
@@ -494,9 +492,8 @@ static void vas_mmap_close(struct vm_area_struct *vma)
 		return;
 	}
 
-	mutex_lock(&txwin->task_ref.mmap_mutex);
-	txwin->task_ref.vma = NULL;
-	mutex_unlock(&txwin->task_ref.mmap_mutex);
+	scoped_guard(mutex, &txwin->task_ref.mmap_mutex)
+		txwin->task_ref.vma = NULL;
 }
 
 static const struct vm_operations_struct vas_vm_ops = {
@@ -543,18 +540,16 @@ static int coproc_mmap(struct file *fp, struct vm_area_struct *vma)
 	 * close/open event and allows mmap() only when the window is
 	 * active.
 	 */
-	mutex_lock(&txwin->task_ref.mmap_mutex);
+	guard(mutex)(&txwin->task_ref.mmap_mutex);
 	if (txwin->status != VAS_WIN_ACTIVE) {
 		pr_err("Window is not active\n");
-		rc = -EACCES;
-		goto out;
+		return -EACCES;
 	}
 
 	paste_addr = cp_inst->coproc->vops->paste_addr(txwin);
 	if (!paste_addr) {
 		pr_err("Window paste address failed\n");
-		rc = -EINVAL;
-		goto out;
+		return -EINVAL;
 	}
 
 	pfn = paste_addr >> PAGE_SHIFT;
@@ -574,8 +569,6 @@ static int coproc_mmap(struct file *fp, struct vm_area_struct *vma)
 	txwin->task_ref.vma = vma;
 	vma->vm_ops = &vas_vm_ops;
 
-out:
-	mutex_unlock(&txwin->task_ref.mmap_mutex);
 	return rc;
 }
 
-- 
2.39.3

Re: [PATCH 4/6] powerpc: book3s: vas: use lock guard for mutex

[Peter Zijlstra]

On Fri, Mar 14, 2025 at 11:15:42AM +0530, Shrikanth Hegde wrote:
> use guard(mutex) for scope based resource management of mutex.
> This would make the code simpler and easier to maintain.
> 
> More details on lock guards can be found at
> https://lore.kernel.org/all/20230612093537.614161713@infradead.org/T/#u
> 
> There is also an example of using scoped_guard. 
> 
> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
> ---
>  arch/powerpc/platforms/book3s/vas-api.c | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/book3s/vas-api.c b/arch/powerpc/platforms/book3s/vas-api.c
> index 0b6365d85d11..eb1a97271afb 100644
> --- a/arch/powerpc/platforms/book3s/vas-api.c
> +++ b/arch/powerpc/platforms/book3s/vas-api.c
> @@ -425,7 +425,7 @@ static vm_fault_t vas_mmap_fault(struct vm_fault *vmf)
>  		return VM_FAULT_SIGBUS;
>  	}
>  
> -	mutex_lock(&txwin->task_ref.mmap_mutex);
> +	guard(mutex)(&txwin->task_ref.mmap_mutex);
>  	/*
>  	 * The window may be inactive due to lost credit (Ex: core
>  	 * removal with DLPAR). If the window is active again when
> @@ -437,11 +437,9 @@ static vm_fault_t vas_mmap_fault(struct vm_fault *vmf)
>  		if (paste_addr) {
>  			fault = vmf_insert_pfn(vma, vma->vm_start,
>  					(paste_addr >> PAGE_SHIFT));
> -			mutex_unlock(&txwin->task_ref.mmap_mutex);
>  			return fault;
>  		}
>  	}
> -	mutex_unlock(&txwin->task_ref.mmap_mutex);

I had to open up this file to check, but this seems incorrect since you
now also run do_fail_paste() with the lock held, where previously you
did not.


>  	/*
>  	 * Received this fault due to closing the actual window.

Re: [PATCH 4/6] powerpc: book3s: vas: use lock guard for mutex

[Shrikanth Hegde]

On 3/14/25 13:55, Peter Zijlstra wrote:
> On Fri, Mar 14, 2025 at 11:15:42AM +0530, Shrikanth Hegde wrote:
>> use guard(mutex) for scope based resource management of mutex.
>> This would make the code simpler and easier to maintain.
>>
>> More details on lock guards can be found at
>> https://lore.kernel.org/all/20230612093537.614161713@infradead.org/T/#u
>>
>> There is also an example of using scoped_guard.
>>
>> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
>> ---
>>   arch/powerpc/platforms/book3s/vas-api.c | 19 ++++++-------------
>>   1 file changed, 6 insertions(+), 13 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/book3s/vas-api.c b/arch/powerpc/platforms/book3s/vas-api.c
>> index 0b6365d85d11..eb1a97271afb 100644
>> --- a/arch/powerpc/platforms/book3s/vas-api.c
>> +++ b/arch/powerpc/platforms/book3s/vas-api.c
>> @@ -425,7 +425,7 @@ static vm_fault_t vas_mmap_fault(struct vm_fault *vmf)
>>   		return VM_FAULT_SIGBUS;
>>   	}
>>   
>> -	mutex_lock(&txwin->task_ref.mmap_mutex);
>> +	guard(mutex)(&txwin->task_ref.mmap_mutex);
>>   	/*
>>   	 * The window may be inactive due to lost credit (Ex: core
>>   	 * removal with DLPAR). If the window is active again when
>> @@ -437,11 +437,9 @@ static vm_fault_t vas_mmap_fault(struct vm_fault *vmf)
>>   		if (paste_addr) {
>>   			fault = vmf_insert_pfn(vma, vma->vm_start,
>>   					(paste_addr >> PAGE_SHIFT));
>> -			mutex_unlock(&txwin->task_ref.mmap_mutex);
>>   			return fault;
>>   		}
>>   	}
>> -	mutex_unlock(&txwin->task_ref.mmap_mutex);
> 
> I had to open up this file to check, but this seems incorrect since you
> now also run do_fail_paste() with the lock held, where previously you
> did not.
> 

Yes. Got it. let me use scoped_guard for it as well. There is get_user 
and other things in the fail parse and having it with mutex will not be 
good.

I went through the rest of the patches too. It is mostly return after 
mutex.

Only in Patch 5/6 there is additional debug statement. Let me put a 
comment there.

> 
>>   	/*
>>   	 * Received this fault due to closing the actual window.

[PATCH 5/6] powerpc: powenv: oxcl: use lock guard for mutex

[Shrikanth Hegde]

use guard(mutex) for scope based resource management of mutex.
This would make the code simpler and easier to maintain.

More details on lock guards can be found at
https://lore.kernel.org/all/20230612093537.614161713@infradead.org/T/#u

Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
---
 arch/powerpc/platforms/powernv/ocxl.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/ocxl.c b/arch/powerpc/platforms/powernv/ocxl.c
index 64a9c7125c29..f8139948348e 100644
--- a/arch/powerpc/platforms/powernv/ocxl.c
+++ b/arch/powerpc/platforms/powernv/ocxl.c
@@ -172,12 +172,11 @@ static void pnv_ocxl_fixup_actag(struct pci_dev *dev)
 	if (phb->type != PNV_PHB_NPU_OCAPI)
 		return;
 
-	mutex_lock(&links_list_lock);
+	guard(mutex)(&links_list_lock);
 
 	link = find_link(dev);
 	if (!link) {
 		dev_warn(&dev->dev, "couldn't update actag information\n");
-		mutex_unlock(&links_list_lock);
 		return;
 	}
 
@@ -206,7 +205,6 @@ static void pnv_ocxl_fixup_actag(struct pci_dev *dev)
 	dev_dbg(&dev->dev, "total actags for function: %d\n",
 		link->fn_desired_actags[PCI_FUNC(dev->devfn)]);
 
-	mutex_unlock(&links_list_lock);
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, pnv_ocxl_fixup_actag);
 
@@ -253,12 +251,11 @@ int pnv_ocxl_get_actag(struct pci_dev *dev, u16 *base, u16 *enabled,
 {
 	struct npu_link *link;
 
-	mutex_lock(&links_list_lock);
+	guard(mutex)(&links_list_lock);
 
 	link = find_link(dev);
 	if (!link) {
 		dev_err(&dev->dev, "actag information not found\n");
-		mutex_unlock(&links_list_lock);
 		return -ENODEV;
 	}
 	/*
@@ -274,7 +271,6 @@ int pnv_ocxl_get_actag(struct pci_dev *dev, u16 *base, u16 *enabled,
 	*enabled   = link->fn_actags[PCI_FUNC(dev->devfn)].count;
 	*supported = link->fn_desired_actags[PCI_FUNC(dev->devfn)];
 
-	mutex_unlock(&links_list_lock);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(pnv_ocxl_get_actag);
@@ -293,12 +289,11 @@ int pnv_ocxl_get_pasid_count(struct pci_dev *dev, int *count)
 	 *
 	 * We only support one AFU-carrying function for now.
 	 */
-	mutex_lock(&links_list_lock);
+	guard(mutex)(&links_list_lock);
 
 	link = find_link(dev);
 	if (!link) {
 		dev_err(&dev->dev, "actag information not found\n");
-		mutex_unlock(&links_list_lock);
 		return -ENODEV;
 	}
 
@@ -309,7 +304,6 @@ int pnv_ocxl_get_pasid_count(struct pci_dev *dev, int *count)
 			break;
 		}
 
-	mutex_unlock(&links_list_lock);
 	dev_dbg(&dev->dev, "%d PASIDs available for function\n",
 		rc ? 0 : *count);
 	return rc;
-- 
2.39.3

Re: [PATCH 5/6] powerpc: powenv: oxcl: use lock guard for mutex

[Andrew Donnellan]

On Fri, 2025-03-14 at 11:15 +0530, Shrikanth Hegde wrote:
> use guard(mutex) for scope based resource management of mutex.
> This would make the code simpler and easier to maintain.
> 
> More details on lock guards can be found at
> https://lore.kernel.org/all/20230612093537.614161713@infradead.org/T/#u
> 
> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>

The subject line of this patch misspells powernv and ocxl.

Otherwise this looks like a nice cleanup.

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited

Re: [PATCH 5/6] powerpc: powenv: oxcl: use lock guard for mutex

[Shrikanth Hegde]

On 3/14/25 11:36, Andrew Donnellan wrote:
> On Fri, 2025-03-14 at 11:15 +0530, Shrikanth Hegde wrote:
>> use guard(mutex) for scope based resource management of mutex.
>> This would make the code simpler and easier to maintain.
>>
>> More details on lock guards can be found at
>> https://lore.kernel.org/all/20230612093537.614161713@infradead.org/T/#u
>>
>> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
> 
> The subject line of this patch misspells powernv and ocxl.

Ah. my bad. will correct it.

> 
> Otherwise this looks like a nice cleanup.

Thanks.

Re: [PATCH 5/6] powerpc: powenv: oxcl: use lock guard for mutex

[Shrikanth Hegde]

On 3/14/25 11:15, Shrikanth Hegde wrote:
> use guard(mutex) for scope based resource management of mutex.
> This would make the code simpler and easier to maintain.
> 
> More details on lock guards can be found at
> https://lore.kernel.org/all/20230612093537.614161713@infradead.org/T/#u
> 
> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
> ---
>   arch/powerpc/platforms/powernv/ocxl.c | 12 +++---------
>   1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/powernv/ocxl.c b/arch/powerpc/platforms/powernv/ocxl.c
> index 64a9c7125c29..f8139948348e 100644
> --- a/arch/powerpc/platforms/powernv/ocxl.c
> +++ b/arch/powerpc/platforms/powernv/ocxl.c
> @@ -172,12 +172,11 @@ static void pnv_ocxl_fixup_actag(struct pci_dev *dev)
>   	if (phb->type != PNV_PHB_NPU_OCAPI)
>   		return;
>   
> -	mutex_lock(&links_list_lock);
> +	guard(mutex)(&links_list_lock);
>   
>   	link = find_link(dev);
>   	if (!link) {
>   		dev_warn(&dev->dev, "couldn't update actag information\n");
> -		mutex_unlock(&links_list_lock);
>   		return;
>   	}
>   
> @@ -206,7 +205,6 @@ static void pnv_ocxl_fixup_actag(struct pci_dev *dev)
>   	dev_dbg(&dev->dev, "total actags for function: %d\n",
>   		link->fn_desired_actags[PCI_FUNC(dev->devfn)]);
>   
> -	mutex_unlock(&links_list_lock);
>   }
>   DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, pnv_ocxl_fixup_actag);
>   
> @@ -253,12 +251,11 @@ int pnv_ocxl_get_actag(struct pci_dev *dev, u16 *base, u16 *enabled,
>   {
>   	struct npu_link *link;
>   
> -	mutex_lock(&links_list_lock);
> +	guard(mutex)(&links_list_lock);
>   
>   	link = find_link(dev);
>   	if (!link) {
>   		dev_err(&dev->dev, "actag information not found\n");
> -		mutex_unlock(&links_list_lock);
>   		return -ENODEV;
>   	}
>   	/*
> @@ -274,7 +271,6 @@ int pnv_ocxl_get_actag(struct pci_dev *dev, u16 *base, u16 *enabled,
>   	*enabled   = link->fn_actags[PCI_FUNC(dev->devfn)].count;
>   	*supported = link->fn_desired_actags[PCI_FUNC(dev->devfn)];
>   
> -	mutex_unlock(&links_list_lock);
>   	return 0;
>   }
>   EXPORT_SYMBOL_GPL(pnv_ocxl_get_actag);
> @@ -293,12 +289,11 @@ int pnv_ocxl_get_pasid_count(struct pci_dev *dev, int *count)
>   	 *
>   	 * We only support one AFU-carrying function for now.
>   	 */
> -	mutex_lock(&links_list_lock);
> +	guard(mutex)(&links_list_lock);
>   
>   	link = find_link(dev);
>   	if (!link) {
>   		dev_err(&dev->dev, "actag information not found\n");
> -		mutex_unlock(&links_list_lock);
>   		return -ENODEV;
>   	}
>   
> @@ -309,7 +304,6 @@ int pnv_ocxl_get_pasid_count(struct pci_dev *dev, int *count)
>   			break;
>   		}
>   
> -	mutex_unlock(&links_list_lock);

Hi. Andrew,

After this change below dev_dbg will be called with mutex held still. Is 
that a concern? I don't see the mutex being used in that path.

Since using scoped_guard cause more code churn here, I would prefer not 
use it.

>   	dev_dbg(&dev->dev, "%d PASIDs available for function\n",
>   		rc ? 0 : *count);
>   	return rc;

Re: [PATCH 5/6] powerpc: powenv: oxcl: use lock guard for mutex

[Shrikanth Hegde]

On 3/14/25 15:00, Shrikanth Hegde wrote:
> 
> 
> On 3/14/25 11:15, Shrikanth Hegde wrote:
>> use guard(mutex) for scope based resource management of mutex.
>> This would make the code simpler and easier to maintain.
>>
>> More details on lock guards can be found at
>> https://lore.kernel.org/all/20230612093537.614161713@infradead.org/T/#u
>>
>> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
>> ---
>>   arch/powerpc/platforms/powernv/ocxl.c | 12 +++---------
>>   1 file changed, 3 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/powernv/ocxl.c b/arch/powerpc/ 
>> platforms/powernv/ocxl.c
>> index 64a9c7125c29..f8139948348e 100644
>> --- a/arch/powerpc/platforms/powernv/ocxl.c
>> +++ b/arch/powerpc/platforms/powernv/ocxl.c
>> @@ -172,12 +172,11 @@ static void pnv_ocxl_fixup_actag(struct pci_dev 
>> *dev)
>>       if (phb->type != PNV_PHB_NPU_OCAPI)
>>           return;
>> -    mutex_lock(&links_list_lock);
>> +    guard(mutex)(&links_list_lock);
>>       link = find_link(dev);
>>       if (!link) {
>>           dev_warn(&dev->dev, "couldn't update actag information\n");
>> -        mutex_unlock(&links_list_lock);
>>           return;
>>       }
>> @@ -206,7 +205,6 @@ static void pnv_ocxl_fixup_actag(struct pci_dev *dev)
>>       dev_dbg(&dev->dev, "total actags for function: %d\n",
>>           link->fn_desired_actags[PCI_FUNC(dev->devfn)]);
>> -    mutex_unlock(&links_list_lock);
>>   }
>>   DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, pnv_ocxl_fixup_actag);
>> @@ -253,12 +251,11 @@ int pnv_ocxl_get_actag(struct pci_dev *dev, u16 
>> *base, u16 *enabled,
>>   {
>>       struct npu_link *link;
>> -    mutex_lock(&links_list_lock);
>> +    guard(mutex)(&links_list_lock);
>>       link = find_link(dev);
>>       if (!link) {
>>           dev_err(&dev->dev, "actag information not found\n");
>> -        mutex_unlock(&links_list_lock);
>>           return -ENODEV;
>>       }
>>       /*
>> @@ -274,7 +271,6 @@ int pnv_ocxl_get_actag(struct pci_dev *dev, u16 
>> *base, u16 *enabled,
>>       *enabled   = link->fn_actags[PCI_FUNC(dev->devfn)].count;
>>       *supported = link->fn_desired_actags[PCI_FUNC(dev->devfn)];
>> -    mutex_unlock(&links_list_lock);
>>       return 0;
>>   }
>>   EXPORT_SYMBOL_GPL(pnv_ocxl_get_actag);
>> @@ -293,12 +289,11 @@ int pnv_ocxl_get_pasid_count(struct pci_dev 
>> *dev, int *count)
>>        *
>>        * We only support one AFU-carrying function for now.
>>        */
>> -    mutex_lock(&links_list_lock);
>> +    guard(mutex)(&links_list_lock);
>>       link = find_link(dev);
>>       if (!link) {
>>           dev_err(&dev->dev, "actag information not found\n");
>> -        mutex_unlock(&links_list_lock);
>>           return -ENODEV;
>>       }
>> @@ -309,7 +304,6 @@ int pnv_ocxl_get_pasid_count(struct pci_dev *dev, 
>> int *count)
>>               break;
>>           }
>> -    mutex_unlock(&links_list_lock);
> 
> Hi. Andrew,
> 
> After this change below dev_dbg will be called with mutex held still. Is 
> that a concern? I don't see the mutex being used in that path.
> 
> Since using scoped_guard cause more code churn here, I would prefer not 
> use it.

I see current code in pnv_ocxl_fixup_actag calls dev_dbg with mutex 
held. So likely not a concern of using just guard in 
pnv_ocxl_get_pasid_count as well.

Assuming that, let me send out v2 with corrected commit subject. :w

> 
>>       dev_dbg(&dev->dev, "%d PASIDs available for function\n",
>>           rc ? 0 : *count);
>>       return rc;
> 

Re: [PATCH 5/6] powerpc: powenv: oxcl: use lock guard for mutex

[Andrew Donnellan]

On Fri, 2025-03-14 at 15:00 +0530, Shrikanth Hegde wrote:
> 
> Hi. Andrew,
> 
> After this change below dev_dbg will be called with mutex held still.
> Is 
> that a concern? I don't see the mutex being used in that path.
> 
> Since using scoped_guard cause more code churn here, I would prefer
> not 
> use it.

I think this is fine.

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited

[PATCH 6/6] powerpc: sysdev: use lock guard for mutex

[Shrikanth Hegde]

use guard(mutex) for scope based resource management of mutex
This would make the code simpler and easier to maintain.

More details on lock guards can be found at
https://lore.kernel.org/all/20230612093537.614161713@infradead.org/T/#u

Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
---
 arch/powerpc/sysdev/fsl_mpic_timer_wakeup.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/arch/powerpc/sysdev/fsl_mpic_timer_wakeup.c b/arch/powerpc/sysdev/fsl_mpic_timer_wakeup.c
index ce6c739c51e5..bbfc7c39b957 100644
--- a/arch/powerpc/sysdev/fsl_mpic_timer_wakeup.c
+++ b/arch/powerpc/sysdev/fsl_mpic_timer_wakeup.c
@@ -75,7 +75,7 @@ static ssize_t fsl_timer_wakeup_store(struct device *dev,
 	if (kstrtoll(buf, 0, &interval))
 		return -EINVAL;
 
-	mutex_lock(&sysfs_lock);
+	guard(mutex)(&sysfs_lock);
 
 	if (fsl_wakeup->timer) {
 		disable_irq_wake(fsl_wakeup->timer->irq);
@@ -84,14 +84,12 @@ static ssize_t fsl_timer_wakeup_store(struct device *dev,
 	}
 
 	if (!interval) {
-		mutex_unlock(&sysfs_lock);
 		return count;
 	}
 
 	fsl_wakeup->timer = mpic_request_timer(fsl_mpic_timer_irq,
 						fsl_wakeup, interval);
 	if (!fsl_wakeup->timer) {
-		mutex_unlock(&sysfs_lock);
 		return -EINVAL;
 	}
 
@@ -99,15 +97,11 @@ static ssize_t fsl_timer_wakeup_store(struct device *dev,
 	if (ret) {
 		mpic_free_timer(fsl_wakeup->timer);
 		fsl_wakeup->timer = NULL;
-		mutex_unlock(&sysfs_lock);
-
 		return ret;
 	}
 
 	mpic_start_timer(fsl_wakeup->timer);
 
-	mutex_unlock(&sysfs_lock);
-
 	return count;
 }
 
-- 
2.39.3