Re: [PATCH v5 3/4] mtd: core: protect access to MTD devices while in suspend

From: Boris Brezillon <boris.brezillon@collabora.com>
To: Sean Nyekjaer <sean@geanix.com>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>,
	linux-kernel@vger.kernel.org,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	Richard Weinberger <richard@nod.at>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Boris Brezillon <bbrezillon@kernel.org>,
	linux-mtd@lists.infradead.org
Subject: Re: [PATCH v5 3/4] mtd: core: protect access to MTD devices while in suspend
Date: Mon, 13 Dec 2021 12:06:58 +0100	[thread overview]
Message-ID: <20211213120658.45f312a6@collabora.com> (raw)
In-Reply-To: <20211213105012.65jk4rylxzncqdfy@skn-laptop>

On Mon, 13 Dec 2021 12:50:12 +0200
Sean Nyekjaer <sean@geanix.com> wrote:

> Hi Miquel and Boris,
> 
> On Mon, Dec 13, 2021 at 10:53:36AM +0100, Boris Brezillon wrote:
> > On Mon, 13 Dec 2021 10:33:50 +0100
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >   
> > > Hello,
> > > 
> > > boris.brezillon@collabora.com wrote on Mon, 13 Dec 2021 10:28:01 +0100:
> > >   
> > > > On Mon, 13 Dec 2021 10:10:25 +0100
> > > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > >     
> > > > > Hi Sean,
> > > > > 
> > > > > sean@geanix.com wrote on Fri, 10 Dec 2021 14:25:35 +0100:
> > > > >       
> > > > > > On Thu, Dec 09, 2021 at 03:28:11PM +0100, Miquel Raynal wrote:        
> > > > > > > Hi Sean,
> > > > > > > 
> > > > > > > sean@geanix.com wrote on Thu, 9 Dec 2021 15:07:21 +0100:
> > > > > > >           
> > > > > > > > On Fri, Dec 03, 2021 at 02:39:58PM +0100, Miquel Raynal wrote:          
> > > > > > > > > Hello,
> > > > > > > > >             
> > > > > > > > > > > Fine by me, lets drop this series.            
> > > > > > > > > 
> > > > > > > > > FYI I've dropped the entire series from mtd/next. I'm waiting for the
> > > > > > > > > fix discussed below (without abusing the chip mutex ;-) ).            
> > > > > > > > 
> > > > > > > > Cool, looking forward to test a patch series :)          
> > > > > > > 
> > > > > > > Test? You mean "write"? :)
> > > > > > > 
> > > > > > > Cheers,
> > > > > > > Miquèl          
> > > > > > 
> > > > > > Hi Miquel,
> > > > > > 
> > > > > > Should we us a atomic for the suspended variable?        
> > > > > 
> > > > > I haven't thought about it extensively, an atomic variable sound fine
> > > > > but I am definitely not a locking expert...      
> > > > 
> > > > No need to use an atomic if the variable is already protected by a lock
> > > > when accessed, and this seems to be case.    
> > > 
> > > Maybe there was a confusion about this lock: I think Boris just do not
> > > want the core to take any lock during a suspend operation. But you can
> > > still use locks, as long as you release them before suspending.
> > > 
> > > And also, that chip lock might not be the one you want to take because
> > > it's been introduced for another purpose.  
> > 
> > Access to the suspended field is already protected by the chip lock,
> > and I think it's just fine to keep it this way.  
> 
> I'm reading the suspended variable in wait_event() outside the lock :/

It doesn't matter because you're checking it again with the lock held
when doing a new loop iteration.