[PATCH] staging: vme_user: vme_bridge.h: Document mutex in vme_dma_resource structure

[Riyan Dhiman]

On Sat, Aug 3, 2024 at 9:54 AM Dan Carpenter <dan.carpenter at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20240814/fd8173d4/attachment-0001.htm>