Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 18 Mar 2020 11:30:36 +0100
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc:     linux-kernel@vger.kernel.org,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Subject: Re: [GIT PULL 3/6] intel_th: msu: Make stopping the trace optional
Message-ID: <20200318103036.GA2183221@kroah.com>
References: <20200317062215.15598-1-alexander.shishkin@linux.intel.com>
 <20200317062215.15598-4-alexander.shishkin@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200317062215.15598-4-alexander.shishkin@linux.intel.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@linux.intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  .../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@linux.intel.com>
> +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