On Sat, Aug 3, 2024 at 9:54 AM Dan Carpenter <dan.carpen...@linaro.org>
wrote:
> 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:
>
I included only the mutex lock and unlock part of the code in the message.
I thought adding the entire code snippet would make the commit message too
lengthy.
> 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.
>
Yes, I included only that part of the code in the commit message to avoid a
lengthy commit message
> 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.
>
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?
> 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?
>
Yes, I have checked the lock usage in all the places where the 'running'
list is accessed.
> > 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.
>
Should I mention the exact resources this mutex protects?
Regards,
Riyan Dhiman
