Re: [GIT PULL 3/6] intel_th: msu: Make stopping the trace optional

[Greg Kroah-Hartman]

On Tue, Mar 17, 2020 at 08:22:12AM +0200, Alexander Shishkin wrote:
> Some use cases prefer to keep collecting the trace data into the last
> available window while the other windows are being offloaded instead of
> stopping the trace. In this scenario, the window switch happens
> automatically when the next window becomes available again.
> 
> Add an option to allow this and a sysfs attribute to enable it.
> 
> Signed-off-by: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
>  .../testing/sysfs-bus-intel_th-devices-msc    |  8 ++++
>  drivers/hwtracing/intel_th/msu.c              | 37 ++++++++++++++++++-
>  2 files changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-intel_th-devices-msc b/Documentation/ABI/testing/sysfs-bus-intel_th-devices-msc
> index 456cb62b384c..7fd2601c2831 100644
> --- a/Documentation/ABI/testing/sysfs-bus-intel_th-devices-msc
> +++ b/Documentation/ABI/testing/sysfs-bus-intel_th-devices-msc
> @@ -40,3 +40,11 @@ Description:	(RW) Trigger window switch for the MSC's buffer, in
>  		triggering a window switch for the buffer. Returns an error in any
>  		other operating mode or attempts to write something other than "1".
>  
> +What:		/sys/bus/intel_th/devices/<intel_th_id>-msc<msc-id>/stop_on_full
> +Date:		March 2020
> +KernelVersion:	5.7
> +Contact:	Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
> +Description:	(RW) Configure whether trace stops when the last available window
> +		becomes full (1/y/Y) or wraps around and continues until the next
> +		window becomes available again (0/n/N).
> +
> diff --git a/drivers/hwtracing/intel_th/msu.c b/drivers/hwtracing/intel_th/msu.c
> index 6e118b790d83..45916b48bcf0 100644
> --- a/drivers/hwtracing/intel_th/msu.c
> +++ b/drivers/hwtracing/intel_th/msu.c
> @@ -138,6 +138,7 @@ struct msc {
>  	struct list_head	win_list;
>  	struct sg_table		single_sgt;
>  	struct msc_window	*cur_win;
> +	struct msc_window	*switch_on_unlock;
>  	unsigned long		nr_pages;
>  	unsigned long		single_sz;
>  	unsigned int		single_wrap : 1;
> @@ -154,6 +155,8 @@ struct msc {
>  
>  	struct list_head	iter_list;
>  
> +	bool			stop_on_full;
> +
>  	/* config */
>  	unsigned int		enabled : 1,
>  				wrap	: 1,
> @@ -1717,6 +1720,10 @@ void intel_th_msc_window_unlock(struct device *dev, struct sg_table *sgt)
>  		return;
>  
>  	msc_win_set_lockout(win, WIN_LOCKED, WIN_READY);
> +	if (msc->switch_on_unlock == win) {
> +		msc->switch_on_unlock = NULL;
> +		msc_win_switch(msc);
> +	}
>  }
>  EXPORT_SYMBOL_GPL(intel_th_msc_window_unlock);
>  
> @@ -1757,7 +1764,11 @@ static irqreturn_t intel_th_msc_interrupt(struct intel_th_device *thdev)
>  
>  	/* next window: if READY, proceed, if LOCKED, stop the trace */
>  	if (msc_win_set_lockout(next_win, WIN_READY, WIN_INUSE)) {
> -		schedule_work(&msc->work);
> +		if (msc->stop_on_full)
> +			schedule_work(&msc->work);
> +		else
> +			msc->switch_on_unlock = next_win;
> +
>  		return IRQ_HANDLED;
>  	}
>  
> @@ -2050,11 +2061,35 @@ win_switch_store(struct device *dev, struct device_attribute *attr,
>  
>  static DEVICE_ATTR_WO(win_switch);
>  
> +static ssize_t stop_on_full_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{
> +	struct msc *msc = dev_get_drvdata(dev);
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", msc->stop_on_full);

No need for the scnprinf() crazyness for a single boolean value.  Just
use sprintf() and keep it simple.

> +static ssize_t stop_on_full_store(struct device *dev,
> +				  struct device_attribute *attr,
> +				  const char *buf, size_t size)
> +{
> +	struct msc *msc = dev_get_drvdata(dev);
> +	unsigned long val;
> +	int ret;
> +
> +	ret = kstrtobool(buf, &msc->stop_on_full);
> +
> +	return ret ? ret : size;
> +}

Here's the problem, you don't use val.

And spell out the ? : crazyness:
	if (ret)
		return ret;
	return size;

much simpler for people to read and understand.

thanks,

greg k-h