Re: [PATCH 8 of 20] ipath - sysfs support for core driver

Re: [PATCH 8 of 20] ipath - sysfs support for core driver

[Pavel Machek]

Hi!

> I'm thinking at this point that I should just route this information
> through the /dev/ipath_sma char device, and maybe
> add /dev/ipath_counters%d and /dev/ipath_stats to go with it.  I think
> that's a pretty crummy approach that sysfs solves more cleanly, but
> there you go.

Too bad for your leg, I'm afraid :-).
								Pavel

-- 
Thanks, Sharp!

Re: [PATCH 8 of 20] ipath - sysfs support for core driver

[Roland Dreier]

 > +static ssize_t show_version(struct device_driver *dev, char *buf)
 > +{
 > +	return scnprintf(buf, PAGE_SIZE, "%s", ipath_core_version);
 > +}

Any reason you left a "\n" off of this attribute?

 > +static ssize_t show_atomic_stats(struct device_driver *dev, char *buf)
 > +{
 > +	memcpy(buf, &ipath_stats, sizeof(ipath_stats));
 > +
 > +	return sizeof(ipath_stats);
 > +}

I think putting a whole binary struct in a sysfs attribute is
considered a no-no.

 > +static ssize_t show_boardversion(struct device *dev,
 > +			       struct device_attribute *attr,
 > +			       char *buf)
 > +{
 > +	struct ipath_devdata *dd = dev_get_drvdata(dev);
 > +	return scnprintf(buf, PAGE_SIZE, "%s", dd->ipath_boardversion);
 > +}

Another missing "\n"

Re: [PATCH 8 of 20] ipath - sysfs support for core driver

[Bryan O'Sullivan]

On Thu, 2006-03-09 at 15:18 -0800, Roland Dreier wrote:
>  > +static ssize_t show_version(struct device_driver *dev, char *buf)
>  > +{
>  > +	return scnprintf(buf, PAGE_SIZE, "%s", ipath_core_version);
>  > +}
> 
> Any reason you left a "\n" off of this attribute?

Nope, just a bogon.

>  > +static ssize_t show_atomic_stats(struct device_driver *dev, char *buf)
>  > +{
>  > +	memcpy(buf, &ipath_stats, sizeof(ipath_stats));
>  > +
>  > +	return sizeof(ipath_stats);
>  > +}
> 
> I think putting a whole binary struct in a sysfs attribute is
> considered a no-no.

Grumble.  it's a fairly small struct, much less than a page in length,
and userspace needs an atomic view of it, instead of reading each of the
umpteen broken-out files that we also provide for humean-readable access
to each counter.

I didn't see any point to implementing the sysfs binary file interface
in order to do exactly what this 6-line function does.  Still don't, in
fact :-)

> Another missing "\n"

Thanks.

	<b

Re: [PATCH 8 of 20] ipath - sysfs support for core driver

[Greg KH]

On Thu, Mar 09, 2006 at 03:18:49PM -0800, Roland Dreier wrote:

Thanks for CC:ing me, but where were the originals of these posted?

>  > +static ssize_t show_version(struct device_driver *dev, char *buf)
>  > +{
>  > +	return scnprintf(buf, PAGE_SIZE, "%s", ipath_core_version);
>  > +}
> 
> Any reason you left a "\n" off of this attribute?
> 
>  > +static ssize_t show_atomic_stats(struct device_driver *dev, char *buf)
>  > +{
>  > +	memcpy(buf, &ipath_stats, sizeof(ipath_stats));
>  > +
>  > +	return sizeof(ipath_stats);
>  > +}
> 
> I think putting a whole binary struct in a sysfs attribute is
> considered a no-no.

That's an understatement, where is the large stick to thwap the author
of this code...

thanks,

greg k-h

Re: [PATCH 8 of 20] ipath - sysfs support for core driver

[Roland Dreier]

    Greg> wrote: Thanks for CC:ing me, but where were the originals of
    Greg> these posted?

I think Bryan's original script was busted, so even though everyone
ended in the To: line (and hence in my reply-to-all), the mail didn't
go everywhere.  In fact I might have been the only one to get it.

Bryan, is it worth reposting the series so that everyone can see what
we're talking about?

 - R.

Re: [PATCH 8 of 20] ipath - sysfs support for core driver

[Bryan O'Sullivan]

On Thu, 2006-03-09 at 15:46 -0800, Greg KH wrote:
> On Thu, Mar 09, 2006 at 03:18:49PM -0800, Roland Dreier wrote:
> 
> Thanks for CC:ing me, but where were the originals of these posted?

My patch posting script screwed up.  Only Roland got them, even though
the envelopes were all correct.

> >  > +static ssize_t show_atomic_stats(struct device_driver *dev, char *buf)
> >  > +{
> >  > +	memcpy(buf, &ipath_stats, sizeof(ipath_stats));
> >  > +
> >  > +	return sizeof(ipath_stats);
> >  > +}
> > 
> > I think putting a whole binary struct in a sysfs attribute is
> > considered a no-no.
> 
> That's an understatement, where is the large stick to thwap the author
> of this code...

I'd like to understand why, though.  As I already explained, it's a
smallish structure (< 1KB), and I can use the special binary sysfs
attribute goo for it if you insist, but ... why?

	<b

Revenge of the sysfs maintainer! (was Re: [PATCH 8 of 20] ipath - sysfs support for core driver)

[Greg KH]

On Thu, Mar 09, 2006 at 03:32:23PM -0800, Bryan O'Sullivan wrote:
> On Thu, 2006-03-09 at 15:18 -0800, Roland Dreier wrote:
> >  > +static ssize_t show_atomic_stats(struct device_driver *dev, char *buf)
> >  > +{
> >  > +	memcpy(buf, &ipath_stats, sizeof(ipath_stats));
> >  > +
> >  > +	return sizeof(ipath_stats);
> >  > +}
> > 
> > I think putting a whole binary struct in a sysfs attribute is
> > considered a no-no.
> 
> Grumble.

Grumble?  Oh come on, don't export binary structures through sysfs, it's
in the DOCUMENTATION THAT SYSFS IS FOR TEXT FILES ONLY!!!!

If you don't want to export a text file, then use something else other
than sysfs, it's that simple.

> it's a fairly small struct, much less than a page in length,
> and userspace needs an atomic view of it, instead of reading each of the
> umpteen broken-out files that we also provide for humean-readable access
> to each counter.
> 
> I didn't see any point to implementing the sysfs binary file interface
> in order to do exactly what this 6-line function does.  Still don't, in
> fact :-)

sysfs binary files are for PASS-THROUGH things ONLY!  stuff like this is
NOT for sysfs binary files, so even if you switched to using it, it
would not be allowed.

And if I sound grumpy about this whole thing, I am.  I'm tired of people
trying to abuse sysfs and putting crappy userspace APIs in it.  They
don't realize how messy it causes things to be over the long run.  If
you want to make your own filesystem to export stuff in whatever way you
want, feel free to do so (only takes about 200 lines including comments
to do so.)  But DO NOT ABUSE SYSFS just because you don't happen to
agree with the way it was designed, or feel inconvienced by it.

Ok, here's a new rule to help this from happening again in the future:

  If you want to add a new sysfs file to the kernel, it MUST be
  accompanied with full documentation that explains exactly what that
  file contains and what it is for.  No exceptions will be allowed.

Structure for this documentation will be in the format that I layed out
last week, I'll update it again and post it to lkml for review later
tonight.

thanks,

greg k-h

[PATCH 8 of 20] ipath - sysfs support for core driver

[Bryan O'Sullivan]

Signed-off-by: Bryan O'Sullivan <bos@pathscale.com>

diff -r a9ed49ad489c -r 1123028ac13a drivers/infiniband/hw/ipath/ipath_sysfs.c
--- /dev/null	Thu Jan  1 00:00:00 1970 +0000
+++ b/drivers/infiniband/hw/ipath/ipath_sysfs.c	Thu Mar  9 16:15:57 2006 -0800
@@ -0,0 +1,950 @@
+/*
+ * Copyright (c) 2003, 2004, 2005, 2006 PathScale, Inc. All rights reserved.
+ *
+ * This software is available to you under a choice of one of two
+ * licenses.  You may choose to be licensed under the terms of the GNU
+ * General Public License (GPL) Version 2, available from the file
+ * COPYING in the main directory of this source tree, or the
+ * OpenIB.org BSD license below:
+ *
+ *     Redistribution and use in source and binary forms, with or
+ *     without modification, are permitted provided that the following
+ *     conditions are met:
+ *
+ *      - Redistributions of source code must retain the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer.
+ *
+ *      - Redistributions in binary form must reproduce the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer in the documentation and/or other materials
+ *        provided with the distribution.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include <linux/ctype.h>
+#include <linux/pci.h>
+
+#include "ipath_kernel.h"
+#include "ips_common.h"
+#include "ipath_layer.h"
+
+/**
+ * ipath_parse_ushort - parse an unsigned short value in an arbitrary base
+ * @str: the string containing the number
+ * @valp: where to put the result
+ *
+ * returns the number of bytes consumed, or negative value on error
+ */
+int ipath_parse_ushort(const char *str, unsigned short *valp)
+{
+	unsigned long val;
+	char *end;
+	int ret;
+
+	if (!isdigit(str[0]))
+		return -EINVAL;
+
+	val = simple_strtoul(str, &end, 0);
+
+	if (val > 0xffff)
+		return -EINVAL;
+
+	*valp = val;
+
+	ret = end + 1 - str;
+	if (ret == 0)
+		ret = -EINVAL;
+
+	return ret;
+}
+
+static ssize_t show_version(struct device_driver *dev, char *buf)
+{
+	/* The string printed here is already newline-terminated. */
+	return scnprintf(buf, PAGE_SIZE, "%s", ipath_core_version);
+}
+
+static ssize_t show_num_units(struct device_driver *dev, char *buf)
+{
+	return scnprintf(buf, PAGE_SIZE, "%d\n",
+			 ipath_count_units(NULL, NULL, NULL));
+}
+
+#define DRIVER_STAT(name, attr) \
+	static ssize_t show_stat_##name(struct device_driver *dev, \
+					char *buf) \
+	{ \
+		return scnprintf( \
+			buf, PAGE_SIZE, "%llu\n", \
+			(unsigned long long) ipath_stats.sps_ ##attr); \
+	} \
+	static DRIVER_ATTR(name, S_IRUGO, show_stat_##name, NULL)
+
+DRIVER_STAT(intrs, ints);
+DRIVER_STAT(err_intrs, errints);
+DRIVER_STAT(errs, errs);
+DRIVER_STAT(pkt_errs, pkterrs);
+DRIVER_STAT(crc_errs, crcerrs);
+DRIVER_STAT(hw_errs, hwerrs);
+DRIVER_STAT(ib_link, iblink);
+DRIVER_STAT(port0_pkts, port0pkts);
+DRIVER_STAT(ether_spkts, ether_spkts);
+DRIVER_STAT(ether_rpkts, ether_rpkts);
+DRIVER_STAT(sma_spkts, sma_spkts);
+DRIVER_STAT(sma_rpkts, sma_rpkts);
+DRIVER_STAT(hdrq_full, hdrqfull);
+DRIVER_STAT(etid_full, etidfull);
+DRIVER_STAT(no_piobufs, nopiobufs);
+DRIVER_STAT(ports, ports);
+DRIVER_STAT(pkey0, pkeys[0]);
+DRIVER_STAT(pkey1, pkeys[1]);
+DRIVER_STAT(pkey2, pkeys[2]);
+DRIVER_STAT(pkey3, pkeys[3]);
+/* XXX fix the following when dynamic table of devices used */
+DRIVER_STAT(lid0, lid[0]);
+DRIVER_STAT(lid1, lid[1]);
+DRIVER_STAT(lid2, lid[2]);
+DRIVER_STAT(lid3, lid[3]);
+
+DRIVER_STAT(nports, nports);
+DRIVER_STAT(null_intr, nullintr);
+DRIVER_STAT(max_pkts_call, maxpkts_call);
+DRIVER_STAT(avg_pkts_call, avgpkts_call);
+DRIVER_STAT(page_locks, pagelocks);
+DRIVER_STAT(page_unlocks, pageunlocks);
+DRIVER_STAT(krdrops, krdrops);
+/* XXX fix the following when dynamic table of devices used */
+DRIVER_STAT(mlid0, mlid[0]);
+DRIVER_STAT(mlid1, mlid[1]);
+DRIVER_STAT(mlid2, mlid[2]);
+DRIVER_STAT(mlid3, mlid[3]);
+
+static struct attribute *driver_stat_attributes[] = {
+	&driver_attr_intrs.attr,
+	&driver_attr_err_intrs.attr,
+	&driver_attr_errs.attr,
+	&driver_attr_pkt_errs.attr,
+	&driver_attr_crc_errs.attr,
+	&driver_attr_hw_errs.attr,
+	&driver_attr_ib_link.attr,
+	&driver_attr_port0_pkts.attr,
+	&driver_attr_ether_spkts.attr,
+	&driver_attr_ether_rpkts.attr,
+	&driver_attr_sma_spkts.attr,
+	&driver_attr_sma_rpkts.attr,
+	&driver_attr_hdrq_full.attr,
+	&driver_attr_etid_full.attr,
+	&driver_attr_no_piobufs.attr,
+	&driver_attr_ports.attr,
+	&driver_attr_pkey0.attr,
+	&driver_attr_pkey1.attr,
+	&driver_attr_pkey2.attr,
+	&driver_attr_pkey3.attr,
+	&driver_attr_lid0.attr,
+	&driver_attr_lid1.attr,
+	&driver_attr_lid2.attr,
+	&driver_attr_lid3.attr,
+	&driver_attr_nports.attr,
+	&driver_attr_null_intr.attr,
+	&driver_attr_max_pkts_call.attr,
+	&driver_attr_avg_pkts_call.attr,
+	&driver_attr_page_locks.attr,
+	&driver_attr_page_unlocks.attr,
+	&driver_attr_krdrops.attr,
+	&driver_attr_mlid0.attr,
+	&driver_attr_mlid1.attr,
+	&driver_attr_mlid2.attr,
+	&driver_attr_mlid3.attr,
+	NULL
+};
+
+static struct attribute_group driver_stat_attr_group = {
+	.name = "stats",
+	.attrs = driver_stat_attributes
+};
+
+static ssize_t show_atomic_stats(struct device_driver *dev, char *buf)
+{
+	memcpy(buf, &ipath_stats, sizeof(ipath_stats));
+
+	return sizeof(ipath_stats);
+}
+
+static ssize_t show_status(struct device *dev,
+			   struct device_attribute *attr,
+			   char *buf)
+{
+	struct ipath_devdata *dd = dev_get_drvdata(dev);
+
+	if (!dd->ipath_statusp)
+		return -EINVAL;
+
+	return scnprintf(buf, PAGE_SIZE, "0x%llx\n",
+			 (unsigned long long) *(dd->ipath_statusp));
+}
+
+static const char *ipath_status_str[] = {
+	"Initted",
+	"Disabled",
+	"4",			/* unused */
+	"OIB_SMA",
+	"SMA",
+	"Present",
+	"IB_link_up",
+	"IB_configured",
+	"NoIBcable",
+	"Fatal_Hardware_Error",
+	NULL,
+};
+
+static ssize_t show_status_str(struct device *dev,
+			       struct device_attribute *attr,
+			       char *buf)
+{
+	struct ipath_devdata *dd = dev_get_drvdata(dev);
+	int i, any;
+	u64 s;
+
+	if (!dd->ipath_statusp)
+		return -EINVAL;
+
+	s = *(dd->ipath_statusp);
+	*buf = '\0';
+	for (any = i = 0; s && ipath_status_str[i]; i++) {
+		if (s & 1) {
+			if (any && strlcat(buf, " ", PAGE_SIZE) >=
+			    PAGE_SIZE)
+				/* overflow */
+				break;
+			if (strlcat(buf, ipath_status_str[i],
+				    PAGE_SIZE) >= PAGE_SIZE)
+				break;
+			any = 1;
+		}
+		s >>= 1;
+	}
+	if (any)
+		strlcat(buf, "\n", PAGE_SIZE);
+
+	return strlen(buf);
+}
+
+static ssize_t show_boardversion(struct device *dev,
+			       struct device_attribute *attr,
+			       char *buf)
+{
+	struct ipath_devdata *dd = dev_get_drvdata(dev);
+	/* The string printed here is already newline-terminated. */
+	return scnprintf(buf, PAGE_SIZE, "%s", dd->ipath_boardversion);
+}
+
+static ssize_t show_node_info(struct device *dev,
+			       struct device_attribute *attr,
+			       char *buf)
+{
+	static const size_t count = 10;
+	struct ipath_devdata *dd = dev_get_drvdata(dev);
+	u32 *nodeinfo;
+	int ret;
+
+	if (!dd->ipath_statusp) {
+		ret = -EINVAL;
+		goto bail;
+	}
+
+	nodeinfo = (u32 *) buf;
+
+	/* so we only initialize non-zero fields. */
+	memset(nodeinfo, 0, count * sizeof(u32));
+
+	nodeinfo[0] =		/* BaseVersion is SMA */
+		/* ClassVersion is SMA */
+		(1 << 8)		/* NodeType  */
+		|(1 << 0);		/* NumPorts */
+	nodeinfo[1] = (u32) (dd->ipath_guid >> 32);
+	nodeinfo[2] = (u32) (dd->ipath_guid & 0xffffffff);
+	/* PortGUID == SystemImageGUID for us */
+	nodeinfo[3] = nodeinfo[1];
+	/* PortGUID == SystemImageGUID for us */
+	nodeinfo[4] = nodeinfo[2];
+	/* PortGUID == NodeGUID for us */
+	nodeinfo[5] = nodeinfo[3];
+	/* PortGUID == NodeGUID for us */
+	nodeinfo[6] = nodeinfo[4];
+	nodeinfo[7] = (4 << 16)	/* we support 4 pkeys */
+		|(dd->ipath_deviceid << 0);
+	/* our chip version as 16 bits major, 16 bits minor */
+	nodeinfo[8] = dd->ipath_minrev | (dd->ipath_majrev << 16);
+	nodeinfo[9] = (dd->ipath_unit << 24) | (dd->ipath_vendorid << 0);
+
+	ret = count * sizeof(u32);
+bail:
+	return ret;
+}
+
+static ssize_t show_port_info(struct device *dev,
+			       struct device_attribute *attr,
+			       char *buf)
+{
+	static const size_t count = 13;
+	int ret;
+	u32 tmp, tmp2;
+	struct ipath_devdata *dd = dev_get_drvdata(dev);
+	u32 *portinfo;
+
+	if (!dd->ipath_statusp) {
+		ret = -EINVAL;
+		goto bail;
+	}
+
+	portinfo = (u32 *) buf;
+
+	/* so we only initialize non-zero fields. */
+	memset(portinfo, 0, count * sizeof portinfo);
+
+	/*
+	 * Notimpl yet M_Key (64)
+	 * Notimpl yet GID (64)
+	 */
+
+	portinfo[4] = (dd->ipath_lid << 16);
+
+	/*
+	 * Notimpl yet SMLID (should we store this in the driver, in case
+	 * SMA dies?)  CapabilityMask is 0, we don't support any of these
+	 * DiagCode is 0; we don't store any diag info for now Notimpl yet
+	 * M_KeyLeasePeriod (we don't support M_Key)
+	 */
+
+	/* LocalPortNum is whichever port number they ask for */
+	portinfo[7] = (dd->ipath_unit << 24)
+		/* LinkWidthEnabled */
+		| (2 << 16)
+		/* LinkWidthSupported (really 2, but not IB valid) */
+		| (3 << 8)
+		/* LinkWidthActive */
+		| (2 << 0);
+	tmp = dd->ipath_lastibcstat & IPATH_IBSTATE_MASK;
+	tmp2 = 5;
+	if (tmp == IPATH_IBSTATE_INIT)
+		tmp = 2;
+	else if (tmp == IPATH_IBSTATE_ARM)
+		tmp = 3;
+	else if (tmp == IPATH_IBSTATE_ACTIVE)
+		tmp = 4;
+	else {
+		tmp = 0;	/* down */
+		tmp2 = tmp & 0xf;
+	}
+
+	portinfo[8] = (1 << 28)	/* LinkSpeedSupported */
+		|(tmp << 24)	/* PortState */
+		|(tmp2 << 20)	/* PortPhysicalState */
+		|(2 << 16)
+
+		/* LinkDownDefaultState */
+		/* M_KeyProtectBits == 0 */
+		/* NotImpl yet LMC == 0 (we can support all values) */
+		| (1 << 4)		/* LinkSpeedActive */
+		|(1 << 0);		/* LinkSpeedEnabled */
+	switch (dd->ipath_ibmtu) {
+	case 4096:
+		tmp = 5;
+		break;
+	case 2048:
+		tmp = 4;
+		break;
+	case 1024:
+		tmp = 3;
+		break;
+	case 512:
+		tmp = 2;
+		break;
+	case 256:
+		tmp = 1;
+		break;
+	default:		/* oops, something is wrong */
+		ipath_dbg("Problem, ipath_ibmtu 0x%x not a valid IB MTU, "
+			  "treat as 2048\n", dd->ipath_ibmtu);
+		tmp = 4;
+		break;
+	}
+	portinfo[9] = (tmp << 28)
+		/* NeighborMTU */
+		/* Notimpl MasterSMSL */
+		| (1 << 20)
+
+		/* VLCap */
+		/* Notimpl InitType (actually, an SMA decision) */
+		/* VLHighLimit is 0 (only one VL) */
+		; /* VLArbitrationHighCap is 0 (only one VL) */
+	portinfo[10] =		/* VLArbitrationLowCap is 0 (only one VL) */
+		/* InitTypeReply is SMA decision */
+		(5 << 16)		/* MTUCap 4096 */
+		|(7 << 13)		/* VLStallCount */
+		|(0x1f << 8)	/* HOQLife */
+		|(1 << 4)
+
+		/* OperationalVLs 0 */
+		/* PartitionEnforcementInbound */
+		/* PartitionEnforcementOutbound not enforced */
+		/* FilterRawinbound not enforced */
+		;			/* FilterRawOutbound not enforced */
+	/* M_KeyViolations are not counted by hardware, SMA can count */
+	tmp = ipath_read_creg32(dd, dd->ipath_cregs->cr_errpkey);
+	/* P_KeyViolations are counted by hardware. */
+	portinfo[11] = ((tmp & 0xffff) << 0);
+	portinfo[12] =
+		/* Q_KeyViolations are not counted by hardware */
+		(1 << 8)
+
+		/* GUIDCap */
+		/* SubnetTimeOut handled by SMA */
+		/* RespTimeValue handled by SMA */
+		;
+	/* LocalPhyErrors are programmed to max */
+	portinfo[12] |= (0xf << 20)
+		| (0xf << 16)	/* OverRunErrors are programmed to max */
+		;
+
+	ret = count * sizeof(u32);
+bail:
+	return ret;
+}
+
+static ssize_t show_lid(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	struct ipath_devdata *dd = dev_get_drvdata(dev);
+
+	return scnprintf(buf, PAGE_SIZE, "0x%x\n", dd->ipath_lid);
+}
+
+static ssize_t store_lid(struct device *dev,
+			 struct device_attribute *attr,
+			  const char *buf,
+			  size_t count)
+{
+	struct ipath_devdata *dd = dev_get_drvdata(dev);
+	u16 lid;
+	int ret;
+
+	ret = ipath_parse_ushort(buf, &lid);
+	if (ret < 0)
+		goto invalid;
+
+	if (lid == 0 || lid >= 0xc000) {
+		ret = -EINVAL;
+		goto invalid;
+	}
+
+	ipath_set_sps_lid(dd, lid, 0);
+
+	goto bail;
+invalid:
+	dev_err(dev, "attempt to set invalid LID\n");
+bail:
+	return ret;
+}
+
+static ssize_t show_mlid(struct device *dev,
+			 struct device_attribute *attr,
+			 char *buf)
+{
+	struct ipath_devdata *dd = dev_get_drvdata(dev);
+
+	return scnprintf(buf, PAGE_SIZE, "0x%x\n", dd->ipath_mlid);
+}
+
+static ssize_t store_mlid(struct device *dev,
+			 struct device_attribute *attr,
+			  const char *buf,
+			  size_t count)
+{
+	struct ipath_devdata *dd = dev_get_drvdata(dev);
+	int unit;
+	u16 mlid;
+	int ret;
+
+	ret = ipath_parse_ushort(buf, &mlid);
+	if (ret < 0)
+		goto invalid;
+
+	unit = dd->ipath_unit;
+
+	dd->ipath_mlid = mlid;
+	ipath_stats.sps_mlid[unit] = mlid;
+	if (dd->ipath_layer.l_intr)
+		dd->ipath_layer.l_intr(unit, IPATH_LAYER_INT_BCAST);
+
+	goto bail;
+invalid:
+	dev_err(dev, "attempt to set invalid MLID\n");
+bail:
+	return ret;
+}
+
+static ssize_t show_guid(struct device *dev,
+			 struct device_attribute *attr,
+			 char *buf)
+{
+	struct ipath_devdata *dd = dev_get_drvdata(dev);
+	u8 *guid;
+
+	guid = (u8 *) & (dd->ipath_guid);
+
+	return scnprintf(buf, PAGE_SIZE,
+			 "%02x:%02x:%02x:%02x:%02x:%02x:%02x:%02x\n",
+			 guid[0], guid[1], guid[2], guid[3],
+			 guid[4], guid[5], guid[6], guid[7]);
+}
+
+static ssize_t store_guid(struct device *dev,
+			 struct device_attribute *attr,
+			  const char *buf,
+			  size_t count)
+{
+	struct ipath_devdata *dd = dev_get_drvdata(dev);
+	ssize_t ret;
+	unsigned short guid[8];
+	u64 nguid;
+	u8 *ng;
+	int i;
+
+	if (sscanf(buf, "%hx:%hx:%hx:%hx:%hx:%hx:%hx:%hx",
+		   &guid[0], &guid[1], &guid[2], &guid[3],
+		   &guid[4], &guid[5], &guid[6], &guid[7]) != 8)
+		goto invalid;
+
+	ng = (u8 *) &nguid;
+
+	for (i = 0; i < 8; i++) {
+		if (guid[i] > 0xff)
+			goto invalid;
+		ng[i] = guid[i];
+	}
+
+	dd->ipath_guid = nguid;
+	dd->ipath_nguid = 1;
+
+	ret = strlen(buf);
+	goto bail;
+
+invalid:
+	dev_err(dev, "attempt to set invalid GUID\n");
+	ret = -EINVAL;
+
+bail:
+	return ret;
+}
+
+static ssize_t show_nguid(struct device *dev,
+			  struct device_attribute *attr,
+			  char *buf)
+{
+	struct ipath_devdata *dd = dev_get_drvdata(dev);
+
+	return scnprintf(buf, PAGE_SIZE, "%u\n", dd->ipath_nguid);
+}
+
+static ssize_t show_serial(struct device *dev,
+			   struct device_attribute *attr,
+			   char *buf)
+{
+	struct ipath_devdata *dd = dev_get_drvdata(dev);
+
+	buf[sizeof dd->ipath_serial] = '\0';
+	memcpy(buf, dd->ipath_serial, sizeof dd->ipath_serial);
+	strcat(buf, "\n");
+	return strlen(buf);
+}
+
+static ssize_t show_unit(struct device *dev,
+			 struct device_attribute *attr,
+			 char *buf)
+{
+	struct ipath_devdata *dd = dev_get_drvdata(dev);
+
+	return scnprintf(buf, PAGE_SIZE, "%u\n", dd->ipath_unit);
+}
+
+static ssize_t show_atomic_counters(struct device *dev,
+			 struct device_attribute *attr,
+			 char *buf)
+{
+	struct ipath_devdata *dd = dev_get_drvdata(dev);
+	u64 *ubuf = (u64 *) buf;
+	u64 val;
+	u16 i;
+
+	for (i = 0; i < sizeof(struct infinipath_counters) /
+		     sizeof(val); i++)
+		ubuf[i] = ipath_snap_cntr(dd, i);
+
+	return i * sizeof(val);
+}
+
+#define DEVICE_COUNTER(name, attr) \
+	static ssize_t show_counter_##name(struct device *dev, \
+					   struct device_attribute *attr, \
+					   char *buf) \
+	{ \
+		struct ipath_devdata *dd = dev_get_drvdata(dev); \
+		return scnprintf(\
+			buf, PAGE_SIZE, "%llu\n", (unsigned long long) \
+			ipath_snap_cntr( \
+				dd, offsetof(struct infinipath_counters, \
+					     attr) / sizeof(u64)));	\
+	} \
+	static DEVICE_ATTR(name, S_IRUGO, show_counter_##name, NULL);
+
+DEVICE_COUNTER(ib_link_downeds, IBLinkDownedCnt);
+DEVICE_COUNTER(ib_link_err_recoveries, IBLinkErrRecoveryCnt);
+DEVICE_COUNTER(ib_status_changes, IBStatusChangeCnt);
+DEVICE_COUNTER(ib_symbol_errs, IBSymbolErrCnt);
+DEVICE_COUNTER(lb_flow_stalls, LBFlowStallCnt);
+DEVICE_COUNTER(lb_ints, LBIntCnt);
+DEVICE_COUNTER(rx_bad_formats, RxBadFormatCnt);
+DEVICE_COUNTER(rx_buf_ovfls, RxBufOvflCnt);
+DEVICE_COUNTER(rx_data_pkts, RxDataPktCnt);
+DEVICE_COUNTER(rx_dropped_pkts, RxDroppedPktCnt);
+DEVICE_COUNTER(rx_dwords, RxDwordCnt);
+DEVICE_COUNTER(rx_ebps, RxEBPCnt);
+DEVICE_COUNTER(rx_flow_ctrl_errs, RxFlowCtrlErrCnt);
+DEVICE_COUNTER(rx_flow_pkts, RxFlowPktCnt);
+DEVICE_COUNTER(rx_icrc_errs, RxICRCErrCnt);
+DEVICE_COUNTER(rx_len_errs, RxLenErrCnt);
+DEVICE_COUNTER(rx_link_problems, RxLinkProblemCnt);
+DEVICE_COUNTER(rx_lpcrc_errs, RxLPCRCErrCnt);
+DEVICE_COUNTER(rx_max_min_len_errs, RxMaxMinLenErrCnt);
+DEVICE_COUNTER(rx_p0_hdr_egr_ovfls, RxP0HdrEgrOvflCnt);
+DEVICE_COUNTER(rx_p1_hdr_egr_ovfls, RxP1HdrEgrOvflCnt);
+DEVICE_COUNTER(rx_p2_hdr_egr_ovfls, RxP2HdrEgrOvflCnt);
+DEVICE_COUNTER(rx_p3_hdr_egr_ovfls, RxP3HdrEgrOvflCnt);
+DEVICE_COUNTER(rx_p4_hdr_egr_ovfls, RxP4HdrEgrOvflCnt);
+DEVICE_COUNTER(rx_p5_hdr_egr_ovfls, RxP5HdrEgrOvflCnt);
+DEVICE_COUNTER(rx_p6_hdr_egr_ovfls, RxP6HdrEgrOvflCnt);
+DEVICE_COUNTER(rx_p7_hdr_egr_ovfls, RxP7HdrEgrOvflCnt);
+DEVICE_COUNTER(rx_p8_hdr_egr_ovfls, RxP8HdrEgrOvflCnt);
+DEVICE_COUNTER(rx_pkey_mismatches, RxPKeyMismatchCnt);
+DEVICE_COUNTER(rx_tid_full_errs, RxTIDFullErrCnt);
+DEVICE_COUNTER(rx_tid_valid_errs, RxTIDValidErrCnt);
+DEVICE_COUNTER(rx_vcrc_errs, RxVCRCErrCnt);
+DEVICE_COUNTER(tx_data_pkts, TxDataPktCnt);
+DEVICE_COUNTER(tx_dropped_pkts, TxDroppedPktCnt);
+DEVICE_COUNTER(tx_dwords, TxDwordCnt);
+DEVICE_COUNTER(tx_flow_pkts, TxFlowPktCnt);
+DEVICE_COUNTER(tx_flow_stalls, TxFlowStallCnt);
+DEVICE_COUNTER(tx_len_errs, TxLenErrCnt);
+DEVICE_COUNTER(tx_max_min_len_errs, TxMaxMinLenErrCnt);
+DEVICE_COUNTER(tx_underruns, TxUnderrunCnt);
+DEVICE_COUNTER(tx_unsup_vl_errs, TxUnsupVLErrCnt);
+
+static struct attribute *dev_counter_attributes[] = {
+	&dev_attr_ib_link_downeds.attr,
+	&dev_attr_ib_link_err_recoveries.attr,
+	&dev_attr_ib_status_changes.attr,
+	&dev_attr_ib_symbol_errs.attr,
+	&dev_attr_lb_flow_stalls.attr,
+	&dev_attr_lb_ints.attr,
+	&dev_attr_rx_bad_formats.attr,
+	&dev_attr_rx_buf_ovfls.attr,
+	&dev_attr_rx_data_pkts.attr,
+	&dev_attr_rx_dropped_pkts.attr,
+	&dev_attr_rx_dwords.attr,
+	&dev_attr_rx_ebps.attr,
+	&dev_attr_rx_flow_ctrl_errs.attr,
+	&dev_attr_rx_flow_pkts.attr,
+	&dev_attr_rx_icrc_errs.attr,
+	&dev_attr_rx_len_errs.attr,
+	&dev_attr_rx_link_problems.attr,
+	&dev_attr_rx_lpcrc_errs.attr,
+	&dev_attr_rx_max_min_len_errs.attr,
+	&dev_attr_rx_p0_hdr_egr_ovfls.attr,
+	&dev_attr_rx_p1_hdr_egr_ovfls.attr,
+	&dev_attr_rx_p2_hdr_egr_ovfls.attr,
+	&dev_attr_rx_p3_hdr_egr_ovfls.attr,
+	&dev_attr_rx_p4_hdr_egr_ovfls.attr,
+	&dev_attr_rx_p5_hdr_egr_ovfls.attr,
+	&dev_attr_rx_p6_hdr_egr_ovfls.attr,
+	&dev_attr_rx_p7_hdr_egr_ovfls.attr,
+	&dev_attr_rx_p8_hdr_egr_ovfls.attr,
+	&dev_attr_rx_pkey_mismatches.attr,
+	&dev_attr_rx_tid_full_errs.attr,
+	&dev_attr_rx_tid_valid_errs.attr,
+	&dev_attr_rx_vcrc_errs.attr,
+	&dev_attr_tx_data_pkts.attr,
+	&dev_attr_tx_dropped_pkts.attr,
+	&dev_attr_tx_dwords.attr,
+	&dev_attr_tx_flow_pkts.attr,
+	&dev_attr_tx_flow_stalls.attr,
+	&dev_attr_tx_len_errs.attr,
+	&dev_attr_tx_max_min_len_errs.attr,
+	&dev_attr_tx_underruns.attr,
+	&dev_attr_tx_unsup_vl_errs.attr,
+	NULL
+};
+
+static struct attribute_group dev_counter_attr_group = {
+	.name = "counters",
+	.attrs = dev_counter_attributes
+};
+
+static ssize_t store_reset(struct device *dev,
+			 struct device_attribute *attr,
+			  const char *buf,
+			  size_t count)
+{
+	struct ipath_devdata *dd = dev_get_drvdata(dev);
+	int ret;
+
+	if (count < 5 || memcmp(buf, "reset", 5)) {
+		ret = -EINVAL;
+		goto bail;
+	}
+
+	ret = ipath_reset_device(dd->ipath_unit);
+bail:
+	return ret<0 ? ret : count;
+}
+
+static ssize_t store_link_state(struct device *dev,
+			 struct device_attribute *attr,
+			  const char *buf,
+			  size_t count)
+{
+	struct ipath_devdata *dd = dev_get_drvdata(dev);
+	int ret, r;
+	u16 state;
+
+	ret = ipath_parse_ushort(buf, &state);
+	if (ret < 0)
+		goto invalid;
+
+	r = ipath_layer_set_linkstate(dd, state);
+	if (r < 0) {
+		ret = r;
+		goto bail;
+	}
+
+	goto bail;
+invalid:
+	dev_err(dev, "attempt to set invalid link state\n");
+bail:
+	return ret;
+}
+
+static ssize_t show_flash(struct device *dev,
+			 struct device_attribute *attr,
+			 char *buf)
+{
+	struct ipath_devdata *dd = dev_get_drvdata(dev);
+	ssize_t ret;
+
+	if (ipath_eeprom_read(dd, 0, buf, sizeof(struct ipath_flash))) {
+		ret = -ENXIO;
+		dev_err(dev, "failed to read from flash\n");
+		goto bail;
+	}
+
+	ret = sizeof(struct ipath_flash);
+bail:
+	return ret;
+}
+
+static ssize_t store_flash(struct device *dev,
+			 struct device_attribute *attr,
+			  const char *buf,
+			  size_t count)
+{
+	struct ipath_devdata *dd = dev_get_drvdata(dev);
+	ssize_t ret;
+
+	if (count != sizeof(struct ipath_flash)) {
+		ret = -EINVAL;
+		goto bail;
+	}
+
+	if (ipath_eeprom_write(dd, 0, buf, count)) {
+		ret = -ENXIO;
+		dev_err(dev, "failed to write to flash\n");
+		goto bail;
+	}
+
+	ret = count;
+bail:
+	return ret;
+}
+
+static ssize_t show_mtu(struct device *dev,
+			 struct device_attribute *attr,
+			 char *buf)
+{
+	struct ipath_devdata *dd = dev_get_drvdata(dev);
+	return scnprintf(buf, PAGE_SIZE, "%u\n", dd->ipath_ibmtu);
+}
+
+static ssize_t store_mtu(struct device *dev,
+			 struct device_attribute *attr,
+			  const char *buf,
+			  size_t count)
+{
+	struct ipath_devdata *dd = dev_get_drvdata(dev);
+	ssize_t ret;
+	u16 mtu = 0;
+	int r;
+
+	ret = ipath_parse_ushort(buf, &mtu);
+	if (ret < 0)
+		goto invalid;
+
+	r = ipath_layer_set_mtu(dd, mtu);
+	if (r < 0)
+		ret = r;
+
+	goto bail;
+invalid:
+	dev_err(dev, "attempt to set invalid MTU\n");
+bail:
+	return ret;
+}
+
+static DRIVER_ATTR(atomic_stats, S_IRUGO, show_atomic_stats, NULL);
+static DRIVER_ATTR(num_units, S_IRUGO, show_num_units, NULL);
+static DRIVER_ATTR(version, S_IRUGO, show_version, NULL);
+
+static struct attribute *driver_attributes[] = {
+	&driver_attr_atomic_stats.attr,
+	&driver_attr_num_units.attr,
+	&driver_attr_version.attr,
+	NULL
+};
+
+static struct attribute_group driver_attr_group = {
+	.attrs = driver_attributes
+};
+
+static DEVICE_ATTR(atomic_counters, S_IRUGO, show_atomic_counters, NULL);
+static DEVICE_ATTR(flash, S_IWUSR | S_IRUGO, show_flash, store_flash);
+static DEVICE_ATTR(guid, S_IWUSR | S_IRUGO, show_guid, store_guid);
+static DEVICE_ATTR(lid, S_IWUSR | S_IRUGO, show_lid, store_lid);
+static DEVICE_ATTR(link_state, S_IWUSR, NULL, store_link_state);
+static DEVICE_ATTR(mlid, S_IWUSR | S_IRUGO, show_mlid, store_mlid);
+static DEVICE_ATTR(mtu, S_IWUSR | S_IRUGO, show_mtu, store_mtu);
+static DEVICE_ATTR(nguid, S_IRUGO, show_nguid, NULL);
+static DEVICE_ATTR(node_info, S_IRUGO, show_node_info, NULL);
+static DEVICE_ATTR(port_info, S_IRUGO, show_port_info, NULL);
+static DEVICE_ATTR(reset, S_IWUSR, NULL, store_reset);
+static DEVICE_ATTR(serial, S_IRUGO, show_serial, NULL);
+static DEVICE_ATTR(status, S_IRUGO, show_status, NULL);
+static DEVICE_ATTR(status_str, S_IRUGO, show_status_str, NULL);
+static DEVICE_ATTR(boardversion, S_IRUGO, show_boardversion, NULL);
+static DEVICE_ATTR(unit, S_IRUGO, show_unit, NULL);
+
+static struct attribute *dev_attributes[] = {
+	&dev_attr_atomic_counters.attr,
+	&dev_attr_flash.attr,
+	&dev_attr_guid.attr,
+	&dev_attr_lid.attr,
+	&dev_attr_link_state.attr,
+	&dev_attr_mlid.attr,
+	&dev_attr_mtu.attr,
+	&dev_attr_nguid.attr,
+	&dev_attr_node_info.attr,
+	&dev_attr_port_info.attr,
+	&dev_attr_serial.attr,
+	&dev_attr_status.attr,
+	&dev_attr_status_str.attr,
+	&dev_attr_boardversion.attr,
+	&dev_attr_unit.attr,
+	NULL
+};
+
+static struct attribute_group dev_attr_group = {
+	.attrs = dev_attributes
+};
+
+/**
+ * ipath_expose_reset - create a device reset file
+ * @dev: the device structure
+ *
+ * Only expose a file that lets us reset the device after someone
+ * enters diag mode.  A device reset is quite likely to crash the
+ * machine entirely, so we don't want to normally make it
+ * available.
+ */
+int ipath_expose_reset(struct device *dev)
+{
+	return device_create_file(dev, &dev_attr_reset);
+}
+
+int ipath_driver_create_group(struct device_driver *drv)
+{
+	int ret;
+
+	if (!drv->kobj.dentry) {
+		ret = -ENODEV;
+		goto bail;
+	}
+
+	ret = sysfs_create_group(&drv->kobj, &driver_attr_group);
+	if (ret)
+		goto bail;
+
+	ret = sysfs_create_group(&drv->kobj, &driver_stat_attr_group);
+	if (ret)
+		sysfs_remove_group(&drv->kobj, &driver_attr_group);
+
+bail:
+	return ret;
+}
+
+void ipath_driver_remove_group(struct device_driver *drv)
+{
+	if (drv->kobj.dentry) {
+		sysfs_remove_group(&drv->kobj, &driver_stat_attr_group);
+		sysfs_remove_group(&drv->kobj, &driver_attr_group);
+	}
+}
+
+int ipath_device_create_group(struct device *dev, struct ipath_devdata *dd)
+{
+	int ret;
+	char unit[5];
+
+	ret = sysfs_create_group(&dev->kobj, &dev_attr_group);
+	if (ret)
+		goto bail;
+
+	ret = sysfs_create_group(&dev->kobj, &dev_counter_attr_group);
+	if (ret)
+		goto bail;
+
+	snprintf(unit, sizeof(unit), "%02u", (unsigned) dd->ipath_unit);
+	ret = sysfs_create_link(&dev->driver->kobj, &dev->kobj, unit);
+bail:
+	return ret;
+}
+
+void ipath_device_remove_group(struct device *dev, struct ipath_devdata *dd)
+{
+	char unit[5];
+
+	snprintf(unit, sizeof(unit), "%02u", (unsigned) dd->ipath_unit);
+	sysfs_remove_link(&dev->driver->kobj, unit);
+
+	sysfs_remove_group(&dev->kobj, &dev_counter_attr_group);
+	sysfs_remove_group(&dev->kobj, &dev_attr_group);
+
+	device_remove_file(dev, &dev_attr_reset);
+}

Re: Revenge of the sysfs maintainer! (was Re: [PATCH 8 of 20] ipath - sysfs support for core driver)

[Bryan O'Sullivan]

On Thu, 2006-03-09 at 16:35 -0800, Greg KH wrote:

> Grumble?  Oh come on, don't export binary structures through sysfs, it's
> in the DOCUMENTATION THAT SYSFS IS FOR TEXT FILES ONLY!!!!

OK, fine.

> If you don't want to export a text file, then use something else other
> than sysfs, it's that simple.

Use what?  Would a sysfs relay file, or whatever they're called now that
relayfs is moving into sysfs, do the trick?  If so, what's a good place
to pull those patches from so I can compile-test my changes?  Should I
just grub through my archives and apply whatever Paul Mundt sent out a
few weeks ago?

> sysfs binary files are for PASS-THROUGH things ONLY!

If there's any documentation on what sysfs binary files are for, I
haven't seen it.  It's not in the include files, the source, or
Documentation/filesystems.  

> Ok, here's a new rule to help this from happening again in the future:
> 
>   If you want to add a new sysfs file to the kernel, it MUST be
>   accompanied with full documentation that explains exactly what that
>   file contains and what it is for.  No exceptions will be allowed.

I'm fine with this rule, but accompanied how?  In a comment in the code?
In the patch description?  In the same way that sysfs binary files are
documented? :-)

Also, I'd suggest that you put a similar requirement on directories and
symlinks, if you're going to clamp down on files.

	<b

Re: Revenge of the sysfs maintainer! (was Re: [PATCH 8 of 20] ipath - sysfs support for core driver)

[Greg KH]

On Thu, Mar 09, 2006 at 04:46:29PM -0800, Bryan O'Sullivan wrote:
> On Thu, 2006-03-09 at 16:35 -0800, Greg KH wrote:
> 
> > Grumble?  Oh come on, don't export binary structures through sysfs, it's
> > in the DOCUMENTATION THAT SYSFS IS FOR TEXT FILES ONLY!!!!
> 
> OK, fine.
> 
> > If you don't want to export a text file, then use something else other
> > than sysfs, it's that simple.
> 
> Use what?  Would a sysfs relay file, or whatever they're called now that
> relayfs is moving into sysfs, do the trick?  If so, what's a good place
> to pull those patches from so I can compile-test my changes?  Should I
> just grub through my archives and apply whatever Paul Mundt sent out a
> few weeks ago?

They are in the latest -mm tree if you wish to use them.  Unfortunatly
it might look like they will not work out, due to the per-cpu relay
files not working properly with Paul's patches at the moment.  But I
think he's still working on them.

What's wrong with debugfs?

> > sysfs binary files are for PASS-THROUGH things ONLY!
> 
> If there's any documentation on what sysfs binary files are for, I
> haven't seen it.  It's not in the include files, the source, or
> Documentation/filesystems.  

Fair enough, you are correct.  There is a serious dearth of sysfs and
kobject documentation lately, I'll work on fixing that up.

> > Ok, here's a new rule to help this from happening again in the future:
> > 
> >   If you want to add a new sysfs file to the kernel, it MUST be
> >   accompanied with full documentation that explains exactly what that
> >   file contains and what it is for.  No exceptions will be allowed.
> 
> I'm fine with this rule, but accompanied how?  In a comment in the code?
> In the patch description?  In the same way that sysfs binary files are
> documented? :-)

Touche :)

I referred to my prior lkml post:
	http://thread.gmane.org/gmane.linux.kernel/383717
which provides a structure for documenting the user<->kernel API, which
is what you are creating here.

> Also, I'd suggest that you put a similar requirement on directories and
> symlinks, if you're going to clamp down on files.

I completly agree, anything that is in sysfs falls under this
requirement.  Sorry, but I think of directories and symlinks as files,
as I've been spelunking through the vfs layer too many times :)

thanks,

greg k-h

Re: [PATCH 8 of 20] ipath - sysfs support for core driver

[Greg KH]

On Thu, Mar 09, 2006 at 03:59:37PM -0800, Bryan O'Sullivan wrote:
> On Thu, 2006-03-09 at 15:46 -0800, Greg KH wrote:
> > On Thu, Mar 09, 2006 at 03:18:49PM -0800, Roland Dreier wrote:
> > 
> > Thanks for CC:ing me, but where were the originals of these posted?
> 
> My patch posting script screwed up.  Only Roland got them, even though
> the envelopes were all correct.
> 
> > >  > +static ssize_t show_atomic_stats(struct device_driver *dev, char *buf)
> > >  > +{
> > >  > +	memcpy(buf, &ipath_stats, sizeof(ipath_stats));
> > >  > +
> > >  > +	return sizeof(ipath_stats);
> > >  > +}
> > > 
> > > I think putting a whole binary struct in a sysfs attribute is
> > > considered a no-no.
> > 
> > That's an understatement, where is the large stick to thwap the author
> > of this code...
> 
> I'd like to understand why, though.  As I already explained, it's a
> smallish structure (< 1KB), and I can use the special binary sysfs
> attribute goo for it if you insist, but ... why?

I think I explained this in my prior post enough, right?  If not, please
let me know.

thanks,

greg k-h

Re: [PATCH 8 of 20] ipath - sysfs support for core driver

[Greg KH]

On Thu, Mar 09, 2006 at 04:35:38PM -0800, Bryan O'Sullivan wrote:
> +static ssize_t show_node_info(struct device *dev,
> +			       struct device_attribute *attr,
> +			       char *buf)
> +{
> +	static const size_t count = 10;
> +	struct ipath_devdata *dd = dev_get_drvdata(dev);
> +	u32 *nodeinfo;
> +	int ret;
> +
> +	if (!dd->ipath_statusp) {
> +		ret = -EINVAL;
> +		goto bail;
> +	}
> +
> +	nodeinfo = (u32 *) buf;
> +
> +	/* so we only initialize non-zero fields. */
> +	memset(nodeinfo, 0, count * sizeof(u32));
> +
> +	nodeinfo[0] =		/* BaseVersion is SMA */
> +		/* ClassVersion is SMA */
> +		(1 << 8)		/* NodeType  */
> +		|(1 << 0);		/* NumPorts */
> +	nodeinfo[1] = (u32) (dd->ipath_guid >> 32);
> +	nodeinfo[2] = (u32) (dd->ipath_guid & 0xffffffff);
> +	/* PortGUID == SystemImageGUID for us */
> +	nodeinfo[3] = nodeinfo[1];
> +	/* PortGUID == SystemImageGUID for us */
> +	nodeinfo[4] = nodeinfo[2];
> +	/* PortGUID == NodeGUID for us */
> +	nodeinfo[5] = nodeinfo[3];
> +	/* PortGUID == NodeGUID for us */
> +	nodeinfo[6] = nodeinfo[4];
> +	nodeinfo[7] = (4 << 16)	/* we support 4 pkeys */
> +		|(dd->ipath_deviceid << 0);
> +	/* our chip version as 16 bits major, 16 bits minor */
> +	nodeinfo[8] = dd->ipath_minrev | (dd->ipath_majrev << 16);
> +	nodeinfo[9] = (dd->ipath_unit << 24) | (dd->ipath_vendorid << 0);
> +
> +	ret = count * sizeof(u32);
> +bail:
> +	return ret;
> +}
> +
> +static ssize_t show_port_info(struct device *dev,
> +			       struct device_attribute *attr,
> +			       char *buf)
> +{
> +	static const size_t count = 13;
> +	int ret;
> +	u32 tmp, tmp2;
> +	struct ipath_devdata *dd = dev_get_drvdata(dev);
> +	u32 *portinfo;
> +
> +	if (!dd->ipath_statusp) {
> +		ret = -EINVAL;
> +		goto bail;
> +	}
> +
> +	portinfo = (u32 *) buf;
> +
> +	/* so we only initialize non-zero fields. */
> +	memset(portinfo, 0, count * sizeof portinfo);
> +
> +	/*
> +	 * Notimpl yet M_Key (64)
> +	 * Notimpl yet GID (64)
> +	 */
> +
> +	portinfo[4] = (dd->ipath_lid << 16);
> +
> +	/*
> +	 * Notimpl yet SMLID (should we store this in the driver, in case
> +	 * SMA dies?)  CapabilityMask is 0, we don't support any of these
> +	 * DiagCode is 0; we don't store any diag info for now Notimpl yet
> +	 * M_KeyLeasePeriod (we don't support M_Key)
> +	 */
> +
> +	/* LocalPortNum is whichever port number they ask for */
> +	portinfo[7] = (dd->ipath_unit << 24)
> +		/* LinkWidthEnabled */
> +		| (2 << 16)
> +		/* LinkWidthSupported (really 2, but not IB valid) */
> +		| (3 << 8)
> +		/* LinkWidthActive */
> +		| (2 << 0);
> +	tmp = dd->ipath_lastibcstat & IPATH_IBSTATE_MASK;
> +	tmp2 = 5;
> +	if (tmp == IPATH_IBSTATE_INIT)
> +		tmp = 2;
> +	else if (tmp == IPATH_IBSTATE_ARM)
> +		tmp = 3;
> +	else if (tmp == IPATH_IBSTATE_ACTIVE)
> +		tmp = 4;
> +	else {
> +		tmp = 0;	/* down */
> +		tmp2 = tmp & 0xf;
> +	}
> +
> +	portinfo[8] = (1 << 28)	/* LinkSpeedSupported */
> +		|(tmp << 24)	/* PortState */
> +		|(tmp2 << 20)	/* PortPhysicalState */
> +		|(2 << 16)
> +
> +		/* LinkDownDefaultState */
> +		/* M_KeyProtectBits == 0 */
> +		/* NotImpl yet LMC == 0 (we can support all values) */
> +		| (1 << 4)		/* LinkSpeedActive */
> +		|(1 << 0);		/* LinkSpeedEnabled */
> +	switch (dd->ipath_ibmtu) {
> +	case 4096:
> +		tmp = 5;
> +		break;
> +	case 2048:
> +		tmp = 4;
> +		break;
> +	case 1024:
> +		tmp = 3;
> +		break;
> +	case 512:
> +		tmp = 2;
> +		break;
> +	case 256:
> +		tmp = 1;
> +		break;
> +	default:		/* oops, something is wrong */
> +		ipath_dbg("Problem, ipath_ibmtu 0x%x not a valid IB MTU, "
> +			  "treat as 2048\n", dd->ipath_ibmtu);
> +		tmp = 4;
> +		break;
> +	}
> +	portinfo[9] = (tmp << 28)
> +		/* NeighborMTU */
> +		/* Notimpl MasterSMSL */
> +		| (1 << 20)
> +
> +		/* VLCap */
> +		/* Notimpl InitType (actually, an SMA decision) */
> +		/* VLHighLimit is 0 (only one VL) */
> +		; /* VLArbitrationHighCap is 0 (only one VL) */
> +	portinfo[10] =		/* VLArbitrationLowCap is 0 (only one VL) */
> +		/* InitTypeReply is SMA decision */
> +		(5 << 16)		/* MTUCap 4096 */
> +		|(7 << 13)		/* VLStallCount */
> +		|(0x1f << 8)	/* HOQLife */
> +		|(1 << 4)
> +
> +		/* OperationalVLs 0 */
> +		/* PartitionEnforcementInbound */
> +		/* PartitionEnforcementOutbound not enforced */
> +		/* FilterRawinbound not enforced */
> +		;			/* FilterRawOutbound not enforced */
> +	/* M_KeyViolations are not counted by hardware, SMA can count */
> +	tmp = ipath_read_creg32(dd, dd->ipath_cregs->cr_errpkey);
> +	/* P_KeyViolations are counted by hardware. */
> +	portinfo[11] = ((tmp & 0xffff) << 0);
> +	portinfo[12] =
> +		/* Q_KeyViolations are not counted by hardware */
> +		(1 << 8)
> +
> +		/* GUIDCap */
> +		/* SubnetTimeOut handled by SMA */
> +		/* RespTimeValue handled by SMA */
> +		;
> +	/* LocalPhyErrors are programmed to max */
> +	portinfo[12] |= (0xf << 20)
> +		| (0xf << 16)	/* OverRunErrors are programmed to max */
> +		;
> +
> +	ret = count * sizeof(u32);
> +bail:
> +	return ret;
> +}

These two files sure do show a lot of different stuff, all in a
predefined structure for a single file.  Please break them up into the
different individual files please.

thanks,

greg k-h

Re: Revenge of the sysfs maintainer! (was Re: [PATCH 8 of 20] ipath - sysfs support for core driver)

[Bryan O'Sullivan]

On Thu, 2006-03-09 at 17:00 -0800, Greg KH wrote:

> They are in the latest -mm tree if you wish to use them.  Unfortunatly
> it might look like they will not work out, due to the per-cpu relay
> files not working properly with Paul's patches at the moment.

Hmm, OK.

> What's wrong with debugfs?

It's not configured into the kernels of either of the distros I use (Red
Hat or SUSE).  I can't have a required part of my driver depend on a
feature that's not enabled in the major distro kernels.

I'd like a mechanism that is (a) always there (b) easy for kernel to use
and (c) easy for userspace to use.  A sysfs file satisfies a, b, and c,
but I can't use it; a sysfs bin file satisfies all three (a bit worse on
b), but I can't use it; debugfs isn't there, so I can't use it.

That leaves me with few options, I think.  What do you suggest?  (Please
don't say netlink.)

	<b

Re: [PATCH 8 of 20] ipath - sysfs support for core driver

[Bryan O'Sullivan]

On Thu, 2006-03-09 at 17:11 -0800, Greg KH wrote:

> These two files sure do show a lot of different stuff, all in a
> predefined structure for a single file.  Please break them up into the
> different individual files please.

The problem is that I want them to be presented together.  They look
like a pile of different stuff, but they're actually Infiniband NodeInfo
and PortInfo structures.  And yes, they are that ugly.

These files fall into the same categories as the atomic_counters and
atomic_snapshots files you raised objections to earlier; it actually
makes sense to look at them as a whole, not their constituent parts.

In the earlier round of review, people suggested that I use netlink for
stuff like this, but I quickly decided I'd rather gnaw my leg off than
use the netlink API.

I'm thinking at this point that I should just route this information
through the /dev/ipath_sma char device, and maybe
add /dev/ipath_counters%d and /dev/ipath_stats to go with it.  I think
that's a pretty crummy approach that sysfs solves more cleanly, but
there you go.

	<b

Re: Revenge of the sysfs maintainer! (was Re: [PATCH 8 of 20] ipath - sysfs support for core driver)

[Greg KH]

On Thu, Mar 09, 2006 at 08:58:13PM -0800, Bryan O'Sullivan wrote:
> On Thu, 2006-03-09 at 17:00 -0800, Greg KH wrote:
> 
> > They are in the latest -mm tree if you wish to use them.  Unfortunatly
> > it might look like they will not work out, due to the per-cpu relay
> > files not working properly with Paul's patches at the moment.
> 
> Hmm, OK.
> 
> > What's wrong with debugfs?
> 
> It's not configured into the kernels of either of the distros I use (Red
> Hat or SUSE).

Well, I can do something about SuSE, it's up to someone else to persuade
Red Hat :)

> I can't have a required part of my driver depend on a feature that's
> not enabled in the major distro kernels.

Fair enough.

> I'd like a mechanism that is (a) always there (b) easy for kernel to use
> and (c) easy for userspace to use.  A sysfs file satisfies a, b, and c,
> but I can't use it; a sysfs bin file satisfies all three (a bit worse on
> b), but I can't use it; debugfs isn't there, so I can't use it.
> 
> That leaves me with few options, I think.  What do you suggest?  (Please
> don't say netlink.)

Write your own filesystem?  Seriously, you do that and you get to set
all of your own rules (well, within reason).  It's only 200 lines of
code, max.

thanks,

greg k-h

Re: [PATCH 8 of 20] ipath - sysfs support for core driver

[Greg KH]

On Thu, Mar 09, 2006 at 09:09:37PM -0800, Bryan O'Sullivan wrote:
> On Thu, 2006-03-09 at 17:11 -0800, Greg KH wrote:
> 
> > These two files sure do show a lot of different stuff, all in a
> > predefined structure for a single file.  Please break them up into the
> > different individual files please.
> 
> The problem is that I want them to be presented together.  They look
> like a pile of different stuff, but they're actually Infiniband NodeInfo
> and PortInfo structures.  And yes, they are that ugly.

Then why not just have a bunch of different files for the different
things, and then a simple shell script to grab them all and put them
together however you want.

The main issue is that if you create a sysfs file like this, and then in
3 months realize that you need to change one of those characters to
be something else, you are in big trouble...

> These files fall into the same categories as the atomic_counters and
> atomic_snapshots files you raised objections to earlier; it actually
> makes sense to look at them as a whole, not their constituent parts.

Sure, lots of different files can be combined by a script into a whole.

> In the earlier round of review, people suggested that I use netlink for
> stuff like this, but I quickly decided I'd rather gnaw my leg off than
> use the netlink API.

Just because you don't want to use it doesn't mean it isn't the proper
tool...

thanks,

greg k-h

Re: Revenge of the sysfs maintainer! (was Re: [PATCH 8 of 20] ipath - sysfs support for core driver)

[Arjan van de Ven]

On Thu, 2006-03-09 at 20:58 -0800, Bryan O'Sullivan wrote:
> On Thu, 2006-03-09 at 17:00 -0800, Greg KH wrote:
> 
> > They are in the latest -mm tree if you wish to use them.  Unfortunatly
> > it might look like they will not work out, due to the per-cpu relay
> > files not working properly with Paul's patches at the moment.
> 
> Hmm, OK.
> 
> > What's wrong with debugfs?
> 
> It's not configured into the kernels of either of the distros I use (Red
> Hat or SUSE).  I can't have a required part of my driver depend on a
> feature that's not enabled in the major distro kernels.

sucks to be you, however I think it's equally or even more unacceptable
to cripple the main kernel because you want to also support antique
kernels (those more than 12 months old). The general rule is "if you
want to support that, do it outside the kernel.org tree".

Re: Revenge of the sysfs maintainer! (was Re: [PATCH 8 of 20] ipath - sysfs support for core driver)

[Bryan O'Sullivan]

On Fri, 2006-03-10 at 08:57 +0100, Arjan van de Ven wrote:
> On Thu, 2006-03-09 at 20:58 -0800, Bryan O'Sullivan wrote:
> > On Thu, 2006-03-09 at 17:00 -0800, Greg KH wrote:
> > 
> > > They are in the latest -mm tree if you wish to use them.  Unfortunatly
> > > it might look like they will not work out, due to the per-cpu relay
> > > files not working properly with Paul's patches at the moment.
> > 
> > Hmm, OK.
> > 
> > > What's wrong with debugfs?
> > 
> > It's not configured into the kernels of either of the distros I use (Red
> > Hat or SUSE).  I can't have a required part of my driver depend on a
> > feature that's not enabled in the major distro kernels.
> 
> sucks to be you, however I think it's equally or even more unacceptable
> to cripple the main kernel because you want to also support antique
> kernels (those more than 12 months old).

What antique kernels?  It's not enabled in the latest SLES beta
(2.6.16-git6 or so), or in Fedora rawhide (also 2.6.16-git).

They mightn't be exactly today's kernels, but they're no more than two
or three weeks old.  CONFIG_DEBUG_FS has been in the kernel for a long
time, and it's still not being picked up.

>  The general rule is "if you
> want to support that, do it outside the kernel.org tree".

Which "that" are you referring to?

	<b

-- 
Bryan O'Sullivan <bos@pathscale.com>

Re: [openib-general] Re: Revenge of the sysfs maintainer! (was Re: [PATCH 8 of 20] ipath - sysfs support for core driver)

[Talpey, Thomas]

At 11:58 PM 3/9/2006, Bryan O'Sullivan wrote:
>I'd like a mechanism that is (a) always there (b) easy for kernel to use
>and (c) easy for userspace to use.  A sysfs file satisfies a, b, and c,
>but I can't use it; a sysfs bin file satisfies all three (a bit worse on
>b), but I can't use it; debugfs isn't there, so I can't use it.
>
>That leaves me with few options, I think.  What do you suggest?  (Please
>don't say netlink.)

mmap()?

Tom.

Re: Revenge of the sysfs maintainer! (was Re: [PATCH 8 of 20] ipath - sysfs support for core driver)

[Arjan van de Ven]

On Fri, 2006-03-10 at 05:51 -0800, Bryan O'Sullivan wrote:
> On Fri, 2006-03-10 at 08:57 +0100, Arjan van de Ven wrote:
> > On Thu, 2006-03-09 at 20:58 -0800, Bryan O'Sullivan wrote:
> > > On Thu, 2006-03-09 at 17:00 -0800, Greg KH wrote:
> > > 
> > > > They are in the latest -mm tree if you wish to use them.  Unfortunatly
> > > > it might look like they will not work out, due to the per-cpu relay
> > > > files not working properly with Paul's patches at the moment.
> > > 
> > > Hmm, OK.
> > > 
> > > > What's wrong with debugfs?
> > > 
> > > It's not configured into the kernels of either of the distros I use (Red
> > > Hat or SUSE).  I can't have a required part of my driver depend on a
> > > feature that's not enabled in the major distro kernels.
> > 
> > sucks to be you, however I think it's equally or even more unacceptable
> > to cripple the main kernel because you want to also support antique
> > kernels (those more than 12 months old).
> 
> What antique kernels?  It's not enabled in the latest SLES beta
> (2.6.16-git6 or so), or in Fedora rawhide (also 2.6.16-git).
> 
> They mightn't be exactly today's kernels, but they're no more than two
> or three weeks old.  CONFIG_DEBUG_FS has been in the kernel for a long
> time, and it's still not being picked up.

but it's a module; you can ship it no problem yourself if you go through
the hell of shipping external modules



> 
> >  The general rule is "if you
> > want to support that, do it outside the kernel.org tree".
> 
> Which "that" are you referring to?

supporting really ancient kernels

Re: [PATCH 8 of 20] ipath - sysfs support for core driver

[Roland Dreier]

    Greg> The main issue is that if you create a sysfs file like this,
    Greg> and then in 3 months realize that you need to change one of
    Greg> those characters to be something else, you are in big
    Greg> trouble...

I think that PortInfo and NodeInfo might be fair game for sysfs files,
because they are actually defined in the IB spec with a binary format
that is sent on the wire.  So they're not going to change.

On the other hand it's not clear to me why using that wire protocol as
an interface to userspace is a good idea...

 - R.

Re: Revenge of the sysfs maintainer! (was Re: [PATCH 8 of 20] ipath - sysfs support for core driver)

[Dave Jones]

On Thu, Mar 09, 2006 at 10:34:01PM -0800, Greg KH wrote:
 > On Thu, Mar 09, 2006 at 08:58:13PM -0800, Bryan O'Sullivan wrote:
 > > On Thu, 2006-03-09 at 17:00 -0800, Greg KH wrote:
 > > 
 > > > They are in the latest -mm tree if you wish to use them.  Unfortunatly
 > > > it might look like they will not work out, due to the per-cpu relay
 > > > files not working properly with Paul's patches at the moment.
 > > 
 > > Hmm, OK.
 > > 
 > > > What's wrong with debugfs?
 > > 
 > > It's not configured into the kernels of either of the distros I use (Red
 > > Hat or SUSE).
 > 
 > Well, I can do something about SuSE, it's up to someone else to persuade
 > Red Hat :)

It's been built into Fedora kernels since it was merged upstream.
It isn't in RHEL4, as it wasn't around back in 2.6.9, and unless there's
a really compelling argument for it, I doubt it'll be backported.

		Dave

-- 
http://www.codemonkey.org.uk

Re: Revenge of the sysfs maintainer! (was Re: [PATCH 8 of 20] ipath - sysfs support for core driver)

[Dave Jones]

On Fri, Mar 10, 2006 at 05:51:41AM -0800, Bryan O'Sullivan wrote:

 > What antique kernels?  It's not enabled in the latest SLES beta
 > (2.6.16-git6 or so), or in Fedora rawhide (also 2.6.16-git).

Bzzzrrt.

(10:54:01:davej@nemesis:~)$ uname -r
2.6.15-1.2027_FC5
(10:54:04:davej@nemesis:~)$ grep debug /proc/filesystems
nodev   debugfs

Been there since it was merged upstream.
It's not mounted anywhere by default, as imo, it shouldn't be.

		Dave

-- 
http://www.codemonkey.org.uk

Re: Revenge of the sysfs maintainer! (was Re: [PATCH 8 of 20] ipath - sysfs support for core driver)

[Bryan O'Sullivan]

On Fri, 2006-03-10 at 15:06 +0100, Arjan van de Ven wrote:

> > They mightn't be exactly today's kernels, but they're no more than two
> > or three weeks old.  CONFIG_DEBUG_FS has been in the kernel for a long
> > time, and it's still not being picked up.
> 
> but it's a module; you can ship it no problem yourself if you go through
> the hell of shipping external modules

Yes, we can ship it ourselves.  But a significant point of this exercise
is to have the drivers be available to people who use unmodified
distros, and don't want to download other bits to do so.

If Greg can get SUSE to turn on debugfs, that's great.  I can ask Dave
Jones or Doug Ledford or some other Fedora/RedHat kernel person to do
likewise, but they're not beholden to me in any way, so god knows what
my chances are :-)

The "have it just work in vendor distros" notion is also why the OpenIB
community as a whole is focusing on rolling out a 1.0 release of the IB
userspace code, so that people can expect their distros to simply work
with Infiniband hardware.

	<b

Re: Revenge of the sysfs maintainer! (was Re: [PATCH 8 of 20] ipath - sysfs support for core driver)

[Arjan van de Ven]

On Fri, 2006-03-10 at 07:55 -0800, Bryan O'Sullivan wrote:
> On Fri, 2006-03-10 at 15:06 +0100, Arjan van de Ven wrote:
> 
> > > They mightn't be exactly today's kernels, but they're no more than two
> > > or three weeks old.  CONFIG_DEBUG_FS has been in the kernel for a long
> > > time, and it's still not being picked up.
> > 
> > but it's a module; you can ship it no problem yourself if you go through
> > the hell of shipping external modules
> 
> Yes, we can ship it ourselves.  But a significant point of this exercise
> is to have the drivers be available to people who use unmodified
> distros, and don't want to download other bits to do so.

well shipping one .ko or two .ko's... same difference. Or maybe in your
case it's a 4->5 transition. No Difference from a user point of view.


So sorry I'm not buying your argument very much.

Re: Revenge of the sysfs maintainer! (was Re: [PATCH 8 of 20] ipath - sysfs support for core driver)

[Dave Jones]

On Fri, Mar 10, 2006 at 07:55:21AM -0800, Bryan O'Sullivan wrote:

 > If Greg can get SUSE to turn on debugfs, that's great.  I can ask Dave
 > Jones or Doug Ledford or some other Fedora/RedHat kernel person to do
 > likewise, but they're not beholden to me in any way, so god knows what
 > my chances are :-)

I've acknowledged it was already enabled.
I've posted a log from a shell session showing that it's there in /proc/filesystems
in current builds.

What more exactly do you want?

		Dave

-- 
http://www.codemonkey.org.uk

Re: Revenge of the sysfs maintainer! (was Re: [PATCH 8 of 20] ipath - sysfs support for core driver)

[Bryan O'Sullivan]

On Fri, 2006-03-10 at 11:25 -0500, Dave Jones wrote:

> What more exactly do you want?

Nothing.  The reason I said it wasn't enabled was that I tried mounting
it, but evidently I fubmled something.

Thanks,

	<b

Re: Revenge of the sysfs maintainer! (was Re: [PATCH 8 of 20] ipath - sysfs support for core driver)

[Bryan O'Sullivan]

On Fri, 2006-03-10 at 17:24 +0100, Arjan van de Ven wrote:

> well shipping one .ko or two .ko's... same difference. Or maybe in your
> case it's a 4->5 transition. No Difference from a user point of view.

I think you misunderstand.  The goal is for *us* to ship *none* :-)

Anyway, I think we have a few different possible workable solutions that
are not netlink, so I'm happy.

	<b

Re: Revenge of the sysfs maintainer! (was Re: [PATCH 8 of 20] ipath - sysfs support for core driver)

[Arjan van de Ven]

On Fri, 2006-03-10 at 08:36 -0800, Bryan O'Sullivan wrote:
> On Fri, 2006-03-10 at 17:24 +0100, Arjan van de Ven wrote:
> 
> > well shipping one .ko or two .ko's... same difference. Or maybe in your
> > case it's a 4->5 transition. No Difference from a user point of view.
> 
> I think you misunderstand.  The goal is for *us* to ship *none* :-)

then it gets easier.. KConfig lets you select debugfs. Done. 

Re: Revenge of the sysfs maintainer! (was Re: [PATCH 8 of 20] ipath - sysfs support for core driver)

[Greg KH]

On Fri, Mar 10, 2006 at 11:25:52AM -0500, Dave Jones wrote:
> On Fri, Mar 10, 2006 at 07:55:21AM -0800, Bryan O'Sullivan wrote:
> 
>  > If Greg can get SUSE to turn on debugfs, that's great.  I can ask Dave
>  > Jones or Doug Ledford or some other Fedora/RedHat kernel person to do
>  > likewise, but they're not beholden to me in any way, so god knows what
>  > my chances are :-)
> 
> I've acknowledged it was already enabled.

And I just looked at the SuSE kernel, and it is already enabled too.  So
it looks like Bryan didn't even check either distro before saying it
wasn't there :(

thanks,

greg k-h

Re: Revenge of the sysfs maintainer! (was Re: [PATCH 8 of 20] ipath - sysfs support for core driver)

[Bryan O'Sullivan]

On Fri, 2006-03-10 at 08:48 -0800, Greg KH wrote:

> And I just looked at the SuSE kernel, and it is already enabled too.  So
> it looks like Bryan didn't even check either distro before saying it
> wasn't there :(

No, I checked both, but it was late at night and I fumbled my mount
command line in each case.  Sigh.

	<b