| Message ID | 20240803001814.7752-1-riyandhiman14@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | staging: vme_user: vme_bridge.h: Document mutex in vme_dma_resource structure | expand |
On Sat, Aug 03, 2024 at 05:48:14AM +0530, Riyan Dhiman wrote: > Adhere to Linux kernel coding style > > Reported by checkpatch: > > CHECK: mutex definition without comment > > Proof for comment: > > 1. The mutex is used to protect access to the 'running' list > (line 1798 tsi148_dma_list_exec function) > mutex_lock(&ctrlrl->mtx); > if (!list_empty(&ctrlr->running)) { > mutex_unlock(&ctrlr->mtx); > return -EBUSY; > } Why did you chop out the "channel = ctrlr->number;" line? That code looks like this: drivers/staging/vme_user/vme_tsi148.c 1798 mutex_lock(&ctrlr->mtx); 1799 1800 channel = ctrlr->number; 1801 1802 if (!list_empty(&ctrlr->running)) { 1803 /* 1804 * XXX We have an active DMA transfer and currently haven't 1805 * sorted out the mechanism for "pending" DMA transfers. 1806 * Return busy. 1807 */ 1808 /* Need to add to pending here */ 1809 mutex_unlock(&ctrlr->mtx); 1810 return -EBUSY; 1811 } 1812 1813 list_add(&list->list, &ctrlr->running); The first line after we take a lock and the last line before we drop the lock are hopefully chosen because they need to be protected by the lock. > This prevents race conditions when multiple threads attempt to start DMA > operations simultaneously. > > 2. It's also used when removing DMA list from running list: > (line 1862 tsi148_dma_list_exec function) > mutex_lock(&ctrlr->mtx); > list_del(&list->list); > mutex_unlock(&ctrlr->mtx); > Ensuring thread-safe modification of the controller's state. > > Without this mutex, concurrent access to the DMA controller's state could > lead to data corruption or inconsistant state. > It's also used in drivers/staging/vme_user/vme.c What you should do is rename the mutex from mtx to XXX_mtx and then rename all the places where it is used. Keep renaming until the driver builds. XXX_mtx is obviously not the correct name. But "mtx" is vague and useless. There are 3 other locks in this header file which have the same name so not only is it useless as a descriptive name, it's also useless for searching. Since you say that it is "protect access to the 'running' list", then that means you need to check all the places where the running list is accessed and ensure that the lock is held. Or if it's not, what makes that thread safe? These comments are supposed to help us understand the locking but it feels like we have a long way to go before it's fully understood. > Signed-off-by: Riyan Dhiman <riyandhiman14@gmail.com> > --- > drivers/staging/vme_user/vme_bridge.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/staging/vme_user/vme_bridge.h b/drivers/staging/vme_user/vme_bridge.h > index 9bdc41bb6602..bb3750b40eb1 100644 > --- a/drivers/staging/vme_user/vme_bridge.h > +++ b/drivers/staging/vme_user/vme_bridge.h > @@ -61,6 +61,7 @@ struct vme_dma_list { > struct vme_dma_resource { > struct list_head list; > struct vme_bridge *parent; > + /* Mutex to protect DMA controller resources and ensure thread-safe operations */ "resources" is too vague. "ensure thread-safe operations" is obvious and doesn't need to be said. regards, dan carpenter
On Wed, Aug 14, 2024 at 09:11:22AM +0530, Riyan Dhiman wrote: > Yes, I agree 'mt' is a vague name and doesn't convey much information. > In this patch, I have added only comments to address the checkpatch error. > Given your suggestion to change the variable name, I'd like to confirm, > Should I create a new patch that includes both the comment and the 'mtx' > variable name change? > Or should I leave this current patch with comments only and > create a separate patch for the variable name changes? I feel like renaming the spinlock is more useful than adding a comment. Plus you can't really understand the locking without at least doing a temporary rename to see what places break. To be honest, we don't merge a lot of "add locking comments" because it's probably one of the trickiest checkpatch warnings. You need to understand the locking before you can add a useful comment. When you're writing the comment, your target audience is Greg. Greg is obviously a very experienced kernel developer. He works in USB, stable kernels, staging, tty, device models stuff, and a bunch of other things. But, he doesn't know *this* driver in great depth. When Greg takes a look at this code, it doesn't take him long to make a very educated guess what the locking is for. If the comment has less information than Greg can see on his own at a glance then it's just a waste of time. If someone had questions about the locking would they be better off asking you or asking Greg? Until you can answer questions better than Greg then it's not much point in it. Again, Greg doesn't know this driver very deeply because he's focused on a million other things so it's not that hard. Trying to figure out the locking is a good exercise. It wouldn't surprise me if there were some locking bugs in this code and you should try to fix those. But it's not super easy either. regards, dan carpenter
diff --git a/drivers/staging/vme_user/vme_bridge.h b/drivers/staging/vme_user/vme_bridge.h index 9bdc41bb6602..bb3750b40eb1 100644 --- a/drivers/staging/vme_user/vme_bridge.h +++ b/drivers/staging/vme_user/vme_bridge.h @@ -61,6 +61,7 @@ struct vme_dma_list { struct vme_dma_resource { struct list_head list; struct vme_bridge *parent; + /* Mutex to protect DMA controller resources and ensure thread-safe operations */ struct mutex mtx; int locked; int number;
Adhere to Linux kernel coding style Reported by checkpatch: CHECK: mutex definition without comment Proof for comment: 1. The mutex is used to protect access to the 'running' list (line 1798 tsi148_dma_list_exec function) mutex_lock(&ctrlrl->mtx); if (!list_empty(&ctrlr->running)) { mutex_unlock(&ctrlr->mtx); return -EBUSY; } This prevents race conditions when multiple threads attempt to start DMA operations simultaneously. 2. It's also used when removing DMA list from running list: (line 1862 tsi148_dma_list_exec function) mutex_lock(&ctrlr->mtx); list_del(&list->list); mutex_unlock(&ctrlr->mtx); Ensuring thread-safe modification of the controller's state. Without this mutex, concurrent access to the DMA controller's state could lead to data corruption or inconsistant state. Signed-off-by: Riyan Dhiman <riyandhiman14@gmail.com> --- drivers/staging/vme_user/vme_bridge.h | 1 + 1 file changed, 1 insertion(+)