[PATCH] scsi/ibmvscsi: /sys/class/scsi_host/hostX/config doesn't show any information

[PATCH] scsi/ibmvscsi: /sys/class/scsi_host/hostX/config doesn't show any information

[olaf]

From: Linda Xie <lxiep@us.ibm.com>

Expected result:
It should show something like this:
x1521p4:~ # cat /sys/class/scsi_host/host1/config
PARTITIONNAME='x1521p4'
NWSDNAME='X1521P4'
HOSTNAME='X1521P4'
DOMAINNAME='RCHLAND.IBM.COM'
NAMESERVERS='9.10.244.100 9.10.244.200'

Actual result:
x1521p4:~ # cat /sys/class/scsi_host/host0/config
x1521p4:~ #

This patch changes the size of the buffer used for transfering config
data to 4K. It was tested against 2.6.19-rc2 tree.

Reported by IBM during SLES11 beta testing:

https://bugzilla.novell.com/show_bug.cgi?id=439970
LTC49349

Signed-off-by: Olaf Hering <olaf@aepfle.de>

diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index e580aa4..1513ca8 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -93,6 +93,8 @@ static int max_requests = IBMVSCSI_MAX_REQUESTS_DEFAULT;
 static int max_events = IBMVSCSI_MAX_REQUESTS_DEFAULT + 2;
 static int fast_fail = 1;
 static int client_reserve = 1;
+/* host data buffer size */
+#define HOST_BUFFER_SIZE 4096
 
 static struct scsi_transport_template *ibmvscsi_transport_template;
 
@@ -1666,7 +1668,7 @@ static ssize_t show_host_srp_version(struct device *dev,
 	struct ibmvscsi_host_data *hostdata = shost_priv(shost);
 	int len;
 
-	len = snprintf(buf, PAGE_SIZE, "%s\n",
+       len = snprintf(buf, HOST_BUFFER_SIZE, "%s\n",
 		       hostdata->madapter_info.srp_version);
 	return len;
 }
@@ -1687,7 +1689,7 @@ static ssize_t show_host_partition_name(struct device *dev,
 	struct ibmvscsi_host_data *hostdata = shost_priv(shost);
 	int len;
 
-	len = snprintf(buf, PAGE_SIZE, "%s\n",
+       len = snprintf(buf, HOST_BUFFER_SIZE, "%s\n",
 		       hostdata->madapter_info.partition_name);
 	return len;
 }
@@ -1708,7 +1710,7 @@ static ssize_t show_host_partition_number(struct device *dev,
 	struct ibmvscsi_host_data *hostdata = shost_priv(shost);
 	int len;
 
-	len = snprintf(buf, PAGE_SIZE, "%d\n",
+       len = snprintf(buf, HOST_BUFFER_SIZE, "%d\n",
 		       hostdata->madapter_info.partition_number);
 	return len;
 }
@@ -1728,7 +1730,7 @@ static ssize_t show_host_mad_version(struct device *dev,
 	struct ibmvscsi_host_data *hostdata = shost_priv(shost);
 	int len;
 
-	len = snprintf(buf, PAGE_SIZE, "%d\n",
+       len = snprintf(buf, HOST_BUFFER_SIZE, "%d\n",
 		       hostdata->madapter_info.mad_version);
 	return len;
 }
@@ -1748,7 +1750,7 @@ static ssize_t show_host_os_type(struct device *dev,
 	struct ibmvscsi_host_data *hostdata = shost_priv(shost);
 	int len;
 
-	len = snprintf(buf, PAGE_SIZE, "%d\n", hostdata->madapter_info.os_type);
+       len = snprintf(buf, HOST_BUFFER_SIZE, "%d\n", hostdata->madapter_info.os_type);
 	return len;
 }
 
@@ -1767,7 +1769,7 @@ static ssize_t show_host_config(struct device *dev,
 	struct ibmvscsi_host_data *hostdata = shost_priv(shost);
 
 	/* returns null-terminated host config data */
-	if (ibmvscsi_do_host_config(hostdata, buf, PAGE_SIZE) == 0)
+       if (ibmvscsi_do_host_config(hostdata, buf, HOST_BUFFER_SIZE) == 0)
 		return strlen(buf);
 	else
 		return 0;
-- 
1.7.10.4

Re: [PATCH] scsi/ibmvscsi: /sys/class/scsi_host/hostX/config doesn't show any information

[Benjamin Herrenschmidt]

On Wed, 2012-07-18 at 18:49 +0200, olaf@aepfle.de wrote:
> From: Linda Xie <lxiep@us.ibm.com>

James, can I assume you're picking up those two ?

Cheers,
Ben.

> Expected result:
> It should show something like this:
> x1521p4:~ # cat /sys/class/scsi_host/host1/config
> PARTITIONNAME='x1521p4'
> NWSDNAME='X1521P4'
> HOSTNAME='X1521P4'
> DOMAINNAME='RCHLAND.IBM.COM'
> NAMESERVERS='9.10.244.100 9.10.244.200'
> 
> Actual result:
> x1521p4:~ # cat /sys/class/scsi_host/host0/config
> x1521p4:~ #
> 
> This patch changes the size of the buffer used for transfering config
> data to 4K. It was tested against 2.6.19-rc2 tree.
> 
> Reported by IBM during SLES11 beta testing:
> 
> https://bugzilla.novell.com/show_bug.cgi?id=439970
> LTC49349
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> 
> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
> index e580aa4..1513ca8 100644
> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
> @@ -93,6 +93,8 @@ static int max_requests = IBMVSCSI_MAX_REQUESTS_DEFAULT;
>  static int max_events = IBMVSCSI_MAX_REQUESTS_DEFAULT + 2;
>  static int fast_fail = 1;
>  static int client_reserve = 1;
> +/* host data buffer size */
> +#define HOST_BUFFER_SIZE 4096
>  
>  static struct scsi_transport_template *ibmvscsi_transport_template;
>  
> @@ -1666,7 +1668,7 @@ static ssize_t show_host_srp_version(struct device *dev,
>  	struct ibmvscsi_host_data *hostdata = shost_priv(shost);
>  	int len;
>  
> -	len = snprintf(buf, PAGE_SIZE, "%s\n",
> +       len = snprintf(buf, HOST_BUFFER_SIZE, "%s\n",
>  		       hostdata->madapter_info.srp_version);
>  	return len;
>  }
> @@ -1687,7 +1689,7 @@ static ssize_t show_host_partition_name(struct device *dev,
>  	struct ibmvscsi_host_data *hostdata = shost_priv(shost);
>  	int len;
>  
> -	len = snprintf(buf, PAGE_SIZE, "%s\n",
> +       len = snprintf(buf, HOST_BUFFER_SIZE, "%s\n",
>  		       hostdata->madapter_info.partition_name);
>  	return len;
>  }
> @@ -1708,7 +1710,7 @@ static ssize_t show_host_partition_number(struct device *dev,
>  	struct ibmvscsi_host_data *hostdata = shost_priv(shost);
>  	int len;
>  
> -	len = snprintf(buf, PAGE_SIZE, "%d\n",
> +       len = snprintf(buf, HOST_BUFFER_SIZE, "%d\n",
>  		       hostdata->madapter_info.partition_number);
>  	return len;
>  }
> @@ -1728,7 +1730,7 @@ static ssize_t show_host_mad_version(struct device *dev,
>  	struct ibmvscsi_host_data *hostdata = shost_priv(shost);
>  	int len;
>  
> -	len = snprintf(buf, PAGE_SIZE, "%d\n",
> +       len = snprintf(buf, HOST_BUFFER_SIZE, "%d\n",
>  		       hostdata->madapter_info.mad_version);
>  	return len;
>  }
> @@ -1748,7 +1750,7 @@ static ssize_t show_host_os_type(struct device *dev,
>  	struct ibmvscsi_host_data *hostdata = shost_priv(shost);
>  	int len;
>  
> -	len = snprintf(buf, PAGE_SIZE, "%d\n", hostdata->madapter_info.os_type);
> +       len = snprintf(buf, HOST_BUFFER_SIZE, "%d\n", hostdata->madapter_info.os_type);
>  	return len;
>  }
>  
> @@ -1767,7 +1769,7 @@ static ssize_t show_host_config(struct device *dev,
>  	struct ibmvscsi_host_data *hostdata = shost_priv(shost);
>  
>  	/* returns null-terminated host config data */
> -	if (ibmvscsi_do_host_config(hostdata, buf, PAGE_SIZE) == 0)
> +       if (ibmvscsi_do_host_config(hostdata, buf, HOST_BUFFER_SIZE) == 0)
>  		return strlen(buf);
>  	else
>  		return 0;

Re: [PATCH] scsi/ibmvscsi: /sys/class/scsi_host/hostX/config doesn't show any information

[James Bottomley]

On Fri, 2012-07-27 at 15:19 +1000, Benjamin Herrenschmidt wrote:
> On Wed, 2012-07-18 at 18:49 +0200, olaf@aepfle.de wrote:
> > From: Linda Xie <lxiep@us.ibm.com>
> 
> James, can I assume you're picking up those two ?

If they get acked by the maintiners ...

James

Re: [PATCH] scsi/ibmvscsi: /sys/class/scsi_host/hostX/config doesn't show any information

[Benjamin Herrenschmidt]

On Fri, 2012-07-27 at 07:56 +0100, James Bottomley wrote:
> On Fri, 2012-07-27 at 15:19 +1000, Benjamin Herrenschmidt wrote:
> > On Wed, 2012-07-18 at 18:49 +0200, olaf@aepfle.de wrote:
> > > From: Linda Xie <lxiep@us.ibm.com>
> > 
> > James, can I assume you're picking up those two ?
> 
> If they get acked by the maintiners ...

I don't think we have a specific upstream maintainer for those drivers,
so let's assume it's my job... NAK on the sysfs one, the other one is
ok, I'll reply to the respective patches.

Cheers,
Ben.

Re: [PATCH] scsi/ibmvscsi: /sys/class/scsi_host/hostX/config doesn't show any information

[Benjamin Herrenschmidt]

n Wed, 2012-07-18 at 18:49 +0200, olaf@aepfle.de wrote:
> From: Linda Xie <lxiep@us.ibm.com>
> 
> Expected result:
> It should show something like this:
> x1521p4:~ # cat /sys/class/scsi_host/host1/config
> PARTITIONNAME='x1521p4'
> NWSDNAME='X1521P4'
> HOSTNAME='X1521P4'
> DOMAINNAME='RCHLAND.IBM.COM'
> NAMESERVERS='9.10.244.100 9.10.244.200'
> 
> Actual result:
> x1521p4:~ # cat /sys/class/scsi_host/host0/config
> x1521p4:~ #
> 
> This patch changes the size of the buffer used for transfering config
> data to 4K. It was tested against 2.6.19-rc2 tree.
> 
> Reported by IBM during SLES11 beta testing:

So this patch just seems to blindly replace all occurrences of PAGE_SIZE
with HOST_PAGE_SIZE which is utterly wrong. Only one of those needs to
be changed, the one passed to ibmvscsi_do_host_config() which is what's
visible to the server, all the rest is just sysfs attributes and should
remain as-is.

Additionally (not even mentioning that there is no explanation as to
what the real problem is anywhere in the changeset) I don't like the
fix. The root of the problem is that the MAD header has a 16-bit length
field, so writing 0x10000 (64K PAGE_SIZE) into it doesn't quite work.

So in addition to a better comment, I would suggest a fix more like
this:

scsi/ibmvscsi: Fix host config length field overflow

The length field in the host config packet is only 16-bit long, so
passing it 0x10000 (64K which is our standard PAGE_SIZE) doesn't
work and result in an empty config from the server.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: <stable@vger.kernel.org>
---

diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 3a6c474..337e8b3 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -1541,6 +1541,9 @@ static int ibmvscsi_do_host_config(struct ibmvscsi_host_data *hostdata,
 
 	host_config = &evt_struct->iu.mad.host_config;
 
+	/* The transport length field is only 16-bit */
+	length = min(0xffff, length);
+
 	/* Set up a lun reset SRP command */
 	memset(host_config, 0x00, sizeof(*host_config));
 	host_config->common.type = VIOSRP_HOST_CONFIG_TYPE;

Re: [PATCH] scsi/ibmvscsi: /sys/class/scsi_host/hostX/config doesn't show any information

[Robert Jennings]

On Sun, Jul 29, 2012 at 8:33 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> n Wed, 2012-07-18 at 18:49 +0200, olaf@aepfle.de wrote:
>> From: Linda Xie <lxiep@us.ibm.com>
>>
>> Expected result:
>> It should show something like this:
>> x1521p4:~ # cat /sys/class/scsi_host/host1/config
>> PARTITIONNAME='x1521p4'
>> NWSDNAME='X1521P4'
>> HOSTNAME='X1521P4'
>> DOMAINNAME='RCHLAND.IBM.COM'
>> NAMESERVERS='9.10.244.100 9.10.244.200'
>>
>> Actual result:
>> x1521p4:~ # cat /sys/class/scsi_host/host0/config
>> x1521p4:~ #
>>
>> This patch changes the size of the buffer used for transfering config
>> data to 4K. It was tested against 2.6.19-rc2 tree.
>>
>> Reported by IBM during SLES11 beta testing:
>
> So this patch just seems to blindly replace all occurrences of PAGE_SIZE
> with HOST_PAGE_SIZE which is utterly wrong. Only one of those needs to
> be changed, the one passed to ibmvscsi_do_host_config() which is what's
> visible to the server, all the rest is just sysfs attributes and should
> remain as-is.
>
> Additionally (not even mentioning that there is no explanation as to
> what the real problem is anywhere in the changeset) I don't like the
> fix. The root of the problem is that the MAD header has a 16-bit length
> field, so writing 0x10000 (64K PAGE_SIZE) into it doesn't quite work.
>
> So in addition to a better comment, I would suggest a fix more like
> this:
>
> scsi/ibmvscsi: Fix host config length field overflow
>
> The length field in the host config packet is only 16-bit long, so
> passing it 0x10000 (64K which is our standard PAGE_SIZE) doesn't
> work and result in an empty config from the server.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> CC: <stable@vger.kernel.org>

Acked-by: Robert Jennings <rcj@linux.vnet.ibm.com>

Tested with an IBM i host and confirmed the fix.

> ---
>
> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
> index 3a6c474..337e8b3 100644
> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
> @@ -1541,6 +1541,9 @@ static int ibmvscsi_do_host_config(struct ibmvscsi_host_data *hostdata,
>
>         host_config = &evt_struct->iu.mad.host_config;
>
> +       /* The transport length field is only 16-bit */
> +       length = min(0xffff, length);
> +
>         /* Set up a lun reset SRP command */
>         memset(host_config, 0x00, sizeof(*host_config));
>         host_config->common.type = VIOSRP_HOST_CONFIG_TYPE;
>
>
> --
> 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

Re: [PATCH] scsi/ibmvscsi: /sys/class/scsi_host/hostX/config doesn't show any information

[Robert Jennings]

On Sun, Jul 29, 2012 at 8:33 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> scsi/ibmvscsi: Fix host config length field overflow
>
> The length field in the host config packet is only 16-bit long, so
> passing it 0x10000 (64K which is our standard PAGE_SIZE) doesn't
> work and result in an empty config from the server.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> CC: <stable@vger.kernel.org>

James, can this be added to your for-next branch so that we can
also get this to the stable trees?  Thanks.

Acked-by: Robert Jennings <rcj@linux.vnet.ibm.com>

> ---
>
> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
> index 3a6c474..337e8b3 100644
> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
> @@ -1541,6 +1541,9 @@ static int ibmvscsi_do_host_config(struct ibmvscsi_host_data *hostdata,
>
>         host_config = &evt_struct->iu.mad.host_config;
>
> +       /* The transport length field is only 16-bit */
> +       length = min(0xffff, length);
> +
>         /* Set up a lun reset SRP command */
>         memset(host_config, 0x00, sizeof(*host_config));
>         host_config->common.type = VIOSRP_HOST_CONFIG_TYPE;
>
>