linux-kernel - Re: [RFC PATCH] staging: r8188eu: Use completions instead of semaphores

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2060953.sJFZD89sIB@localhost.localdomain>
Date:   Fri, 15 Oct 2021 14:11:41 +0200
From:   "Fabio M. De Francesco" <fmdefrancesco@...il.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>,
        Larry Finger <Larry.Finger@...inger.net>,
        Phillip Potter <phil@...lpotter.co.uk>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     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 1:37:15 PM CEST Dan Carpenter wrote:
> On Fri, Oct 15, 2021 at 01:02:38PM +0200, Fabio M. De Francesco wrote:
> > rtw_cmd_thread() "up(s)" a semaphore twice, first to notify callers when
> > its execution is started and then to notify when it is about to end.
> > 
> > It makes the same semaphore go "up" twice in the same thread. This
> > construct makes Smatch to warn of duplicate "up(s)".
> >
> > [...]
> >
> > I'm waiting for Maintainers and other Reviewers to say if this patch is
> > actually needed and, if so, also for suggestions about how to improve
> > it. In particular I'm interested to know what they think of using the
> > uninterruptible version of wait_for_completion*().
> > 
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@...il.com>
> 
> This is basically what Arnd did to rtl8723bs in commit:
> 
> commit 09a8ea34cf431bfb77159197e46753d101c528c5
> Author: Arnd Bergmann <arnd@...db.de>
> Date:   Mon Dec 10 22:40:30 2018 +0100
> 
>     staging: rtl8723bs: change semaphores to completions
> 
> But there are some differences.  His patch is a little bit cleaner
> because it gets rid of "pcmdpriv->cmd_queue_sema".  Could you basically
> just ports Arnd's patch for this driver?
> 
> His patch goes quite a bit further as well, and change some other
> semaphors but we could do it piece meal and just change the
> rtw_cmd_thread() related ones.
> 
> regards,
> dan carpenter
> 
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?

Again, thank you very much.

Regards,

Fabio M. De Francesco