Re: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary locking

[Neil Horman]

On Thu, Jun 02, 2016 at 05:02:11AM +0000, Sell, Timothy C wrote:
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman@xxxxxxxxxx]
> > Sent: Wednesday, June 01, 2016 2:43 PM
> > To: Sell, Timothy C
> > Cc: Kershner, David A; corbet@xxxxxxx; tglx@xxxxxxxxxxxxx;
> > mingo@xxxxxxxxxx; hpa@xxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx;
> > Arfvidson, Erik; hofrat@xxxxxxxxx; dzickus@xxxxxxxxxx;
> > jes.sorensen@xxxxxxxxxx; Curtin, Alexander Paul;
> > janani.rvchndrn@xxxxxxxxx; sudipm.mukherjee@xxxxxxxxx;
> > prarit@xxxxxxxxxx; Binder, David Anthony; dan.j.williams@xxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx; driverdev-
> > devel@xxxxxxxxxxxxxxxxxxxxxx; *S-Par-Maintainer
> > Subject: Re: [PATCH v2 10/27] staging: unisys: visorinput: remove
> > unnecessary locking
> > 
> > On Wed, Jun 01, 2016 at 03:09:13PM +0000, Sell, Timothy C wrote:
> > > > -----Original Message-----
> > > > From: Neil Horman [mailto:nhorman@xxxxxxxxxx]
> > > > Sent: Wednesday, June 01, 2016 10:18 AM
> > > > To: Kershner, David A
> > > > Cc: corbet@xxxxxxx; tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx;
> > > > hpa@xxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; Arfvidson, Erik; Sell,
> > Timothy
> > > > C; hofrat@xxxxxxxxx; dzickus@xxxxxxxxxx; jes.sorensen@xxxxxxxxxx;
> > > > Curtin, Alexander Paul; janani.rvchndrn@xxxxxxxxx;
> > > > sudipm.mukherjee@xxxxxxxxx; prarit@xxxxxxxxxx; Binder, David
> > Anthony;
> > > > dan.j.williams@xxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> > > > doc@xxxxxxxxxxxxxxx; driverdev-devel@xxxxxxxxxxxxxxxxxxxxxx; *S-Par-
> > > > Maintainer
> > > > Subject: Re: [PATCH v2 10/27] staging: unisys: visorinput: remove
> > > > unnecessary locking
> > > >
> > > > On Tue, May 31, 2016 at 10:26:36PM -0400, David Kershner wrote:
> > > > > From: Tim Sell <Timothy.Sell@xxxxxxxxxx>
> > > > >
> > > > > Locking in the _interrupt() function is NOT necessary so long as we
> > ensure
> > > > > that interrupts have been stopped whenever we need to pause or
> > resume
> > > > the
> > > > > device, which we now do.
> > > > >
> > > > > While a device is paused, we ensure that interrupts stay disabled, i.e.
> > > > > that the _interrupt() function will NOT be called, yet remember the
> > > > desired
> > > > > state in devdata->interrupts_enabled if open() or close() are called are
> > > > > called while the device is paused.  Then when the device is resumed,
> > we
> > > > > restore the actual state of interrupts (i.e., whether _interrupt() is going
> > > > > to be called or not) to the desired state in devdata-
> > >interrupts_enabled.
> > > > >
> > > > > Signed-off-by: Tim Sell <Timothy.Sell@xxxxxxxxxx>
> > > > > Signed-off-by: David Kershner <david.kershner@xxxxxxxxxx>
> > > > > ---
> > > > >  drivers/staging/unisys/visorinput/visorinput.c | 57
> > > > +++++++++++++++++++++-----
> > > > >  1 file changed, 47 insertions(+), 10 deletions(-)
> > > > >
> > > > > diff --git a/drivers/staging/unisys/visorinput/visorinput.c
> > > > b/drivers/staging/unisys/visorinput/visorinput.c
> > > > > index 12a3570..9c00710 100644
> > > > > --- a/drivers/staging/unisys/visorinput/visorinput.c
> > > > > +++ b/drivers/staging/unisys/visorinput/visorinput.c
> > > > > @@ -66,6 +66,7 @@ struct visorinput_devdata {
> > > > >  	struct rw_semaphore lock_visor_dev; /* lock for dev */
> > > > >  	struct input_dev *visorinput_dev;
> > > > >  	bool paused;
> > > > > +	bool interrupts_enabled;
> > > > >  	unsigned int keycode_table_bytes; /* size of following array */
> > > > >  	/* for keyboard devices: visorkbd_keycode[] +
> > > > visorkbd_ext_keycode[] */
> > > > >  	unsigned char keycode_table[0];
> > > > > @@ -228,7 +229,21 @@ static int visorinput_open(struct input_dev
> > > > *visorinput_dev)
> > > > >  		return -EINVAL;
> > > > >  	}
> > > > >  	dev_dbg(&visorinput_dev->dev, "%s opened\n", __func__);
> > > > > +
> > > > > +	/*
> > > > > +	 * If we're not paused, really enable interrupts.
> > > > > +	 * Regardless of whether we are paused, set a flag indicating
> > > > > +	 * interrupts should be enabled so when we resume, interrupts
> > > > > +	 * will really be enabled.
> > > > > +	 */
> > > > > +	down_write(&devdata->lock_visor_dev);
> > > > > +	devdata->interrupts_enabled = true;
> > > > > +	if (devdata->paused)
> > > > > +		goto out_unlock;
> > > > Don't you want to wait until you actually enable interrupts here to set
> > > > interrupts_enabled to true?  Otherwise, if devdata->paused is true, you
> > will
> > > > be
> > > > out of sync.
> > >
> > > No.  That's the intent of this code, to remember what the
> > > state of interrupts SHOULD be (via devdata->interrupts_enabled), at
> > > a point in time when interrupts can NOT be enabled, e.g., when
> > > the device is paused (devdata->paused).  After the device is resumed,
> > > the real interrupt state (visorbus_enable_channel_interrupts())
> > > will be synchronized with the remembered state.
> > >
> > 
> > Ok, I'll buy that, but it still looks rather racy to me.  It appears to me that
> > the code path in which the paused state is toggled
> > (visorinput_pause|resume), is
> > called from a path that originates in visorchipset, specifically in the work
> > queue function controlvm_periodic_work.  Given that, its entirely possible
> > for
> > the paused state of the virutal hardware to change while the device is being
> > opened.  That is to say devdata->paused can become true immediately after
> > its
> > checked in visorinput_open above, and so we can enable interrupts on
> > hardware
> > that is paused, which seems to be what this code is trying to avoid.
> > 
> 
> You are absolutely correct about the 2 different threads of execution
> where these functions can be called.
> 
> But in this code, we hold devdata->lock_visor_dev in order to prevent
> the scenario you describe.  I.e., the code in all of the paths involved:
> * never changes dev->paused or dev->interrupts_enabled without
> holding devdata->lock_visor_dev
> * never makes any decisions based on dev->paused or
> dev->interrupts_enabled without holding devdata->lock_visor_dev
> 
> > > >
> > > > >  	visorbus_enable_channel_interrupts(devdata->dev);
> > > > > +
> > > > > +out_unlock:
> > > > > +	up_write(&devdata->lock_visor_dev);
> > > > >  	return 0;
> > > > >  }
> > > > >
> > > > > @@ -243,7 +258,22 @@ static void visorinput_close(struct input_dev
> > > > *visorinput_dev)
> > > > >  		return;
> > > > >  	}
> > > > >  	dev_dbg(&visorinput_dev->dev, "%s closed\n", __func__);
> > > > > +
> > > > > +	/*
> > > > > +	 * If we're not paused, really disable interrupts.
> > > > > +	 * Regardless of whether we are paused, set a flag indicating
> > > > > +	 * interrupts should be disabled so when we resume we will
> > > > > +	 * not re-enable them.
> > > > > +	 */
> > > > > +
> > > > > +	down_write(&devdata->lock_visor_dev);
> > > > > +	devdata->interrupts_enabled = false;
> > > > > +	if (devdata->paused)
> > > > > +		goto out_unlock;
> > > > Ditto to my above comment
> > >
> > > Ditto my response above.
> > >
> > Same comment regarding racyness.
> > 
> > > >
> > > > >  	visorbus_disable_channel_interrupts(devdata->dev);
> > > > > +
> > > > > +out_unlock:
> > > > > +	up_write(&devdata->lock_visor_dev);
> > > > >  }
> > > > >
> > > > >  /*
> > > > > @@ -438,10 +468,8 @@ visorinput_remove(struct visor_device *dev)
> > > > >  	 * in visorinput_channel_interrupt()
> > > > >  	 */
> > > > >
> > > > > -	down_write(&devdata->lock_visor_dev);
> > > > >  	dev_set_drvdata(&dev->device, NULL);
> > > > >  	unregister_client_input(devdata->visorinput_dev);
> > > > > -	up_write(&devdata->lock_visor_dev);
> > > > >  	kfree(devdata);
> > > > >  }
> > > > >
> > > > > @@ -529,13 +557,7 @@ visorinput_channel_interrupt(struct
> > visor_device
> > > > *dev)
> > > > >  	if (!devdata)
> > > > >  		return;
> > > > >
> > > > > -	down_write(&devdata->lock_visor_dev);
> > > > > -	if (devdata->paused) /* don't touch device/channel when paused */
> > > > > -		goto out_locked;
> > > > > -
> > > > >  	visorinput_dev = devdata->visorinput_dev;
> > > > > -	if (!visorinput_dev)
> > > > > -		goto out_locked;
> > > > >
> > > > >  	while (visorchannel_signalremove(dev->visorchannel, 0, &r)) {
> > > > >  		scancode = r.activity.arg1;
> > > > > @@ -611,8 +633,6 @@ visorinput_channel_interrupt(struct
> > visor_device
> > > > *dev)
> > > > >  			break;
> > > > >  		}
> > > > >  	}
> > > > > -out_locked:
> > > > > -	up_write(&devdata->lock_visor_dev);
> > > > >  }
> > > > >
> > > > >  static int
> > > > > @@ -632,6 +652,14 @@ visorinput_pause(struct visor_device *dev,
> > > > >  		rc = -EBUSY;
> > > > >  		goto out_locked;
> > > > >  	}
> > > > > +	if (devdata->interrupts_enabled)
> > > > > +		visorbus_disable_channel_interrupts(dev);
> > > > > +
> > > > > +	/*
> > > > > +	 * due to above, at this time no thread of execution will be
> > > > > +	 * in visorinput_channel_interrupt()
> > > > > +	 */
> > > > > +
> > > > >  	devdata->paused = true;
> > > > >  	complete_func(dev, 0);
> > > > >  	rc = 0;
> > > > > @@ -659,6 +687,15 @@ visorinput_resume(struct visor_device *dev,
> > > > >  	}
> > > > >  	devdata->paused = false;
> > > > >  	complete_func(dev, 0);
> > > > > +
> > > > > +	/*
> > > > > +	 * Re-establish calls to visorinput_channel_interrupt() if that is
> > > > > +	 * the desired state that we've kept track of in interrupts_enabled
> > > > > +	 * while the device was paused.
> > > > > +	 */
> > > > > +	if (devdata->interrupts_enabled)
> > > > > +		visorbus_enable_channel_interrupts(dev);
> > > > > +
> > > >
> > > > Unless I'm mistaken, it seems that visorinput_pause and
> > visorinput_open or
> > > > close
> > > > can be called in parallel on different cpus.  As such the state of
> > > > interrupts_enabled may change during the execution of this function,
> > which
> > > > would
> > > > lead to interrupts not getting properly enabled.
> > > >
> > >
> > >
> > > You are correct that visorinput_pause and visorinput_open/close
> > > can be called in parallel.  However, as I alluded to in my comment
> > > above, the intent of this code is to just restore the actual interrupt
> > > state with the desired state (remembered in
> > > devdata->interrupts_enabled).  It's ok if interrupts don't get
> > > enabled, because that would be our intent if there are no longer
> > > any users of the device.  (In this case visorinput_close() would have
> > > been called and devdata->interrupts_enabled would have got set
> > > false while the device was paused.)
> > >
> > 
> > 
> > Heres an illustration of my concern.  Assume the visorinput device is
> > currently
> > paused, and someone has called open on it while at the same time
> > resuming it
> > 
> > CPU0				CPU1
> > 				visoinput_resume
> > visorinput_open
> >  <handle random smi>		check ->interrupts_enabled (false)
> >  <return from smi>		<handle random smi>
> >  set interrupts_enabled=true
> >  check ->paused (true)		<return from smi>
> > 				set ->paused = true
> >  return 0
> > 
> > In the above scenario visorinput_open and visorinput_resume will both
> > return
> > without having enabled interrupts, rendering the device non-responsive.
> > 
> > A simmmilar scenario can be seen on close/pause, in which interrupts are
> > left
> > enabled on a device that is paused.
> > 
> > It seems you can't remove all level of serialization here (though you can
> > remove
> > some).  I would recommend that, instead of keeping your own mutex, you
> > instead
> > augment visorinput_pause/resume, to extract the input_device structure
> > from the
> > driver private data and hold the input device mutex when
> > pausing/resuming the
> > device.  That will ensure that neither the paused or interrupts_enabled
> > state
> > will change during the execution of visorinput_open/close
> > 
> > Neil
> 
> Nice illustration.  That would usually be enough to drill something thru
> my thick skull, but I'm still missing something in this case.  ;-(
> 
> I'm still missing how this scenario could happen given our usage of 
> devdata->lock_visor_dev.  We hold that lock for the entire execution of
> visorinput_open(), visorinput_close(), visorinput_pause(), and
> visorinput_resume(), where we are dealing with the checks and state
> transitions of devdata->paused, devdata->interrupts_enabled, and
> the actual state of channel interrupts.  So even if the circumstance
> presented itself where we were ready to run thru 2 of those functions
> for the same device on mutiple cpus at the same exact time, the
> execution would be serialized due to devdata->lock_visor_dev.
> 
Ok, there it is, that works then, thanks for the clarification

Neil

> Tim Sell
>