* [PATCH RFC] use struct scsi_lun in generic code
@ 2005-10-23 4:33 Jeff Garzik
2005-10-23 4:49 ` [PATCH RFC] more struct scsi_lun Jeff Garzik
` (5 more replies)
0 siblings, 6 replies; 32+ messages in thread
From: Jeff Garzik @ 2005-10-23 4:33 UTC (permalink / raw)
To: linux-scsi
A further experiment: once dev_printk() has been used to eliminate
direct references to HCIL address (see previous patch), we can see what
happens when we update the core to use struct scsi_lun.
DO NOT APPLY.
Depends on previous "kill scsi_device::{channel,id} in generic code"
patch.
Changes:
* replace 'unsigned int lun' with 'struct scsi_lun lun' in struct scsi_device
* change various function args to receive 'const struct scsi_lun *'
rather than unsigned int.
* export scsilun_to_int()
* create scsilun_to_str() helper
* create scsilun_eq() helper
* update all references to scsi_device::lun, as caught by the compiler.
Again, generic code was 100% converted, driver code 0% converted.
* int_to_scsilun() is used to convert SCSI-2 luns, and luns passed
from userspace as integers, to struct scsi_lun.
* shost->max_lun check moved into scsi_scan_host_selected() callers
drivers/scsi/scsi.c | 16 ++++++-----
drivers/scsi/scsi_error.c | 6 ++--
drivers/scsi/scsi_ioctl.c | 4 ++
drivers/scsi/scsi_priv.h | 5 ++-
drivers/scsi/scsi_proc.c | 23 +++++++++++-----
drivers/scsi/scsi_scan.c | 54 ++++++++++++++++++++++----------------
drivers/scsi/scsi_sysfs.c | 18 +++++++++---
drivers/scsi/scsi_transport_fc.c | 2 -
drivers/scsi/scsi_transport_sas.c | 3 +-
drivers/scsi/sg.c | 19 +++++++++----
include/scsi/scsi_device.h | 29 ++++++++++++++------
11 files changed, 118 insertions(+), 61 deletions(-)
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 0cb69a5..e6269f0 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -570,8 +570,10 @@ int scsi_dispatch_cmd(struct scsi_cmnd *
* If SCSI-2 or lower, store the LUN value in cmnd.
*/
if (cmd->device->scsi_level <= SCSI_2) {
+ unsigned int tmp_lun =
+ (unsigned int) scsilun_to_int(&cmd->device->lun);
cmd->cmnd[1] = (cmd->cmnd[1] & 0x1f) |
- (cmd->device->lun << 5 & 0xe0);
+ (tmp_lun << 5 & 0xe0);
}
/*
@@ -1132,12 +1134,12 @@ EXPORT_SYMBOL(starget_for_each_device);
* really want to use scsi_device_lookup_by_target instead.
**/
struct scsi_device *__scsi_device_lookup_by_target(struct scsi_target *starget,
- uint lun)
+ const struct scsi_lun *lun)
{
struct scsi_device *sdev;
list_for_each_entry(sdev, &starget->devices, same_target_siblings) {
- if (sdev->lun ==lun)
+ if (scsilun_eq(&sdev->lun, lun))
return sdev;
}
@@ -1155,7 +1157,7 @@ EXPORT_SYMBOL(__scsi_device_lookup_by_ta
* needs to be release with scsi_host_put once you're done with it.
**/
struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *starget,
- uint lun)
+ const struct scsi_lun *lun)
{
struct scsi_device *sdev;
struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
@@ -1188,14 +1190,14 @@ EXPORT_SYMBOL(scsi_device_lookup_by_targ
* really want to use scsi_device_lookup instead.
**/
struct scsi_device *__scsi_device_lookup(struct Scsi_Host *shost,
- uint channel, uint id, uint lun)
+ uint channel, uint id, const struct scsi_lun *lun)
{
struct scsi_device *sdev;
list_for_each_entry(sdev, &shost->__devices, siblings) {
if (sdev_channel(sdev) == channel &&
sdev_id(sdev) == id &&
- sdev->lun ==lun)
+ scsilun_eq(&sdev->lun, lun))
return sdev;
}
@@ -1215,7 +1217,7 @@ EXPORT_SYMBOL(__scsi_device_lookup);
* needs to be release with scsi_host_put once you're done with it.
**/
struct scsi_device *scsi_device_lookup(struct Scsi_Host *shost,
- uint channel, uint id, uint lun)
+ uint channel, uint id, const struct scsi_lun *lun)
{
struct scsi_device *sdev;
unsigned long flags;
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index fe5b9b9..29b0e2b 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -480,9 +480,11 @@ static int scsi_send_eh_cmnd(struct scsi
* we will use a queued command if possible, otherwise we will
* emulate the queuing and calling of completion function ourselves.
*/
- if (sdev->scsi_level <= SCSI_2)
+ if (sdev->scsi_level <= SCSI_2) {
+ unsigned int tmp_lun = scsilun_to_int(&sdev->lun);
scmd->cmnd[1] = (scmd->cmnd[1] & 0x1f) |
- (sdev->lun << 5 & 0xe0);
+ (tmp_lun << 5 & 0xe0);
+ }
scsi_add_timer(scmd, timeout, scsi_eh_times_out);
diff --git a/drivers/scsi/scsi_ioctl.c b/drivers/scsi/scsi_ioctl.c
index 43cb1f1..884cb06 100644
--- a/drivers/scsi/scsi_ioctl.c
+++ b/drivers/scsi/scsi_ioctl.c
@@ -359,6 +359,7 @@ static int scsi_ioctl_get_pci(struct scs
int scsi_ioctl(struct scsi_device *sdev, int cmd, void __user *arg)
{
char scsi_cmd[MAX_COMMAND_SIZE];
+ int tmp_lun;
/* No idea how this happens.... */
if (!sdev)
@@ -394,8 +395,9 @@ int scsi_ioctl(struct scsi_device *sdev,
if (!access_ok(VERIFY_WRITE, arg, sizeof(struct scsi_idlun)))
return -EFAULT;
+ tmp_lun = scsilun_to_int(&sdev->lun);
__put_user((sdev_id(sdev) & 0xff)
- + ((sdev->lun & 0xff) << 8)
+ + ((tmp_lun & 0xff) << 8)
+ ((sdev_channel(sdev) & 0xff) << 16)
+ ((sdev->host->host_no & 0xff) << 24),
&((struct scsi_idlun __user *)arg)->dev_id);
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index d05f778..afb0f86 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -31,7 +31,8 @@ struct Scsi_Host;
* Special value for scanning to specify scanning or rescanning of all
* possible channels, (target) ids, or luns on a given shost.
*/
-#define SCAN_WILD_CARD ~0
+#define SCAN_WILD_CARD ~0
+#define SCAN_LUN_WILD_CARD NULL
/* hosts.c */
extern int scsi_init_hosts(void);
@@ -103,7 +104,7 @@ extern void scsi_exit_procfs(void);
/* scsi_scan.c */
extern int scsi_scan_host_selected(struct Scsi_Host *, unsigned int,
- unsigned int, unsigned int, int);
+ unsigned int, const struct scsi_lun *, int);
extern void scsi_forget_host(struct Scsi_Host *);
extern void scsi_rescan_device(struct device *);
diff --git a/drivers/scsi/scsi_proc.c b/drivers/scsi/scsi_proc.c
index 43a9ec2..d0c2160 100644
--- a/drivers/scsi/scsi_proc.c
+++ b/drivers/scsi/scsi_proc.c
@@ -147,11 +147,12 @@ static int proc_print_scsidevice(struct
struct scsi_device *sdev = to_scsi_device(dev);
struct seq_file *s = data;
int i;
+ char lunstr[33];
seq_printf(s,
- "Host: scsi%d Channel: %02d Id: %02d Lun: %02d\n Vendor: ",
+ "Host: scsi%d Channel: %02d Id: %02d Lun: %s\n Vendor: ",
sdev->host->host_no, sdev_channel(sdev), sdev_id(sdev),
- sdev->lun);
+ scsilun_to_str(sdev, lunstr));
for (i = 0; i < 8; i++) {
if (sdev->vendor[i] >= 0x20)
@@ -192,21 +193,28 @@ static int proc_print_scsidevice(struct
return 0;
}
-static int scsi_add_single_device(uint host, uint channel, uint id, uint lun)
+static int scsi_add_single_device(uint host, uint channel, uint id,
+ uint lun)
{
struct Scsi_Host *shost;
+ struct scsi_lun __lun;
int error = -ENXIO;
shost = scsi_host_lookup(host);
if (IS_ERR(shost))
return PTR_ERR(shost);
- error = scsi_scan_host_selected(shost, channel, id, lun, 1);
+ if (lun > shost->max_lun)
+ return -EINVAL;
+
+ int_to_scsilun(lun, &__lun);
+ error = scsi_scan_host_selected(shost, channel, id, &__lun, 1);
scsi_host_put(shost);
return error;
}
-static int scsi_remove_single_device(uint host, uint channel, uint id, uint lun)
+static int scsi_remove_single_device(uint host, uint channel, uint id,
+ const struct scsi_lun *lun)
{
struct scsi_device *sdev;
struct Scsi_Host *shost;
@@ -271,14 +279,17 @@ static ssize_t proc_scsi_write(struct fi
* with "0 1 2 3" replaced by your "Host Channel Id Lun".
*/
} else if (!strncmp("scsi remove-single-device", buffer, 25)) {
+ struct scsi_lun __lun;
+
p = buffer + 26;
host = simple_strtoul(p, &p, 0);
channel = simple_strtoul(p + 1, &p, 0);
id = simple_strtoul(p + 1, &p, 0);
lun = simple_strtoul(p + 1, &p, 0);
+ int_to_scsilun(lun, &__lun);
- err = scsi_remove_single_device(host, channel, id, lun);
+ err = scsi_remove_single_device(host, channel, id, &__lun);
}
out:
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index cf6f8bc..5315beb 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -199,7 +199,7 @@ static void print_inquiry(unsigned char
* scsi_Device pointer, or NULL on failure.
**/
static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
- unsigned int lun, void *hostdata)
+ const struct scsi_lun *lun, void *hostdata)
{
struct scsi_device *sdev;
int display_failure_msg = 1, ret;
@@ -215,7 +215,7 @@ static struct scsi_device *scsi_alloc_sd
sdev->model = scsi_null_device_strs;
sdev->rev = scsi_null_device_strs;
sdev->host = shost;
- sdev->lun = lun;
+ memcpy(&sdev->lun, lun, sizeof(*lun));
sdev->sdev_state = SDEV_CREATED;
INIT_LIST_HEAD(&sdev->siblings);
INIT_LIST_HEAD(&sdev->same_target_siblings);
@@ -608,6 +608,8 @@ static int scsi_probe_lun(struct scsi_de
**/
static int scsi_add_lun(struct scsi_device *sdev, char *inq_result, int *bflags)
{
+ char lunstr[33];
+
/*
* XXX do not save the inquiry, since it can change underneath us,
* save just vendor/model/rev.
@@ -692,9 +694,10 @@ static int scsi_add_lun(struct scsi_devi
if (inq_result[7] & 0x10)
sdev->sdtr = 1;
- sprintf(sdev->devfs_name, "scsi/host%d/bus%d/target%d/lun%d",
+ sprintf(sdev->devfs_name, "scsi/host%d/bus%d/target%d/lun%s",
sdev->host->host_no, sdev_channel(sdev),
- sdev_id(sdev), sdev->lun);
+ sdev_id(sdev),
+ scsilun_to_str(sdev, lunstr));
/*
* End driverfs/devfs code.
@@ -797,7 +800,7 @@ static inline void scsi_destroy_sdev(str
* SCSI_SCAN_LUN_PRESENT: a new Scsi_Device was allocated and initialized
**/
static int scsi_probe_and_add_lun(struct scsi_target *starget,
- uint lun, int *bflagsp,
+ const struct scsi_lun *lun, int *bflagsp,
struct scsi_device **sdevp, int rescan,
void *hostdata)
{
@@ -972,11 +975,14 @@ static void scsi_sequential_lun_scan(str
* until we reach the max, or no LUN is found and we are not
* sparse_lun.
*/
- for (lun = 1; lun < max_dev_lun; ++lun)
- if ((scsi_probe_and_add_lun(starget, lun, NULL, NULL, rescan,
+ for (lun = 1; lun < max_dev_lun; ++lun) {
+ struct scsi_lun __lun;
+ int_to_scsilun(lun, &__lun);
+ if ((scsi_probe_and_add_lun(starget, &__lun, NULL, NULL, rescan,
NULL) != SCSI_SCAN_LUN_PRESENT) &&
!sparse_lun)
return;
+ }
}
/**
@@ -998,7 +1004,7 @@ static void scsi_sequential_lun_scan(str
* Given a struct scsi_lun of: 0a 04 0b 03 00 00 00 00, this function returns
* the integer: 0x0b030a04
**/
-static int scsilun_to_int(struct scsi_lun *scsilun)
+int scsilun_to_int(const struct scsi_lun *scsilun)
{
int i;
unsigned int lun;
@@ -1009,6 +1015,7 @@ static int scsilun_to_int(struct scsi_lu
scsilun->scsi_lun[i + 1]) << (i * 8));
return lun;
}
+EXPORT_SYMBOL(scsilun_to_int);
/**
* int_to_scsilun: reverts an int into a scsi_lun
@@ -1222,7 +1229,7 @@ static int scsi_report_lun_scan(struct s
int res;
res = scsi_probe_and_add_lun(starget,
- lun, NULL, NULL, rescan, NULL);
+ lunp, NULL, NULL, rescan, NULL);
if (res == SCSI_SCAN_NO_RESPONSE) {
/*
* Got some results, but now none, abort.
@@ -1247,7 +1254,7 @@ static int scsi_report_lun_scan(struct s
}
struct scsi_device *__scsi_add_device(struct Scsi_Host *shost, uint channel,
- uint id, uint lun, void *hostdata)
+ uint id, const struct scsi_lun *lun, void *hostdata)
{
struct scsi_device *sdev;
struct device *parent = &shost->shost_gendev;
@@ -1274,7 +1281,7 @@ struct scsi_device *__scsi_add_device(st
EXPORT_SYMBOL(__scsi_add_device);
int scsi_add_device(struct Scsi_Host *host, uint channel,
- uint target, uint lun)
+ uint target, const struct scsi_lun *lun)
{
struct scsi_device *sdev =
__scsi_add_device(host, channel, target, lun, NULL);
@@ -1303,7 +1310,7 @@ void scsi_rescan_device(struct device *d
EXPORT_SYMBOL(scsi_rescan_device);
static void __scsi_scan_target(struct device *parent, unsigned int channel,
- unsigned int id, unsigned int lun, int rescan)
+ unsigned int id, const struct scsi_lun *lun, int rescan)
{
struct Scsi_Host *shost = dev_to_shost(parent);
int bflags = 0;
@@ -1321,7 +1328,7 @@ static void __scsi_scan_target(struct de
return;
get_device(&starget->dev);
- if (lun != SCAN_WILD_CARD) {
+ if (lun != SCAN_LUN_WILD_CARD) {
/*
* Scan for a specific host/chan/id/lun.
*/
@@ -1369,7 +1376,7 @@ static void __scsi_scan_target(struct de
* sequential scan of LUNs on the target id.
**/
void scsi_scan_target(struct device *parent, unsigned int channel,
- unsigned int id, unsigned int lun, int rescan)
+ unsigned int id, const struct scsi_lun *lun, int rescan)
{
struct Scsi_Host *shost = dev_to_shost(parent);
@@ -1381,7 +1388,7 @@ void scsi_scan_target(struct device *par
EXPORT_SYMBOL(scsi_scan_target);
static void scsi_scan_channel(struct Scsi_Host *shost, unsigned int channel,
- unsigned int id, unsigned int lun, int rescan)
+ unsigned int id, const struct scsi_lun *lun, int rescan)
{
uint order_id;
@@ -1412,14 +1419,17 @@ static void scsi_scan_channel(struct Scs
}
int scsi_scan_host_selected(struct Scsi_Host *shost, unsigned int channel,
- unsigned int id, unsigned int lun, int rescan)
+ unsigned int id, const struct scsi_lun *lun, int rescan)
{
- SCSI_LOG_SCAN_BUS(3, printk (KERN_INFO "%s: <%u:%u:%u:%u>\n",
- __FUNCTION__, shost->host_no, channel, id, lun));
+ char lunstr[33];
+
+ sprintf(lunstr, "%d", scsilun_to_int(lun));
+
+ SCSI_LOG_SCAN_BUS(3, printk (KERN_INFO "%s: <%u:%u:%u:%s>\n",
+ __FUNCTION__, shost->host_no, channel, id, lunstr));
if (((channel != SCAN_WILD_CARD) && (channel > shost->max_channel)) ||
- ((id != SCAN_WILD_CARD) && (id > shost->max_id)) ||
- ((lun != SCAN_WILD_CARD) && (lun > shost->max_lun)))
+ ((id != SCAN_WILD_CARD) && (id > shost->max_id)))
return -EINVAL;
down(&shost->scan_mutex);
@@ -1444,7 +1454,7 @@ int scsi_scan_host_selected(struct Scsi_
void scsi_scan_host(struct Scsi_Host *shost)
{
scsi_scan_host_selected(shost, SCAN_WILD_CARD, SCAN_WILD_CARD,
- SCAN_WILD_CARD, 0);
+ SCAN_LUN_WILD_CARD, 0);
}
EXPORT_SYMBOL(scsi_scan_host);
@@ -1457,7 +1467,7 @@ EXPORT_SYMBOL(scsi_scan_host);
void scsi_scan_single_target(struct Scsi_Host *shost,
unsigned int chan, unsigned int id)
{
- scsi_scan_host_selected(shost, chan, id, SCAN_WILD_CARD, 1);
+ scsi_scan_host_selected(shost, chan, id, SCAN_LUN_WILD_CARD, 1);
}
EXPORT_SYMBOL(scsi_scan_single_target);
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 3a914cf..a52e963 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -96,6 +96,7 @@ static int scsi_scan(struct Scsi_Host *s
char s1[15], s2[15], s3[15], junk;
unsigned int channel, id, lun;
int res;
+ struct scsi_lun __lun;
res = sscanf(str, "%10s %10s %10s %c", s1, s2, s3, &junk);
if (res != 3)
@@ -106,7 +107,12 @@ static int scsi_scan(struct Scsi_Host *s
return -EINVAL;
if (check_set(&lun, s3))
return -EINVAL;
- res = scsi_scan_host_selected(shost, channel, id, lun, 1);
+
+ if (lun > shost->max_lun)
+ return -EINVAL;
+ int_to_scsilun(lun, &__lun);
+
+ res = scsi_scan_host_selected(shost, channel, id, &__lun, 1);
return res;
}
@@ -859,20 +865,22 @@ void scsi_sysfs_device_initialize(struct
unsigned long flags;
struct Scsi_Host *shost = sdev->host;
struct scsi_target *starget = sdev->sdev_target;
+ char lunstr[33];
device_initialize(&sdev->sdev_gendev);
sdev->sdev_gendev.bus = &scsi_bus_type;
sdev->sdev_gendev.release = scsi_device_dev_release;
- sprintf(sdev->sdev_gendev.bus_id,"%d:%d:%d:%d",
+ sprintf(sdev->sdev_gendev.bus_id,"%d:%d:%d:%s",
sdev->host->host_no, sdev_channel(sdev), sdev_id(sdev),
- sdev->lun);
+ scsilun_to_str(sdev, lunstr));
class_device_initialize(&sdev->sdev_classdev);
sdev->sdev_classdev.dev = &sdev->sdev_gendev;
sdev->sdev_classdev.class = &sdev_class;
snprintf(sdev->sdev_classdev.class_id, BUS_ID_SIZE,
- "%d:%d:%d:%d", sdev->host->host_no,
- sdev_channel(sdev), sdev_id(sdev), sdev->lun);
+ "%d:%d:%d:%s", sdev->host->host_no,
+ sdev_channel(sdev), sdev_id(sdev),
+ scsilun_to_str(sdev, lunstr));
sdev->scsi_level = SCSI_2;
transport_setup_device(&sdev->sdev_gendev);
spin_lock_irqsave(shost->host_lock, flags);
diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index 2cab556..9f1d15e 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -1674,7 +1674,7 @@ fc_scsi_scan_rport(void *data)
struct fc_rport *rport = (struct fc_rport *)data;
scsi_scan_target(&rport->dev, rport->channel, rport->scsi_target_id,
- SCAN_WILD_CARD, 1);
+ SCAN_LUN_WILD_CARD, 1);
}
diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
index 1d145d2..50f50d7 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -31,6 +31,7 @@
#include <scsi/scsi_host.h>
#include <scsi/scsi_transport.h>
#include <scsi/scsi_transport_sas.h>
+#include "scsi_priv.h" /* for SCAN_LUN_WILD_CARD */
#define SAS_HOST_ATTRS 0
@@ -579,7 +580,7 @@ int sas_rphy_add(struct sas_rphy *rphy)
if (rphy->scsi_target_id != -1) {
scsi_scan_target(&rphy->dev, parent->number,
- rphy->scsi_target_id, ~0, 0);
+ rphy->scsi_target_id, SCAN_LUN_WILD_CARD, 0);
}
return 0;
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index b03e2fd..3ab60e5 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -879,6 +879,8 @@ sg_ioctl(struct inode *inode, struct fil
if (!access_ok(VERIFY_WRITE, p, sizeof (sg_scsi_id_t)))
return -EFAULT;
else {
+ unsigned int tmp_lun =
+ scsilun_to_int(&sdp->device->lun);
sg_scsi_id_t __user *sg_idp = p;
if (sdp->detached)
@@ -888,7 +890,7 @@ sg_ioctl(struct inode *inode, struct fil
__put_user((int) sdev_channel(sdp->device),
&sg_idp->channel);
__put_user((int) sdev_id(sdp->device), &sg_idp->scsi_id);
- __put_user((int) sdp->device->lun, &sg_idp->lun);
+ __put_user((int) tmp_lun, &sg_idp->lun);
__put_user((int) sdp->device->type, &sg_idp->scsi_type);
__put_user((short) sdp->device->host->cmd_per_lun,
&sg_idp->h_cmd_per_lun);
@@ -3001,12 +3003,15 @@ static int sg_proc_seq_show_dev(struct s
struct sg_proc_deviter * it = (struct sg_proc_deviter *) v;
Sg_device *sdp;
struct scsi_device *scsidp;
+ char lunstr[33];
sdp = it ? sg_get_dev(it->index) : NULL;
if (sdp && (scsidp = sdp->device) && (!sdp->detached))
- seq_printf(s, "%d\t%d\t%d\t%d\t%d\t%d\t%d\t%d\t%d\n",
+ seq_printf(s, "%d\t%d\t%d\t%s\t%d\t%d\t%d\t%d\t%d\n",
scsidp->host->host_no, sdev_channel(scsidp),
- sdev_id(scsidp), scsidp->lun, (int) scsidp->type,
+ sdev_id(scsidp),
+ scsilun_to_str(scsidp, lunstr),
+ (int) scsidp->type,
1,
(int) scsidp->queue_depth,
(int) scsidp->device_busy,
@@ -3127,13 +3132,15 @@ static int sg_proc_seq_show_debug(struct
sdp->disk->disk_name);
if (sdp->detached)
seq_printf(s, "detached pending close ");
- else
+ else {
+ char lunstr[33];
seq_printf
- (s, "scsi%d chan=%d id=%d lun=%d em=%d",
+ (s, "scsi%d chan=%d id=%d lun=%s em=%d",
scsidp->host->host_no,
sdev_channel(scsidp), sdev_id(scsidp),
- scsidp->lun,
+ scsilun_to_str(scsidp, lunstr),
scsidp->host->hostt->emulated);
+ }
seq_printf(s, " sg_tablesize=%d excl=%d\n",
sdp->sg_tablesize, sdp->exclude);
}
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 8fdd70b..03b381e 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -4,6 +4,7 @@
#include <linux/device.h>
#include <linux/list.h>
#include <linux/spinlock.h>
+#include <scsi/scsi.h> /* for struct scsi_lun */
#include <asm/atomic.h>
struct request_queue;
@@ -66,7 +67,7 @@ struct scsi_device {
jiffie count on our counter, they
could all be from the same event. */
- unsigned int lun;
+ struct scsi_lun lun;
unsigned int manufacturer; /* Manufacturer of device, for using
* vendor-specific cmd's */
@@ -178,22 +179,22 @@ static inline struct scsi_target *scsi_t
to_scsi_target(class_dev->dev)
extern struct scsi_device *__scsi_add_device(struct Scsi_Host *,
- uint, uint, uint, void *hostdata);
+ uint, uint, const struct scsi_lun *, void *hostdata);
extern int scsi_add_device(struct Scsi_Host *host, uint channel,
- uint target, uint lun);
+ uint target, const struct scsi_lun *lun);
extern void scsi_remove_device(struct scsi_device *);
extern int scsi_device_cancel(struct scsi_device *, int);
extern int scsi_device_get(struct scsi_device *);
extern void scsi_device_put(struct scsi_device *);
extern struct scsi_device *scsi_device_lookup(struct Scsi_Host *,
- uint, uint, uint);
+ uint, uint, const struct scsi_lun *);
extern struct scsi_device *__scsi_device_lookup(struct Scsi_Host *,
- uint, uint, uint);
+ uint, uint, const struct scsi_lun *);
extern struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *,
- uint);
+ const struct scsi_lun *);
extern struct scsi_device *__scsi_device_lookup_by_target(struct scsi_target *,
- uint);
+ const struct scsi_lun *);
extern void starget_for_each_device(struct scsi_target *, void *,
void (*fn)(struct scsi_device *, void *));
@@ -249,12 +250,13 @@ extern void scsi_device_resume(struct sc
extern void scsi_target_quiesce(struct scsi_target *);
extern void scsi_target_resume(struct scsi_target *);
extern void scsi_scan_target(struct device *parent, unsigned int channel,
- unsigned int id, unsigned int lun, int rescan);
+ unsigned int id, const struct scsi_lun *lun, int rescan);
extern void scsi_target_reap(struct scsi_target *);
extern void scsi_target_block(struct device *);
extern void scsi_target_unblock(struct device *);
extern void scsi_remove_target(struct device *);
extern void int_to_scsilun(unsigned int, struct scsi_lun *);
+extern int scsilun_to_int(const struct scsi_lun *scsilun);
extern const char *scsi_device_state_name(enum scsi_device_state);
extern int scsi_is_sdev_device(const struct device *);
extern int scsi_is_target_device(const struct device *);
@@ -276,6 +278,17 @@ static inline unsigned int sdev_id(struc
return sdev->sdev_target->id;
}
+static inline char *scsilun_to_str(struct scsi_device *sdev, char *s)
+{
+ sprintf(s, "%d", scsilun_to_int(&sdev->lun));
+ return s;
+}
+
+static inline int scsilun_eq(const struct scsi_lun *a, const struct scsi_lun *b)
+{
+ return (memcmp(a, b, sizeof(struct scsi_lun)) == 0);
+}
+
static inline int scsi_device_online(struct scsi_device *sdev)
{
return sdev->sdev_state != SDEV_OFFLINE;
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH RFC] more struct scsi_lun
2005-10-23 4:33 [PATCH RFC] use struct scsi_lun in generic code Jeff Garzik
@ 2005-10-23 4:49 ` Jeff Garzik
2005-10-23 9:47 ` Stefan Richter
2005-10-23 5:20 ` [PATCH RFC] use struct scsi_lun in generic code Jeff Garzik
` (4 subsequent siblings)
5 siblings, 1 reply; 32+ messages in thread
From: Jeff Garzik @ 2005-10-23 4:49 UTC (permalink / raw)
To: linux-scsi
Minor cleanups following primary scsi_lun patch (just sent):
- create and use SCSILUN_STR_LEN
- pass struct scsi_lun to scsilun_to_str(), not struct scsi_device
drivers/scsi/scsi_proc.c | 4 ++--
drivers/scsi/scsi_scan.c | 11 +++++------
drivers/scsi/scsi_sysfs.c | 6 +++---
drivers/scsi/sg.c | 8 ++++----
include/scsi/scsi.h | 1 +
include/scsi/scsi_device.h | 4 ++--
6 files changed, 17 insertions(+), 17 deletions(-)
7c838ac815a38b5f99bf717b1a7aa2f6b57b51bc
diff --git a/drivers/scsi/scsi_proc.c b/drivers/scsi/scsi_proc.c
index d0c2160..6eadb5b 100644
--- a/drivers/scsi/scsi_proc.c
+++ b/drivers/scsi/scsi_proc.c
@@ -147,12 +147,12 @@ static int proc_print_scsidevice(struct
struct scsi_device *sdev = to_scsi_device(dev);
struct seq_file *s = data;
int i;
- char lunstr[33];
+ char lunstr[SCSILUN_STR_LEN];
seq_printf(s,
"Host: scsi%d Channel: %02d Id: %02d Lun: %s\n Vendor: ",
sdev->host->host_no, sdev_channel(sdev), sdev_id(sdev),
- scsilun_to_str(sdev, lunstr));
+ scsilun_to_str(&sdev->lun, lunstr));
for (i = 0; i < 8; i++) {
if (sdev->vendor[i] >= 0x20)
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 5315beb..fb1d9c7 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -608,7 +608,7 @@ static int scsi_probe_lun(struct scsi_de
**/
static int scsi_add_lun(struct scsi_device *sdev, char *inq_result, int *bflags)
{
- char lunstr[33];
+ char lunstr[SCSILUN_STR_LEN];
/*
* XXX do not save the inquiry, since it can change underneath us,
@@ -697,7 +697,7 @@ static int scsi_add_lun(struct scsi_devi
sprintf(sdev->devfs_name, "scsi/host%d/bus%d/target%d/lun%s",
sdev->host->host_no, sdev_channel(sdev),
sdev_id(sdev),
- scsilun_to_str(sdev, lunstr));
+ scsilun_to_str(&sdev->lun, lunstr));
/*
* End driverfs/devfs code.
@@ -1421,12 +1421,11 @@ static void scsi_scan_channel(struct Scs
int scsi_scan_host_selected(struct Scsi_Host *shost, unsigned int channel,
unsigned int id, const struct scsi_lun *lun, int rescan)
{
- char lunstr[33];
-
- sprintf(lunstr, "%d", scsilun_to_int(lun));
+ char lunstr[SCSILUN_STR_LEN];
SCSI_LOG_SCAN_BUS(3, printk (KERN_INFO "%s: <%u:%u:%u:%s>\n",
- __FUNCTION__, shost->host_no, channel, id, lunstr));
+ __FUNCTION__, shost->host_no, channel, id,
+ scsilun_to_str(lun, lunstr)));
if (((channel != SCAN_WILD_CARD) && (channel > shost->max_channel)) ||
((id != SCAN_WILD_CARD) && (id > shost->max_id)))
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index a52e963..63352fc 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -865,14 +865,14 @@ void scsi_sysfs_device_initialize(struct
unsigned long flags;
struct Scsi_Host *shost = sdev->host;
struct scsi_target *starget = sdev->sdev_target;
- char lunstr[33];
+ char lunstr[SCSILUN_STR_LEN];
device_initialize(&sdev->sdev_gendev);
sdev->sdev_gendev.bus = &scsi_bus_type;
sdev->sdev_gendev.release = scsi_device_dev_release;
sprintf(sdev->sdev_gendev.bus_id,"%d:%d:%d:%s",
sdev->host->host_no, sdev_channel(sdev), sdev_id(sdev),
- scsilun_to_str(sdev, lunstr));
+ scsilun_to_str(&sdev->lun, lunstr));
class_device_initialize(&sdev->sdev_classdev);
sdev->sdev_classdev.dev = &sdev->sdev_gendev;
@@ -880,7 +880,7 @@ void scsi_sysfs_device_initialize(struct
snprintf(sdev->sdev_classdev.class_id, BUS_ID_SIZE,
"%d:%d:%d:%s", sdev->host->host_no,
sdev_channel(sdev), sdev_id(sdev),
- scsilun_to_str(sdev, lunstr));
+ scsilun_to_str(&sdev->lun, lunstr));
sdev->scsi_level = SCSI_2;
transport_setup_device(&sdev->sdev_gendev);
spin_lock_irqsave(shost->host_lock, flags);
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 3ab60e5..1c2ca07 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -3003,14 +3003,14 @@ static int sg_proc_seq_show_dev(struct s
struct sg_proc_deviter * it = (struct sg_proc_deviter *) v;
Sg_device *sdp;
struct scsi_device *scsidp;
- char lunstr[33];
+ char lunstr[SCSILUN_STR_LEN];
sdp = it ? sg_get_dev(it->index) : NULL;
if (sdp && (scsidp = sdp->device) && (!sdp->detached))
seq_printf(s, "%d\t%d\t%d\t%s\t%d\t%d\t%d\t%d\t%d\n",
scsidp->host->host_no, sdev_channel(scsidp),
sdev_id(scsidp),
- scsilun_to_str(scsidp, lunstr),
+ scsilun_to_str(&scsidp->lun, lunstr),
(int) scsidp->type,
1,
(int) scsidp->queue_depth,
@@ -3133,12 +3133,12 @@ static int sg_proc_seq_show_debug(struct
if (sdp->detached)
seq_printf(s, "detached pending close ");
else {
- char lunstr[33];
+ char lunstr[SCSILUN_STR_LEN];
seq_printf
(s, "scsi%d chan=%d id=%d lun=%s em=%d",
scsidp->host->host_no,
sdev_channel(scsidp), sdev_id(scsidp),
- scsilun_to_str(scsidp, lunstr),
+ scsilun_to_str(&scsidp->lun, lunstr),
scsidp->host->hostt->emulated);
}
seq_printf(s, " sg_tablesize=%d excl=%d\n",
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index b361172..deb1a27 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -241,6 +241,7 @@ struct ccs_modesel_head {
struct scsi_lun {
__u8 scsi_lun[8];
};
+#define SCSILUN_STR_LEN 33
/*
* MESSAGE CODES
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 03b381e..769469c 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -278,9 +278,9 @@ static inline unsigned int sdev_id(struc
return sdev->sdev_target->id;
}
-static inline char *scsilun_to_str(struct scsi_device *sdev, char *s)
+static inline char *scsilun_to_str(const struct scsi_lun *lun, char *s)
{
- sprintf(s, "%d", scsilun_to_int(&sdev->lun));
+ sprintf(s, "%d", scsilun_to_int(lun));
return s;
}
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH RFC] more struct scsi_lun
2005-10-23 4:49 ` [PATCH RFC] more struct scsi_lun Jeff Garzik
@ 2005-10-23 9:47 ` Stefan Richter
2005-10-23 13:14 ` Stefan Richter
0 siblings, 1 reply; 32+ messages in thread
From: Stefan Richter @ 2005-10-23 9:47 UTC (permalink / raw)
To: Jeff Garzik; +Cc: linux-scsi
Jeff Garzik wrote:
> +static inline char *scsilun_to_str(const struct scsi_lun *lun, char *s)
> {
> - sprintf(s, "%d", scsilun_to_int(&sdev->lun));
> + sprintf(s, "%d", scsilun_to_int(lun));
> return s;
> }
What about an 8-byte representation?
#define SCSILUN_STR_LEN (2*8+1)
static inline char *scsilun_to_str(const struct scsi_lun *lun, char *s)
{
int i;
for (i = 0; i < 8; ++i)
sprintf(s+2*i, %02x, lun[i]);
return s;
}
--
Stefan Richter
-=====-=-=-= =-=- =-===
http://arcgraph.de/sr/
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH RFC] more struct scsi_lun
2005-10-23 9:47 ` Stefan Richter
@ 2005-10-23 13:14 ` Stefan Richter
2005-10-23 16:49 ` Jeff Garzik
0 siblings, 1 reply; 32+ messages in thread
From: Stefan Richter @ 2005-10-23 13:14 UTC (permalink / raw)
To: Jeff Garzik; +Cc: linux-scsi
I wrote:
> for (i = 0; i < 8; ++i)
> sprintf(s+2*i, %02x, lun[i]);
sprintf(s+2*i, "%02x", lun.scsi_lun[i]);
#-)
--
Stefan Richter
-=====-=-=-= =-=- =-===
http://arcgraph.de/sr/
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH RFC] more struct scsi_lun
2005-10-23 13:14 ` Stefan Richter
@ 2005-10-23 16:49 ` Jeff Garzik
2005-10-24 16:27 ` Luben Tuikov
0 siblings, 1 reply; 32+ messages in thread
From: Jeff Garzik @ 2005-10-23 16:49 UTC (permalink / raw)
To: Stefan Richter; +Cc: linux-scsi
Stefan Richter wrote:
> I wrote:
>
>> for (i = 0; i < 8; ++i)
>> sprintf(s+2*i, %02x, lun[i]);
>
> sprintf(s+2*i, "%02x", lun.scsi_lun[i]);
> #-)
There is obviously room for improvement. Any naive representation is
sub-optimal, be it for small luns (my current code) or larger luns (your
example).
For situations with smaller luns, we should probably continue to use the
current scsilun_to_int() conversion, while using your example for larger
luns, i.e.
if (upper 4 bytes zero)
scsilun to int
printk %d
else
for each byte
printk %x
But Douglas's code suggested that if we are more motivated, we could
provide an even better representation.
Regards,
Jeff
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC] more struct scsi_lun
2005-10-23 16:49 ` Jeff Garzik
@ 2005-10-24 16:27 ` Luben Tuikov
2005-10-24 20:03 ` Stefan Richter
0 siblings, 1 reply; 32+ messages in thread
From: Luben Tuikov @ 2005-10-24 16:27 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Stefan Richter, linux-scsi
On 10/23/05 12:49, Jeff Garzik wrote:
> Stefan Richter wrote:
>
>>I wrote:
>>
>>
>>> for (i = 0; i < 8; ++i)
>>> sprintf(s+2*i, %02x, lun[i]);
>>
>> sprintf(s+2*i, "%02x", lun.scsi_lun[i]);
>>#-)
>
>
>
> There is obviously room for improvement. Any naive representation is
> sub-optimal, be it for small luns (my current code) or larger luns (your
> example).
>
> For situations with smaller luns, we should probably continue to use the
> current scsilun_to_int() conversion, while using your example for larger
> luns, i.e.
>
> if (upper 4 bytes zero)
> scsilun to int
> printk %d
> else
> for each byte
> printk %x
>
> But Douglas's code suggested that if we are more motivated, we could
> provide an even better representation.
If a LUN is u8[8], then,
#define SAS_ADDR(_sa) ((unsigned long long) be64_to_cpu(*(__be64 *)(_sa)))
"%016llx", SAS_ADDR(LUN) prints it like this (e.g.):
sas: 5000c50000513329 probing LUN:0000000000000000
sas: device 500000e000031c12 LUN: 0000000000000000 powering up or not ready yet, sleeping...
sas: 500000e000031c12 probing LUN:0000000000000000
sas: device 50001c171601060d LUN: 0000000000000000 powering up or not ready yet, sleeping...
This is from drivers/scsi/sas/sas_discover.c.
Luben
--
http://linux.adaptec.com/sas/
http://www.adaptec.com/sas/
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC] more struct scsi_lun
2005-10-24 16:27 ` Luben Tuikov
@ 2005-10-24 20:03 ` Stefan Richter
2005-10-24 20:10 ` Luben Tuikov
` (2 more replies)
0 siblings, 3 replies; 32+ messages in thread
From: Stefan Richter @ 2005-10-24 20:03 UTC (permalink / raw)
To: linux-scsi; +Cc: Luben Tuikov, Jeff Garzik
Luben Tuikov wrote:
> On 10/23/05 12:49, Jeff Garzik wrote:
>>Stefan Richter wrote:
>>> sprintf(s+2*i, "%02x", lun.scsi_lun[i]);
>>
>>There is obviously room for improvement. Any naive representation is
>>sub-optimal, be it for small luns (my current code) or larger luns (your
>>example).
>>
>>For situations with smaller luns, we should probably continue to use the
>>current scsilun_to_int() conversion, while using your example for larger
>>luns, i.e.
>>
>> if (upper 4 bytes zero)
>> scsilun to int
>> printk %d
>> else
>> for each byte
>> printk %x
>>
>>But Douglas's code suggested that if we are more motivated, we could
>>provide an even better representation.
A plain, non-interpreted representation should suffice. Compact but
easily parseable, i.e. uniform.
> If a LUN is u8[8], then,
>
> #define SAS_ADDR(_sa) ((unsigned long long) be64_to_cpu(*(__be64 *)(_sa)))
>
> "%016llx", SAS_ADDR(LUN) prints it like this (e.g.):
>
> sas: 5000c50000513329 probing LUN:0000000000000000
> sas: device 500000e000031c12 LUN: 0000000000000000 powering up or not ready yet, sleeping...
> sas: 500000e000031c12 probing LUN:0000000000000000
> sas: device 50001c171601060d LUN: 0000000000000000 powering up or not ready yet, sleeping...
>
> This is from drivers/scsi/sas/sas_discover.c.
>
> Luben
What about an 64bit integer as carrier of LUN in the first place? The
most frequent occurrence of LUN data is when it is passed through in
function calls but it seems rarely to be manipulated.
--- linux/include/scsi/scsi.h.orig 2005-10-20 08:23:05.000000000 +0200
+++ linux/include/scsi/scsi.h 2005-10-24 21:45:41.000000000 +0200
@@ -238,9 +238,7 @@
/*
* ScsiLun: 8 byte LUN.
*/
-struct scsi_lun {
- __u8 scsi_lun[8];
-};
+typedef __be64 scsi_lun;
/*
* MESSAGE CODES
--
Stefan Richter
-=====-=-=-= =-=- ==---
http://arcgraph.de/sr/
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH RFC] more struct scsi_lun
2005-10-24 20:03 ` Stefan Richter
@ 2005-10-24 20:10 ` Luben Tuikov
2005-10-24 20:28 ` Mark Rustad
2005-10-24 22:27 ` Douglas Gilbert
2 siblings, 0 replies; 32+ messages in thread
From: Luben Tuikov @ 2005-10-24 20:10 UTC (permalink / raw)
To: Stefan Richter; +Cc: linux-scsi, Jeff Garzik
On 10/24/05 16:03, Stefan Richter wrote:
> What about an 64bit integer as carrier of LUN in the first place? The
> most frequent occurrence of LUN data is when it is passed through in
> function calls but it seems rarely to be manipulated.
Nah, we don't want to do that. A LUN is like a CDB, there is no
MSB or LSB, just a sequence of bytes, 1st, 2nd, etc. Only when
being printed, is when you want to impose MSB, so that you can
print it humanly readable -- i.e. so that when the customer looks
at their manual/storage app, they can see the LUN and recognize it.
Luben
--
http://linux.adaptec.com/sas/
http://www.adaptec.com/sas/
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC] more struct scsi_lun
2005-10-24 20:03 ` Stefan Richter
2005-10-24 20:10 ` Luben Tuikov
@ 2005-10-24 20:28 ` Mark Rustad
2005-10-24 22:27 ` Douglas Gilbert
2 siblings, 0 replies; 32+ messages in thread
From: Mark Rustad @ 2005-10-24 20:28 UTC (permalink / raw)
To: Stefan Richter; +Cc: linux-scsi, Luben Tuikov, Jeff Garzik
On Oct 24, 2005, at 3:03 PM, Stefan Richter wrote:
> Luben Tuikov wrote:
>
>> On 10/23/05 12:49, Jeff Garzik wrote:
>>
>>> Stefan Richter wrote:
>>>
>>>> sprintf(s+2*i, "%02x", lun.scsi_lun[i]);
>>>
>>> There is obviously room for improvement. Any naive
>>> representation is sub-optimal, be it for small luns (my current
>>> code) or larger luns (your example).
>>>
>>> For situations with smaller luns, we should probably continue to
>>> use the current scsilun_to_int() conversion, while using your
>>> example for larger luns, i.e.
>>>
>>> if (upper 4 bytes zero)
>>> scsilun to int
>>> printk %d
>>> else
>>> for each byte
>>> printk %x
>>>
>>> But Douglas's code suggested that if we are more motivated, we
>>> could provide an even better representation.
>
> A plain, non-interpreted representation should suffice. Compact but
> easily parseable, i.e. uniform.
>
>> If a LUN is u8[8], then,
>> #define SAS_ADDR(_sa) ((unsigned long long) be64_to_cpu(*(__be64
>> *)(_sa)))
This could be a problem because if a LUN is u8[8], it may not be
aligned, so casting to __be64 and such could be a problem in some
architectures.
>> "%016llx", SAS_ADDR(LUN) prints it like this (e.g.):
>> sas: 5000c50000513329 probing LUN:0000000000000000
>> sas: device 500000e000031c12 LUN: 0000000000000000 powering up or
>> not ready yet, sleeping...
>> sas: 500000e000031c12 probing LUN:0000000000000000
>> sas: device 50001c171601060d LUN: 0000000000000000 powering up or
>> not ready yet, sleeping...
>> This is from drivers/scsi/sas/sas_discover.c.
>> Luben
>
> What about an 64bit integer as carrier of LUN in the first place? The
> most frequent occurrence of LUN data is when it is passed through in
> function calls but it seems rarely to be manipulated.
>
> --- linux/include/scsi/scsi.h.orig 2005-10-20
> 08:23:05.000000000 +0200
> +++ linux/include/scsi/scsi.h 2005-10-24 21:45:41.000000000 +0200
> @@ -238,9 +238,7 @@
> /*
> * ScsiLun: 8 byte LUN.
> */
> -struct scsi_lun {
> - __u8 scsi_lun[8];
> -};
> +typedef __be64 scsi_lun;
>
> /*
> * MESSAGE CODES
Typedef-ing to __be64 would seem to avoid the likely alignment
problem that the casting solution above could run into. I have to say
that I don't know enough about SCSI to make a recommendation, but I
know enough about alignment problems in some architectures to worry
about that.
--
Mark Rustad, MRustad@mac.com
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH RFC] more struct scsi_lun
2005-10-24 20:03 ` Stefan Richter
2005-10-24 20:10 ` Luben Tuikov
2005-10-24 20:28 ` Mark Rustad
@ 2005-10-24 22:27 ` Douglas Gilbert
2 siblings, 0 replies; 32+ messages in thread
From: Douglas Gilbert @ 2005-10-24 22:27 UTC (permalink / raw)
To: Stefan Richter; +Cc: linux-scsi, Luben Tuikov, Jeff Garzik
Stefan Richter wrote:
> Luben Tuikov wrote:
>
>> On 10/23/05 12:49, Jeff Garzik wrote:
>>
>>> Stefan Richter wrote:
>>>
>>>> sprintf(s+2*i, "%02x", lun.scsi_lun[i]);
>>>
>>>
>>> There is obviously room for improvement. Any naive representation is
>>> sub-optimal, be it for small luns (my current code) or larger luns
>>> (your example).
>>>
>>> For situations with smaller luns, we should probably continue to use
>>> the current scsilun_to_int() conversion, while using your example for
>>> larger luns, i.e.
>>>
>>> if (upper 4 bytes zero)
>>> scsilun to int
>>> printk %d
>>> else
>>> for each byte
>>> printk %x
>>>
>>> But Douglas's code suggested that if we are more motivated, we could
>>> provide an even better representation.
>
>
> A plain, non-interpreted representation should suffice. Compact but
> easily parseable, i.e. uniform.
>
>> If a LUN is u8[8], then,
>>
>> #define SAS_ADDR(_sa) ((unsigned long long) be64_to_cpu(*(__be64
>> *)(_sa)))
>>
>> "%016llx", SAS_ADDR(LUN) prints it like this (e.g.):
>>
>> sas: 5000c50000513329 probing LUN:0000000000000000
>> sas: device 500000e000031c12 LUN: 0000000000000000 powering up or not
>> ready yet, sleeping...
>> sas: 500000e000031c12 probing LUN:0000000000000000
>> sas: device 50001c171601060d LUN: 0000000000000000 powering up or not
>> ready yet, sleeping...
>>
>> This is from drivers/scsi/sas/sas_discover.c.
>>
>> Luben
>
>
> What about an 64bit integer as carrier of LUN in the first place? The
> most frequent occurrence of LUN data is when it is passed through in
> function calls but it seems rarely to be manipulated.
>
> --- linux/include/scsi/scsi.h.orig 2005-10-20 08:23:05.000000000 +0200
> +++ linux/include/scsi/scsi.h 2005-10-24 21:45:41.000000000 +0200
> @@ -238,9 +238,7 @@
> /*
> * ScsiLun: 8 byte LUN.
> */
> -struct scsi_lun {
> - __u8 scsi_lun[8];
> -};
> +typedef __be64 scsi_lun;
Stefan,
IMO it is very important to keep luns as __u8[8]. Lots
of different lun conventions map into that array, almost
all of which would look horrible viewed as hex or
decimal integers. For example SCSI-2 luns (embedded
in cbds) run from 0 to 7 and map (in hex) to:
00 00 00 00 00 00 00 00
00 07 00 00 00 00 00 00
Just like a SCSI cdb, to decode a lun, first one needs
to examine scsi_lun[0], then depending on ... . As
you are no doubt aware, the only current transport
defined to have 2 byte luns is sbp-2/sbp-3 and it is
obvious what those 2 bytes will be: scsi_lun[0] and
scsi_lun[1].
Generic target+initiator (port) identifiers in SAM
(see sam4r03.pdf Annex A) should also be a sequence of
bytes (not an integer). iSCSI seems to be the limiting
case with up to 241 (utf-8) bytes. Linux may want to keep
its enumerating "id" integer, in which case a mapping
should be visible (as SDI does).
Doug Gilbert
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC] use struct scsi_lun in generic code
2005-10-23 4:33 [PATCH RFC] use struct scsi_lun in generic code Jeff Garzik
2005-10-23 4:49 ` [PATCH RFC] more struct scsi_lun Jeff Garzik
@ 2005-10-23 5:20 ` Jeff Garzik
2005-10-23 5:22 ` Douglas Gilbert
` (3 subsequent siblings)
5 siblings, 0 replies; 32+ messages in thread
From: Jeff Garzik @ 2005-10-23 5:20 UTC (permalink / raw)
To: linux-scsi
Jeff Garzik wrote:
> A further experiment: once dev_printk() has been used to eliminate
> direct references to HCIL address (see previous patch), we can see what
> happens when we update the core to use struct scsi_lun.
Two notes to this patch:
1) A REPORT LUNS [non-sequential] scan works quite nicely with this patch.
2) However, display a valid LUN received from REPORT LUNS will
inevitably be mangled by scsilun_to_str(). The lun data itself is NOT
mangled, of course; just the display.
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH RFC] use struct scsi_lun in generic code
2005-10-23 4:33 [PATCH RFC] use struct scsi_lun in generic code Jeff Garzik
2005-10-23 4:49 ` [PATCH RFC] more struct scsi_lun Jeff Garzik
2005-10-23 5:20 ` [PATCH RFC] use struct scsi_lun in generic code Jeff Garzik
@ 2005-10-23 5:22 ` Douglas Gilbert
2005-10-23 7:01 ` Jeff Garzik
2005-10-24 14:55 ` Luben Tuikov
2005-10-23 7:00 ` [PATCH RFC] yet more struct scsi_lun Jeff Garzik
` (2 subsequent siblings)
5 siblings, 2 replies; 32+ messages in thread
From: Douglas Gilbert @ 2005-10-23 5:22 UTC (permalink / raw)
To: Jeff Garzik; +Cc: linux-scsi
[-- Attachment #1: Type: text/plain, Size: 1383 bytes --]
_rbuf_getJeff Garzik wrote:
> A further experiment: once dev_printk() has been used to eliminate
> direct references to HCIL address (see previous patch), we can see what
> happens when we update the core to use struct scsi_lun.
>
> DO NOT APPLY.
>
> Depends on previous "kill scsi_device::{channel,id} in generic code"
> patch.
>
> Changes:
>
> * replace 'unsigned int lun' with 'struct scsi_lun lun' in struct scsi_device
>
> * change various function args to receive 'const struct scsi_lun *'
> rather than unsigned int.
>
> * export scsilun_to_int()
>
> * create scsilun_to_str() helper
>
> * create scsilun_eq() helper
>
> * update all references to scsi_device::lun, as caught by the compiler.
>
> Again, generic code was 100% converted, driver code 0% converted.
>
> * int_to_scsilun() is used to convert SCSI-2 luns, and luns passed
> from userspace as integers, to struct scsi_lun.
>
> * shost->max_lun check moved into scsi_scan_host_selected() callers
Jeff,
With 8 byte luns, max_lun is legacy. I found it a nuisance
when I played with wluns (well known lus). Looks like
you may be coping with that.
If you want to decode SAM-4 luns, perhaps for verbose
error purposes, feel free to borrow code from the
attached sg_luns (which is slightly more up to date
than the one found in sg3_utils version 1.17).
Back to libata's MODE SELECT ...
Doug Gilbert
[-- Attachment #2: sg_luns.c --]
[-- Type: text/x-csrc, Size: 11679 bytes --]
/*
* Copyright (c) 2004 Douglas Gilbert.
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
* are met:
* 1. Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* 2. 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.
* 3. The name of the author may not be used to endorse or promote products
* derived from this software without specific prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
* ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
* OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE.
*
*/
#include <unistd.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <errno.h>
#include <getopt.h>
#include <sys/ioctl.h>
#include <sys/types.h>
#include <sys/stat.h>
#include "sg_include.h"
#include "sg_lib.h"
#include "sg_cmds.h"
/* A utility program for the Linux OS SCSI subsystem.
*
*
* This program issues the SCSI command REPORT LUNS to the given SCSI device.
*/
static char * version_str = "1.04 20051013";
#define REPORT_LUNS_BUFF_LEN 1024
#define ME "sg_luns: "
static struct option long_options[] = {
{"decode", 0, 0, 'd'},
{"help", 0, 0, 'h'},
{"select", 1, 0, 's'},
{"verbose", 0, 0, 'v'},
{"version", 0, 0, 'V'},
{0, 0, 0, 0},
};
static void usage()
{
fprintf(stderr, "Usage: "
"sg_luns [--decode] [--help] [--select=<n>] [--verbose] "
"[--version]\n"
" <scsi_device>\n"
" where: --decode|-d decode all luns into parts\n"
" --help|-h print out usage message\n"
" --select=<n>|-s <n> select report <n> (def: 0)\n"
" 0 -> luns apart from 'well "
"known' lus\n"
" 1 -> only 'well known' "
"logical unit numbers\n"
" 2 -> all luns\n"
" --verbose|-v increase verbosity\n"
" --version|-V print version string and exit\n\n"
"Performs a REPORT LUNS SCSI command\n"
);
}
/* Decoded according to SAM-4 rev 3 (plus 05-376r2). Note that one
* draft: BCC rev 0, defines its own "bridge addressing method" in
* place of the SAM-3 "logical addressing method". */
static void decode_lun(const char * leadin, unsigned char * lunp)
{
int k, j, x, a_method, bus_id, target, lun, len, e_a_method, next_level;
unsigned char not_spec[8] = {0xff, 0xff, 0xff, 0xff,
0xff, 0xff, 0xff, 0xff};
char l_leadin[128];
unsigned long long ull;
if (0 == memcmp(lunp, not_spec, sizeof(not_spec))) {
printf("%sLogical unit not specified\n", leadin);
return;
}
memset(l_leadin, 0, sizeof(l_leadin));
for (k = 0; k < 4; ++k, lunp += 2) {
next_level = 0;
strncpy(l_leadin, leadin, sizeof(l_leadin) - 3);
if (k > 0) {
printf("%s>>%s level addressing:\n", l_leadin,
((1 == k) ? "Second" : ((2 == k) ? "Third" : "Fourth")));
strcat(l_leadin, " ");
}
a_method = (lunp[0] >> 6) & 0x3;
switch (a_method) {
case 0: /* peripheral device addressing method */
bus_id = lunp[0] & 0x3f;
if (0 == bus_id)
printf("%sPeripheral device addressing: lun=%d\n",
l_leadin, lunp[1]);
else {
printf("%sPeripheral device addressing: bus_id=%d, "
"target=%d\n", l_leadin, bus_id, lunp[1]);
next_level = 1;
}
break;
case 1: /* flat space addressing method */
lun = ((lunp[0] & 0x3f) << 8) + lunp[1];
printf("%sFlat space addressing: lun=%d\n", l_leadin, lun);
break;
case 2: /* logical unit addressing method */
target = (lunp[0] & 0x3f);
bus_id = (lunp[1] >> 5) & 0x7;
lun = lunp[1] & 0x1f;
printf("%sLogical unit addressing: bus_id=%d, target=%d, "
"lun=%d\n", l_leadin, bus_id, target, lun);
break;
case 3: /* extended logical unit addressing method */
len = (lunp[0] & 0x30) >> 4;
e_a_method = lunp[0] & 0xf;
x = lunp[1];
if ((0 == len) && (1 == e_a_method)) {
switch (x) {
case 1:
printf("%sREPORT LUNS well known logical unit\n",
l_leadin);
break;
case 2:
printf("%sACCESS CONTROLS well known logical unit\n",
l_leadin);
break;
case 3:
printf("%sTARGET LOG PAGES well known logical unit\n",
l_leadin);
break;
default:
printf("%swell known logical unit %d\n", l_leadin, x);
break;
}
} else if ((1 == len) && (2 == e_a_method)) {
x = (lunp[1] << 16) + (lunp[2] << 8) + lunp[3];
printf("%sExtended flat space logical unit addressing: "
"value=0x%x\n", l_leadin, x);
} else if ((3 == len) && (0xf == e_a_method))
printf("%sLogical unit _not_ specified addressing\n",
l_leadin);
else {
if (len < 2) {
if (1 == len)
x = (lunp[1] << 16) + (lunp[2] << 8) + lunp[3];
printf("%sExtended logical unit addressing: length=%d, "
"e.a. method=%d, value=0x%x\n", l_leadin, len,
e_a_method, x);
} else {
ull = 0;
x = (2 == len) ? 5 : 7;
for (j = 0; j < x; ++j) {
if (j > 0)
ull <<= 8;
ull |= lunp[1 + j];
}
printf("%sExtended logical unit addressing: length=%d, "
"e. a. method=%d, value=0x%llx\n", l_leadin, len,
e_a_method, ull);
}
}
break;
default:
printf("%s<<decode_lun: faulty logic>>\n", l_leadin);
break;
}
if (next_level)
continue;
if ((2 == a_method) && (k < 3) && (lunp[2] || lunp[3]))
printf("%s<<unexpected data at next level, continue>>\n",
l_leadin);
break;
}
}
int main(int argc, char * argv[])
{
int sg_fd, k, m, off, res, c, list_len, luns, trunc;
unsigned char reportLunsBuff[REPORT_LUNS_BUFF_LEN];
int decode = 0;
int select_rep = 0;
int verbose = 0;
char device_name[256];
int ret = 1;
memset(device_name, 0, sizeof device_name);
while (1) {
int option_index = 0;
c = getopt_long(argc, argv, "dhs:vV", long_options,
&option_index);
if (c == -1)
break;
switch (c) {
case 'd':
decode = 1;
break;
case 'h':
case '?':
usage();
return 0;
case 's':
if ((1 != sscanf(optarg, "%d", &select_rep)) ||
(select_rep < 0) || (select_rep > 255)) {
fprintf(stderr, "bad argument to '--select'\n");
return 1;
}
break;
case 'v':
++verbose;
break;
case 'V':
fprintf(stderr, ME "version: %s\n", version_str);
return 0;
default:
fprintf(stderr, "unrecognised switch code 0x%x ??\n", c);
usage();
return 1;
}
}
if (optind < argc) {
if ('\0' == device_name[0]) {
strncpy(device_name, argv[optind], sizeof(device_name) - 1);
device_name[sizeof(device_name) - 1] = '\0';
++optind;
}
if (optind < argc) {
for (; optind < argc; ++optind)
fprintf(stderr, "Unexpected extra argument: %s\n",
argv[optind]);
usage();
return 1;
}
}
if (0 == device_name[0]) {
fprintf(stderr, "missing device name!\n");
usage();
return 1;
}
sg_fd = open(device_name, O_RDWR | O_NONBLOCK);
if (sg_fd < 0) {
fprintf(stderr, ME "open error: %s: ", device_name);
perror("");
return 1;
}
memset(reportLunsBuff, 0x0, sizeof(reportLunsBuff));
trunc = 0;
res = sg_ll_report_luns(sg_fd, select_rep, reportLunsBuff,
sizeof(reportLunsBuff), 1, verbose);
if (0 == res) {
list_len = (reportLunsBuff[0] << 24) + (reportLunsBuff[1] << 16) +
(reportLunsBuff[2] << 8) + reportLunsBuff[3];
luns = (list_len / 8);
printf("Lun list length = %d which imples %d lun entr%s\n",
list_len, luns, ((1 == luns) ? "y" : "ies"));
if ((list_len + 8) > (int)sizeof(reportLunsBuff)) {
luns = ((sizeof(reportLunsBuff) - 8) / 8);
trunc = 1;
printf(" <<too many luns for internal buffer, will show %d "
"luns>>\n", luns);
}
if (verbose) {
fprintf(stderr, "\nOutput response in hex\n");
dStrHex((const char *)reportLunsBuff,
(trunc ? (int)sizeof(reportLunsBuff) : list_len + 8), 1);
}
for (k = 0, off = 8; k < luns; ++k) {
if (0 == k)
printf("Report luns [select_report=%d]:\n", select_rep);
printf(" ");
for (m = 0; m < 8; ++m, ++off)
printf("%02x", reportLunsBuff[off]);
printf("\n");
if (decode)
decode_lun(" ", reportLunsBuff + off - 8);
}
ret = 0;
} else if (SG_LIB_CAT_INVALID_OP == res)
fprintf(stderr, "Report Luns command not supported (support "
"mandatory in SPC-3)\n");
else if (SG_LIB_CAT_ILLEGAL_REQ == res)
fprintf(stderr, "Report Luns command has bad fields in cdb\n");
else {
fprintf(stderr, "Report Luns command failed\n");
if (0 == verbose)
fprintf(stderr, " try '-v' option for more information\n");
}
res = close(sg_fd);
if (res < 0) {
perror(ME "close error");
return 1;
}
return ret;
}
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH RFC] use struct scsi_lun in generic code
2005-10-23 5:22 ` Douglas Gilbert
@ 2005-10-23 7:01 ` Jeff Garzik
2005-10-24 14:55 ` Luben Tuikov
1 sibling, 0 replies; 32+ messages in thread
From: Jeff Garzik @ 2005-10-23 7:01 UTC (permalink / raw)
To: dougg; +Cc: linux-scsi
Douglas Gilbert wrote:
> With 8 byte luns, max_lun is legacy. I found it a nuisance
> when I played with wluns (well known lus). Looks like
> you may be coping with that.
See the patch I just posted...
> If you want to decode SAM-4 luns, perhaps for verbose
> error purposes, feel free to borrow code from the
> attached sg_luns (which is slightly more up to date
> than the one found in sg3_utils version 1.17).
If you're bored, feel free to contribute a better version of
scsilun_to_str()... ;-)
Jeff
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC] use struct scsi_lun in generic code
2005-10-23 5:22 ` Douglas Gilbert
2005-10-23 7:01 ` Jeff Garzik
@ 2005-10-24 14:55 ` Luben Tuikov
1 sibling, 0 replies; 32+ messages in thread
From: Luben Tuikov @ 2005-10-24 14:55 UTC (permalink / raw)
To: dougg; +Cc: Jeff Garzik, linux-scsi
On 10/23/05 01:22, Douglas Gilbert wrote:
>>A further experiment: once dev_printk() has been used to eliminate
>>direct references to HCIL address (see previous patch), we can see what
>>happens when we update the core to use struct scsi_lun.
>>
>>DO NOT APPLY.
>>
>>Depends on previous "kill scsi_device::{channel,id} in generic code"
>>patch.
>>
>>Changes:
>>
>>* replace 'unsigned int lun' with 'struct scsi_lun lun' in struct scsi_device
>>
>>* change various function args to receive 'const struct scsi_lun *'
>> rather than unsigned int.
>>
>>* export scsilun_to_int()
>>
>>* create scsilun_to_str() helper
>>
>>* create scsilun_eq() helper
>>
>>* update all references to scsi_device::lun, as caught by the compiler.
>>
>> Again, generic code was 100% converted, driver code 0% converted.
>>
>>* int_to_scsilun() is used to convert SCSI-2 luns, and luns passed
>> from userspace as integers, to struct scsi_lun.
>>
>>* shost->max_lun check moved into scsi_scan_host_selected() callers
>
>
> Jeff,
> With 8 byte luns, max_lun is legacy. I found it a nuisance
I agree.
Furthermore, a LUN is just u8[8] and I don't think there's a reason
to wrap that in a struct. (Incidentally a logical unit number is
not a number but a structure.)
Other than application clients and device servers, I don't think
that the kernel should in anyway interpret the LUN structure.
It is just u8[8] and this is what the kernel should pass around.
Cf. drivers/scsi/sas/sas_discover.c and include/scsi/sas/sas_task.h.
Luben
--
http://linux.adaptec.com/sas/
http://www.adaptec.com/sas/
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH RFC] yet more struct scsi_lun
2005-10-23 4:33 [PATCH RFC] use struct scsi_lun in generic code Jeff Garzik
` (2 preceding siblings ...)
2005-10-23 5:22 ` Douglas Gilbert
@ 2005-10-23 7:00 ` Jeff Garzik
2005-10-23 10:48 ` Douglas Gilbert
2005-10-23 7:16 ` [PATCH RFC] even " Jeff Garzik
2005-10-24 15:27 ` [PATCH RFC] use struct scsi_lun in generic code Patrick Mansfield
5 siblings, 1 reply; 32+ messages in thread
From: Jeff Garzik @ 2005-10-23 7:00 UTC (permalink / raw)
To: linux-scsi
Douglas Gilbert reminded me of a lun limitation I could kill. This
patch applied on top of the previous two lun patches.
DO NOT APPLY. For discussion only.
Changes:
* add SCSILUN_UNLIMITED for LLDDs without lun limits; set
shost->max_lun = SCSILUN_UNLIMITED;
* remove the lun limits in scsi_report_lun_scan()
Special note:
With this patch (and previous patches in series), max_lun limitation
only remains in two areas, AFAICS:
1) legacy LLDDs that perform sequential lun scans
2) legacy userspace interfaces
3) Any LLDD that fails to set max_lun = SCSILUN_UNLIMITED.
drivers/scsi/scsi_scan.c | 39 ++++++++++++++-------------------------
include/scsi/scsi.h | 1 +
2 files changed, 15 insertions(+), 25 deletions(-)
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index fb1d9c7..09df4e7 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1070,7 +1070,6 @@ static int scsi_report_lun_scan(struct s
char devname[64];
unsigned char scsi_cmd[MAX_COMMAND_SIZE];
unsigned int length;
- unsigned int lun;
unsigned int num_luns;
unsigned int retries;
int result;
@@ -1202,29 +1201,17 @@ static int scsi_report_lun_scan(struct s
* the header, so start at 1 and go up to and including num_luns.
*/
for (lunp = &lun_data[1]; lunp <= &lun_data[num_luns]; lunp++) {
- lun = scsilun_to_int(lunp);
+ int lun;
+ char lunstr[SCSILUN_STR_LEN];
- /*
- * Check if the unused part of lunp is non-zero, and so
- * does not fit in lun.
- */
- if (memcmp(&lunp->scsi_lun[sizeof(lun)], "\0\0\0\0", 4)) {
- int i;
+ lun = scsilun_to_int(lunp);
- /*
- * Output an error displaying the LUN in byte order,
- * this differs from what linux would print for the
- * integer LUN value.
- */
- printk(KERN_WARNING "scsi: %s lun 0x", devname);
- data = (char *)lunp->scsi_lun;
- for (i = 0; i < sizeof(struct scsi_lun); i++)
- printk("%02x", data[i]);
- printk(" has a LUN larger than currently supported.\n");
- } else if (lun > sdev->host->max_lun) {
- printk(KERN_WARNING "scsi: %s lun%d has a LUN larger"
- " than allowed by the host adapter\n",
- devname, lun);
+ if ((sdev->host->max_lun != SCSILUN_UNLIMITED) &&
+ (lun > sdev->host->max_lun)) {
+ dev_printk(KERN_WARNING, &sdev->sdev_gendev,
+ "lun %s larger"
+ " than allowed by the host adapter\n",
+ scsilun_to_str(lunp, lunstr));
} else {
int res;
@@ -1234,9 +1221,11 @@ static int scsi_report_lun_scan(struct s
/*
* Got some results, but now none, abort.
*/
- printk(KERN_ERR "scsi: Unexpected response"
- " from %s lun %d while scanning, scan"
- " aborted\n", devname, lun);
+ dev_printk(KERN_ERR, &sdev->sdev_gendev,
+ "Unexpected response"
+ " from lun %s while scanning, scan"
+ " aborted\n",
+ scsilun_to_str(lunp, lunstr));
break;
}
}
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index deb1a27..7876a64 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -242,6 +242,7 @@ struct scsi_lun {
__u8 scsi_lun[8];
};
#define SCSILUN_STR_LEN 33
+#define SCSILUN_UNLIMITED ~0
/*
* MESSAGE CODES
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH RFC] yet more struct scsi_lun
2005-10-23 7:00 ` [PATCH RFC] yet more struct scsi_lun Jeff Garzik
@ 2005-10-23 10:48 ` Douglas Gilbert
2005-10-23 11:53 ` Arjan van de Ven
2005-10-23 16:43 ` Jeff Garzik
0 siblings, 2 replies; 32+ messages in thread
From: Douglas Gilbert @ 2005-10-23 10:48 UTC (permalink / raw)
To: Jeff Garzik; +Cc: linux-scsi
Jeff Garzik wrote:
> Douglas Gilbert reminded me of a lun limitation I could kill. This
> patch applied on top of the previous two lun patches.
>
> DO NOT APPLY. For discussion only.
>
> Changes:
> * add SCSILUN_UNLIMITED for LLDDs without lun limits; set
> shost->max_lun = SCSILUN_UNLIMITED;
Which in turn makes me think of applying the same idea
to max_sectors
shost->max_sectors = MAX_512B_SECTORS_UNLIMITED;
which would be the default unless the HBA (RAID controller
or whatever) really does have a (practical) limit. If a
device beyond the target has a limit, then that is not the
LLD's concern.
Doug Gilbert
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH RFC] yet more struct scsi_lun
2005-10-23 10:48 ` Douglas Gilbert
@ 2005-10-23 11:53 ` Arjan van de Ven
2005-10-23 14:27 ` max_sectors [was Re: [PATCH RFC] yet more struct scsi_lun] Douglas Gilbert
2005-10-23 16:44 ` [PATCH RFC] yet more struct scsi_lun Jeff Garzik
2005-10-23 16:43 ` Jeff Garzik
1 sibling, 2 replies; 32+ messages in thread
From: Arjan van de Ven @ 2005-10-23 11:53 UTC (permalink / raw)
To: dougg; +Cc: linux-scsi, Jeff Garzik
> Which in turn makes me think of applying the same idea
> to max_sectors
>
> shost->max_sectors = MAX_512B_SECTORS_UNLIMITED;
>
> which would be the default unless the HBA (RAID controller
> or whatever) really does have a (practical) limit. If a
> device beyond the target has a limit, then that is not the
> LLD's concern.
this is a bit of a problem since we don't know all the limits. We do
know that Microsoft Windows at least used to have a 64Kb limit, and that
that limit is "safe" in the sense that all hw was tested/designed for
that. So setting the default safe, and overriding it per driver sounds
best. It only really matters for a handful of drivers anyway, and those
either already set something or ought to.
^ permalink raw reply [flat|nested] 32+ messages in thread
* max_sectors [was Re: [PATCH RFC] yet more struct scsi_lun]
2005-10-23 11:53 ` Arjan van de Ven
@ 2005-10-23 14:27 ` Douglas Gilbert
2005-10-23 14:42 ` Arjan van de Ven
2005-10-23 16:44 ` [PATCH RFC] yet more struct scsi_lun Jeff Garzik
1 sibling, 1 reply; 32+ messages in thread
From: Douglas Gilbert @ 2005-10-23 14:27 UTC (permalink / raw)
To: Arjan van de Ven; +Cc: linux-scsi, Jeff Garzik, michaelc, Kai.Makisara
Arjan van de Ven wrote:
>>Which in turn makes me think of applying the same idea
>>to max_sectors
>>
>> shost->max_sectors = MAX_512B_SECTORS_UNLIMITED;
>>
>>which would be the default unless the HBA (RAID controller
>>or whatever) really does have a (practical) limit. If a
>>device beyond the target has a limit, then that is not the
>>LLD's concern.
>
>
> this is a bit of a problem since we don't know all the limits. We do
> know that Microsoft Windows at least used to have a 64Kb limit, and that
> that limit is "safe" in the sense that all hw was tested/designed for
> that. So setting the default safe, and overriding it per driver sounds
> best. It only really matters for a handful of drivers anyway, and those
> either already set something or ought to.
I would be very surprised if Microsoft limited their
SCSI or ATA pass through to 64 KB per command. It would
make sending microcode to a device (WRITE BUFFER in
SCSI command sets, DOWNLOAD MICROCODE in ATA) a real pain.
WRITE BUFFER can send smaller amounts with offsets but not
all devices support that. Why needlessly widen the window
on a process that if it fails may trash the device.
As I have pointed out before, the SBC-2 Block Limits VPD
page has both optimal and maximum values. I see no reason
why normal VFS reads and writes would need to be above
the optimal value (which the SCSI mid level could default
in the absence of that page to say 64 KB). The maximum
value is another matter. A REQ_SPECIAL conveys
layered errors back to the invoker; so the block layer
shouldn't "block" such requests on suspicion; if something
lower down doesn't like it then it can complain.
Does anyone know a PCI HBA produced in the last 6 years
that can't DMA at least 16 MB?
Mike Christie has done some good work on both sg and st
to make them use common code to build scatter gather lists.
However, while max_sectors is in place with its current
semantics that work can go no further.
Doug Gilbert
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC] yet more struct scsi_lun
2005-10-23 11:53 ` Arjan van de Ven
2005-10-23 14:27 ` max_sectors [was Re: [PATCH RFC] yet more struct scsi_lun] Douglas Gilbert
@ 2005-10-23 16:44 ` Jeff Garzik
1 sibling, 0 replies; 32+ messages in thread
From: Jeff Garzik @ 2005-10-23 16:44 UTC (permalink / raw)
To: Arjan van de Ven; +Cc: dougg, linux-scsi
Arjan van de Ven wrote:
> this is a bit of a problem since we don't know all the limits. We do
> know that Microsoft Windows at least used to have a 64Kb limit, and that
> that limit is "safe" in the sense that all hw was tested/designed for
> that. So setting the default safe, and overriding it per driver sounds
> best. It only really matters for a handful of drivers anyway, and those
> either already set something or ought to.
It's more a VM thing. Default is 512K (1024 sectors).
Jeff
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC] yet more struct scsi_lun
2005-10-23 10:48 ` Douglas Gilbert
2005-10-23 11:53 ` Arjan van de Ven
@ 2005-10-23 16:43 ` Jeff Garzik
2005-10-23 18:53 ` Kai Makisara
2005-10-24 7:59 ` Jens Axboe
1 sibling, 2 replies; 32+ messages in thread
From: Jeff Garzik @ 2005-10-23 16:43 UTC (permalink / raw)
To: dougg; +Cc: linux-scsi
Douglas Gilbert wrote:
> Which in turn makes me think of applying the same idea
> to max_sectors
>
> shost->max_sectors = MAX_512B_SECTORS_UNLIMITED;
Won't work. max_sectors is communicated to the block layer, where we
limit the overall size of the request for practical reasons.
Read the comment in libata-scsi's slave_configure:
/* TODO: 1024 is an arbitrary number, not the
* hardware maximum. This should be increased to
* 65534 when Jens Axboe's patch for dynamically
* determining max_sectors is merged.
*/
Right now, setting the true hardware / command set maximum would use way
too much memory, with no way to get feedback from the VM.
This is why SCSI_DEFAULT_MAX_SECTORS is defined to 1024.
Jeff
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH RFC] yet more struct scsi_lun
2005-10-23 16:43 ` Jeff Garzik
@ 2005-10-23 18:53 ` Kai Makisara
2005-10-24 7:59 ` Jens Axboe
1 sibling, 0 replies; 32+ messages in thread
From: Kai Makisara @ 2005-10-23 18:53 UTC (permalink / raw)
To: Jeff Garzik; +Cc: dougg, linux-scsi
On Sun, 23 Oct 2005, Jeff Garzik wrote:
> Douglas Gilbert wrote:
> > Which in turn makes me think of applying the same idea
> > to max_sectors
> >
> > shost->max_sectors = MAX_512B_SECTORS_UNLIMITED;
>
>
> Won't work. max_sectors is communicated to the block layer, where we limit
> the overall size of the request for practical reasons.
>
> Read the comment in libata-scsi's slave_configure:
>
> /* TODO: 1024 is an arbitrary number, not the
> * hardware maximum. This should be increased to
> * 65534 when Jens Axboe's patch for dynamically
> * determining max_sectors is merged.
> */
>
> Right now, setting the true hardware / command set maximum would use way too
> much memory, with no way to get feedback from the VM.
>
> This is why SCSI_DEFAULT_MAX_SECTORS is defined to 1024.
>
So, the block layer is just for devices using the page cache, after all!
If max_sectors is a VM thing, why is it set by _device drivers_?
Now, let me try to more be constructive. max_sectors is enforced in
__bio_add_page(). Maybe this is not the correct place. Bios are advocated
as general i/o concepts and they don't have any direct relation with VM.
--
Kai
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC] yet more struct scsi_lun
2005-10-23 16:43 ` Jeff Garzik
2005-10-23 18:53 ` Kai Makisara
@ 2005-10-24 7:59 ` Jens Axboe
2005-10-28 17:24 ` Mike Christie
1 sibling, 1 reply; 32+ messages in thread
From: Jens Axboe @ 2005-10-24 7:59 UTC (permalink / raw)
To: Jeff Garzik; +Cc: dougg, linux-scsi
On Sun, Oct 23 2005, Jeff Garzik wrote:
> Douglas Gilbert wrote:
> >Which in turn makes me think of applying the same idea
> >to max_sectors
> >
> > shost->max_sectors = MAX_512B_SECTORS_UNLIMITED;
>
>
> Won't work. max_sectors is communicated to the block layer, where we
> limit the overall size of the request for practical reasons.
>
> Read the comment in libata-scsi's slave_configure:
>
> /* TODO: 1024 is an arbitrary number, not the
> * hardware maximum. This should be increased to
> * 65534 when Jens Axboe's patch for dynamically
> * determining max_sectors is merged.
> */
>
> Right now, setting the true hardware / command set maximum would use way
> too much memory, with no way to get feedback from the VM.
>
> This is why SCSI_DEFAULT_MAX_SECTORS is defined to 1024.
The block layer has had split values for quite some time, ->max_sectors
and max_hw_sectors. scsi_ioctl.c needs a patch to look at max_hw_sectors
instead and SCSI drivers could then easily be updated to advertise a
real hardware value as well. That is what shost->max_sectors should be,
SCSI mid layer would then set q->max_sectors to SCSI_DEFAULT_MAX_SECTORS
and q->max_hw_sectors to shost->max_sectors.
Then the limiting factor becomes BIO_MAX_PAGES for mapping in the user
data, which caps us at 1MiB currently.
--
Jens Axboe
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC] yet more struct scsi_lun
2005-10-24 7:59 ` Jens Axboe
@ 2005-10-28 17:24 ` Mike Christie
2005-10-31 10:24 ` Jens Axboe
0 siblings, 1 reply; 32+ messages in thread
From: Mike Christie @ 2005-10-28 17:24 UTC (permalink / raw)
To: Jens Axboe; +Cc: Jeff Garzik, dougg, linux-scsi
Jens Axboe wrote:
> On Sun, Oct 23 2005, Jeff Garzik wrote:
>
>>Douglas Gilbert wrote:
>>
>>>Which in turn makes me think of applying the same idea
>>>to max_sectors
>>>
>>> shost->max_sectors = MAX_512B_SECTORS_UNLIMITED;
>>
>>
>>Won't work. max_sectors is communicated to the block layer, where we
>>limit the overall size of the request for practical reasons.
>>
>>Read the comment in libata-scsi's slave_configure:
>>
>> /* TODO: 1024 is an arbitrary number, not the
>> * hardware maximum. This should be increased to
>> * 65534 when Jens Axboe's patch for dynamically
>> * determining max_sectors is merged.
>> */
>>
>>Right now, setting the true hardware / command set maximum would use way
>>too much memory, with no way to get feedback from the VM.
>>
>>This is why SCSI_DEFAULT_MAX_SECTORS is defined to 1024.
>
>
> The block layer has had split values for quite some time, ->max_sectors
> and max_hw_sectors. scsi_ioctl.c needs a patch to look at max_hw_sectors
> instead and SCSI drivers could then easily be updated to advertise a
> real hardware value as well. That is what shost->max_sectors should be,
> SCSI mid layer would then set q->max_sectors to SCSI_DEFAULT_MAX_SECTORS
> and q->max_hw_sectors to shost->max_sectors.
>
> Then the limiting factor becomes BIO_MAX_PAGES for mapping in the user
> data, which caps us at 1MiB currently.
>
I was just wondering if you give a little more detail in case someone
wanted to implement this for you.
Would the bio functions like __bio_add_page() and bio_get_nr_vecs()
continue to test against q->max_sectors. And then have the request
merging code test against q->max_hw_sectors. scsi or blk would need some
check that max_sectors was not larger than max_sectors, and for scsi we
would have to increase SCSI_DEFAULT_MAX_SECTORS to 2048 to match the
1MiB limit and not make q->max_sectors the limit factor. Or how would
this work?
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC] yet more struct scsi_lun
2005-10-28 17:24 ` Mike Christie
@ 2005-10-31 10:24 ` Jens Axboe
2005-11-04 2:23 ` Mike Christie
0 siblings, 1 reply; 32+ messages in thread
From: Jens Axboe @ 2005-10-31 10:24 UTC (permalink / raw)
To: Mike Christie; +Cc: Jeff Garzik, dougg, linux-scsi
On Fri, Oct 28 2005, Mike Christie wrote:
> Jens Axboe wrote:
> >On Sun, Oct 23 2005, Jeff Garzik wrote:
> >
> >>Douglas Gilbert wrote:
> >>
> >>>Which in turn makes me think of applying the same idea
> >>>to max_sectors
> >>>
> >>> shost->max_sectors = MAX_512B_SECTORS_UNLIMITED;
> >>
> >>
> >>Won't work. max_sectors is communicated to the block layer, where we
> >>limit the overall size of the request for practical reasons.
> >>
> >>Read the comment in libata-scsi's slave_configure:
> >>
> >> /* TODO: 1024 is an arbitrary number, not the
> >> * hardware maximum. This should be increased to
> >> * 65534 when Jens Axboe's patch for dynamically
> >> * determining max_sectors is merged.
> >> */
> >>
> >>Right now, setting the true hardware / command set maximum would use way
> >>too much memory, with no way to get feedback from the VM.
> >>
> >>This is why SCSI_DEFAULT_MAX_SECTORS is defined to 1024.
> >
> >
> >The block layer has had split values for quite some time, ->max_sectors
> >and max_hw_sectors. scsi_ioctl.c needs a patch to look at max_hw_sectors
> >instead and SCSI drivers could then easily be updated to advertise a
> >real hardware value as well. That is what shost->max_sectors should be,
> >SCSI mid layer would then set q->max_sectors to SCSI_DEFAULT_MAX_SECTORS
> >and q->max_hw_sectors to shost->max_sectors.
> >
> >Then the limiting factor becomes BIO_MAX_PAGES for mapping in the user
> >data, which caps us at 1MiB currently.
> >
>
> I was just wondering if you give a little more detail in case someone
> wanted to implement this for you.
Certainly!
> Would the bio functions like __bio_add_page() and bio_get_nr_vecs()
> continue to test against q->max_sectors. And then have the request
> merging code test against q->max_hw_sectors. scsi or blk would need some
> check that max_sectors was not larger than max_sectors, and for scsi we
> would have to increase SCSI_DEFAULT_MAX_SECTORS to 2048 to match the
> 1MiB limit and not make q->max_sectors the limit factor. Or how would
> this work?
On the SCSI side, I would suggest just making shost->max_sectors set
q->max_hw_sectors and leave q->max_sectors to some generic kernel-wide
block layer define (of course making sure that ->max_sectors <=
->max_hw_sectors). That's the easy part.
The bio_add_page() stuff is a little trickier, since it wants to know if
this is fs or 'generic' io. For fs io, we would like to cap the building
of the bio to ->max_sectors, but for eg SG_IO issued io it should go as
high as ->max_hw_sectors. Perhaps the easiest is just to have
bio_fs_add_page() and bio_pc_add_page(), each just passing in the max
value as an integer to bio_add_page(). But it's not exactly pretty.
The ll_rw_blk.c merging is easy, since you don't need to do anything
there. It should test against ->max_sectors as it already does, since
this (sadly) is still the primary way we build large ios.
Make sense?
--
Jens Axboe
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC] yet more struct scsi_lun
2005-10-31 10:24 ` Jens Axboe
@ 2005-11-04 2:23 ` Mike Christie
2005-11-04 2:25 ` Mike Christie
2005-11-04 7:37 ` Jens Axboe
0 siblings, 2 replies; 32+ messages in thread
From: Mike Christie @ 2005-11-04 2:23 UTC (permalink / raw)
To: Jens Axboe; +Cc: Jeff Garzik, dougg, linux-scsi
Jens Axboe wrote:
>
> On the SCSI side, I would suggest just making shost->max_sectors set
> q->max_hw_sectors and leave q->max_sectors to some generic kernel-wide
> block layer define (of course making sure that ->max_sectors <=
If the value for this block layer define was around 16,000 sectors,
would that be ok? The reason I ask is becuase when I get .....
> ->max_hw_sectors). That's the easy part.
>
> The bio_add_page() stuff is a little trickier, since it wants to know if
> this is fs or 'generic' io. For fs io, we would like to cap the building
> of the bio to ->max_sectors, but for eg SG_IO issued io it should go as
> high as ->max_hw_sectors. Perhaps the easiest is just to have
> bio_fs_add_page() and bio_pc_add_page(), each just passing in the max
> value as an integer to bio_add_page(). But it's not exactly pretty.
>
> The ll_rw_blk.c merging is easy, since you don't need to do anything
> there. It should test against ->max_sectors as it already does, since
> this (sadly) is still the primary way we build large ios.
>
.... here, I am running into a problem. Basically, as you know the
largest BIO we can make is 1 MB due to BIO_MAX_PAGES, and for st and sg
we need to support commands around 6 MB, so we would have a request with
6 BIOs. To make this monster request I wanted to use the block layer
functions and do something like this:
+ for (i = 0; i < nsegs; i++) {
+ bio = bio_map_pages(q, sg[i].page, sg[i].length,
sg[i].offset, gfp);
+ if (IS_ERR(bio)) {
+ err = PTR_ERR(bio);
+ goto free_bios;
+ }
+ len += sg[i].length;
+
+ bio->bi_flags &= ~(1 << BIO_SEG_VALID);
+ if (rq_data_dir(rq) == WRITE)
+ bio->bi_rw |= (1 << BIO_RW);
+ blk_queue_bounce(q, &bio);
+
+ if (i == 0)
+ blk_rq_bio_prep(q, rq, bio);
/* hope to carve out the __make_request code that does the below
operations and make a fucntion that can be shared */
+ else if (!q->back_merge_fn(q, rq, bio)) {
+ err = -EINVAL;
+ bio_endio(bio, bio->bi_size, 0);
+ goto free_bios;
+ } else {
+ rq->biotail->bi_next = bio;
+ rq->biotail = bio;
+ rq->hard_nr_sectors += bio_sectors(bio);
+ rq->nr_sectors = rq->hard_nr_sectors;
........
But since q->back_merge_fn() tests against q->max_sectors, it must be a
high value so that we can merge in those BIOs. I mean if q->max_sectors
is some reasonable number like only 1024 sectors, q->back_merge_fn will
return a failure. Should I instead seperate ll_back_merge_fn into two
functions, one that checks the sectors and one that checks the segments
or if ll_back_merge_fn tested for max_hw_sectors we would be ok too?
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH RFC] yet more struct scsi_lun
2005-11-04 2:23 ` Mike Christie
@ 2005-11-04 2:25 ` Mike Christie
2005-11-04 7:37 ` Jens Axboe
1 sibling, 0 replies; 32+ messages in thread
From: Mike Christie @ 2005-11-04 2:25 UTC (permalink / raw)
To: Jens Axboe; +Cc: Jeff Garzik, dougg, linux-scsi
Mike Christie wrote:
> Jens Axboe wrote:
>
>>
>> On the SCSI side, I would suggest just making shost->max_sectors set
>> q->max_hw_sectors and leave q->max_sectors to some generic kernel-wide
>> block layer define (of course making sure that ->max_sectors <=
>
>
> If the value for this block layer define was around 16,000 sectors,
> would that be ok? The reason I ask is becuase when I get .....
>
>
>> ->max_hw_sectors). That's the easy part.
>>
>> The bio_add_page() stuff is a little trickier, since it wants to know if
>> this is fs or 'generic' io. For fs io, we would like to cap the building
>> of the bio to ->max_sectors, but for eg SG_IO issued io it should go as
>> high as ->max_hw_sectors. Perhaps the easiest is just to have
>> bio_fs_add_page() and bio_pc_add_page(), each just passing in the max
>> value as an integer to bio_add_page(). But it's not exactly pretty.
>>
>> The ll_rw_blk.c merging is easy, since you don't need to do anything
>> there. It should test against ->max_sectors as it already does, since
>> this (sadly) is still the primary way we build large ios.
>>
>
> .... here, I am running into a problem. Basically, as you know the
> largest BIO we can make is 1 MB due to BIO_MAX_PAGES, and for st and sg
> we need to support commands around 6 MB, so we would have a request with
> 6 BIOs. To make this monster request I wanted to use the block layer
> functions and do something like this:
>
> + for (i = 0; i < nsegs; i++) {
> + bio = bio_map_pages(q, sg[i].page, sg[i].length,
> sg[i].offset, gfp);
oh yeah bio_map_pages, just takes an array of pages and does
bio_*_add_page on them.
> + if (IS_ERR(bio)) {
> + err = PTR_ERR(bio);
> + goto free_bios;
> + }
> + len += sg[i].length;
> +
> + bio->bi_flags &= ~(1 << BIO_SEG_VALID);
> + if (rq_data_dir(rq) == WRITE)
> + bio->bi_rw |= (1 << BIO_RW);
> + blk_queue_bounce(q, &bio);
> +
> + if (i == 0)
> + blk_rq_bio_prep(q, rq, bio);
>
> /* hope to carve out the __make_request code that does the below
> operations and make a fucntion that can be shared */
>
> + else if (!q->back_merge_fn(q, rq, bio)) {
> + err = -EINVAL;
> + bio_endio(bio, bio->bi_size, 0);
> + goto free_bios;
> + } else {
> + rq->biotail->bi_next = bio;
> + rq->biotail = bio;
> + rq->hard_nr_sectors += bio_sectors(bio);
> + rq->nr_sectors = rq->hard_nr_sectors;
>
> ........
>
> But since q->back_merge_fn() tests against q->max_sectors, it must be a
> high value so that we can merge in those BIOs. I mean if q->max_sectors
> is some reasonable number like only 1024 sectors, q->back_merge_fn will
> return a failure. Should I instead seperate ll_back_merge_fn into two
> functions, one that checks the sectors and one that checks the segments
> or if ll_back_merge_fn tested for max_hw_sectors we would be ok too?
>
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH RFC] yet more struct scsi_lun
2005-11-04 2:23 ` Mike Christie
2005-11-04 2:25 ` Mike Christie
@ 2005-11-04 7:37 ` Jens Axboe
2005-11-04 17:27 ` Mike Christie
1 sibling, 1 reply; 32+ messages in thread
From: Jens Axboe @ 2005-11-04 7:37 UTC (permalink / raw)
To: Mike Christie; +Cc: Jeff Garzik, dougg, linux-scsi
On Thu, Nov 03 2005, Mike Christie wrote:
> Jens Axboe wrote:
> >
> >On the SCSI side, I would suggest just making shost->max_sectors set
> >q->max_hw_sectors and leave q->max_sectors to some generic kernel-wide
> >block layer define (of course making sure that ->max_sectors <=
>
> If the value for this block layer define was around 16,000 sectors,
> would that be ok? The reason I ask is becuase when I get .....
Nope :)
> >->max_hw_sectors). That's the easy part.
> >
> >The bio_add_page() stuff is a little trickier, since it wants to know if
> >this is fs or 'generic' io. For fs io, we would like to cap the building
> >of the bio to ->max_sectors, but for eg SG_IO issued io it should go as
> >high as ->max_hw_sectors. Perhaps the easiest is just to have
> >bio_fs_add_page() and bio_pc_add_page(), each just passing in the max
> >value as an integer to bio_add_page(). But it's not exactly pretty.
> >
> >The ll_rw_blk.c merging is easy, since you don't need to do anything
> >there. It should test against ->max_sectors as it already does, since
> >this (sadly) is still the primary way we build large ios.
> >
>
> .... here, I am running into a problem. Basically, as you know the
> largest BIO we can make is 1 MB due to BIO_MAX_PAGES, and for st and sg
> we need to support commands around 6 MB, so we would have a request with
> 6 BIOs. To make this monster request I wanted to use the block layer
> functions and do something like this:
>
> + for (i = 0; i < nsegs; i++) {
> + bio = bio_map_pages(q, sg[i].page, sg[i].length,
> sg[i].offset, gfp);
> + if (IS_ERR(bio)) {
> + err = PTR_ERR(bio);
> + goto free_bios;
> + }
> + len += sg[i].length;
> +
> + bio->bi_flags &= ~(1 << BIO_SEG_VALID);
> + if (rq_data_dir(rq) == WRITE)
> + bio->bi_rw |= (1 << BIO_RW);
> + blk_queue_bounce(q, &bio);
> +
> + if (i == 0)
> + blk_rq_bio_prep(q, rq, bio);
>
> /* hope to carve out the __make_request code that does the below
> operations and make a fucntion that can be shared */
>
> + else if (!q->back_merge_fn(q, rq, bio)) {
> + err = -EINVAL;
> + bio_endio(bio, bio->bi_size, 0);
> + goto free_bios;
> + } else {
> + rq->biotail->bi_next = bio;
> + rq->biotail = bio;
> + rq->hard_nr_sectors += bio_sectors(bio);
> + rq->nr_sectors = rq->hard_nr_sectors;
>
> ........
>
> But since q->back_merge_fn() tests against q->max_sectors, it must be a
> high value so that we can merge in those BIOs. I mean if q->max_sectors
> is some reasonable number like only 1024 sectors, q->back_merge_fn will
> return a failure. Should I instead seperate ll_back_merge_fn into two
> functions, one that checks the sectors and one that checks the segments
> or if ll_back_merge_fn tested for max_hw_sectors we would be ok too?
It will take a whole lot of factoring out to make this happen I fear,
the results will not be easier for the eyes.
How about just keeping it simple - add a bio and request flag that
basically just says "don't honor soft max size" and whenever that is
set, you check for ->max_hw_sectors instead of ->max_sectors? Might even
be enough to just do this for the request and just require stuffing more
bio's into the request. But the bio has plenty of flag room left, so...
--
Jens Axboe
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH RFC] yet more struct scsi_lun
2005-11-04 7:37 ` Jens Axboe
@ 2005-11-04 17:27 ` Mike Christie
0 siblings, 0 replies; 32+ messages in thread
From: Mike Christie @ 2005-11-04 17:27 UTC (permalink / raw)
To: Jens Axboe; +Cc: Jeff Garzik, dougg, linux-scsi
Jens Axboe wrote:
> On Thu, Nov 03 2005, Mike Christie wrote:
>
>>Jens Axboe wrote:
>>
>>>On the SCSI side, I would suggest just making shost->max_sectors set
>>>q->max_hw_sectors and leave q->max_sectors to some generic kernel-wide
>>>block layer define (of course making sure that ->max_sectors <=
>>
>>If the value for this block layer define was around 16,000 sectors,
>>would that be ok? The reason I ask is becuase when I get .....
>
>
> Nope :)
Didn't think so :)
>
>
>>>->max_hw_sectors). That's the easy part.
>>>
>>>The bio_add_page() stuff is a little trickier, since it wants to know if
>>>this is fs or 'generic' io. For fs io, we would like to cap the building
>>>of the bio to ->max_sectors, but for eg SG_IO issued io it should go as
>>>high as ->max_hw_sectors. Perhaps the easiest is just to have
>>>bio_fs_add_page() and bio_pc_add_page(), each just passing in the max
>>>value as an integer to bio_add_page(). But it's not exactly pretty.
>>>
>>>The ll_rw_blk.c merging is easy, since you don't need to do anything
>>>there. It should test against ->max_sectors as it already does, since
>>>this (sadly) is still the primary way we build large ios.
>>>
>>
>>.... here, I am running into a problem. Basically, as you know the
>>largest BIO we can make is 1 MB due to BIO_MAX_PAGES, and for st and sg
>>we need to support commands around 6 MB, so we would have a request with
>> 6 BIOs. To make this monster request I wanted to use the block layer
>>functions and do something like this:
>>
>>+ for (i = 0; i < nsegs; i++) {
>>+ bio = bio_map_pages(q, sg[i].page, sg[i].length,
>>sg[i].offset, gfp);
>>+ if (IS_ERR(bio)) {
>>+ err = PTR_ERR(bio);
>>+ goto free_bios;
>>+ }
>>+ len += sg[i].length;
>>+
>>+ bio->bi_flags &= ~(1 << BIO_SEG_VALID);
>>+ if (rq_data_dir(rq) == WRITE)
>>+ bio->bi_rw |= (1 << BIO_RW);
>>+ blk_queue_bounce(q, &bio);
>>+
>>+ if (i == 0)
>>+ blk_rq_bio_prep(q, rq, bio);
>>
>>/* hope to carve out the __make_request code that does the below
>>operations and make a fucntion that can be shared */
>>
>>+ else if (!q->back_merge_fn(q, rq, bio)) {
>>+ err = -EINVAL;
>>+ bio_endio(bio, bio->bi_size, 0);
>>+ goto free_bios;
>>+ } else {
>>+ rq->biotail->bi_next = bio;
>>+ rq->biotail = bio;
>>+ rq->hard_nr_sectors += bio_sectors(bio);
>>+ rq->nr_sectors = rq->hard_nr_sectors;
>>
>>........
>>
>>But since q->back_merge_fn() tests against q->max_sectors, it must be a
>>high value so that we can merge in those BIOs. I mean if q->max_sectors
>>is some reasonable number like only 1024 sectors, q->back_merge_fn will
>>return a failure. Should I instead seperate ll_back_merge_fn into two
>>functions, one that checks the sectors and one that checks the segments
>>or if ll_back_merge_fn tested for max_hw_sectors we would be ok too?
>
>
> It will take a whole lot of factoring out to make this happen I fear,
> the results will not be easier for the eyes.
>
> How about just keeping it simple - add a bio and request flag that
> basically just says "don't honor soft max size" and whenever that is
> set, you check for ->max_hw_sectors instead of ->max_sectors? Might even
> be enough to just do this for the request and just require stuffing more
> bio's into the request. But the bio has plenty of flag room left, so...
>
ok this works, thanks.
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH RFC] even yet more struct scsi_lun
2005-10-23 4:33 [PATCH RFC] use struct scsi_lun in generic code Jeff Garzik
` (3 preceding siblings ...)
2005-10-23 7:00 ` [PATCH RFC] yet more struct scsi_lun Jeff Garzik
@ 2005-10-23 7:16 ` Jeff Garzik
2005-10-24 15:27 ` [PATCH RFC] use struct scsi_lun in generic code Patrick Mansfield
5 siblings, 0 replies; 32+ messages in thread
From: Jeff Garzik @ 2005-10-23 7:16 UTC (permalink / raw)
To: linux-scsi
And here is, perhaps, a better way to do unlimited luns.
Attempting to pass large luns through the legacy userspace interfaces
may lead to useless results.
drivers/scsi/scsi_proc.c | 2 +-
drivers/scsi/scsi_scan.c | 5 ++---
drivers/scsi/scsi_sysfs.c | 2 +-
include/scsi/scsi.h | 1 -
include/scsi/scsi_host.h | 5 +++++
5 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/scsi_proc.c b/drivers/scsi/scsi_proc.c
index 6eadb5b..dc70188 100644
--- a/drivers/scsi/scsi_proc.c
+++ b/drivers/scsi/scsi_proc.c
@@ -204,7 +204,7 @@ static int scsi_add_single_device(uint h
if (IS_ERR(shost))
return PTR_ERR(shost);
- if (lun > shost->max_lun)
+ if ((!shost->unlimited_luns) && (lun > shost->max_lun))
return -EINVAL;
int_to_scsilun(lun, &__lun);
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 09df4e7..2edf967 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1201,13 +1201,12 @@ static int scsi_report_lun_scan(struct s
* the header, so start at 1 and go up to and including num_luns.
*/
for (lunp = &lun_data[1]; lunp <= &lun_data[num_luns]; lunp++) {
- int lun;
+ unsigned int lun;
char lunstr[SCSILUN_STR_LEN];
lun = scsilun_to_int(lunp);
- if ((sdev->host->max_lun != SCSILUN_UNLIMITED) &&
- (lun > sdev->host->max_lun)) {
+ if ((!shost->unlimited_luns) && (lun > sdev->host->max_lun)) {
dev_printk(KERN_WARNING, &sdev->sdev_gendev,
"lun %s larger"
" than allowed by the host adapter\n",
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 63352fc..4c8a4bf 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -108,7 +108,7 @@ static int scsi_scan(struct Scsi_Host *s
if (check_set(&lun, s3))
return -EINVAL;
- if (lun > shost->max_lun)
+ if ((!shost->unlimited_luns) && (lun > shost->max_lun))
return -EINVAL;
int_to_scsilun(lun, &__lun);
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index 7876a64..deb1a27 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -242,7 +242,6 @@ struct scsi_lun {
__u8 scsi_lun[8];
};
#define SCSILUN_STR_LEN 33
-#define SCSILUN_UNLIMITED ~0
/*
* MESSAGE CODES
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 69313ba..e5a276c 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -553,6 +553,11 @@ struct Scsi_Host {
unsigned ordered_tag:1;
/*
+ * unlimited luns
+ */
+ unsigned unlimited_luns:1;
+
+ /*
* Optional work queue to be utilized by the transport
*/
char work_q_name[KOBJ_NAME_LEN];
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH RFC] use struct scsi_lun in generic code
2005-10-23 4:33 [PATCH RFC] use struct scsi_lun in generic code Jeff Garzik
` (4 preceding siblings ...)
2005-10-23 7:16 ` [PATCH RFC] even " Jeff Garzik
@ 2005-10-24 15:27 ` Patrick Mansfield
2005-10-24 22:40 ` Douglas Gilbert
5 siblings, 1 reply; 32+ messages in thread
From: Patrick Mansfield @ 2005-10-24 15:27 UTC (permalink / raw)
To: Jeff Garzik; +Cc: linux-scsi
Nice work Jeff ...
On Sun, Oct 23, 2005 at 12:33:01AM -0400, Jeff Garzik wrote:
> * export scsilun_to_int()
>
> * create scsilun_to_str() helper
> - if (sdev->scsi_level <= SCSI_2)
> + if (sdev->scsi_level <= SCSI_2) {
> + unsigned int tmp_lun = scsilun_to_int(&sdev->lun);
> scmd->cmnd[1] = (scmd->cmnd[1] & 0x1f) |
> - (sdev->lun << 5 & 0xe0);
> + (tmp_lun << 5 & 0xe0);
> + }
Why convert the lun value for every IO? Not efficient here and in the
driver code.
Instead call scsilun_to_int once per sdev, and store the value in
sdev->lun, sort of like James earlier patch, but only do so based on a
shost->opaque_lun_value or better if we could call a transport function
once to setup sdev->lun as needed by the driver, except we have transports
that include drivers/HBA's that use different sized LUNs.
Then struct scsi_lun has to be a u64 or another sXXX_lun(XXX).
> snprintf(sdev->sdev_classdev.class_id, BUS_ID_SIZE,
> - "%d:%d:%d:%d", sdev->host->host_no,
> - sdev_channel(sdev), sdev_id(sdev), sdev->lun);
> + "%d:%d:%d:%s", sdev->host->host_no,
> + sdev_channel(sdev), sdev_id(sdev),
> + scsilun_to_str(sdev, lunstr));
BUS_ID_SIZE is KOBJ_NAME_LEN is still 20. I thought we were moving to hex
for the bus_id value?
> +static inline char *scsilun_to_str(struct scsi_device *sdev, char *s)
> +{
> + sprintf(s, "%d", scsilun_to_int(&sdev->lun));
> + return s;
And format the LUN output based on the shost->opaque_luns. IMO for
!opaque_lun_value print LUN in the same order as today, else print 8 byte
hex string.
Some (probably most) users of drivers/transports with 8 byte LUN support
will be confused one way or another. Yes, most will expect to see 1 for
LUN 0001000000000000. We can't always display the LUN the same as used by
storage vendors in their setup/administration tools.
Perhaps base parsing of user space LUN values on the shost->opaque_lun_value,
I don't know how we can support 8 byte LUNs from user space and be
backwards compatible without adding another attribute or interface.
Note that current SAM (sam4r02) sort of allows for different sized LUNs,
it says "A logical unit number shall contain 64 bits or 16 bits, with the
size being defined by the SCSI transport protocol. For SCSI transport
protocols that define 16-bit logical unit numbers, the two bytes shall be
formatted as described for the FIRST LEVEL ADDRESSING field (see table 5
in 4.9.5)."
-- Patrick Mansfield
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH RFC] use struct scsi_lun in generic code
2005-10-24 15:27 ` [PATCH RFC] use struct scsi_lun in generic code Patrick Mansfield
@ 2005-10-24 22:40 ` Douglas Gilbert
0 siblings, 0 replies; 32+ messages in thread
From: Douglas Gilbert @ 2005-10-24 22:40 UTC (permalink / raw)
To: Patrick Mansfield; +Cc: Jeff Garzik, linux-scsi
Patrick Mansfield wrote:
<snip>
> Note that current SAM (sam4r02) sort of allows for different sized LUNs,
> it says "A logical unit number shall contain 64 bits or 16 bits, with the
> size being defined by the SCSI transport protocol. For SCSI transport
> protocols that define 16-bit logical unit numbers, the two bytes shall be
> formatted as described for the FIRST LEVEL ADDRESSING field (see table 5
> in 4.9.5)."
The "or 16 bits" was added to SAM after it was pointed
out that a new standard was recently approved that mandated
16 bit luns. That is SBP-3 (i.e. IEEE 1394 transport). All
other "current" transports shown in sam4r03.pdf, Annex A
are 64 bits (with a note on "SPI-5").
Aside: Annex A shows SPI-5 (Ultra 640) as the "current"
SPI standard. This is an example of where standards (or
drafts) diverge from reality :-)
Doug Gilbert
^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2005-11-04 18:27 UTC | newest]
Thread overview: 32+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2005-10-23 4:33 [PATCH RFC] use struct scsi_lun in generic code Jeff Garzik
2005-10-23 4:49 ` [PATCH RFC] more struct scsi_lun Jeff Garzik
2005-10-23 9:47 ` Stefan Richter
2005-10-23 13:14 ` Stefan Richter
2005-10-23 16:49 ` Jeff Garzik
2005-10-24 16:27 ` Luben Tuikov
2005-10-24 20:03 ` Stefan Richter
2005-10-24 20:10 ` Luben Tuikov
2005-10-24 20:28 ` Mark Rustad
2005-10-24 22:27 ` Douglas Gilbert
2005-10-23 5:20 ` [PATCH RFC] use struct scsi_lun in generic code Jeff Garzik
2005-10-23 5:22 ` Douglas Gilbert
2005-10-23 7:01 ` Jeff Garzik
2005-10-24 14:55 ` Luben Tuikov
2005-10-23 7:00 ` [PATCH RFC] yet more struct scsi_lun Jeff Garzik
2005-10-23 10:48 ` Douglas Gilbert
2005-10-23 11:53 ` Arjan van de Ven
2005-10-23 14:27 ` max_sectors [was Re: [PATCH RFC] yet more struct scsi_lun] Douglas Gilbert
2005-10-23 14:42 ` Arjan van de Ven
2005-10-23 16:44 ` [PATCH RFC] yet more struct scsi_lun Jeff Garzik
2005-10-23 16:43 ` Jeff Garzik
2005-10-23 18:53 ` Kai Makisara
2005-10-24 7:59 ` Jens Axboe
2005-10-28 17:24 ` Mike Christie
2005-10-31 10:24 ` Jens Axboe
2005-11-04 2:23 ` Mike Christie
2005-11-04 2:25 ` Mike Christie
2005-11-04 7:37 ` Jens Axboe
2005-11-04 17:27 ` Mike Christie
2005-10-23 7:16 ` [PATCH RFC] even " Jeff Garzik
2005-10-24 15:27 ` [PATCH RFC] use struct scsi_lun in generic code Patrick Mansfield
2005-10-24 22:40 ` Douglas Gilbert
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).