On Wed, Jul 24, 2019 at 11:41:10AM +0200, Greg KH wrote: > On Tue, Jul 23, 2019 at 12:51:27PM +0800, Wu Hao wrote: > > This patch introduces more sysfs interfaces for Accelerated > > Function Unit (AFU). These interfaces allow users to read > > current AFU Power State (APx), read / clear AFU Power (APx) > > events which are sticky to identify transient APx state, > > and manage AFU's LTR (latency tolerance reporting). > > > > Signed-off-by: Ananda Ravuri <[email protected]> > > Signed-off-by: Xu Yilun <[email protected]> > > Signed-off-by: Wu Hao <[email protected]> > > Acked-by: Alan Tull <[email protected]> > > Signed-off-by: Moritz Fischer <[email protected]> > > --- > > v2: rebased, and remove DRV/MODULE_VERSION modifications > > v3: update kernel version and date in sysfs doc > > --- > > Documentation/ABI/testing/sysfs-platform-dfl-port | 30 +++++ > > drivers/fpga/dfl-afu-main.c | 137 > > ++++++++++++++++++++++ > > drivers/fpga/dfl.h | 11 ++ > > 3 files changed, 178 insertions(+) > > > > diff --git a/Documentation/ABI/testing/sysfs-platform-dfl-port > > b/Documentation/ABI/testing/sysfs-platform-dfl-port > > index 6a92dda..5961fb2 100644 > > --- a/Documentation/ABI/testing/sysfs-platform-dfl-port > > +++ b/Documentation/ABI/testing/sysfs-platform-dfl-port > > @@ -14,3 +14,33 @@ Description: Read-only. User can program different > > PR bitstreams to FPGA > > Accelerator Function Unit (AFU) for different functions. It > > returns uuid which could be used to identify which PR bitstream > > is programmed in this AFU. > > + > > +What: /sys/bus/platform/devices/dfl-port.0/power_state > > +Date: July 2019 > > +KernelVersion: 5.4 > > +Contact: Wu Hao <[email protected]> > > +Description: Read-only. It reports the APx (AFU Power) state, > > different APx > > + means different throttling level. When reading this file, it > > + returns "0" - Normal / "1" - AP1 / "2" - AP2 / "6" - AP6. > > + > > +What: /sys/bus/platform/devices/dfl-port.0/ap1_event > > +Date: July 2019 > > +KernelVersion: 5.4 > > +Contact: Wu Hao <[email protected]> > > +Description: Read-write. Read or set 1 to clear AP1 (AFU Power State > > 1) > > + event. It's used to indicate transient AP1 state. > > So reading the value changes the state of the system? That's almost > always never a good idea. > > Force userspace to write the value to change something. Otherwise all > libraries that use sysfs will be accidentally changing the state of your > system without you ever knowing it.
Oh.. I think the description makes some misunderstanding here, will fix it in the next version. This AP1/AP2 event will only be cleared by write 1 to it, read will not change the state. > > > + > > +What: /sys/bus/platform/devices/dfl-port.0/ap2_event > > +Date: July 2019 > > +KernelVersion: 5.4 > > +Contact: Wu Hao <[email protected]> > > +Description: Read-write. Read or set 1 to clear AP2 (AFU Power State > > 2) > > + event. It's used to indicate transient AP2 state. > > + > > +What: /sys/bus/platform/devices/dfl-port.0/ltr > > +Date: July 2019 > > +KernelVersion: 5.4 > > +Contact: Wu Hao <[email protected]> > > +Description: Read-write. Read and set AFU latency tolerance > > reporting value. > > + Set ltr to 1 if the AFU can tolerate latency >= 40us or set it > > + to 0 if it is latency sensitive. > > diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c > > index 68b4d08..cb3f73e 100644 > > --- a/drivers/fpga/dfl-afu-main.c > > +++ b/drivers/fpga/dfl-afu-main.c > > @@ -141,8 +141,145 @@ static int port_get_id(struct platform_device *pdev) > > } > > static DEVICE_ATTR_RO(id); > > > > +static ssize_t > > +ltr_show(struct device *dev, struct device_attribute *attr, char *buf) > > +{ > > + struct dfl_feature_platform_data *pdata = dev_get_platdata(dev); > > + void __iomem *base; > > + u64 v; > > + > > + base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER); > > + > > + mutex_lock(&pdata->lock); > > + v = readq(base + PORT_HDR_CTRL); > > + mutex_unlock(&pdata->lock); > > Why do you need a lock to call readq()? What are you protecting here? If this code is running on 32bit machine, readq will be replaced with 2 readl operation. If that is the case, should we protect the code against it? > > > > + > > + return sprintf(buf, "%x\n", (u8)FIELD_GET(PORT_CTRL_LATENCY, v)); > > +} > > + > > +static ssize_t > > +ltr_store(struct device *dev, struct device_attribute *attr, > > + const char *buf, size_t count) > > +{ > > + struct dfl_feature_platform_data *pdata = dev_get_platdata(dev); > > + void __iomem *base; > > + u8 ltr; > > + u64 v; > > + > > + if (kstrtou8(buf, 0, <r) || ltr > 1) > > + return -EINVAL; > > Are you doing anything with this value? If not, how about just using > the sysfs boolean read function and if it is 1, then do the clearing? > > Same for all other show/store functions in here. Sure, will fix this in the next version. Thanks a lot for the comments. Hao > > thanks, > > greg k-h
