[PATCH] lpfc : add module parameter that allows adapter instances to avoid attachment

[PATCH] lpfc : add module parameter that allows adapter instances to avoid attachment

[James Smart]

This patch adds a module parameter that supplies a text string containing a
list of PCI <bus>:<slot>.<func> values to identify adapter instances that
should *not* be attached to by the driver.

-- james s


 Signed-off-by: James Smart <james.smart@emulex.com>

 ---

 lpfc_attr.c |   21 ++++++++++++++++++++
 lpfc_crtn.h |    1 
 lpfc_init.c |   61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 83 insertions(+)


diff -upNr a/drivers/scsi/lpfc/lpfc_attr.c b/drivers/scsi/lpfc/lpfc_attr.c
--- a/drivers/scsi/lpfc/lpfc_attr.c	2009-01-05 11:50:39.000000000 -0500
+++ b/drivers/scsi/lpfc/lpfc_attr.c	2009-03-26 13:57:34.000000000 -0400
@@ -1451,6 +1451,26 @@ lpfc_##attr##_store(struct device *dev, 
 		return -EINVAL;\
 }
 
+/*
+# lpfc_exclude_hba: This parameter contain a list of PCI slots with lpfc HBAs
+# 	which need to be excluded from attachment by the driver.
+#  String Syntax: <bus>:<slot>.<func>[|<bus>:<slot>.<func>...]
+*/
+char *lpfc_exclude_hba;
+module_param(lpfc_exclude_hba, charp, S_IRUGO);
+MODULE_PARM_DESC(lpfc_exclude_hba, "list of lpfc HBA PCI locations"
+	" to be excluded from attachment:  '<bus>:<slot>.<func>' separated by"
+	" | character");
+static ssize_t
+lpfc_exclude_hba_show(struct device *dev, struct device_attribute *attr,
+	char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%s\n",
+		(lpfc_exclude_hba == NULL) ? "" : lpfc_exclude_hba);
+}
+
+static DEVICE_ATTR(lpfc_exclude_hba, S_IRUGO, lpfc_exclude_hba_show,
+		NULL);
 
 #define LPFC_ATTR(name, defval, minval, maxval, desc) \
 static int lpfc_##name = defval;\
@@ -2939,6 +2959,7 @@ struct device_attribute *lpfc_hba_attrs[
 	&dev_attr_lpfc_max_scsicmpl_time,
 	&dev_attr_lpfc_stat_data_ctrl,
 	&dev_attr_lpfc_prot_sg_seg_cnt,
+	&dev_attr_lpfc_exclude_hba,
 	NULL,
 };
 
diff -upNr a/drivers/scsi/lpfc/lpfc_crtn.h b/drivers/scsi/lpfc/lpfc_crtn.h
--- a/drivers/scsi/lpfc/lpfc_crtn.h	2008-12-16 09:34:12.000000000 -0500
+++ b/drivers/scsi/lpfc/lpfc_crtn.h	2009-03-26 13:41:47.000000000 -0400
@@ -264,6 +264,7 @@ extern struct fc_function_template lpfc_
 extern struct fc_function_template lpfc_vport_transport_functions;
 extern int lpfc_sli_mode;
 extern int lpfc_enable_npiv;
+extern char *lpfc_exclude_hba;
 
 int  lpfc_vport_symbolic_node_name(struct lpfc_vport *, char *, size_t);
 int  lpfc_vport_symbolic_port_name(struct lpfc_vport *, char *,	size_t);
diff -upNr a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
--- a/drivers/scsi/lpfc/lpfc_init.c	2009-01-05 11:50:39.000000000 -0500
+++ b/drivers/scsi/lpfc/lpfc_init.c	2009-03-26 13:51:01.000000000 -0400
@@ -33,6 +33,7 @@
 #include <scsi/scsi_device.h>
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_transport_fc.h>
+#include <linux/ctype.h>
 
 #include "lpfc_hw.h"
 #include "lpfc_sli.h"
@@ -54,6 +55,7 @@ spinlock_t _dump_buf_lock;
 static int lpfc_parse_vpd(struct lpfc_hba *, uint8_t *, int);
 static void lpfc_get_hba_model_desc(struct lpfc_hba *, uint8_t *, uint8_t *);
 static int lpfc_post_rcv_buf(struct lpfc_hba *);
+static uint32_t lpfc_is_excluded_hba(struct pci_dev *pdev);
 
 static struct scsi_transport_template *lpfc_transport_template = NULL;
 static struct scsi_transport_template *lpfc_vport_transport_template = NULL;
@@ -2607,6 +2609,17 @@ lpfc_pci_probe_one(struct pci_dev *pdev,
 	int bars = pci_select_bars(pdev, IORESOURCE_MEM);
 	struct lpfc_adapter_event_header adapter_event;
 
+	/*
+	 * Check if FC controller is excluded from binding to lpfc driver.
+	 */
+	if (lpfc_is_excluded_hba(pdev)) {
+		dev_printk(KERN_ERR, &pdev->dev, "%s: controller %02x:%02x.%x"
+			" is excluded from binding.\n",
+			LPFC_DRIVER_NAME, pdev->bus->number,
+			PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+		return -EPERM;
+	}
+
 	if (pci_enable_device_mem(pdev))
 		goto out;
 	if (pci_request_selected_regions(pdev, bars, LPFC_DRIVER_NAME))
@@ -3471,6 +3484,54 @@ lpfc_init(void)
 	return error;
 }
 
+/*
+ * lpfc_is_excluded_hba - Check if the PCI device is excluded by user.
+ * @pdev: pointer to pci device.
+ *
+ * Return 1 if the pci device is excluded by the lpfc_exclude_hba
+ * parameter else return 0.
+ */
+static uint32_t
+lpfc_is_excluded_hba(struct pci_dev *pdev)
+{
+	u_int	bus, slot, func;
+	char	*cp, pci_buf[9];
+	char	*conf_p = lpfc_exclude_hba;
+
+	if (!conf_p)
+		return 0;
+
+	while (*conf_p) {
+		if (*conf_p == '|' || isspace((int)*conf_p)) {
+			conf_p++;
+			continue;
+		}
+		cp = pci_buf;
+
+		do {
+			*cp = *conf_p;
+			cp++;
+			conf_p++;
+			if (!(*conf_p) || (*conf_p == '-') || (*conf_p == '|')
+				|| isspace((int)*conf_p))
+				break;
+		} while (cp < pci_buf + sizeof(pci_buf));
+
+		*cp = '\0';
+
+		if (sscanf(pci_buf, "%x:%x.%x", &bus, &slot, &func) != 3 ||
+			(bus | slot | func) > 0xff)
+			continue;
+
+		if ((u_char)bus == pdev->bus->number &&
+		    (u_char)slot == PCI_SLOT(pdev->devfn) &&
+		    (u_char)func == PCI_FUNC(pdev->devfn)) {
+			return 1;
+		}
+	}
+	return 0;
+}
+
 /**
  * lpfc_exit: lpfc module removal routine.
  *

Re: [PATCH] lpfc : add module parameter that allows adapter instances to avoid attachment

[James Bottomley]

On Thu, 2009-03-26 at 15:55 -0400, James Smart wrote:
> This patch adds a module parameter that supplies a text string containing a
> list of PCI <bus>:<slot>.<func> values to identify adapter instances that
> should *not* be attached to by the driver.
> 
> -- james s
> 
> 
>  Signed-off-by: James Smart <james.smart@emulex.com>

Is there a reason why you can't just use the generic unbind interface?

James

Re: [PATCH] lpfc : add module parameter that allows adapter instances to avoid attachment

[James Smart]

The oem requirement we have explicitly states not to use the unbind interface
(we proposed unbind at first as well).

The issue is what happens on the link while we are bound for that short
amount of time. It confuses the things on the other side of the link.
There's a secondary driver that ends up binding to the adapters we exclude,
and the things on the other side only expected to see the second driver.

-- james s


James Bottomley wrote:
> On Thu, 2009-03-26 at 15:55 -0400, James Smart wrote:
>> This patch adds a module parameter that supplies a text string containing a
>> list of PCI <bus>:<slot>.<func> values to identify adapter instances that
>> should *not* be attached to by the driver.
>>
>> -- james s
>>
>>
>>  Signed-off-by: James Smart <james.smart@emulex.com>
> 
> Is there a reason why you can't just use the generic unbind interface?
> 
> James
> 
> 
> 

Re: [PATCH] lpfc : add module parameter that allows adapter instances to avoid attachment

[Randy Dunlap]

James Smart wrote:
> The oem requirement we have explicitly states not to use the unbind
> interface
> (we proposed unbind at first as well).
> 
> The issue is what happens on the link while we are bound for that short
> amount of time. It confuses the things on the other side of the link.
> There's a secondary driver that ends up binding to the adapters we exclude,
> and the things on the other side only expected to see the second driver.
> 
> -- james s
> 
> 
> James Bottomley wrote:
>> On Thu, 2009-03-26 at 15:55 -0400, James Smart wrote:
>>> This patch adds a module parameter that supplies a text string
>>> containing a
>>> list of PCI <bus>:<slot>.<func> values to identify adapter instances
>>> that
>>> should *not* be attached to by the driver.

What does this do on systems that use PCI domains?

  domain:bus:slot.func  (??)


>>> -- james s
>>>
>>>
>>>  Signed-off-by: James Smart <james.smart@emulex.com>
>>
>> Is there a reason why you can't just use the generic unbind interface?
>>
>> James


-- 
~Randy

Re: [PATCH] lpfc : add module parameter that allows adapter instances to avoid attachment

[Matthew Wilcox]

On Thu, Mar 26, 2009 at 03:55:00PM -0400, James Smart wrote:
> This patch adds a module parameter that supplies a text string containing a
> list of PCI <bus>:<slot>.<func> values to identify adapter instances that
> should *not* be attached to by the driver.

This has a problem with domains.

If something like this is going in, and people do keep proposing it, we
need a central place for it in the PCI core, so drivers aren't all
parsing strings themselves.

Something like:

pci_device_listed(char *s, struct pci_dev *pdev);

If we don't do that, each driver will have its own separator, and have
its own set of parsing bugs.


It's still not clear to me that this is the right approach to be taking
to v12n, but if we do end up going down the route of having users pass
complex instructions on the command line, it does need to not be done in
the driver.

> -- james s
> 
> 
>  Signed-off-by: James Smart <james.smart@emulex.com>
> 
>  ---
> 
>  lpfc_attr.c |   21 ++++++++++++++++++++
>  lpfc_crtn.h |    1 
>  lpfc_init.c |   61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 83 insertions(+)
> 
> 
> diff -upNr a/drivers/scsi/lpfc/lpfc_attr.c b/drivers/scsi/lpfc/lpfc_attr.c
> --- a/drivers/scsi/lpfc/lpfc_attr.c	2009-01-05 11:50:39.000000000 -0500
> +++ b/drivers/scsi/lpfc/lpfc_attr.c	2009-03-26 13:57:34.000000000 -0400
> @@ -1451,6 +1451,26 @@ lpfc_##attr##_store(struct device *dev, 
>  		return -EINVAL;\
>  }
>  
> +/*
> +# lpfc_exclude_hba: This parameter contain a list of PCI slots with lpfc HBAs
> +# 	which need to be excluded from attachment by the driver.
> +#  String Syntax: <bus>:<slot>.<func>[|<bus>:<slot>.<func>...]
> +*/
> +char *lpfc_exclude_hba;
> +module_param(lpfc_exclude_hba, charp, S_IRUGO);
> +MODULE_PARM_DESC(lpfc_exclude_hba, "list of lpfc HBA PCI locations"
> +	" to be excluded from attachment:  '<bus>:<slot>.<func>' separated by"
> +	" | character");
> +static ssize_t
> +lpfc_exclude_hba_show(struct device *dev, struct device_attribute *attr,
> +	char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "%s\n",
> +		(lpfc_exclude_hba == NULL) ? "" : lpfc_exclude_hba);
> +}
> +
> +static DEVICE_ATTR(lpfc_exclude_hba, S_IRUGO, lpfc_exclude_hba_show,
> +		NULL);
>  
>  #define LPFC_ATTR(name, defval, minval, maxval, desc) \
>  static int lpfc_##name = defval;\
> @@ -2939,6 +2959,7 @@ struct device_attribute *lpfc_hba_attrs[
>  	&dev_attr_lpfc_max_scsicmpl_time,
>  	&dev_attr_lpfc_stat_data_ctrl,
>  	&dev_attr_lpfc_prot_sg_seg_cnt,
> +	&dev_attr_lpfc_exclude_hba,
>  	NULL,
>  };
>  
> diff -upNr a/drivers/scsi/lpfc/lpfc_crtn.h b/drivers/scsi/lpfc/lpfc_crtn.h
> --- a/drivers/scsi/lpfc/lpfc_crtn.h	2008-12-16 09:34:12.000000000 -0500
> +++ b/drivers/scsi/lpfc/lpfc_crtn.h	2009-03-26 13:41:47.000000000 -0400
> @@ -264,6 +264,7 @@ extern struct fc_function_template lpfc_
>  extern struct fc_function_template lpfc_vport_transport_functions;
>  extern int lpfc_sli_mode;
>  extern int lpfc_enable_npiv;
> +extern char *lpfc_exclude_hba;
>  
>  int  lpfc_vport_symbolic_node_name(struct lpfc_vport *, char *, size_t);
>  int  lpfc_vport_symbolic_port_name(struct lpfc_vport *, char *,	size_t);
> diff -upNr a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
> --- a/drivers/scsi/lpfc/lpfc_init.c	2009-01-05 11:50:39.000000000 -0500
> +++ b/drivers/scsi/lpfc/lpfc_init.c	2009-03-26 13:51:01.000000000 -0400
> @@ -33,6 +33,7 @@
>  #include <scsi/scsi_device.h>
>  #include <scsi/scsi_host.h>
>  #include <scsi/scsi_transport_fc.h>
> +#include <linux/ctype.h>
>  
>  #include "lpfc_hw.h"
>  #include "lpfc_sli.h"
> @@ -54,6 +55,7 @@ spinlock_t _dump_buf_lock;
>  static int lpfc_parse_vpd(struct lpfc_hba *, uint8_t *, int);
>  static void lpfc_get_hba_model_desc(struct lpfc_hba *, uint8_t *, uint8_t *);
>  static int lpfc_post_rcv_buf(struct lpfc_hba *);
> +static uint32_t lpfc_is_excluded_hba(struct pci_dev *pdev);
>  
>  static struct scsi_transport_template *lpfc_transport_template = NULL;
>  static struct scsi_transport_template *lpfc_vport_transport_template = NULL;
> @@ -2607,6 +2609,17 @@ lpfc_pci_probe_one(struct pci_dev *pdev,
>  	int bars = pci_select_bars(pdev, IORESOURCE_MEM);
>  	struct lpfc_adapter_event_header adapter_event;
>  
> +	/*
> +	 * Check if FC controller is excluded from binding to lpfc driver.
> +	 */
> +	if (lpfc_is_excluded_hba(pdev)) {
> +		dev_printk(KERN_ERR, &pdev->dev, "%s: controller %02x:%02x.%x"
> +			" is excluded from binding.\n",
> +			LPFC_DRIVER_NAME, pdev->bus->number,
> +			PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> +		return -EPERM;
> +	}
> +
>  	if (pci_enable_device_mem(pdev))
>  		goto out;
>  	if (pci_request_selected_regions(pdev, bars, LPFC_DRIVER_NAME))
> @@ -3471,6 +3484,54 @@ lpfc_init(void)
>  	return error;
>  }
>  
> +/*
> + * lpfc_is_excluded_hba - Check if the PCI device is excluded by user.
> + * @pdev: pointer to pci device.
> + *
> + * Return 1 if the pci device is excluded by the lpfc_exclude_hba
> + * parameter else return 0.
> + */
> +static uint32_t
> +lpfc_is_excluded_hba(struct pci_dev *pdev)
> +{
> +	u_int	bus, slot, func;
> +	char	*cp, pci_buf[9];
> +	char	*conf_p = lpfc_exclude_hba;
> +
> +	if (!conf_p)
> +		return 0;
> +
> +	while (*conf_p) {
> +		if (*conf_p == '|' || isspace((int)*conf_p)) {
> +			conf_p++;
> +			continue;
> +		}
> +		cp = pci_buf;
> +
> +		do {
> +			*cp = *conf_p;
> +			cp++;
> +			conf_p++;
> +			if (!(*conf_p) || (*conf_p == '-') || (*conf_p == '|')
> +				|| isspace((int)*conf_p))
> +				break;
> +		} while (cp < pci_buf + sizeof(pci_buf));
> +
> +		*cp = '\0';
> +
> +		if (sscanf(pci_buf, "%x:%x.%x", &bus, &slot, &func) != 3 ||
> +			(bus | slot | func) > 0xff)
> +			continue;
> +
> +		if ((u_char)bus == pdev->bus->number &&
> +		    (u_char)slot == PCI_SLOT(pdev->devfn) &&
> +		    (u_char)func == PCI_FUNC(pdev->devfn)) {
> +			return 1;
> +		}
> +	}
> +	return 0;
> +}
> +
>  /**
>   * lpfc_exit: lpfc module removal routine.
>   *
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] lpfc : add module parameter that allows adapter instances to avoid attachment

[James Bottomley]

On Thu, 2009-03-26 at 16:37 -0400, James Smart wrote:
> The oem requirement we have explicitly states not to use the unbind interface
> (we proposed unbind at first as well).
> 
> The issue is what happens on the link while we are bound for that short
> amount of time. It confuses the things on the other side of the link.
> There's a secondary driver that ends up binding to the adapters we exclude,
> and the things on the other side only expected to see the second driver.

OK, so it sounds like a proposed extension to the generic unbind
interfaces where you specify a no_bind list on the kernel or module
command line.

Also (as Randy pointed out) you don't account for the domain.  However,
you could just by not bothering to convert the string to numbers for the
compare, just compare on dev->bus_id ... which is how a generic one
would work.

James

Re: [PATCH] lpfc : add module parameter that allows adapter instances to avoid attachment

[James Smart]

Ok - I'll see what I can come up with.

-- james s

James Bottomley wrote:
> On Thu, 2009-03-26 at 16:37 -0400, James Smart wrote:
>> The oem requirement we have explicitly states not to use the unbind interface
>> (we proposed unbind at first as well).
>>
>> The issue is what happens on the link while we are bound for that short
>> amount of time. It confuses the things on the other side of the link.
>> There's a secondary driver that ends up binding to the adapters we exclude,
>> and the things on the other side only expected to see the second driver.
> 
> OK, so it sounds like a proposed extension to the generic unbind
> interfaces where you specify a no_bind list on the kernel or module
> command line.
> 
> Also (as Randy pointed out) you don't account for the domain.  However,
> you could just by not bothering to convert the string to numbers for the
> compare, just compare on dev->bus_id ... which is how a generic one
> would work.
> 
> James
> 
> 
> 

Re: [PATCH] lpfc : add module parameter that allows adapter instances to avoid attachment

[Matthew Wilcox]

On Thu, Mar 26, 2009 at 03:54:09PM -0500, James Bottomley wrote:
> Also (as Randy pointed out) you don't account for the domain.  However,
> you could just by not bothering to convert the string to numbers for the
> compare, just compare on dev->bus_id ... which is how a generic one
> would work.

I think it needs to be PCI and not device core.  bus_id isn't guaranteed
to be unique between different bus types.  I don't know of any that
conflict, but we don't want to inadvertently disable, say, an SBUS
device while trying to disable a PCI device.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] lpfc : add module parameter that allows adapter instances to avoid attachment

[Matthew Wilcox]

On Thu, Mar 26, 2009 at 04:37:06PM -0400, James Smart wrote:
> The oem requirement we have explicitly states not to use the unbind 
> interface
> (we proposed unbind at first as well).
> 
> The issue is what happens on the link while we are bound for that short
> amount of time. It confuses the things on the other side of the link.
> There's a secondary driver that ends up binding to the adapters we exclude,
> and the things on the other side only expected to see the second driver.

By the way, are these interfaces real or virtual (SR-IOV virtual)?  PCI
bus numbers aren't the most stable way of referring to devices any more.
I don't suppose we can key this off PCIe DSN, can we?

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] lpfc : add module parameter that allows adapter instances to avoid attachment

[James Bottomley]

On Fri, 2009-03-27 at 09:09 -0600, Matthew Wilcox wrote:
> On Thu, Mar 26, 2009 at 03:54:09PM -0500, James Bottomley wrote:
> > Also (as Randy pointed out) you don't account for the domain.  However,
> > you could just by not bothering to convert the string to numbers for the
> > compare, just compare on dev->bus_id ... which is how a generic one
> > would work.
> 
> I think it needs to be PCI and not device core.  bus_id isn't guaranteed
> to be unique between different bus types.  I don't know of any that
> conflict, but we don't want to inadvertently disable, say, an SBUS
> device while trying to disable a PCI device.

Actually they better be ... we'll get into real trouble if they're not
because of the way we flatten the space for multiple bus binding
drivers.

Even if I accepted your argument, I still can't see why we'd only
implement this for PCI, and thus why it shouldn't be in the generic
device part (except possibly with a bus type name qualifier).

James

Re: [PATCH] lpfc : add module parameter that allows adapter instances to avoid attachment

[Matthew Wilcox]

On Fri, Mar 27, 2009 at 03:33:57PM +0000, James Bottomley wrote:
> Actually they better be ... we'll get into real trouble if they're not
> because of the way we flatten the space for multiple bus binding
> drivers.

Er ... where do we do that?  As far as I'm aware, to bind to multiple
busses, you register multiple foo_driver.  They're separate namespaces.

> Even if I accepted your argument, I still can't see why we'd only
> implement this for PCI, and thus why it shouldn't be in the generic
> device part (except possibly with a bus type name qualifier).

Adding the bus name qualifier would work.  It seems like much more typing,
in order to get what advantage over just implementing it for PCI?

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] lpfc : add module parameter that allows adapter instances to avoid attachment

[James Bottomley]

On Fri, 2009-03-27 at 10:14 -0600, Matthew Wilcox wrote:
> On Fri, Mar 27, 2009 at 03:33:57PM +0000, James Bottomley wrote:
> > Actually they better be ... we'll get into real trouble if they're not
> > because of the way we flatten the space for multiple bus binding
> > drivers.
> 
> Er ... where do we do that?  As far as I'm aware, to bind to multiple
> busses, you register multiple foo_driver.  They're separate namespaces.

Yes, you're right.  I was sure there was some way of flattening the
namespace so we mixed bus_ids but perhaps Kay fixed it all.

> > Even if I accepted your argument, I still can't see why we'd only
> > implement this for PCI, and thus why it shouldn't be in the generic
> > device part (except possibly with a bus type name qualifier).
> 
> Adding the bus name qualifier would work.  It seems like much more typing,
> in order to get what advantage over just implementing it for PCI?

The advantage is it's done generically and so works for all buses
instead of just PCI.  The disadvantage, I suppose, is that the matching
must be on only things the generic model knows about, however, the
proposal was PCI IDs, that doesn't seem to be a problem ... if you were
to make it PCIe DSN then yes, it would be harder to do it generically.

James