Date:   Sat, 16 Oct 2021 08:43:32 +0200
From:   "Fabio M. De Francesco" <fmdefrancesco@...il.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     Larry Finger <Larry.Finger@...inger.net>,
        Phillip Potter <phil@...lpotter.co.uk>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] staging: r8188eu: Use completions instead of semaphores

On Friday, October 15, 2021 2:50:20 PM CEST Dan Carpenter wrote:
> On Fri, Oct 15, 2021 at 02:11:41PM +0200, Fabio M. De Francesco wrote:

[...]

> > Hi Dan,
> > 
> > Thanks for your review. 
> > 
> > I wasn't aware of Arnd's patch. If I were I would have sent a "normal" 
patch.
> > 
> > Beyond this, I noticed that other semaphore (pcmdpriv->cmd_queue_sema) 
but, 
> > since I was not 100% sure that my changes would be accepted, I decided to 
> > leave it as-is for now and wait for reviews like yours.
> > 
> > Now that I know that this changes are welcome I'll also make the other 
> > changes. 
> > 
> > I guess that I have to change one semaphore per patch and make a series. 
> > However, now I see that Arnd's patch makes all the necessary changes in a 
> > single patch. What is the correct approach? Is one patch per semaphore 
> > preferred or one big patch for all of those that need to be changed?
> > 
> 
> The two semaphores used in that function are very connected so I don't
> think it makes sense to split those up.

I agree with you: the two semaphores in rtw_cmd_thread() are (somewhat) 
connected. However they serve different purposes. 

The first is used in to signal start and end of command thread 
(rtw_cmd_thread()). The second is used to notify that same thread that some 
commands have been enqueued. They serve two different purposes.

I prefer to make a series of three patches that I'll call "staging: r8188eu: 
use conditions variables and clean rtw_cmd_thread()". This choice is based on 
the above-mentioned fact that the two semaphores are there for different 
purposes. Let me explain what I'll put in each of the three patches... 

1) The first semaphore came to my attention because of a Smatch warning about 
duplicate releases ("up(s)) of the same semaphore in the same function. While 
addressing that problem I noticed that completions variables are more suited 
than semaphores for doing the work. 

So I didn't merely change the names of the semaphores in order to silence 
Smatch, instead I chose a more radical approach that is to replace semaphores 
with condition variables.

The first patch is there, I have the commit message ready to be re-used, and  
it is self-contained even if we still have a second semaphore that is there 
for servicing another purpose.

2) The second semaphore came to my attention while working on the first and 
trying to understand what rtw_cmd_thread() is meant for. It is there for very 
different reasons. There is only one relation between them, that is that they 
are used into the same function. Nothing else.

So I prefer to write a second commit message (in patch 2/3) that explains 
what the semaphore does and why it is better to replace it with a condition 
variable. Obviously, this "why are you changing it?" has a different answer 
with respect of what I say in patch 1/3.

3) With 3/3 there is the "clean" part of "staging: r8188eu: use conditions 
variables and clean rtw_cmd_thread()". While reading rtw_cmd_thread(), I 
noticed that there is an unnecessary duplicate of an 'if' statement. I want 
to remove the first (the one before the "_next:" label.

Other semaphores is r8188eu (if there are more) will be eventually addressed 
in future patches.

I hope that you and Phillip agree with me on this step by step approach.

Regards,

Fabio M. De Francesco

> The others are less connected.
> 
> regards,
> dan carpenter
> 
> 

Powered by blists - more mailing lists