Messages in this thread | | | Date | Wed, 18 Mar 2020 11:30:36 +0100 | From | Greg Kroah-Hartman <> | Subject | Re: [GIT PULL 3/6] intel_th: msu: Make stopping the trace optional |
| |
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
| |