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]
Date:   Fri, 15 Oct 2021 15:50:20 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     "Fabio M. De Francesco" <fmdefrancesco@...il.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 Fri, Oct 15, 2021 at 02:11:41PM +0200, Fabio M. De Francesco wrote:
> 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?
> 

The two semaphores used in that function are very connected so I don't
think it makes sense to split those up.  The others are less connected.

regards,
dan carpenter

Powered by blists - more mailing lists