On 30/10/13 20:32, Frank Haverkamp wrote:
> From: Frank Haverkamp <ha...@vnet.ibm.com>
> Signed-off-by: Frank Haverkamp <ha...@linux.vnet.ibm.com>
> Co-authors: Joerg-Stephan Vogt <jsv...@de.ibm.com>,
> Michael Jung <mij...@de.ibm.com>,
> Michael Ruettger <mich...@ibmra.de>
> ---
> Documentation/ABI/testing/debugfs-driver-genwqe | 57 +
> Documentation/ABI/testing/sysfs-driver-genwqe | 46 +
> drivers/misc/Kconfig | 1 +
> drivers/misc/Makefile | 1 +
> drivers/misc/genwqe/Kconfig | 23 +
> drivers/misc/genwqe/Makefile | 8 +
> drivers/misc/genwqe/card_base.c | 1238 +++++++++++++++++++
> drivers/misc/genwqe/card_base.h | 569 +++++++++
> drivers/misc/genwqe/card_ddcb.c | 1380 +++++++++++++++++++++
> drivers/misc/genwqe/card_ddcb.h | 188 +++
> drivers/misc/genwqe/card_debugfs.c | 566 +++++++++
> drivers/misc/genwqe/card_dev.c | 1499
> +++++++++++++++++++++++
> drivers/misc/genwqe/card_sysfs.c | 306 +++++
> drivers/misc/genwqe/card_utils.c | 983 +++++++++++++++
> drivers/misc/genwqe/genwqe_driver.h | 75 ++
> include/linux/genwqe/genwqe_card.h | 559 +++++++++
> 16 files changed, 7499 insertions(+), 0 deletions(-)
Please consider breaking this into multiple patches. One behemoth patch
is harder to review and less likely for people to read all of it. Since
this adds a new driver, you can add it in parts, e.g: core, sysfs,
documentation, etc, and add the Makefile/Kconfig bits in the final
patch. That way it won't break the build at any point.
Couple of comments on the sysfs bits below.
~Ryan
> +++ b/drivers/misc/genwqe/card_sysfs.c
> @@ -0,0 +1,306 @@
> +/**
> + * IBM Accelerator Family 'GenWQE'
> + *
> + * (C) Copyright IBM Corp. 2013
> + *
> + * Author: Frank Haverkamp <ha...@linux.vnet.ibm.com>
> + * Author: Joerg-Stephan Vogt <jsv...@de.ibm.com>
> + * Author: Michael Jung <mij...@de.ibm.com>
> + * Author: Michael Ruettger <mich...@ibmra.de>
> + *
> + * 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. Additionally there are some attributes which help to debug
> + * potential problems.
> + */
> +
> +#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 "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 show_card_status(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + ssize_t len = 0;
> + struct genwqe_dev *cd = dev_get_drvdata(dev);
> + const char *cs[GENWQE_CARD_STATE_MAX] = { "unused", "used", "error" };
> +
> + len += scnprintf(&buf[len], PAGE_SIZE - len,
> + "%s\n", cs[cd->card_state]);
> + return len;
This is a bit confusingly written. Why do:
scnprintf(&buf[len], ...
When len is always zero at that point? Since you are printing from an
array of statically sized strings that are guaranteed to be smaller than
PAGE_SIZE, you can safely do:
sprintf(buf, "%s\n", cs[cd->card_state]);
You might want to add some checking for cd->card_state being out of
bounds, and printing "unknown" or something in that case.
Same issue exists in other sysfs handlers.
> +}
> +
> +static ssize_t show_card_appid(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + ssize_t len = 0;
> + char app_name[5];
> + struct genwqe_dev *cd = dev_get_drvdata(dev);
> +
> + genwqe_read_app_id(cd, app_name, sizeof(app_name));
> + len += scnprintf(&buf[len], PAGE_SIZE - len,
> + "%s\n", app_name);
> + return len;
> +}
> +
> +static ssize_t show_card_version(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + ssize_t len = 0;
> + 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);
> +
> + len += scnprintf(&buf[len], PAGE_SIZE - len,
> + "%016llx.%016llx\n", slu_id, app_id);
> + return len;
> +}
> +
> +/**
> + * FIXME Implement me!
> + */
> +static ssize_t show_cpld_version(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + ssize_t len = 0;
> + len += scnprintf(&buf[len], PAGE_SIZE - len,
> + "unknown (FIXME)\n");
> + return len;
> +}
> +
> +static ssize_t show_card_type(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + ssize_t len = 0;
> + u8 card_type;
> + struct genwqe_dev *cd = dev_get_drvdata(dev);
> +
> + card_type = genwqe_card_type(cd);
> + len += scnprintf(&buf[len], PAGE_SIZE - len,
> + "%s\n", (card_type >= ARRAY_SIZE(genwqe_types)) ?
> + "invalid" : genwqe_types[card_type]);
> + return len;
> +}
> +
> +static ssize_t show_card_driver(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + ssize_t len = 0;
> + len += scnprintf(&buf[len], PAGE_SIZE - len,
> + "%s\n", DRV_VERS_STRING);
> + return len;
> +}
> +
> +static ssize_t show_card_tempsens(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + ssize_t len = 0;
> + u64 tempsens;
> + struct genwqe_dev *cd = dev_get_drvdata(dev);
> +
> + tempsens = __genwqe_readq(cd, IO_SLU_TEMPERATURE_SENSOR);
> + len += scnprintf(&buf[len], PAGE_SIZE - len,
> + "%016llx\n", tempsens);
> + return len;
> +}
> +
> +static ssize_t show_card_base_clock(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + ssize_t len = 0;
> + u64 base_clock;
> + struct genwqe_dev *cd = dev_get_drvdata(dev);
> +
> + base_clock = genwqe_base_clock_frequency(cd);
> + len += scnprintf(&buf[len], PAGE_SIZE - len,
> + "%lld\n", base_clock);
> + return len;
> +}
> +
> +/**
> + * show_card_curr_bitstream() - 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 show_card_curr_bitstream(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + ssize_t len = 0;
> + int curr_bitstream;
> + struct genwqe_dev *cd = dev_get_drvdata(dev);
> +
> + curr_bitstream = __genwqe_readq(cd, IO_SLU_BITSTREAM) & 0x1;
> + len += scnprintf(&buf[len], PAGE_SIZE - len,
> + "%d\n", curr_bitstream);
> + return len;
> +}
> +
> +/**
> + * show_card_next_bitstream() - Show the next activated bitstream
> + *
> + * IO_SLC_CFGREG_SOFTRESET: This register can only be accessed by the PF.
> + */
> +static ssize_t show_card_next_bitstream(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + ssize_t len = 0;
> + int next_bitstream;
> + struct genwqe_dev *cd = dev_get_drvdata(dev);
> +
> + switch ((cd->softreset & 0xCull) >> 2) {
> + case 0x2:
> + next_bitstream = 0;
> + break;
> + case 0x3:
> + next_bitstream = 1;
> + break;
> + default:
> + next_bitstream = -1;
> + break; /* error */
> + }
> + len += scnprintf(&buf[len], PAGE_SIZE - len,
> + "%d\n", next_bitstream);
> + return len;
> +}
> +
> +static ssize_t store_card_next_bitstream(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;
Does this give a type error? partition should be unsigned long long,
which is _at least_ 64 bits.
> +
> + 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;
> +}
> +
> +/*
> + * Create device_attribute structures / params: name, mode, show, store
> + * additional flag if valid in VF
> + */
> +struct genwqe_dev_attrib {
> + struct device_attribute att; /* sysfs entry attributes */
> + int vf; /* may exist in VF or not */
> +};
> +
> +static struct genwqe_dev_attrib dev_attr_tab[] = {
> + {__ATTR(tempsens, S_IRUGO, show_card_tempsens, NULL), 0},
> + {__ATTR(next_bitstream, (S_IRUGO | S_IWUSR),
> + show_card_next_bitstream, store_card_next_bitstream), 0},
> + {__ATTR(curr_bitstream, S_IRUGO, show_card_curr_bitstream, NULL), 0},
> + {__ATTR(cpld_version, S_IRUGO, show_cpld_version, NULL), 0},
> + {__ATTR(base_clock, S_IRUGO, show_card_base_clock, NULL), 0},
> +
> + {__ATTR(driver, S_IRUGO, show_card_driver, NULL), 1},
> + {__ATTR(type, S_IRUGO, show_card_type, NULL), 1},
> + {__ATTR(version, S_IRUGO, show_card_version, NULL), 1},
> + {__ATTR(appid, S_IRUGO, show_card_appid, NULL), 1},
> + {__ATTR(status, S_IRUGO, show_card_status, NULL), 1},
> +};
> +
> +/**
> + * create_card_sysfs() - Setup sysfs entries of the card device
> + *
> + * VFs have restricted mmio capabilities, so not all sysfs entries
> + * are allowed in VFs.
> + */
> +int create_card_sysfs(struct genwqe_dev *cd)
This is extern, and has a very generic name. Prefix it with genwqe or
something. Same for the function below, and for any other extern symbols
you have.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
