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

[Dan Carpenter <dan.carpenter@linaro.org>]

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