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

[Sell, Timothy C]

> -----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.

> 
> >  	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.

> 
> >  	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.)

Tim Sell

> >  	rc = 0;
> >  out_locked:
> >  	up_write(&devdata->lock_visor_dev);
> > --
> > 1.9.1
> >