<div dir="ltr"><div dir="ltr">On Sat, Aug 3, 2024 at 9:54 AM Dan Carpenter <<a href="mailto:dan.carpenter@linaro.org">dan.carpenter@linaro.org</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Sat, Aug 03, 2024 at 05:48:14AM +0530, Riyan Dhiman wrote:<br>
> Adhere to Linux kernel coding style<br>
> <br>
> Reported by checkpatch:<br>
> <br>
> CHECK: mutex definition without comment<br>
> <br>
> Proof for comment:<br>
> <br>
> 1. The mutex is used to protect access to the 'running' list<br>
> (line 1798 tsi148_dma_list_exec function)<br>
> mutex_lock(&ctrlrl->mtx);<br>
> if (!list_empty(&ctrlr->running)) {<br>
> mutex_unlock(&ctrlr->mtx);<br>
> return -EBUSY;<br>
> }<br>
<br>
Why did you chop out the "channel = ctrlr->number;" line? That code<br>
looks like this:<br></blockquote><div><br></div><div>I included only the mutex lock and unlock part of the code in the message.</div><div>I thought adding the entire code snippet would make the commit message too lengthy.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
drivers/staging/vme_user/vme_tsi148.c<br>
1798 mutex_lock(&ctrlr->mtx);<br>
1799 <br>
1800 channel = ctrlr->number;<br>
1801 <br>
1802 if (!list_empty(&ctrlr->running)) {<br>
1803 /*<br>
1804 * XXX We have an active DMA transfer and currently haven't<br>
1805 * sorted out the mechanism for "pending" DMA transfers.<br>
1806 * Return busy.<br>
1807 */<br>
1808 /* Need to add to pending here */<br>
1809 mutex_unlock(&ctrlr->mtx);<br>
1810 return -EBUSY;<br>
1811 }<br>
1812 <br>
1813 list_add(&list->list, &ctrlr->running);<br>
<br>
<br>
The first line after we take a lock and the last line before we drop<br>
the lock are hopefully chosen because they need to be protected by the<br>
lock.<br></blockquote><div><br></div><div>Yes, I included only that part of the code in the commit message to avoid a lengthy commit message</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> 2. It's also used when removing DMA list from running list:<br>
> (line 1862 tsi148_dma_list_exec function)<br>
> mutex_lock(&ctrlr->mtx);<br>
> list_del(&list->list);<br>
> mutex_unlock(&ctrlr->mtx);<br>
> Ensuring thread-safe modification of the controller's state.<br>
> <br>
> Without this mutex, concurrent access to the DMA controller's state could<br>
> lead to data corruption or inconsistant state.<br>
><br>
<br>
It's also used in drivers/staging/vme_user/vme.c<br>
What you should do is rename the mutex from mtx to XXX_mtx and then<br>
rename all the places where it is used. Keep renaming until the driver<br>
builds.<br>
<br>
XXX_mtx is obviously not the correct name. But "mtx" is vague and<br>
useless. There are 3 other locks in this header file which have the<br>
same name so not only is it useless as a descriptive name, it's also<br>
useless for searching.<br></blockquote><div><br></div><div>Yes, I agree 'mt' is a vague name and doesn't convey much information.</div><div>In this patch, I have added only comments to address the checkpatch error. </div><div>Given your suggestion to change the variable name, I'd like to confirm,</div><div>Should I create a new patch that includes both the comment and the 'mtx' variable name change? </div><div>Or should I leave this current patch with comments only and</div><div>create a separate patch for the variable name changes?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Since you say that it is "protect access to the 'running' list", then<br>
that means you need to check all the places where the running list is<br>
accessed and ensure that the lock is held. Or if it's not, what makes<br>
that thread safe?<br></blockquote><div><br></div><div>Yes, I have checked the lock usage in all the places where the 'running' list is accessed.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> diff --git a/drivers/staging/vme_user/vme_bridge.h b/drivers/staging/vme_user/vme_bridge.h<br>
> index 9bdc41bb6602..bb3750b40eb1 100644<br>
> --- a/drivers/staging/vme_user/vme_bridge.h<br>
> +++ b/drivers/staging/vme_user/vme_bridge.h<br>
> @@ -61,6 +61,7 @@ struct vme_dma_list {<br>
> struct vme_dma_resource {<br>
> struct list_head list;<br>
> struct vme_bridge *parent;<br>
> + /* Mutex to protect DMA controller resources and ensure thread-safe operations */<br>
<br>
"resources" is too vague. "ensure thread-safe operations" is obvious<br>
and doesn't need to be said.<br></blockquote><div> </div><div>Should I mention the exact resources this mutex protects?</div><div><br></div><div>Regards,</div><div>Riyan Dhiman</div><div> </div></div></div>