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 <ananda.ravuri@xxxxxxxxx>
> > Signed-off-by: Xu Yilun <yilun.xu@xxxxxxxxx>
> > Signed-off-by: Wu Hao <hao.wu@xxxxxxxxx>
> > Acked-by: Alan Tull <atull@xxxxxxxxxx>
> > Signed-off-by: Moritz Fischer <mdf@xxxxxxxxxx>
> > ---
> > 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 <hao.wu@xxxxxxxxx>
> > +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 <hao.wu@xxxxxxxxx>
> > +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 <hao.wu@xxxxxxxxx>
> > +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 <hao.wu@xxxxxxxxx>
> > +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
