Re: [PATCH 5/6] GenWQE Sysfs interfaces

From: Ryan Mallon
Date: Tue Nov 05 2013 - 15:42:40 EST


On 05/11/13 19:44, Frank Haverkamp wrote:

> Sysfs interfaces for the GenWQE card. There are attributes to query
> the version of the bitstream as well as some for the driver. For
> debugging, please also see the debugfs interfaces of this driver.
>
> Signed-off-by: Frank Haverkamp <haver@xxxxxxxxxxxxxxxxxx>
> Co-authors: Joerg-Stephan Vogt <jsvogt@xxxxxxxxxx>,
> Michael Jung <MIJUNG@xxxxxxxxxx>,
> Michael Ruettger <michael@xxxxxxxx>


Handful of small comments below,

~Ryan

> ---
> Documentation/ABI/testing/sysfs-driver-genwqe | 55 +++++
> drivers/misc/genwqe/card_sysfs.c | 290 +++++++++++++++++++++++++
> 2 files changed, 345 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/ABI/testing/sysfs-driver-genwqe
> create mode 100644 drivers/misc/genwqe/card_sysfs.c
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-genwqe b/Documentation/ABI/testing/sysfs-driver-genwqe
> new file mode 100644
> index 0000000..0976901
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-genwqe
> @@ -0,0 +1,55 @@
> +What: /sys/class/genwqe/genwqe<n>_card/version
> +Date: Oct 2013
> +Contact: haver@xxxxxxxxxxxxxxxxxx
> +Description: Unique bitstream identification e.g.
> + '0000000330336283.00000000475a4950'.
> +
> +What: /sys/class/genwqe/genwqe<n>_card/appid
> +Date: Oct 2013
> +Contact: haver@xxxxxxxxxxxxxxxxxx
> +Description: Identifies the currently active card application e.g. 'GZIP'
> + for compression/decompression.
> +
> +What: /sys/class/genwqe/genwqe<n>_card/type
> +Date: Oct 2013
> +Contact: haver@xxxxxxxxxxxxxxxxxx
> +Description: Type of the card e.g. 'GenWQE5-A7'.
> +
> +What: /sys/class/genwqe/genwqe<n>_card/cpld_version
> +Date: Oct 2013
> +Contact: haver@xxxxxxxxxxxxxxxxxx
> +Description: Version id of the cards CPLD.
> +
> +What: /sys/class/genwqe/genwqe<n>_card/curr_bitstream
> +Date: Oct 2013
> +Contact: haver@xxxxxxxxxxxxxxxxxx
> +Description: Currently active bitstream. 1 is default, 0 is backup.
> +
> +What: /sys/class/genwqe/genwqe<n>_card/next_bitstream
> +Date: Oct 2013
> +Contact: haver@xxxxxxxxxxxxxxxxxx
> +Description: Interface to set the next bitstream to be used.
> +
> +What: /sys/class/genwqe/genwqe<n>_card/tempsens
> +Date: Oct 2013
> +Contact: haver@xxxxxxxxxxxxxxxxxx
> +Description: Interface to read the cards temperature sense register.
> +
> +What: /sys/class/genwqe/genwqe<n>_card/state
> +Date: Oct 2013
> +Contact: haver@xxxxxxxxxxxxxxxxxx
> +Description: State of the card: "unused", "used", "error".
> +
> +What: /sys/class/genwqe/genwqe<n>_card/base_clock
> +Date: Oct 2013
> +Contact: haver@xxxxxxxxxxxxxxxxxx
> +Description: Base clock frequency of the card.
> +
> +What: /sys/class/genwqe/genwqe<n>_card/device/sriov_numvfs
> +Date: Oct 2013
> +Contact: haver@xxxxxxxxxxxxxxxxxx
> +Description: Enable VFs (1..15):
> + sudo sh -c 'echo 15 > \
> + /sys/bus/pci/devices/0000\:1b\:00.0/sriov_numvfs'
> + Disable VFs:
> + Write a 0 into the same sysfs entry.
> diff --git a/drivers/misc/genwqe/card_sysfs.c b/drivers/misc/genwqe/card_sysfs.c
> new file mode 100644
> index 0000000..f29ce55
> --- /dev/null
> +++ b/drivers/misc/genwqe/card_sysfs.c
> @@ -0,0 +1,290 @@
> +/**
> + * IBM Accelerator Family 'GenWQE'
> + *
> + * (C) Copyright IBM Corp. 2013
> + *
> + * Author: Frank Haverkamp <haver@xxxxxxxxxxxxxxxxxx>
> + * Author: Joerg-Stephan Vogt <jsvogt@xxxxxxxxxx>
> + * Author: Michael Jung <mijung@xxxxxxxxxx>
> + * Author: Michael Ruettger <michael@xxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License (version 2 only)
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +/*
> + * Sysfs interfaces for the GenWQE card. There are attributes to query
> + * the version of the bitstream as well as some for the driver. For
> + * debugging, please also see the debugfs interfaces of this driver.
> + */
> +
> +#include <linux/version.h>
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/string.h>
> +#include <linux/fs.h>
> +#include <linux/sysfs.h>
> +#include <linux/ctype.h>
> +#include <linux/device.h>
> +
> +#include "card_base.h"
> +#include "card_ddcb.h"
> +
> +static const char * const genwqe_types[] = {
> + [GENWQE_TYPE_ALTERA_230] = "GenWQE4-230",
> + [GENWQE_TYPE_ALTERA_530] = "GenWQE4-530",
> + [GENWQE_TYPE_ALTERA_A4] = "GenWQE5-A4",
> + [GENWQE_TYPE_ALTERA_A7] = "GenWQE5-A7",
> +};
> +
> +static ssize_t status_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct genwqe_dev *cd = dev_get_drvdata(dev);
> + const char *cs[GENWQE_CARD_STATE_MAX] = { "unused", "used", "error" };
> +
> + return scnprintf(buf, PAGE_SIZE, "%s\n", cs[cd->card_state]);
> +}
> +static DEVICE_ATTR_RO(status);
> +
> +static ssize_t appid_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + char app_name[5];
> + struct genwqe_dev *cd = dev_get_drvdata(dev);
> +
> + genwqe_read_app_id(cd, app_name, sizeof(app_name));
> + return scnprintf(buf, PAGE_SIZE, "%s\n", app_name);
> +}
> +static DEVICE_ATTR_RO(appid);
> +
> +static ssize_t version_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + u64 slu_id, app_id;
> + struct genwqe_dev *cd = dev_get_drvdata(dev);
> +
> + slu_id = __genwqe_readq(cd, IO_SLU_UNITCFG);
> + app_id = __genwqe_readq(cd, IO_APP_UNITCFG);
> +
> + return scnprintf(buf, PAGE_SIZE, "%016llx.%016llx\n", slu_id, app_id);
> +}
> +static DEVICE_ATTR_RO(version);
> +
> +static ssize_t type_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + u8 card_type;
> + struct genwqe_dev *cd = dev_get_drvdata(dev);
> +
> + card_type = genwqe_card_type(cd);
> + return scnprintf(buf, PAGE_SIZE, "%s\n",
> + (card_type >= ARRAY_SIZE(genwqe_types)) ?
> + "invalid" : genwqe_types[card_type]);
> +}
> +static DEVICE_ATTR_RO(type);
> +
> +static ssize_t driver_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + return scnprintf(buf, PAGE_SIZE, "%s\n", DRV_VERS_STRING);
> +}
> +static DEVICE_ATTR_RO(driver);
> +
> +static ssize_t tempsens_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + u64 tempsens;
> + struct genwqe_dev *cd = dev_get_drvdata(dev);
> +
> + tempsens = __genwqe_readq(cd, IO_SLU_TEMPERATURE_SENSOR);
> + return scnprintf(buf, PAGE_SIZE, "%016llx\n", tempsens);
> +}
> +static DEVICE_ATTR_RO(tempsens);
> +
> +static ssize_t base_clock_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + u64 base_clock;
> + struct genwqe_dev *cd = dev_get_drvdata(dev);
> +
> + base_clock = genwqe_base_clock_frequency(cd);
> + return scnprintf(buf, PAGE_SIZE, "%lld\n", base_clock);
> +}
> +static DEVICE_ATTR_RO(base_clock);
> +
> +/**
> + * curr_bitstream_show() - Show the current bitstream id
> + *
> + * There is a bug in some old versions of the CPLD which selects the
> + * bitstream, which causes the IO_SLU_BITSTREAM register to report
> + * unreliable data in very rare cases. This makes this sysfs
> + * unreliable up to the point were a new CPLD version is being used.
> + *
> + * Unfortunately there is no automatic way yet to query the CPLD
> + * version (See show_cpld_version() ;-)), such that you need to
> + * manually ensure via programming tools that you have a recent
> + * version of the CPLD software.
> + *
> + * The proposed circumvention is to use a special recovery bitstream
> + * on the backup partition (0) to identify problems while loading the
> + * image.
> + */
> +static ssize_t curr_bitstream_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + int curr_bitstream;
> + struct genwqe_dev *cd = dev_get_drvdata(dev);
> +
> + curr_bitstream = __genwqe_readq(cd, IO_SLU_BITSTREAM) & 0x1;
> + return scnprintf(buf, PAGE_SIZE, "%d\n", curr_bitstream);
> +}
> +static DEVICE_ATTR_RO(curr_bitstream);
> +
> +/**
> + * next_bitstream_show() - Show the next activated bitstream
> + *
> + * IO_SLC_CFGREG_SOFTRESET: This register can only be accessed by the PF.
> + */
> +static ssize_t next_bitstream_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + int next_bitstream;
> + struct genwqe_dev *cd = dev_get_drvdata(dev);
> +
> + switch ((cd->softreset & 0xcull) >> 2) {


The ull suffix isn't really necessary here, and can just be removed.

> + case 0x2:
> + next_bitstream = 0;
> + break;
> + case 0x3:
> + next_bitstream = 1;
> + break;
> + default:
> + next_bitstream = -1;
> + break; /* error */
> + }
> + return scnprintf(buf, PAGE_SIZE, "%d\n", next_bitstream);


sprintf is fine, a %d is not going to be larger than PAGE_SIZE.

> +}
> +
> +static ssize_t next_bitstream_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + u64 partition;
> + struct genwqe_dev *cd = dev_get_drvdata(dev);
> +
> + if (kstrtoull(buf, 0, &partition) < 0)
> + return -EINVAL;


Why are you using kstrtoull for a value which is only 0 or 1?

> +
> + switch (partition) {
> + case 0x0:
> + cd->softreset = 0x78ull;
> + break;
> + case 0x1:
> + cd->softreset = 0x7cull;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + __genwqe_writeq(cd, IO_SLC_CFGREG_SOFTRESET, cd->softreset);
> + return count;
> +}
> +static DEVICE_ATTR_RW(next_bitstream);
> +
> +/**
> + * FIXME Implement me!
> + */
> +static ssize_t cpld_version_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return scnprintf(buf, PAGE_SIZE, "unknown (FIXME)\n");
> +}
> +static DEVICE_ATTR_RO(cpld_version);


Why provide this if it doesn't work? User-space obviously can't do the
right thing with it, so just remove it until it gets implemented
properly.

> +
> +/*
> + * Create device_attribute structures / params: name, mode, show, store
> + * additional flag if valid in VF
> + */
> +static struct attribute *genwqe_attributes[] = {
> + &dev_attr_tempsens.attr,
> + &dev_attr_next_bitstream.attr,
> + &dev_attr_curr_bitstream.attr,
> + &dev_attr_base_clock.attr,
> + &dev_attr_driver.attr,
> + &dev_attr_type.attr,
> + &dev_attr_version.attr,
> + &dev_attr_appid.attr,
> + &dev_attr_status.attr,
> + &dev_attr_cpld_version.attr,
> + NULL,
> +};
> +
> +static struct attribute *genwqe_normal_attributes[] = {
> + &dev_attr_driver.attr,
> + &dev_attr_type.attr,
> + &dev_attr_version.attr,
> + &dev_attr_appid.attr,
> + &dev_attr_status.attr,
> + NULL,
> +};
> +
> +/**
> + * genwqe_is_visible() - Determine if sysfs attribute should be visible or not
> + *
> + * VFs have restricted mmio capabilities, so not all sysfs entries
> + * are allowed in VFs.
> + */
> +static umode_t genwqe_is_visible(struct kobject *kobj,
> + struct attribute *attr, int n)

> +{
> + unsigned int j;
> + struct device *dev = container_of(kobj, struct device, kobj);
> + struct genwqe_dev *cd = dev_get_drvdata(dev);
> + umode_t mode = attr->mode;
> +
> + if (genwqe_is_privileged(cd))
> + return mode;
> +
> + for (j = 0; genwqe_normal_attributes[j] != NULL; j++)
> + if (genwqe_normal_attributes[j] == attr)
> + return mode;
> +
> + return 0;
> +}
> +
> +static struct attribute_group genwqe_attribute_group = {
> + .is_visible = genwqe_is_visible,
> + .attrs = genwqe_attributes,
> +};
> +
> +/**
> + * genwqe_init_sysfs() - Setup sysfs entries of the card device
> + */
> +int genwqe_init_sysfs(struct genwqe_dev *cd)
> +{
> + int rc;
> +
> + rc = sysfs_create_group(&cd->dev->kobj, &genwqe_attribute_group);
> + if (rc)
> + return -ENXIO;
> +
> + return 0;
> +}
> +
> +/**
> + * genwqe_exit_sysfs() - Remove sysfs entries
> + */
> +void genwqe_exit_sysfs(struct genwqe_dev *cd)
> +{
> + sysfs_remove_group(&cd->dev->kobj, &genwqe_attribute_group);
> +}


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/