query about locking in IIO

query about locking in IIO

[Rohit Sarkar]

Hi,
Could someone explain why using indio_dev->mlock directly is a bad idea?
Further examples of cases where it cannot be replaced will be helpful

Thanks,
Rohit

Re: query about locking in IIO

[Ardelean, Alexandru]

On Tue, 2020-02-25 at 22:41 +0530, Rohit Sarkar wrote:
> Hi,
> Could someone explain why using indio_dev->mlock directly is a bad idea?
> Further examples of cases where it cannot be replaced will be helpful
> 

Jonathan may add more here.

But in general, each driver should define it's own explicit lock if it needs to.
Some drivers need explicit locking, some don't.

A lot of other frameworks already define locks already.
Like, for example, when an IIO driver uses some SPI transfers, the SPI framework
already uses some locks. So, you don't typically need extra locking; which for
some IIO drivers translates to: no extra explicit locking.

I guess Jonathan also wants to move the mlock to be used only in the IIO
framework.
In some cases, if drivers use this mlock, and the framework uses it, you can end
up trying to acquire the same mlock twice, which can end-up in a deadlock.
These things can sometimes slip through the code-review.

I guess the docs need a bit of update.
Because:

* @mlock:              [DRIVER] lock used to prevent simultaneous device state
*                      changes

I think it should be converted to [INTERN]

> Thanks,
> Rohit
> 

Re: query about locking in IIO

[Rohit Sarkar]

On Wed, Feb 26, 2020 at 06:54:21AM +0000, Ardelean, Alexandru wrote:
> On Tue, 2020-02-25 at 22:41 +0530, Rohit Sarkar wrote:
> > Hi,
> > Could someone explain why using indio_dev->mlock directly is a bad idea?
> > Further examples of cases where it cannot be replaced will be helpful
> > 
> 
> Jonathan may add more here.
> 
> But in general, each driver should define it's own explicit lock if it needs to.
> Some drivers need explicit locking, some don't.
> 
> A lot of other frameworks already define locks already.
> Like, for example, when an IIO driver uses some SPI transfers, the SPI framework
> already uses some locks. So, you don't typically need extra locking; which for
> some IIO drivers translates to: no extra explicit locking.
> 
> I guess Jonathan also wants to move the mlock to be used only in the IIO
> framework.
> In some cases, if drivers use this mlock, and the framework uses it, you can end
> up trying to acquire the same mlock twice, which can end-up in a deadlock.
> These things can sometimes slip through the code-review.

This makes sense

> I guess the docs need a bit of update.
> Because:
> 
> * @mlock:              [DRIVER] lock used to prevent simultaneous device state
> *                      changes
> 
> I think it should be converted to [INTERN]
> 
> > Thanks,
> > Rohit
> > 

As a follow up would I be right to assume that as long as the mlock is
not being in the IIO framework, explicit locking should be the way to
go?

Thanks,
Rohit

Re: query about locking in IIO

[Ardelean, Alexandru]

On Wed, 2020-02-26 at 22:55 +0530, Rohit Sarkar wrote:
> [External]
> 
> On Wed, Feb 26, 2020 at 06:54:21AM +0000, Ardelean, Alexandru wrote:
> > On Tue, 2020-02-25 at 22:41 +0530, Rohit Sarkar wrote:
> > > Hi,
> > > Could someone explain why using indio_dev->mlock directly is a bad idea?
> > > Further examples of cases where it cannot be replaced will be helpful
> > > 
> > 
> > Jonathan may add more here.
> > 
> > But in general, each driver should define it's own explicit lock if it needs
> > to.
> > Some drivers need explicit locking, some don't.
> > 
> > A lot of other frameworks already define locks already.
> > Like, for example, when an IIO driver uses some SPI transfers, the SPI
> > framework
> > already uses some locks. So, you don't typically need extra locking; which
> > for
> > some IIO drivers translates to: no extra explicit locking.
> > 
> > I guess Jonathan also wants to move the mlock to be used only in the IIO
> > framework.
> > In some cases, if drivers use this mlock, and the framework uses it, you can
> > end
> > up trying to acquire the same mlock twice, which can end-up in a deadlock.
> > These things can sometimes slip through the code-review.
> 
> This makes sense
> 
> > I guess the docs need a bit of update.
> > Because:
> > 
> > * @mlock:              [DRIVER] lock used to prevent simultaneous device
> > state
> > *                      changes
> > 
> > I think it should be converted to [INTERN]
> > 
> > > Thanks,
> > > Rohit
> > > 
> 
> As a follow up would I be right to assume that as long as the mlock is
> not being in the IIO framework, explicit locking should be the way to
> go?

The question sounds a bit hard to follow.
Each driver should define it's own locking if it needs it.
mlock will continued to be used only in the IIO framework; it won't be removed.
[INTERN] is just a marker in the doc-string to make sure people don't use
these fields in drivers.


> Thanks,
> Rohit

Re: query about locking in IIO

[Jonathan Cameron]

On Thu, 27 Feb 2020 06:58:11 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Wed, 2020-02-26 at 22:55 +0530, Rohit Sarkar wrote:
> > [External]
> > 
> > On Wed, Feb 26, 2020 at 06:54:21AM +0000, Ardelean, Alexandru wrote:  
> > > On Tue, 2020-02-25 at 22:41 +0530, Rohit Sarkar wrote:  
> > > > Hi,
> > > > Could someone explain why using indio_dev->mlock directly is a bad idea?
> > > > Further examples of cases where it cannot be replaced will be helpful
> > > >   
> > > 
> > > Jonathan may add more here.
> > > 
> > > But in general, each driver should define it's own explicit lock if it needs
> > > to.
> > > Some drivers need explicit locking, some don't.
> > > 
> > > A lot of other frameworks already define locks already.
> > > Like, for example, when an IIO driver uses some SPI transfers, the SPI
> > > framework
> > > already uses some locks. So, you don't typically need extra locking; which
> > > for
> > > some IIO drivers translates to: no extra explicit locking.
> > > 
> > > I guess Jonathan also wants to move the mlock to be used only in the IIO
> > > framework.
> > > In some cases, if drivers use this mlock, and the framework uses it, you can
> > > end
> > > up trying to acquire the same mlock twice, which can end-up in a deadlock.
> > > These things can sometimes slip through the code-review.  
> > 
> > This makes sense
> >   
> > > I guess the docs need a bit of update.
> > > Because:
> > > 
> > > * @mlock:              [DRIVER] lock used to prevent simultaneous device
> > > state
> > > *                      changes
> > > 
> > > I think it should be converted to [INTERN]
> > >   
> > > > Thanks,
> > > > Rohit
> > > >   
> > 
> > As a follow up would I be right to assume that as long as the mlock is
> > not being in the IIO framework, explicit locking should be the way to
> > go?  
> 
> The question sounds a bit hard to follow.
> Each driver should define it's own locking if it needs it.
> mlock will continued to be used only in the IIO framework; it won't be removed.
> [INTERN] is just a marker in the doc-string to make sure people don't use
> these fields in drivers.

Yes.  That's basically it.  mlock is a framework internal lock and we may change
how it is implemented at any time.

Various drivers using it make any such changes impossible and are much harder to
reason about because the mlock uses in the core aren't visible within the
driver.

So basically its a software architecture problem rather than there being any
known bugs!

Thanks,

Jonathan

> 
> 
> > Thanks,
> > Rohit  

Re: query about locking in IIO

[Rohit Sarkar]

On Fri, Feb 28, 2020 at 01:24:02PM +0000, Jonathan Cameron wrote:
> On Thu, 27 Feb 2020 06:58:11 +0000
> "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
> 
> > On Wed, 2020-02-26 at 22:55 +0530, Rohit Sarkar wrote:
> > > [External]
> > > 
> > > On Wed, Feb 26, 2020 at 06:54:21AM +0000, Ardelean, Alexandru wrote:  
> > > > On Tue, 2020-02-25 at 22:41 +0530, Rohit Sarkar wrote:  
> > > > > Hi,
> > > > > Could someone explain why using indio_dev->mlock directly is a bad idea?
> > > > > Further examples of cases where it cannot be replaced will be helpful
> > > > >   
> > > > 
> > > > Jonathan may add more here.
> > > > 
> > > > But in general, each driver should define it's own explicit lock if it needs
> > > > to.
> > > > Some drivers need explicit locking, some don't.
> > > > 
> > > > A lot of other frameworks already define locks already.
> > > > Like, for example, when an IIO driver uses some SPI transfers, the SPI
> > > > framework
> > > > already uses some locks. So, you don't typically need extra locking; which
> > > > for
> > > > some IIO drivers translates to: no extra explicit locking.
> > > > 
> > > > I guess Jonathan also wants to move the mlock to be used only in the IIO
> > > > framework.
> > > > In some cases, if drivers use this mlock, and the framework uses it, you can
> > > > end
> > > > up trying to acquire the same mlock twice, which can end-up in a deadlock.
> > > > These things can sometimes slip through the code-review.  
> > > 
> > > This makes sense
> > >   
> > > > I guess the docs need a bit of update.
> > > > Because:
> > > > 
> > > > * @mlock:              [DRIVER] lock used to prevent simultaneous device
> > > > state
> > > > *                      changes
> > > > 
> > > > I think it should be converted to [INTERN]
> > > >   
> > > > > Thanks,
> > > > > Rohit
> > > > >   
> > > 
> > > As a follow up would I be right to assume that as long as the mlock is
> > > not being in the IIO framework, explicit locking should be the way to
> > > go?  
> > 
> > The question sounds a bit hard to follow.
> > Each driver should define it's own locking if it needs it.
> > mlock will continued to be used only in the IIO framework; it won't be removed.
> > [INTERN] is just a marker in the doc-string to make sure people don't use
> > these fields in drivers.
> 
> Yes.  That's basically it.  mlock is a framework internal lock and we may change
> how it is implemented at any time.
> 
> Various drivers using it make any such changes impossible and are much harder to
> reason about because the mlock uses in the core aren't visible within the
> driver.
> 
> So basically its a software architecture problem rather than there being any
> known bugs!

Got it, Thanks
> Thanks,
> 
> Jonathan
> 
> > 
> > 
> > > Thanks,
> > > Rohit  
> 
>