2009-04-22 09:09:57

by Jeff Garzik

[permalink] [raw]
Subject: [PATCH] libata: rewrite SCSI host scheme to be one per ATA host


Currently, libata creates a Scsi_Host per port. This was originally
done to leverage SCSI's infrastructure to arbitrate among master/slave
devices, but is not needed for most modern SATA controllers. And I
_think_ it is not needed for master/slave if done properly, either.

The patch below converts libata such that there is now a 1:1
correspondence between struct Scsi_Host and struct ata_host. ATA ports
are represented as SCSI layer 'channels', which is more natural.

This patch is an experiment, and not meant for upstream anytime soon.
I just wanted to see what kind of effort would be required to get it
to work.

I was able to successfully boot the following patch on
AHCI/x86-64/Fedora.

It may work with other controllers -- TRY AT YOUR OWN RISK. It will
probably fail for master/slave configurations, and SAS & PMP also
need looking at. It yielded this lsscsi output on my AHCI box:

[0:0:0:0] disk ATA ST3500320AS SD15 /dev/sda
[0:2:0:0] disk ATA G.SKILL 128GB SS 02.1 /dev/sdb
[0:5:0:0] cd/dvd PIONEER BD-ROM BDC-202 1.04 /dev/sr0

NOT-signed-off-by: me

drivers/ata/ahci.c | 4
drivers/ata/libata-core.c | 17 +--
drivers/ata/libata-eh.c | 47 ++++++--
drivers/ata/libata-scsi.c | 237 +++++++++++++++++++++---------------------
drivers/ata/libata.h | 3
drivers/scsi/libsas/sas_ata.c | 2
include/linux/libata.h | 9 -
7 files changed, 184 insertions(+), 135 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 08186ec..b0468a8 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -326,14 +326,18 @@ static ssize_t ahci_activity_store(struct ata_device *dev,
static void ahci_init_sw_activity(struct ata_link *link);

static struct device_attribute *ahci_shost_attrs[] = {
+#if 0
&dev_attr_link_power_management_policy,
&dev_attr_em_message_type,
&dev_attr_em_message,
+#endif
NULL
};

static struct device_attribute *ahci_sdev_attrs[] = {
+#if 0
&dev_attr_sw_activity,
+#endif
&dev_attr_unload_heads,
NULL
};
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 17c5d48..71f32dc 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -95,6 +95,7 @@ static void ata_dev_xfermask(struct ata_device *dev);
static unsigned long ata_dev_blacklisted(const struct ata_device *dev);

unsigned int ata_print_id = 1;
+unsigned int ata_host_print_id = 1;
static struct workqueue_struct *ata_wq;

struct workqueue_struct *ata_aux_wq;
@@ -2308,7 +2309,7 @@ static void ata_dev_config_ncq(struct ata_device *dev,
return;
}
if (ap->flags & ATA_FLAG_NCQ) {
- hdepth = min(ap->scsi_host->can_queue, ATA_MAX_QUEUE - 1);
+ hdepth = min(ap->host->scsi_host->can_queue, ATA_MAX_QUEUE - 1);
dev->flags |= ATA_DFLAG_NCQ;
}

@@ -5635,15 +5636,15 @@ static void ata_host_release(struct device *gendev, void *res)
if (!ap)
continue;

- if (ap->scsi_host)
- scsi_host_put(ap->scsi_host);
-
kfree(ap->pmp_link);
kfree(ap->slave_link);
kfree(ap);
host->ports[i] = NULL;
}

+ if (host->scsi_host)
+ scsi_host_put(host->scsi_host);
+
dev_set_drvdata(gendev, NULL);
}

@@ -6089,6 +6090,8 @@ int ata_host_register(struct ata_host *host, struct scsi_host_template *sht)
for (i = 0; i < host->n_ports; i++)
host->ports[i]->print_id = ata_print_id++;

+ host->print_id = ata_host_print_id++;
+
rc = ata_scsi_add_hosts(host, sht);
if (rc)
return rc;
@@ -6222,8 +6225,7 @@ static void ata_port_detach(struct ata_port *ap)
cancel_rearming_delayed_work(&ap->hotplug_task);

skip_eh:
- /* remove the associated SCSI host */
- scsi_remove_host(ap->scsi_host);
+ return;
}

/**
@@ -6242,6 +6244,9 @@ void ata_host_detach(struct ata_host *host)
for (i = 0; i < host->n_ports; i++)
ata_port_detach(host->ports[i]);

+ /* remove the associated SCSI host */
+ scsi_remove_host(host->scsi_host);
+
/* the host is dead now, dissociate ACPI */
ata_acpi_dissociate(host);
}
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 0183131..db8a66f 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -466,14 +466,22 @@ static void ata_eh_clear_action(struct ata_link *link, struct ata_device *dev,
*/
enum blk_eh_timer_return ata_scsi_timed_out(struct scsi_cmnd *cmd)
{
- struct Scsi_Host *host = cmd->device->host;
- struct ata_port *ap = ata_shost_to_port(host);
+ struct Scsi_Host *shost = cmd->device->host;
+ struct ata_port *ap;
+ struct ata_host *host;
+ struct ata_device *atadev;
unsigned long flags;
struct ata_queued_cmd *qc;
- enum blk_eh_timer_return ret;
+ enum blk_eh_timer_return ret = BLK_EH_NOT_HANDLED;

DPRINTK("ENTER\n");

+ host = ata_shost_to_host(shost);
+ atadev = ata_scsi_find_dev(host, cmd->device);
+ if (!atadev)
+ goto out;
+ ap = atadev->link->ap;
+
if (ap->ops->error_handler) {
ret = BLK_EH_NOT_HANDLED;
goto out;
@@ -532,13 +540,12 @@ static void ata_eh_unload(struct ata_port *ap)
* RETURNS:
* Zero.
*/
-void ata_scsi_error(struct Scsi_Host *host)
+static void __ata_scsi_error(struct Scsi_Host *shost, struct ata_port *ap)
{
- struct ata_port *ap = ata_shost_to_port(host);
int i;
unsigned long flags;

- DPRINTK("ENTER\n");
+ DPRINTK("ENTER, port_no %u\n", ap->port_no);

/* synchronize with port task */
ata_port_flush_task(ap);
@@ -575,7 +582,7 @@ void ata_scsi_error(struct Scsi_Host *host)
if (ap->ops->lost_interrupt)
ap->ops->lost_interrupt(ap);

- list_for_each_entry_safe(scmd, tmp, &host->eh_cmd_q, eh_entry) {
+ list_for_each_entry_safe(scmd, tmp, &shost->eh_cmd_q, eh_entry) {
struct ata_queued_cmd *qc;

for (i = 0; i < ATA_MAX_QUEUE; i++) {
@@ -698,7 +705,7 @@ void ata_scsi_error(struct Scsi_Host *host)
* before EH completion, SCSI midlayer will
* re-initiate EH.
*/
- host->host_eh_scheduled = 0;
+ shost->host_eh_scheduled = 0;

spin_unlock_irqrestore(ap->lock, flags);
} else {
@@ -707,7 +714,7 @@ void ata_scsi_error(struct Scsi_Host *host)
}

/* finish or retry handled scmd's and clean up */
- WARN_ON(host->host_failed || !list_empty(&host->eh_cmd_q));
+ WARN_ON(shost->host_failed || !list_empty(&shost->eh_cmd_q));

scsi_eh_flush_done_q(&ap->eh_done_q);

@@ -733,6 +740,24 @@ void ata_scsi_error(struct Scsi_Host *host)
DPRINTK("EXIT\n");
}

+void ata_scsi_error(struct Scsi_Host *shost)
+{
+ struct ata_host *host = ata_shost_to_host(shost);
+ unsigned int i;
+
+ DPRINTK("ENTER\n");
+
+ for (i = 0; i < host->n_ports; i++) {
+ struct ata_port *ap = host->ports[i];
+
+ if (ap->pflags & ATA_PFLAG_EH_PENDING)
+ __ata_scsi_error(shost, ap);
+ }
+
+ DPRINTK("EXIT\n");
+}
+
+
/**
* ata_port_wait_eh - Wait for the currently pending EH to complete
* @ap: Port to wait EH for
@@ -761,7 +786,7 @@ void ata_port_wait_eh(struct ata_port *ap)
spin_unlock_irqrestore(ap->lock, flags);

/* make sure SCSI EH is complete */
- if (scsi_host_in_recovery(ap->scsi_host)) {
+ if (scsi_host_in_recovery(ap->host->scsi_host)) {
msleep(10);
goto retry;
}
@@ -901,7 +926,7 @@ void ata_port_schedule_eh(struct ata_port *ap)
return;

ata_eh_set_pending(ap, 1);
- scsi_schedule_eh(ap->scsi_host);
+ scsi_schedule_eh(ap->host->scsi_host);

DPRINTK("port EH scheduled\n");
}
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 2733b0c..7aa6aa6 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -58,10 +58,8 @@ static u8 ata_scsi_rbuf[ATA_SCSI_RBUF_SIZE];

typedef unsigned int (*ata_xlat_func_t)(struct ata_queued_cmd *qc);

-static struct ata_device *__ata_scsi_find_dev(struct ata_port *ap,
+static struct ata_device *__ata_scsi_find_dev(struct ata_host *,
const struct scsi_device *scsidev);
-static struct ata_device *ata_scsi_find_dev(struct ata_port *ap,
- const struct scsi_device *scsidev);
static int ata_scsi_user_scan(struct Scsi_Host *shost, unsigned int channel,
unsigned int id, unsigned int lun);

@@ -136,6 +134,7 @@ static const char *ata_scsi_lpm_get(enum link_pm policy)
return NULL;
}

+#if 0
static ssize_t ata_scsi_lpm_put(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
@@ -183,6 +182,7 @@ ata_scsi_lpm_show(struct device *dev, struct device_attribute *attr, char *buf)
DEVICE_ATTR(link_power_management_policy, S_IRUGO | S_IWUSR,
ata_scsi_lpm_show, ata_scsi_lpm_put);
EXPORT_SYMBOL_GPL(dev_attr_link_power_management_policy);
+#endif

static ssize_t ata_scsi_park_show(struct device *device,
struct device_attribute *attr, char *buf)
@@ -190,19 +190,21 @@ static ssize_t ata_scsi_park_show(struct device *device,
struct scsi_device *sdev = to_scsi_device(device);
struct ata_port *ap;
struct ata_link *link;
+ struct ata_host *host;
struct ata_device *dev;
unsigned long flags, now;
unsigned int uninitialized_var(msecs);
int rc = 0;

- ap = ata_shost_to_port(sdev->host);
+ host = ata_shost_to_host(sdev->host);

- spin_lock_irqsave(ap->lock, flags);
- dev = ata_scsi_find_dev(ap, sdev);
+ spin_lock_irqsave(&host->lock, flags);
+ dev = ata_scsi_find_dev(host, sdev);
if (!dev) {
rc = -ENODEV;
goto unlock;
}
+ ap = dev->link->ap;
if (dev->flags & ATA_DFLAG_NO_UNLOAD) {
rc = -EOPNOTSUPP;
goto unlock;
@@ -218,7 +220,7 @@ static ssize_t ata_scsi_park_show(struct device *device,
msecs = 0;

unlock:
- spin_unlock_irq(ap->lock);
+ spin_unlock_irq(&host->lock);

return rc ? rc : snprintf(buf, 20, "%u\n", msecs);
}
@@ -230,6 +232,7 @@ static ssize_t ata_scsi_park_store(struct device *device,
struct scsi_device *sdev = to_scsi_device(device);
struct ata_port *ap;
struct ata_device *dev;
+ struct ata_host *host;
long int input;
unsigned long flags;
int rc;
@@ -242,14 +245,15 @@ static ssize_t ata_scsi_park_store(struct device *device,
input = ATA_TMOUT_MAX_PARK;
}

- ap = ata_shost_to_port(sdev->host);
+ host = ata_shost_to_host(sdev->host);

- spin_lock_irqsave(ap->lock, flags);
- dev = ata_scsi_find_dev(ap, sdev);
+ spin_lock_irqsave(&host->lock, flags);
+ dev = ata_scsi_find_dev(host, sdev);
if (unlikely(!dev)) {
rc = -ENODEV;
goto unlock;
}
+ ap = dev->link->ap;
if (dev->class != ATA_DEV_ATA) {
rc = -EOPNOTSUPP;
goto unlock;
@@ -276,7 +280,7 @@ static ssize_t ata_scsi_park_store(struct device *device,
}
}
unlock:
- spin_unlock_irqrestore(ap->lock, flags);
+ spin_unlock_irqrestore(&host->lock, flags);

return rc ? rc : len;
}
@@ -291,6 +295,7 @@ static void ata_scsi_set_sense(struct scsi_cmnd *cmd, u8 sk, u8 asc, u8 ascq)
scsi_build_sense_buffer(0, cmd->sense_buffer, sk, asc, ascq);
}

+#if 0
static ssize_t
ata_scsi_em_message_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
@@ -335,8 +340,9 @@ ata_scsi_activity_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
struct scsi_device *sdev = to_scsi_device(dev);
- struct ata_port *ap = ata_shost_to_port(sdev->host);
- struct ata_device *atadev = ata_scsi_find_dev(ap, sdev);
+ struct ata_host *host = ata_shost_to_host(sdev->host);
+ struct ata_device *atadev = ata_scsi_find_dev(host, sdev);
+ struct ata_port *ap = atadev->ap;

if (ap->ops->sw_activity_show && (ap->flags & ATA_FLAG_SW_ACTIVITY))
return ap->ops->sw_activity_show(atadev, buf);
@@ -348,8 +354,9 @@ ata_scsi_activity_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
struct scsi_device *sdev = to_scsi_device(dev);
- struct ata_port *ap = ata_shost_to_port(sdev->host);
- struct ata_device *atadev = ata_scsi_find_dev(ap, sdev);
+ struct ata_host *host = ata_shost_to_host(sdev->host);
+ struct ata_device *atadev = ata_scsi_find_dev(host, sdev);
+ struct ata_port *ap = atadev->ap;
enum sw_activity val;
int rc;

@@ -369,6 +376,7 @@ ata_scsi_activity_store(struct device *dev, struct device_attribute *attr,
DEVICE_ATTR(sw_activity, S_IWUGO | S_IRUGO, ata_scsi_activity_show,
ata_scsi_activity_store);
EXPORT_SYMBOL_GPL(dev_attr_sw_activity);
+#endif

struct device_attribute *ata_common_sdev_attrs[] = {
&dev_attr_unload_heads,
@@ -425,10 +433,10 @@ int ata_std_bios_param(struct scsi_device *sdev, struct block_device *bdev,
* RETURNS:
* Zero on success, negative errno on error.
*/
-static int ata_get_identity(struct ata_port *ap, struct scsi_device *sdev,
+static int ata_get_identity(struct ata_host *host, struct scsi_device *sdev,
void __user *arg)
{
- struct ata_device *dev = ata_scsi_find_dev(ap, sdev);
+ struct ata_device *dev = ata_scsi_find_dev(host, sdev);
u16 __user *dst = arg;
char buf[40];

@@ -656,11 +664,18 @@ static int ata_ioc32(struct ata_port *ap)
return 0;
}

-int ata_sas_scsi_ioctl(struct ata_port *ap, struct scsi_device *scsidev,
+int ata_sas_scsi_ioctl(struct ata_host *host, struct scsi_device *scsidev,
int cmd, void __user *arg)
{
int val = -EINVAL, rc = -EINVAL;
unsigned long flags;
+ struct ata_port *ap;
+ struct ata_device *atadev;
+
+ atadev = ata_scsi_find_dev(host, scsidev);
+ if (!atadev)
+ return -ENOENT;
+ ap = atadev->link->ap;

switch (cmd) {
case ATA_IOC_GET_IO32:
@@ -688,7 +703,7 @@ int ata_sas_scsi_ioctl(struct ata_port *ap, struct scsi_device *scsidev,
return rc;

case HDIO_GET_IDENTITY:
- return ata_get_identity(ap, scsidev, arg);
+ return ata_get_identity(host, scsidev, arg);

case HDIO_DRIVE_CMD:
if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
@@ -711,7 +726,7 @@ EXPORT_SYMBOL_GPL(ata_sas_scsi_ioctl);

int ata_scsi_ioctl(struct scsi_device *scsidev, int cmd, void __user *arg)
{
- return ata_sas_scsi_ioctl(ata_shost_to_port(scsidev->host),
+ return ata_sas_scsi_ioctl(ata_shost_to_host(scsidev->host),
scsidev, cmd, arg);
}
EXPORT_SYMBOL_GPL(ata_scsi_ioctl);
@@ -1157,8 +1172,8 @@ static int ata_scsi_dev_config(struct scsi_device *sdev,

int ata_scsi_slave_config(struct scsi_device *sdev)
{
- struct ata_port *ap = ata_shost_to_port(sdev->host);
- struct ata_device *dev = __ata_scsi_find_dev(ap, sdev);
+ struct ata_host *host = ata_shost_to_host(sdev->host);
+ struct ata_device *dev = __ata_scsi_find_dev(host, sdev);
int rc = 0;

ata_scsi_sdev_config(sdev);
@@ -1185,27 +1200,34 @@ int ata_scsi_slave_config(struct scsi_device *sdev)
*/
void ata_scsi_slave_destroy(struct scsi_device *sdev)
{
- struct ata_port *ap = ata_shost_to_port(sdev->host);
+ struct ata_host *host = ata_shost_to_host(sdev->host);
struct request_queue *q = sdev->request_queue;
unsigned long flags;
struct ata_device *dev;
+ bool new_eh = true;

- if (!ap->ops->error_handler)
- return;
-
- spin_lock_irqsave(ap->lock, flags);
- dev = __ata_scsi_find_dev(ap, sdev);
+ spin_lock_irqsave(&host->lock, flags);
+ dev = __ata_scsi_find_dev(host, sdev);
if (dev && dev->sdev) {
+ struct ata_port *ap = dev->link->ap;
+
+ if (!ap->ops->error_handler)
+ new_eh = false;
+
+ else {
/* SCSI device already in CANCEL state, no need to offline it */
dev->sdev = NULL;
dev->flags |= ATA_DFLAG_DETACH;
ata_port_schedule_eh(ap);
+ }
}
- spin_unlock_irqrestore(ap->lock, flags);
+ spin_unlock_irqrestore(&host->lock, flags);

- kfree(q->dma_drain_buffer);
- q->dma_drain_buffer = NULL;
- q->dma_drain_size = 0;
+ if (new_eh) {
+ kfree(q->dma_drain_buffer);
+ q->dma_drain_buffer = NULL;
+ q->dma_drain_size = 0;
+ }
}

/**
@@ -1225,25 +1247,25 @@ void ata_scsi_slave_destroy(struct scsi_device *sdev)
*/
int ata_scsi_change_queue_depth(struct scsi_device *sdev, int queue_depth)
{
- struct ata_port *ap = ata_shost_to_port(sdev->host);
+ struct ata_host *host = ata_shost_to_host(sdev->host);
struct ata_device *dev;
unsigned long flags;

if (queue_depth < 1 || queue_depth == sdev->queue_depth)
return sdev->queue_depth;

- dev = ata_scsi_find_dev(ap, sdev);
+ dev = ata_scsi_find_dev(host, sdev);
if (!dev || !ata_dev_enabled(dev))
return sdev->queue_depth;

/* NCQ enabled? */
- spin_lock_irqsave(ap->lock, flags);
+ spin_lock_irqsave(&host->lock, flags);
dev->flags &= ~ATA_DFLAG_NCQ_OFF;
if (queue_depth == 1 || !ata_ncq_enabled(dev)) {
dev->flags |= ATA_DFLAG_NCQ_OFF;
queue_depth = 1;
}
- spin_unlock_irqrestore(ap->lock, flags);
+ spin_unlock_irqrestore(&host->lock, flags);

/* limit and apply queue depth */
queue_depth = min(queue_depth, sdev->host->can_queue);
@@ -2690,21 +2712,20 @@ static struct ata_device *ata_find_dev(struct ata_port *ap, int devno)
return NULL;
}

-static struct ata_device *__ata_scsi_find_dev(struct ata_port *ap,
+static struct ata_device *__ata_scsi_find_dev(struct ata_host *host,
const struct scsi_device *scsidev)
{
int devno;
+ struct ata_port *ap;

- /* skip commands not addressed to targets we simulate */
- if (!sata_pmp_attached(ap)) {
- if (unlikely(scsidev->channel || scsidev->lun))
- return NULL;
- devno = scsidev->id;
- } else {
- if (unlikely(scsidev->id || scsidev->lun))
- return NULL;
- devno = scsidev->channel;
- }
+ if (scsidev->channel > host->n_ports)
+ return NULL;
+
+ ap = host->ports[scsidev->channel];
+ if (ata_port_is_dummy(ap))
+ return NULL;
+
+ devno = scsidev->id;

return ata_find_dev(ap, devno);
}
@@ -2725,10 +2746,10 @@ static struct ata_device *__ata_scsi_find_dev(struct ata_port *ap,
* RETURNS:
* Associated ATA device, or %NULL if not found.
*/
-static struct ata_device *
-ata_scsi_find_dev(struct ata_port *ap, const struct scsi_device *scsidev)
+struct ata_device *
+ata_scsi_find_dev(struct ata_host *host, const struct scsi_device *scsidev)
{
- struct ata_device *dev = __ata_scsi_find_dev(ap, scsidev);
+ struct ata_device *dev = __ata_scsi_find_dev(host, scsidev);

if (unlikely(!dev || !ata_dev_enabled(dev)))
return NULL;
@@ -2983,15 +3004,13 @@ static inline ata_xlat_func_t ata_get_xlat_func(struct ata_device *dev, u8 cmd)
* Prints the contents of a SCSI command via printk().
*/

-static inline void ata_scsi_dump_cdb(struct ata_port *ap,
- struct scsi_cmnd *cmd)
+static inline void ata_scsi_dump_cdb(struct scsi_cmnd *cmd)
{
#ifdef ATA_DEBUG
struct scsi_device *scsidev = cmd->device;
u8 *scsicmd = cmd->cmnd;

- DPRINTK("CDB (%u:%d,%d,%d) %02x %02x %02x %02x %02x %02x %02x %02x %02x\n",
- ap->print_id,
+ DPRINTK("CDB (%d,%d,%d) %02x %02x %02x %02x %02x %02x %02x %02x %02x\n",
scsidev->channel, scsidev->id, scsidev->lun,
scsicmd[0], scsicmd[1], scsicmd[2], scsicmd[3],
scsicmd[4], scsicmd[5], scsicmd[6], scsicmd[7],
@@ -3069,20 +3088,20 @@ static inline int __ata_scsi_queuecmd(struct scsi_cmnd *scmd,
*/
int ata_scsi_queuecmd(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *))
{
- struct ata_port *ap;
struct ata_device *dev;
+ struct ata_host *host;
struct scsi_device *scsidev = cmd->device;
struct Scsi_Host *shost = scsidev->host;
int rc = 0;

- ap = ata_shost_to_port(shost);
+ host = ata_shost_to_host(shost);

spin_unlock(shost->host_lock);
- spin_lock(ap->lock);
+ spin_lock(&host->lock);

- ata_scsi_dump_cdb(ap, cmd);
+ ata_scsi_dump_cdb(cmd);

- dev = ata_scsi_find_dev(ap, scsidev);
+ dev = ata_scsi_find_dev(host, scsidev);
if (likely(dev))
rc = __ata_scsi_queuecmd(cmd, done, dev);
else {
@@ -3090,7 +3109,7 @@ int ata_scsi_queuecmd(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *))
done(cmd);
}

- spin_unlock(ap->lock);
+ spin_unlock(&host->lock);
spin_lock(shost->host_lock);
return rc;
}
@@ -3217,56 +3236,43 @@ void ata_scsi_simulate(struct ata_device *dev, struct scsi_cmnd *cmd,

int ata_scsi_add_hosts(struct ata_host *host, struct scsi_host_template *sht)
{
- int i, rc;
-
- for (i = 0; i < host->n_ports; i++) {
- struct ata_port *ap = host->ports[i];
- struct Scsi_Host *shost;
+ int rc;
+ struct Scsi_Host *shost;

- rc = -ENOMEM;
- shost = scsi_host_alloc(sht, sizeof(struct ata_port *));
- if (!shost)
- goto err_alloc;
+ shost = scsi_host_alloc(sht, sizeof(struct ata_host *));
+ if (!shost)
+ return -ENOMEM;

- *(struct ata_port **)&shost->hostdata[0] = ap;
- ap->scsi_host = shost;
+ *(struct ata_host **)&shost->hostdata[0] = host;
+ host->scsi_host = shost;

- shost->transportt = &ata_scsi_transport_template;
- shost->unique_id = ap->print_id;
- shost->max_id = 16;
- shost->max_lun = 1;
- shost->max_channel = 1;
- shost->max_cmd_len = 16;
+ shost->transportt = &ata_scsi_transport_template;
+ shost->unique_id = host->print_id;
+ shost->max_id = 32;
+ shost->max_lun = 1;
+ shost->max_channel = 32;
+ shost->max_cmd_len = 16;

- /* Schedule policy is determined by ->qc_defer()
- * callback and it needs to see every deferred qc.
- * Set host_blocked to 1 to prevent SCSI midlayer from
- * automatically deferring requests.
- */
- shost->max_host_blocked = 1;
+ /* Schedule policy is determined by ->qc_defer()
+ * callback and it needs to see every deferred qc.
+ * Set host_blocked to 1 to prevent SCSI midlayer from
+ * automatically deferring requests.
+ */
+ shost->max_host_blocked = 1;

- rc = scsi_add_host(ap->scsi_host, ap->host->dev);
- if (rc)
- goto err_add;
+ rc = scsi_add_host(shost, host->dev);
+ if (rc) {
+ scsi_host_put(shost);
+ return rc;
}

return 0;
-
- err_add:
- scsi_host_put(host->ports[i]->scsi_host);
- err_alloc:
- while (--i >= 0) {
- struct Scsi_Host *shost = host->ports[i]->scsi_host;
-
- scsi_remove_host(shost);
- scsi_host_put(shost);
- }
- return rc;
}

void ata_scsi_scan_host(struct ata_port *ap, int sync)
{
int tries = 5;
+ struct ata_host *host = ap->host;
struct ata_device *last_failed_dev = NULL;
struct ata_link *link;
struct ata_device *dev;
@@ -3278,7 +3284,7 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
ata_for_each_link(link, ap, EDGE) {
ata_for_each_dev(dev, link, ENABLED) {
struct scsi_device *sdev;
- int channel = 0, id = 0;
+ int id = 0;

if (dev->sdev)
continue;
@@ -3286,9 +3292,10 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
if (ata_is_host_link(link))
id = dev->devno;
else
- channel = link->pmp;
+ id = link->pmp;

- sdev = __scsi_add_device(ap->scsi_host, channel, id, 0,
+ sdev = __scsi_add_device(host->scsi_host,
+ ap->port_no, id, 0,
NULL);
if (!IS_ERR(sdev)) {
dev->sdev = sdev;
@@ -3376,6 +3383,7 @@ int ata_scsi_offline_dev(struct ata_device *dev)
static void ata_scsi_remove_dev(struct ata_device *dev)
{
struct ata_port *ap = dev->link->ap;
+ struct ata_host *host = ap->host;
struct scsi_device *sdev;
unsigned long flags;

@@ -3385,7 +3393,7 @@ static void ata_scsi_remove_dev(struct ata_device *dev)
* be removed if there is __scsi_device_get() interface which
* increments reference counts regardless of device state.
*/
- mutex_lock(&ap->scsi_host->scan_mutex);
+ mutex_lock(&host->scsi_host->scan_mutex);
spin_lock_irqsave(ap->lock, flags);

/* clearing dev->sdev is protected by host lock */
@@ -3411,7 +3419,7 @@ static void ata_scsi_remove_dev(struct ata_device *dev)
}

spin_unlock_irqrestore(ap->lock, flags);
- mutex_unlock(&ap->scsi_host->scan_mutex);
+ mutex_unlock(&host->scsi_host->scan_mutex);

if (sdev) {
ata_dev_printk(dev, KERN_INFO, "detaching (SCSI %s)\n",
@@ -3517,25 +3525,28 @@ void ata_scsi_hotplug(struct work_struct *work)
static int ata_scsi_user_scan(struct Scsi_Host *shost, unsigned int channel,
unsigned int id, unsigned int lun)
{
- struct ata_port *ap = ata_shost_to_port(shost);
+ struct ata_port *ap;
+ struct ata_host *host = ata_shost_to_host(shost);
unsigned long flags;
int devno, rc = 0;

+ if (channel == SCAN_WILD_CARD)
+ ap = host->ports[0];
+ else {
+ if (channel > host->n_ports)
+ return -EINVAL;
+ ap = host->ports[channel];
+ }
+
if (!ap->ops->error_handler)
return -EOPNOTSUPP;

if (lun != SCAN_WILD_CARD && lun)
return -EINVAL;

- if (!sata_pmp_attached(ap)) {
- if (channel != SCAN_WILD_CARD && channel)
- return -EINVAL;
- devno = id;
- } else {
- if (id != SCAN_WILD_CARD && id)
- return -EINVAL;
- devno = channel;
- }
+ if (id != SCAN_WILD_CARD && id)
+ return -EINVAL;
+ devno = id;

spin_lock_irqsave(ap->lock, flags);

@@ -3749,7 +3760,7 @@ int ata_sas_queuecmd(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *),
{
int rc = 0;

- ata_scsi_dump_cdb(ap, cmd);
+ ata_scsi_dump_cdb(cmd);

if (likely(ata_dev_enabled(ap->link.device)))
rc = __ata_scsi_queuecmd(cmd, done, ap->link.device);
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 89a1e00..af890b4 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -205,4 +205,7 @@ extern u8 ata_irq_on(struct ata_port *ap);
extern void ata_pio_task(struct work_struct *work);
#endif /* CONFIG_ATA_SFF */

+struct ata_device *
+ata_scsi_find_dev(struct ata_host *host, const struct scsi_device *scsidev);
+
#endif /* __LIBATA_H__ */
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index e155011..758df0b 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -375,6 +375,7 @@ int sas_ata_init_host_and_port(struct domain_device *found_dev,
ha->dev,
sata_port_info.flags,
&sas_sata_ops);
+ found_dev->sata_dev.ata_host.scsi_host = shost;
ap = ata_sas_port_alloc(&found_dev->sata_dev.ata_host,
&sata_port_info,
shost);
@@ -385,7 +386,6 @@ int sas_ata_init_host_and_port(struct domain_device *found_dev,

ap->private_data = found_dev;
ap->cbl = ATA_CBL_SATA;
- ap->scsi_host = shost;
found_dev->sata_dev.ap = ap;

return 0;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 3d501db..2ea4dce 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -507,10 +507,12 @@ struct ata_ioports {
#endif /* CONFIG_ATA_SFF */

struct ata_host {
+ struct Scsi_Host *scsi_host; /* our co-allocated scsi host */
spinlock_t lock;
struct device *dev;
void __iomem * const *iomap;
unsigned int n_ports;
+ unsigned int print_id; /* user visible unique host ID */
void *private_data;
struct ata_port_operations *ops;
unsigned long flags;
@@ -690,7 +692,6 @@ struct ata_link {
};

struct ata_port {
- struct Scsi_Host *scsi_host; /* our co-allocated scsi host */
struct ata_port_operations *ops;
spinlock_t *lock;
/* Flags owned by the EH context. Only EH should touch these once the
@@ -947,7 +948,7 @@ extern void ata_host_init(struct ata_host *, struct device *,
extern int ata_scsi_detect(struct scsi_host_template *sht);
extern int ata_scsi_ioctl(struct scsi_device *dev, int cmd, void __user *arg);
extern int ata_scsi_queuecmd(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *));
-extern int ata_sas_scsi_ioctl(struct ata_port *ap, struct scsi_device *dev,
+extern int ata_sas_scsi_ioctl(struct ata_host *, struct scsi_device *dev,
int cmd, void __user *arg);
extern void ata_sas_port_destroy(struct ata_port *);
extern struct ata_port *ata_sas_port_alloc(struct ata_host *,
@@ -1472,9 +1473,9 @@ static inline unsigned int __ac_err_mask(u8 status)
return mask;
}

-static inline struct ata_port *ata_shost_to_port(struct Scsi_Host *host)
+static inline struct ata_host *ata_shost_to_host(struct Scsi_Host *host)
{
- return *(struct ata_port **)&host->hostdata[0];
+ return *(struct ata_host **)&host->hostdata[0];
}

static inline int ata_check_ready(u8 status)


Attachments:
(No filename) (26.80 kB)
dmesg.txt (42.74 kB)
Download all attachments

2009-04-22 09:24:24

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] libata: rewrite SCSI host scheme to be one per ATA host

Jeff Garzik wrote:
> Currently, libata creates a Scsi_Host per port. This was originally
> done to leverage SCSI's infrastructure to arbitrate among master/slave
> devices, but is not needed for most modern SATA controllers. And I
> _think_ it is not needed for master/slave if done properly, either.

BTW note the above, with regards to the libata SCSI->block conversion.
libata currently relies on SCSI for some amount of generic device
arbitration, in several situations (see ->qc_defer,
SCSI_MLQUEUE_.*_BUSY). libata expects SCSI to be intelligent and not
starve devices, etc.


> I was able to successfully boot the following patch on
> AHCI/x86-64/Fedora.
>
> It may work with other controllers -- TRY AT YOUR OWN RISK. It will
> probably fail for master/slave configurations, and SAS & PMP also
> need looking at. It yielded this lsscsi output on my AHCI box:
>
> [0:0:0:0] disk ATA ST3500320AS SD15 /dev/sda
> [0:2:0:0] disk ATA G.SKILL 128GB SS 02.1 /dev/sdb
> [0:5:0:0] cd/dvd PIONEER BD-ROM BDC-202 1.04 /dev/sr0

For comparison, here is unmodified 2.6.30-rc3:

[jgarzik@bd ~]$ lsscsi
[0:0:0:0] disk ATA ST3500320AS SD15 /dev/sda
[2:0:0:0] disk ATA G.SKILL 128GB SS 02.1 /dev/sdb
[5:0:0:0] cd/dvd PIONEER BD-ROM BDC-202 1.04 /dev/sr0

2009-04-22 12:18:08

by Boaz Harrosh

[permalink] [raw]
Subject: Re: [PATCH] libata: rewrite SCSI host scheme to be one per ATA host

On 04/22/2009 12:23 PM, Jeff Garzik wrote:
> Jeff Garzik wrote:
>> Currently, libata creates a Scsi_Host per port. This was originally
>> done to leverage SCSI's infrastructure to arbitrate among master/slave
>> devices, but is not needed for most modern SATA controllers. And I
>> _think_ it is not needed for master/slave if done properly, either.
>
> BTW note the above, with regards to the libata SCSI->block conversion.
> libata currently relies on SCSI for some amount of generic device
> arbitration, in several situations (see ->qc_defer,
> SCSI_MLQUEUE_.*_BUSY). libata expects SCSI to be intelligent and not
> starve devices, etc.
>
>
>> I was able to successfully boot the following patch on
>> AHCI/x86-64/Fedora.
>>
>> It may work with other controllers -- TRY AT YOUR OWN RISK. It will
>> probably fail for master/slave configurations, and SAS & PMP also
>> need looking at. It yielded this lsscsi output on my AHCI box:
>>
>> [0:0:0:0] disk ATA ST3500320AS SD15 /dev/sda
>> [0:2:0:0] disk ATA G.SKILL 128GB SS 02.1 /dev/sdb
>> [0:5:0:0] cd/dvd PIONEER BD-ROM BDC-202 1.04 /dev/sr0
>
> For comparison, here is unmodified 2.6.30-rc3:
>
> [jgarzik@bd ~]$ lsscsi
> [0:0:0:0] disk ATA ST3500320AS SD15 /dev/sda
> [2:0:0:0] disk ATA G.SKILL 128GB SS 02.1 /dev/sdb
> [5:0:0:0] cd/dvd PIONEER BD-ROM BDC-202 1.04 /dev/sr0
>

Could the master/slave be simply solved by emulating a SCSI LUN
for example below is my machine today:
[]$ lsscsi
[0:0:0:0] disk ATA ST3160023A 3.01 /dev/sda
[1:0:0:0] cd/dvd _NEC DVD_RW ND-3550A 1.05 /dev/sr0
[4:0:0:0] disk ATA WDC WD1600JS-60M 10.0 /dev/sdb

the /dev/sda and /dev/sr0 share a master/slave wide cable (sdb is sata)

it could be made to scan as:
[]$ lsscsi
[0:0:0:0] disk ATA ST3160023A 3.01 /dev/sda
[0:0:0:1] cd/dvd _NEC DVD_RW ND-3550A 1.05 /dev/sr0
[1:0:0:0] disk ATA WDC WD1600JS-60M 10.0 /dev/sdb

So we need to emulate the REPORT_LUN (or what ever else) to return
two LUNs. Or do you want to report a separate target for the master/slave?

Thanks
Boaz

2009-04-22 13:09:24

by Mark Lord

[permalink] [raw]
Subject: Re: [PATCH] libata: rewrite SCSI host scheme to be one per ATA host

Jeff Garzik wrote:
> Currently, libata creates a Scsi_Host per port. This was originally
> done to leverage SCSI's infrastructure to arbitrate among master/slave
> devices, but is not needed for most modern SATA controllers. And I
> _think_ it is not needed for master/slave if done properly, either.
>
> The patch below converts libata such that there is now a 1:1
> correspondence between struct Scsi_Host and struct ata_host. ATA ports
> are represented as SCSI layer 'channels', which is more natural.
>
> This patch is an experiment, and not meant for upstream anytime soon.
..

Could you perhaps explain how error handling would behave in this scheme?

Currently, one SATA port can have failures without any impact whatsoever
on concurrent operation of other ports, in part because each port is treated
as a completely independent SCSI host.

I wonder if that changes with the new (better) scheme proposed here?

2009-04-22 15:11:16

by Daniela Engert

[permalink] [raw]
Subject: Re: [PATCH] libata: rewrite SCSI host scheme to be one per ATA host

Boaz Harrosh wrote:
> On 04/22/2009 12:23 PM, Jeff Garzik wrote:
>> Jeff Garzik wrote:
>>> Currently, libata creates a Scsi_Host per port. This was originally
>>> done to leverage SCSI's infrastructure to arbitrate among master/slave
>>> devices, but is not needed for most modern SATA controllers. And I
>>> _think_ it is not needed for master/slave if done properly, either.

>>> It may work with other controllers -- TRY AT YOUR OWN RISK. It will
>>> probably fail for master/slave configurations, and SAS & PMP also
>>> need looking at. It yielded this lsscsi output on my AHCI box:
>>>
>>> [0:0:0:0] disk ATA ST3500320AS SD15 /dev/sda
>>> [0:2:0:0] disk ATA G.SKILL 128GB SS 02.1 /dev/sdb
>>> [0:5:0:0] cd/dvd PIONEER BD-ROM BDC-202 1.04 /dev/sr0
>> For comparison, here is unmodified 2.6.30-rc3:
>>
>> [jgarzik@bd ~]$ lsscsi
>> [0:0:0:0] disk ATA ST3500320AS SD15 /dev/sda
>> [2:0:0:0] disk ATA G.SKILL 128GB SS 02.1 /dev/sdb
>> [5:0:0:0] cd/dvd PIONEER BD-ROM BDC-202 1.04 /dev/sr0
>>
>
> Could the master/slave be simply solved by emulating a SCSI LUN

Don't forget, there are ATAPI devices (some Sony CD burners and old
phase-changers come into mind) which *do* have multiple LUNs sitting
beyond the PATA port. I don't know if libata supports such setups (my
old OS/2 driver does) but one shouldn't hijack LUNs to emulate targets.

Ciao,
Dani

2009-04-22 15:29:50

by Alan

[permalink] [raw]
Subject: Re: [PATCH] libata: rewrite SCSI host scheme to be one per ATA host

> Don't forget, there are ATAPI devices (some Sony CD burners and old
> phase-changers come into mind) which *do* have multiple LUNs sitting
> beyond the PATA port. I don't know if libata supports such setups (my
> old OS/2 driver does) but one shouldn't hijack LUNs to emulate targets.

It's a long time ago since I tested it but my 5 CD changer was correctly
supported by libata (or more accurately by sr...).

2009-04-22 15:37:25

by James Bottomley

[permalink] [raw]
Subject: Re: [PATCH] libata: rewrite SCSI host scheme to be one per ATA host

On Wed, 2009-04-22 at 16:18 +0100, Alan Cox wrote:
> > Don't forget, there are ATAPI devices (some Sony CD burners and old
> > phase-changers come into mind) which *do* have multiple LUNs sitting
> > beyond the PATA port. I don't know if libata supports such setups (my
> > old OS/2 driver does) but one shouldn't hijack LUNs to emulate targets.
>
> It's a long time ago since I tested it but my 5 CD changer was correctly
> supported by libata (or more accurately by sr...).

Well, sr supports the discovered CD/DVD; ch is the actual changer
manager. Usually ch attaches to one LUN and sr attaches to another.

James

2009-04-22 16:27:33

by Alan

[permalink] [raw]
Subject: Re: [PATCH] libata: rewrite SCSI host scheme to be one per ATA host

> > It's a long time ago since I tested it but my 5 CD changer was correctly
> > supported by libata (or more accurately by sr...).
>
> Well, sr supports the discovered CD/DVD; ch is the actual changer
> manager. Usually ch attaches to one LUN and sr attaches to another.

I get 5 LUNS and the hardware looks after the rest - no changer manager
needed. Early versions of the gnome folks CD probing stuff did give it a
bit of a fit as the firmware clearly didn't cache all the state that the
cd mangler asked for.

2009-04-22 16:48:23

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] libata: rewrite SCSI host scheme to be one per ATA host

Boaz Harrosh wrote:
> On 04/22/2009 12:23 PM, Jeff Garzik wrote:
>> Jeff Garzik wrote:
>>> Currently, libata creates a Scsi_Host per port. This was originally
>>> done to leverage SCSI's infrastructure to arbitrate among master/slave
>>> devices, but is not needed for most modern SATA controllers. And I
>>> _think_ it is not needed for master/slave if done properly, either.
>> BTW note the above, with regards to the libata SCSI->block conversion.
>> libata currently relies on SCSI for some amount of generic device
>> arbitration, in several situations (see ->qc_defer,
>> SCSI_MLQUEUE_.*_BUSY). libata expects SCSI to be intelligent and not
>> starve devices, etc.
>>
>>
>>> I was able to successfully boot the following patch on
>>> AHCI/x86-64/Fedora.
>>>
>>> It may work with other controllers -- TRY AT YOUR OWN RISK. It will
>>> probably fail for master/slave configurations, and SAS & PMP also
>>> need looking at. It yielded this lsscsi output on my AHCI box:
>>>
>>> [0:0:0:0] disk ATA ST3500320AS SD15 /dev/sda
>>> [0:2:0:0] disk ATA G.SKILL 128GB SS 02.1 /dev/sdb
>>> [0:5:0:0] cd/dvd PIONEER BD-ROM BDC-202 1.04 /dev/sr0
>> For comparison, here is unmodified 2.6.30-rc3:
>>
>> [jgarzik@bd ~]$ lsscsi
>> [0:0:0:0] disk ATA ST3500320AS SD15 /dev/sda
>> [2:0:0:0] disk ATA G.SKILL 128GB SS 02.1 /dev/sdb
>> [5:0:0:0] cd/dvd PIONEER BD-ROM BDC-202 1.04 /dev/sr0
>>
>
> Could the master/slave be simply solved by emulating a SCSI LUN
> for example below is my machine today:
> []$ lsscsi
> [0:0:0:0] disk ATA ST3160023A 3.01 /dev/sda
> [1:0:0:0] cd/dvd _NEC DVD_RW ND-3550A 1.05 /dev/sr0
> [4:0:0:0] disk ATA WDC WD1600JS-60M 10.0 /dev/sdb
>
> the /dev/sda and /dev/sr0 share a master/slave wide cable (sdb is sata)
>
> it could be made to scan as:
> []$ lsscsi
> [0:0:0:0] disk ATA ST3160023A 3.01 /dev/sda
> [0:0:0:1] cd/dvd _NEC DVD_RW ND-3550A 1.05 /dev/sr0
> [1:0:0:0] disk ATA WDC WD1600JS-60M 10.0 /dev/sdb
>
> So we need to emulate the REPORT_LUN (or what ever else) to return
> two LUNs. Or do you want to report a separate target for the master/slave?

Mapping master/slave is not difficult -- each should be a separate
target, just like with parallel SCSI.

The issue with master/slave and simplex is guaranteeing that only _one_
command may be executing at a time, for a given set of targets, i.e.
only one command per master/slave pair, only one command per pair of
simplex ports (== 4 ATA devices max).

Originally this was done by setting can_queue==1, cmd_per_lun==1, and
assigning each master/slave pair to a different Scsi_Host.

Jeff



2009-04-22 16:53:00

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] libata: rewrite SCSI host scheme to be one per ATA host

Mark Lord wrote:
> Jeff Garzik wrote:
>> Currently, libata creates a Scsi_Host per port. This was originally
>> done to leverage SCSI's infrastructure to arbitrate among master/slave
>> devices, but is not needed for most modern SATA controllers. And I
>> _think_ it is not needed for master/slave if done properly, either.
>>
>> The patch below converts libata such that there is now a 1:1
>> correspondence between struct Scsi_Host and struct ata_host. ATA ports
>> are represented as SCSI layer 'channels', which is more natural.
>>
>> This patch is an experiment, and not meant for upstream anytime soon.
> ..
>
> Could you perhaps explain how error handling would behave in this scheme?
>
> Currently, one SATA port can have failures without any impact whatsoever
> on concurrent operation of other ports, in part because each port is
> treated
> as a completely independent SCSI host.
>
> I wonder if that changes with the new (better) scheme proposed here?

It changes, yes, most definitely. We just have to pay close attention,
and make sure to indicate which EH actions are host-wide, channel-wide
(== per port, in ATA parlance) or per-device.

SCSI handles all these cases, because e.g. you might not want to disrupt
all 1,000 SAN devices actively talking to a single SCSI host in Linux.

So... error handling should behave how it needs to behave ;-)

There might be an issue with concurrent error handling, because of
potential sharing of EH threads (== one port's EH must wait for
another's, I think)....but not with concurrent and independent
operation. You should be able to reset an AHCI port without affecting
data xfer on the other ports.

Jeff


2009-04-22 18:36:46

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] libata: rewrite SCSI host scheme to be one per ATA host

James Bottomley wrote:
> On Wed, 2009-04-22 at 16:18 +0100, Alan Cox wrote:
>>> Don't forget, there are ATAPI devices (some Sony CD burners and old
>>> phase-changers come into mind) which *do* have multiple LUNs sitting
>>> beyond the PATA port. I don't know if libata supports such setups (my
>>> old OS/2 driver does) but one shouldn't hijack LUNs to emulate targets.
>> It's a long time ago since I tested it but my 5 CD changer was correctly
>> supported by libata (or more accurately by sr...).
>
> Well, sr supports the discovered CD/DVD; ch is the actual changer
> manager. Usually ch attaches to one LUN and sr attaches to another.

Like Alan's, if I understand him correctly, my PATA ATAPI CD changer
simply presents a bunch of addressible LUNs. I never loaded, nor seemed
to need, ch.

I should boot a current libata and see how it behaves...

Jeff


2009-04-22 19:09:29

by Grant Grundler

[permalink] [raw]
Subject: Re: [PATCH] libata: rewrite SCSI host scheme to be one per ATA host

On Wed, Apr 22, 2009 at 2:09 AM, Jeff Garzik <[email protected]> wrote:
>
> Currently, libata creates a Scsi_Host per port.  This was originally
> done to leverage SCSI's infrastructure to arbitrate among master/slave
> devices, but is not needed for most modern SATA controllers.   And I
> _think_ it is not needed for master/slave if done properly, either.
>
> The patch below converts libata such that there is now a 1:1
> correspondence between struct Scsi_Host and struct ata_host.  ATA ports
> are represented as SCSI layer 'channels', which is more natural.

Jeff,
So far in reading this, the only reasons I gather for changing this
mapping are "not needed" and "is more natural". Data Center
environments (not just Google's) like to track disks in many different
ways, including the SCSI identifiers since this one "key" for physical
location. Breaking the current mappings is going to cause some people
a world of pain since they will need to manually build (and integrate)
old->new maps of the SCSI identifiers. Can you propose some real,
tangible benefit to making this change? (e.g. enables some other
feature)

Mark already pointed out this might cause issues with Error Handling
(forcing a review of all that code). So before triggering other
developers (e.g. HW vendors) do that kind of work I'd like to hear
what the reward is going to be at the end.

thanks,
grant

>
> This patch is an experiment, and not meant for upstream anytime soon.
> I just wanted to see what kind of effort would be required to get it
> to work.
>
> I was able to successfully boot the following patch on
> AHCI/x86-64/Fedora.
>
> It may work with other controllers -- TRY AT YOUR OWN RISK.  It will
> probably fail for master/slave configurations, and SAS & PMP also
> need looking at.  It yielded this lsscsi output on my AHCI box:
>
> [0:0:0:0]    disk    ATA      ST3500320AS      SD15  /dev/sda
> [0:2:0:0]    disk    ATA      G.SKILL 128GB SS 02.1  /dev/sdb
> [0:5:0:0]    cd/dvd  PIONEER  BD-ROM  BDC-202  1.04  /dev/sr0
>
> NOT-signed-off-by: me
>
>  drivers/ata/ahci.c            |    4
>  drivers/ata/libata-core.c     |   17 +--
>  drivers/ata/libata-eh.c       |   47 ++++++--
>  drivers/ata/libata-scsi.c     |  237 +++++++++++++++++++++---------------------
>  drivers/ata/libata.h          |    3
>  drivers/scsi/libsas/sas_ata.c |    2
>  include/linux/libata.h        |    9 -
>  7 files changed, 184 insertions(+), 135 deletions(-)
>
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 08186ec..b0468a8 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -326,14 +326,18 @@ static ssize_t ahci_activity_store(struct ata_device *dev,
>  static void ahci_init_sw_activity(struct ata_link *link);
>
>  static struct device_attribute *ahci_shost_attrs[] = {
> +#if 0
>        &dev_attr_link_power_management_policy,
>        &dev_attr_em_message_type,
>        &dev_attr_em_message,
> +#endif
>        NULL
>  };
>
>  static struct device_attribute *ahci_sdev_attrs[] = {
> +#if 0
>        &dev_attr_sw_activity,
> +#endif
>        &dev_attr_unload_heads,
>        NULL
>  };
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 17c5d48..71f32dc 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -95,6 +95,7 @@ static void ata_dev_xfermask(struct ata_device *dev);
>  static unsigned long ata_dev_blacklisted(const struct ata_device *dev);
>
>  unsigned int ata_print_id = 1;
> +unsigned int ata_host_print_id = 1;
>  static struct workqueue_struct *ata_wq;
>
>  struct workqueue_struct *ata_aux_wq;
> @@ -2308,7 +2309,7 @@ static void ata_dev_config_ncq(struct ata_device *dev,
>                return;
>        }
>        if (ap->flags & ATA_FLAG_NCQ) {
> -               hdepth = min(ap->scsi_host->can_queue, ATA_MAX_QUEUE - 1);
> +               hdepth = min(ap->host->scsi_host->can_queue, ATA_MAX_QUEUE - 1);
>                dev->flags |= ATA_DFLAG_NCQ;
>        }
>
> @@ -5635,15 +5636,15 @@ static void ata_host_release(struct device *gendev, void *res)
>                if (!ap)
>                        continue;
>
> -               if (ap->scsi_host)
> -                       scsi_host_put(ap->scsi_host);
> -
>                kfree(ap->pmp_link);
>                kfree(ap->slave_link);
>                kfree(ap);
>                host->ports[i] = NULL;
>        }
>
> +       if (host->scsi_host)
> +               scsi_host_put(host->scsi_host);
> +
>        dev_set_drvdata(gendev, NULL);
>  }
>
> @@ -6089,6 +6090,8 @@ int ata_host_register(struct ata_host *host, struct scsi_host_template *sht)
>        for (i = 0; i < host->n_ports; i++)
>                host->ports[i]->print_id = ata_print_id++;
>
> +       host->print_id = ata_host_print_id++;
> +
>        rc = ata_scsi_add_hosts(host, sht);
>        if (rc)
>                return rc;
> @@ -6222,8 +6225,7 @@ static void ata_port_detach(struct ata_port *ap)
>        cancel_rearming_delayed_work(&ap->hotplug_task);
>
>  skip_eh:
> -       /* remove the associated SCSI host */
> -       scsi_remove_host(ap->scsi_host);
> +       return;
>  }
>
>  /**
> @@ -6242,6 +6244,9 @@ void ata_host_detach(struct ata_host *host)
>        for (i = 0; i < host->n_ports; i++)
>                ata_port_detach(host->ports[i]);
>
> +       /* remove the associated SCSI host */
> +       scsi_remove_host(host->scsi_host);
> +
>        /* the host is dead now, dissociate ACPI */
>        ata_acpi_dissociate(host);
>  }
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 0183131..db8a66f 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -466,14 +466,22 @@ static void ata_eh_clear_action(struct ata_link *link, struct ata_device *dev,
>  */
>  enum blk_eh_timer_return ata_scsi_timed_out(struct scsi_cmnd *cmd)
>  {
> -       struct Scsi_Host *host = cmd->device->host;
> -       struct ata_port *ap = ata_shost_to_port(host);
> +       struct Scsi_Host *shost = cmd->device->host;
> +       struct ata_port *ap;
> +       struct ata_host *host;
> +       struct ata_device *atadev;
>        unsigned long flags;
>        struct ata_queued_cmd *qc;
> -       enum blk_eh_timer_return ret;
> +       enum blk_eh_timer_return ret = BLK_EH_NOT_HANDLED;
>
>        DPRINTK("ENTER\n");
>
> +       host = ata_shost_to_host(shost);
> +       atadev = ata_scsi_find_dev(host, cmd->device);
> +       if (!atadev)
> +               goto out;
> +       ap = atadev->link->ap;
> +
>        if (ap->ops->error_handler) {
>                ret = BLK_EH_NOT_HANDLED;
>                goto out;
> @@ -532,13 +540,12 @@ static void ata_eh_unload(struct ata_port *ap)
>  *     RETURNS:
>  *     Zero.
>  */
> -void ata_scsi_error(struct Scsi_Host *host)
> +static void __ata_scsi_error(struct Scsi_Host *shost, struct ata_port *ap)
>  {
> -       struct ata_port *ap = ata_shost_to_port(host);
>        int i;
>        unsigned long flags;
>
> -       DPRINTK("ENTER\n");
> +       DPRINTK("ENTER, port_no %u\n", ap->port_no);
>
>        /* synchronize with port task */
>        ata_port_flush_task(ap);
> @@ -575,7 +582,7 @@ void ata_scsi_error(struct Scsi_Host *host)
>                if (ap->ops->lost_interrupt)
>                        ap->ops->lost_interrupt(ap);
>
> -               list_for_each_entry_safe(scmd, tmp, &host->eh_cmd_q, eh_entry) {
> +               list_for_each_entry_safe(scmd, tmp, &shost->eh_cmd_q, eh_entry) {
>                        struct ata_queued_cmd *qc;
>
>                        for (i = 0; i < ATA_MAX_QUEUE; i++) {
> @@ -698,7 +705,7 @@ void ata_scsi_error(struct Scsi_Host *host)
>                 * before EH completion, SCSI midlayer will
>                 * re-initiate EH.
>                 */
> -               host->host_eh_scheduled = 0;
> +               shost->host_eh_scheduled = 0;
>
>                spin_unlock_irqrestore(ap->lock, flags);
>        } else {
> @@ -707,7 +714,7 @@ void ata_scsi_error(struct Scsi_Host *host)
>        }
>
>        /* finish or retry handled scmd's and clean up */
> -       WARN_ON(host->host_failed || !list_empty(&host->eh_cmd_q));
> +       WARN_ON(shost->host_failed || !list_empty(&shost->eh_cmd_q));
>
>        scsi_eh_flush_done_q(&ap->eh_done_q);
>
> @@ -733,6 +740,24 @@ void ata_scsi_error(struct Scsi_Host *host)
>        DPRINTK("EXIT\n");
>  }
>
> +void ata_scsi_error(struct Scsi_Host *shost)
> +{
> +       struct ata_host *host = ata_shost_to_host(shost);
> +       unsigned int i;
> +
> +       DPRINTK("ENTER\n");
> +
> +       for (i = 0; i < host->n_ports; i++) {
> +               struct ata_port *ap = host->ports[i];
> +
> +               if (ap->pflags & ATA_PFLAG_EH_PENDING)
> +                       __ata_scsi_error(shost, ap);
> +       }
> +
> +       DPRINTK("EXIT\n");
> +}
> +
> +
>  /**
>  *     ata_port_wait_eh - Wait for the currently pending EH to complete
>  *     @ap: Port to wait EH for
> @@ -761,7 +786,7 @@ void ata_port_wait_eh(struct ata_port *ap)
>        spin_unlock_irqrestore(ap->lock, flags);
>
>        /* make sure SCSI EH is complete */
> -       if (scsi_host_in_recovery(ap->scsi_host)) {
> +       if (scsi_host_in_recovery(ap->host->scsi_host)) {
>                msleep(10);
>                goto retry;
>        }
> @@ -901,7 +926,7 @@ void ata_port_schedule_eh(struct ata_port *ap)
>                return;
>
>        ata_eh_set_pending(ap, 1);
> -       scsi_schedule_eh(ap->scsi_host);
> +       scsi_schedule_eh(ap->host->scsi_host);
>
>        DPRINTK("port EH scheduled\n");
>  }
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 2733b0c..7aa6aa6 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -58,10 +58,8 @@ static u8 ata_scsi_rbuf[ATA_SCSI_RBUF_SIZE];
>
>  typedef unsigned int (*ata_xlat_func_t)(struct ata_queued_cmd *qc);
>
> -static struct ata_device *__ata_scsi_find_dev(struct ata_port *ap,
> +static struct ata_device *__ata_scsi_find_dev(struct ata_host *,
>                                        const struct scsi_device *scsidev);
> -static struct ata_device *ata_scsi_find_dev(struct ata_port *ap,
> -                                           const struct scsi_device *scsidev);
>  static int ata_scsi_user_scan(struct Scsi_Host *shost, unsigned int channel,
>                              unsigned int id, unsigned int lun);
>
> @@ -136,6 +134,7 @@ static const char *ata_scsi_lpm_get(enum link_pm policy)
>        return NULL;
>  }
>
> +#if 0
>  static ssize_t ata_scsi_lpm_put(struct device *dev,
>                                struct device_attribute *attr,
>                                const char *buf, size_t count)
> @@ -183,6 +182,7 @@ ata_scsi_lpm_show(struct device *dev, struct device_attribute *attr, char *buf)
>  DEVICE_ATTR(link_power_management_policy, S_IRUGO | S_IWUSR,
>                ata_scsi_lpm_show, ata_scsi_lpm_put);
>  EXPORT_SYMBOL_GPL(dev_attr_link_power_management_policy);
> +#endif
>
>  static ssize_t ata_scsi_park_show(struct device *device,
>                                  struct device_attribute *attr, char *buf)
> @@ -190,19 +190,21 @@ static ssize_t ata_scsi_park_show(struct device *device,
>        struct scsi_device *sdev = to_scsi_device(device);
>        struct ata_port *ap;
>        struct ata_link *link;
> +       struct ata_host *host;
>        struct ata_device *dev;
>        unsigned long flags, now;
>        unsigned int uninitialized_var(msecs);
>        int rc = 0;
>
> -       ap = ata_shost_to_port(sdev->host);
> +       host = ata_shost_to_host(sdev->host);
>
> -       spin_lock_irqsave(ap->lock, flags);
> -       dev = ata_scsi_find_dev(ap, sdev);
> +       spin_lock_irqsave(&host->lock, flags);
> +       dev = ata_scsi_find_dev(host, sdev);
>        if (!dev) {
>                rc = -ENODEV;
>                goto unlock;
>        }
> +       ap = dev->link->ap;
>        if (dev->flags & ATA_DFLAG_NO_UNLOAD) {
>                rc = -EOPNOTSUPP;
>                goto unlock;
> @@ -218,7 +220,7 @@ static ssize_t ata_scsi_park_show(struct device *device,
>                msecs = 0;
>
>  unlock:
> -       spin_unlock_irq(ap->lock);
> +       spin_unlock_irq(&host->lock);
>
>        return rc ? rc : snprintf(buf, 20, "%u\n", msecs);
>  }
> @@ -230,6 +232,7 @@ static ssize_t ata_scsi_park_store(struct device *device,
>        struct scsi_device *sdev = to_scsi_device(device);
>        struct ata_port *ap;
>        struct ata_device *dev;
> +       struct ata_host *host;
>        long int input;
>        unsigned long flags;
>        int rc;
> @@ -242,14 +245,15 @@ static ssize_t ata_scsi_park_store(struct device *device,
>                input = ATA_TMOUT_MAX_PARK;
>        }
>
> -       ap = ata_shost_to_port(sdev->host);
> +       host = ata_shost_to_host(sdev->host);
>
> -       spin_lock_irqsave(ap->lock, flags);
> -       dev = ata_scsi_find_dev(ap, sdev);
> +       spin_lock_irqsave(&host->lock, flags);
> +       dev = ata_scsi_find_dev(host, sdev);
>        if (unlikely(!dev)) {
>                rc = -ENODEV;
>                goto unlock;
>        }
> +       ap = dev->link->ap;
>        if (dev->class != ATA_DEV_ATA) {
>                rc = -EOPNOTSUPP;
>                goto unlock;
> @@ -276,7 +280,7 @@ static ssize_t ata_scsi_park_store(struct device *device,
>                }
>        }
>  unlock:
> -       spin_unlock_irqrestore(ap->lock, flags);
> +       spin_unlock_irqrestore(&host->lock, flags);
>
>        return rc ? rc : len;
>  }
> @@ -291,6 +295,7 @@ static void ata_scsi_set_sense(struct scsi_cmnd *cmd, u8 sk, u8 asc, u8 ascq)
>        scsi_build_sense_buffer(0, cmd->sense_buffer, sk, asc, ascq);
>  }
>
> +#if 0
>  static ssize_t
>  ata_scsi_em_message_store(struct device *dev, struct device_attribute *attr,
>                          const char *buf, size_t count)
> @@ -335,8 +340,9 @@ ata_scsi_activity_show(struct device *dev, struct device_attribute *attr,
>                char *buf)
>  {
>        struct scsi_device *sdev = to_scsi_device(dev);
> -       struct ata_port *ap = ata_shost_to_port(sdev->host);
> -       struct ata_device *atadev = ata_scsi_find_dev(ap, sdev);
> +       struct ata_host *host = ata_shost_to_host(sdev->host);
> +       struct ata_device *atadev = ata_scsi_find_dev(host, sdev);
> +       struct ata_port *ap = atadev->ap;
>
>        if (ap->ops->sw_activity_show && (ap->flags & ATA_FLAG_SW_ACTIVITY))
>                return ap->ops->sw_activity_show(atadev, buf);
> @@ -348,8 +354,9 @@ ata_scsi_activity_store(struct device *dev, struct device_attribute *attr,
>        const char *buf, size_t count)
>  {
>        struct scsi_device *sdev = to_scsi_device(dev);
> -       struct ata_port *ap = ata_shost_to_port(sdev->host);
> -       struct ata_device *atadev = ata_scsi_find_dev(ap, sdev);
> +       struct ata_host *host = ata_shost_to_host(sdev->host);
> +       struct ata_device *atadev = ata_scsi_find_dev(host, sdev);
> +       struct ata_port *ap = atadev->ap;
>        enum sw_activity val;
>        int rc;
>
> @@ -369,6 +376,7 @@ ata_scsi_activity_store(struct device *dev, struct device_attribute *attr,
>  DEVICE_ATTR(sw_activity, S_IWUGO | S_IRUGO, ata_scsi_activity_show,
>                        ata_scsi_activity_store);
>  EXPORT_SYMBOL_GPL(dev_attr_sw_activity);
> +#endif
>
>  struct device_attribute *ata_common_sdev_attrs[] = {
>        &dev_attr_unload_heads,
> @@ -425,10 +433,10 @@ int ata_std_bios_param(struct scsi_device *sdev, struct block_device *bdev,
>  *     RETURNS:
>  *     Zero on success, negative errno on error.
>  */
> -static int ata_get_identity(struct ata_port *ap, struct scsi_device *sdev,
> +static int ata_get_identity(struct ata_host *host, struct scsi_device *sdev,
>                            void __user *arg)
>  {
> -       struct ata_device *dev = ata_scsi_find_dev(ap, sdev);
> +       struct ata_device *dev = ata_scsi_find_dev(host, sdev);
>        u16 __user *dst = arg;
>        char buf[40];
>
> @@ -656,11 +664,18 @@ static int ata_ioc32(struct ata_port *ap)
>        return 0;
>  }
>
> -int ata_sas_scsi_ioctl(struct ata_port *ap, struct scsi_device *scsidev,
> +int ata_sas_scsi_ioctl(struct ata_host *host, struct scsi_device *scsidev,
>                     int cmd, void __user *arg)
>  {
>        int val = -EINVAL, rc = -EINVAL;
>        unsigned long flags;
> +       struct ata_port *ap;
> +       struct ata_device *atadev;
> +
> +       atadev = ata_scsi_find_dev(host, scsidev);
> +       if (!atadev)
> +               return -ENOENT;
> +       ap = atadev->link->ap;
>
>        switch (cmd) {
>        case ATA_IOC_GET_IO32:
> @@ -688,7 +703,7 @@ int ata_sas_scsi_ioctl(struct ata_port *ap, struct scsi_device *scsidev,
>                return rc;
>
>        case HDIO_GET_IDENTITY:
> -               return ata_get_identity(ap, scsidev, arg);
> +               return ata_get_identity(host, scsidev, arg);
>
>        case HDIO_DRIVE_CMD:
>                if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
> @@ -711,7 +726,7 @@ EXPORT_SYMBOL_GPL(ata_sas_scsi_ioctl);
>
>  int ata_scsi_ioctl(struct scsi_device *scsidev, int cmd, void __user *arg)
>  {
> -       return ata_sas_scsi_ioctl(ata_shost_to_port(scsidev->host),
> +       return ata_sas_scsi_ioctl(ata_shost_to_host(scsidev->host),
>                                scsidev, cmd, arg);
>  }
>  EXPORT_SYMBOL_GPL(ata_scsi_ioctl);
> @@ -1157,8 +1172,8 @@ static int ata_scsi_dev_config(struct scsi_device *sdev,
>
>  int ata_scsi_slave_config(struct scsi_device *sdev)
>  {
> -       struct ata_port *ap = ata_shost_to_port(sdev->host);
> -       struct ata_device *dev = __ata_scsi_find_dev(ap, sdev);
> +       struct ata_host *host = ata_shost_to_host(sdev->host);
> +       struct ata_device *dev = __ata_scsi_find_dev(host, sdev);
>        int rc = 0;
>
>        ata_scsi_sdev_config(sdev);
> @@ -1185,27 +1200,34 @@ int ata_scsi_slave_config(struct scsi_device *sdev)
>  */
>  void ata_scsi_slave_destroy(struct scsi_device *sdev)
>  {
> -       struct ata_port *ap = ata_shost_to_port(sdev->host);
> +       struct ata_host *host = ata_shost_to_host(sdev->host);
>        struct request_queue *q = sdev->request_queue;
>        unsigned long flags;
>        struct ata_device *dev;
> +       bool new_eh = true;
>
> -       if (!ap->ops->error_handler)
> -               return;
> -
> -       spin_lock_irqsave(ap->lock, flags);
> -       dev = __ata_scsi_find_dev(ap, sdev);
> +       spin_lock_irqsave(&host->lock, flags);
> +       dev = __ata_scsi_find_dev(host, sdev);
>        if (dev && dev->sdev) {
> +               struct ata_port *ap = dev->link->ap;
> +
> +               if (!ap->ops->error_handler)
> +                       new_eh = false;
> +
> +               else {
>                /* SCSI device already in CANCEL state, no need to offline it */
>                dev->sdev = NULL;
>                dev->flags |= ATA_DFLAG_DETACH;
>                ata_port_schedule_eh(ap);
> +               }
>        }
> -       spin_unlock_irqrestore(ap->lock, flags);
> +       spin_unlock_irqrestore(&host->lock, flags);
>
> -       kfree(q->dma_drain_buffer);
> -       q->dma_drain_buffer = NULL;
> -       q->dma_drain_size = 0;
> +       if (new_eh) {
> +               kfree(q->dma_drain_buffer);
> +               q->dma_drain_buffer = NULL;
> +               q->dma_drain_size = 0;
> +       }
>  }
>
>  /**
> @@ -1225,25 +1247,25 @@ void ata_scsi_slave_destroy(struct scsi_device *sdev)
>  */
>  int ata_scsi_change_queue_depth(struct scsi_device *sdev, int queue_depth)
>  {
> -       struct ata_port *ap = ata_shost_to_port(sdev->host);
> +       struct ata_host *host = ata_shost_to_host(sdev->host);
>        struct ata_device *dev;
>        unsigned long flags;
>
>        if (queue_depth < 1 || queue_depth == sdev->queue_depth)
>                return sdev->queue_depth;
>
> -       dev = ata_scsi_find_dev(ap, sdev);
> +       dev = ata_scsi_find_dev(host, sdev);
>        if (!dev || !ata_dev_enabled(dev))
>                return sdev->queue_depth;
>
>        /* NCQ enabled? */
> -       spin_lock_irqsave(ap->lock, flags);
> +       spin_lock_irqsave(&host->lock, flags);
>        dev->flags &= ~ATA_DFLAG_NCQ_OFF;
>        if (queue_depth == 1 || !ata_ncq_enabled(dev)) {
>                dev->flags |= ATA_DFLAG_NCQ_OFF;
>                queue_depth = 1;
>        }
> -       spin_unlock_irqrestore(ap->lock, flags);
> +       spin_unlock_irqrestore(&host->lock, flags);
>
>        /* limit and apply queue depth */
>        queue_depth = min(queue_depth, sdev->host->can_queue);
> @@ -2690,21 +2712,20 @@ static struct ata_device *ata_find_dev(struct ata_port *ap, int devno)
>        return NULL;
>  }
>
> -static struct ata_device *__ata_scsi_find_dev(struct ata_port *ap,
> +static struct ata_device *__ata_scsi_find_dev(struct ata_host *host,
>                                              const struct scsi_device *scsidev)
>  {
>        int devno;
> +       struct ata_port *ap;
>
> -       /* skip commands not addressed to targets we simulate */
> -       if (!sata_pmp_attached(ap)) {
> -               if (unlikely(scsidev->channel || scsidev->lun))
> -                       return NULL;
> -               devno = scsidev->id;
> -       } else {
> -               if (unlikely(scsidev->id || scsidev->lun))
> -                       return NULL;
> -               devno = scsidev->channel;
> -       }
> +       if (scsidev->channel > host->n_ports)
> +               return NULL;
> +
> +       ap = host->ports[scsidev->channel];
> +       if (ata_port_is_dummy(ap))
> +               return NULL;
> +
> +       devno = scsidev->id;
>
>        return ata_find_dev(ap, devno);
>  }
> @@ -2725,10 +2746,10 @@ static struct ata_device *__ata_scsi_find_dev(struct ata_port *ap,
>  *     RETURNS:
>  *     Associated ATA device, or %NULL if not found.
>  */
> -static struct ata_device *
> -ata_scsi_find_dev(struct ata_port *ap, const struct scsi_device *scsidev)
> +struct ata_device *
> +ata_scsi_find_dev(struct ata_host *host, const struct scsi_device *scsidev)
>  {
> -       struct ata_device *dev = __ata_scsi_find_dev(ap, scsidev);
> +       struct ata_device *dev = __ata_scsi_find_dev(host, scsidev);
>
>        if (unlikely(!dev || !ata_dev_enabled(dev)))
>                return NULL;
> @@ -2983,15 +3004,13 @@ static inline ata_xlat_func_t ata_get_xlat_func(struct ata_device *dev, u8 cmd)
>  *     Prints the contents of a SCSI command via printk().
>  */
>
> -static inline void ata_scsi_dump_cdb(struct ata_port *ap,
> -                                    struct scsi_cmnd *cmd)
> +static inline void ata_scsi_dump_cdb(struct scsi_cmnd *cmd)
>  {
>  #ifdef ATA_DEBUG
>        struct scsi_device *scsidev = cmd->device;
>        u8 *scsicmd = cmd->cmnd;
>
> -       DPRINTK("CDB (%u:%d,%d,%d) %02x %02x %02x %02x %02x %02x %02x %02x %02x\n",
> -               ap->print_id,
> +       DPRINTK("CDB (%d,%d,%d) %02x %02x %02x %02x %02x %02x %02x %02x %02x\n",
>                scsidev->channel, scsidev->id, scsidev->lun,
>                scsicmd[0], scsicmd[1], scsicmd[2], scsicmd[3],
>                scsicmd[4], scsicmd[5], scsicmd[6], scsicmd[7],
> @@ -3069,20 +3088,20 @@ static inline int __ata_scsi_queuecmd(struct scsi_cmnd *scmd,
>  */
>  int ata_scsi_queuecmd(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *))
>  {
> -       struct ata_port *ap;
>        struct ata_device *dev;
> +       struct ata_host *host;
>        struct scsi_device *scsidev = cmd->device;
>        struct Scsi_Host *shost = scsidev->host;
>        int rc = 0;
>
> -       ap = ata_shost_to_port(shost);
> +       host = ata_shost_to_host(shost);
>
>        spin_unlock(shost->host_lock);
> -       spin_lock(ap->lock);
> +       spin_lock(&host->lock);
>
> -       ata_scsi_dump_cdb(ap, cmd);
> +       ata_scsi_dump_cdb(cmd);
>
> -       dev = ata_scsi_find_dev(ap, scsidev);
> +       dev = ata_scsi_find_dev(host, scsidev);
>        if (likely(dev))
>                rc = __ata_scsi_queuecmd(cmd, done, dev);
>        else {
> @@ -3090,7 +3109,7 @@ int ata_scsi_queuecmd(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *))
>                done(cmd);
>        }
>
> -       spin_unlock(ap->lock);
> +       spin_unlock(&host->lock);
>        spin_lock(shost->host_lock);
>        return rc;
>  }
> @@ -3217,56 +3236,43 @@ void ata_scsi_simulate(struct ata_device *dev, struct scsi_cmnd *cmd,
>
>  int ata_scsi_add_hosts(struct ata_host *host, struct scsi_host_template *sht)
>  {
> -       int i, rc;
> -
> -       for (i = 0; i < host->n_ports; i++) {
> -               struct ata_port *ap = host->ports[i];
> -               struct Scsi_Host *shost;
> +       int rc;
> +       struct Scsi_Host *shost;
>
> -               rc = -ENOMEM;
> -               shost = scsi_host_alloc(sht, sizeof(struct ata_port *));
> -               if (!shost)
> -                       goto err_alloc;
> +       shost = scsi_host_alloc(sht, sizeof(struct ata_host *));
> +       if (!shost)
> +               return -ENOMEM;
>
> -               *(struct ata_port **)&shost->hostdata[0] = ap;
> -               ap->scsi_host = shost;
> +       *(struct ata_host **)&shost->hostdata[0] = host;
> +       host->scsi_host = shost;
>
> -               shost->transportt = &ata_scsi_transport_template;
> -               shost->unique_id = ap->print_id;
> -               shost->max_id = 16;
> -               shost->max_lun = 1;
> -               shost->max_channel = 1;
> -               shost->max_cmd_len = 16;
> +       shost->transportt = &ata_scsi_transport_template;
> +       shost->unique_id = host->print_id;
> +       shost->max_id = 32;
> +       shost->max_lun = 1;
> +       shost->max_channel = 32;
> +       shost->max_cmd_len = 16;
>
> -               /* Schedule policy is determined by ->qc_defer()
> -                * callback and it needs to see every deferred qc.
> -                * Set host_blocked to 1 to prevent SCSI midlayer from
> -                * automatically deferring requests.
> -                */
> -               shost->max_host_blocked = 1;
> +       /* Schedule policy is determined by ->qc_defer()
> +        * callback and it needs to see every deferred qc.
> +        * Set host_blocked to 1 to prevent SCSI midlayer from
> +        * automatically deferring requests.
> +        */
> +       shost->max_host_blocked = 1;
>
> -               rc = scsi_add_host(ap->scsi_host, ap->host->dev);
> -               if (rc)
> -                       goto err_add;
> +       rc = scsi_add_host(shost, host->dev);
> +       if (rc) {
> +               scsi_host_put(shost);
> +               return rc;
>        }
>
>        return 0;
> -
> - err_add:
> -       scsi_host_put(host->ports[i]->scsi_host);
> - err_alloc:
> -       while (--i >= 0) {
> -               struct Scsi_Host *shost = host->ports[i]->scsi_host;
> -
> -               scsi_remove_host(shost);
> -               scsi_host_put(shost);
> -       }
> -       return rc;
>  }
>
>  void ata_scsi_scan_host(struct ata_port *ap, int sync)
>  {
>        int tries = 5;
> +       struct ata_host *host = ap->host;
>        struct ata_device *last_failed_dev = NULL;
>        struct ata_link *link;
>        struct ata_device *dev;
> @@ -3278,7 +3284,7 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
>        ata_for_each_link(link, ap, EDGE) {
>                ata_for_each_dev(dev, link, ENABLED) {
>                        struct scsi_device *sdev;
> -                       int channel = 0, id = 0;
> +                       int id = 0;
>
>                        if (dev->sdev)
>                                continue;
> @@ -3286,9 +3292,10 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
>                        if (ata_is_host_link(link))
>                                id = dev->devno;
>                        else
> -                               channel = link->pmp;
> +                               id = link->pmp;
>
> -                       sdev = __scsi_add_device(ap->scsi_host, channel, id, 0,
> +                       sdev = __scsi_add_device(host->scsi_host,
> +                                                ap->port_no, id, 0,
>                                                 NULL);
>                        if (!IS_ERR(sdev)) {
>                                dev->sdev = sdev;
> @@ -3376,6 +3383,7 @@ int ata_scsi_offline_dev(struct ata_device *dev)
>  static void ata_scsi_remove_dev(struct ata_device *dev)
>  {
>        struct ata_port *ap = dev->link->ap;
> +       struct ata_host *host = ap->host;
>        struct scsi_device *sdev;
>        unsigned long flags;
>
> @@ -3385,7 +3393,7 @@ static void ata_scsi_remove_dev(struct ata_device *dev)
>         * be removed if there is __scsi_device_get() interface which
>         * increments reference counts regardless of device state.
>         */
> -       mutex_lock(&ap->scsi_host->scan_mutex);
> +       mutex_lock(&host->scsi_host->scan_mutex);
>        spin_lock_irqsave(ap->lock, flags);
>
>        /* clearing dev->sdev is protected by host lock */
> @@ -3411,7 +3419,7 @@ static void ata_scsi_remove_dev(struct ata_device *dev)
>        }
>
>        spin_unlock_irqrestore(ap->lock, flags);
> -       mutex_unlock(&ap->scsi_host->scan_mutex);
> +       mutex_unlock(&host->scsi_host->scan_mutex);
>
>        if (sdev) {
>                ata_dev_printk(dev, KERN_INFO, "detaching (SCSI %s)\n",
> @@ -3517,25 +3525,28 @@ void ata_scsi_hotplug(struct work_struct *work)
>  static int ata_scsi_user_scan(struct Scsi_Host *shost, unsigned int channel,
>                              unsigned int id, unsigned int lun)
>  {
> -       struct ata_port *ap = ata_shost_to_port(shost);
> +       struct ata_port *ap;
> +       struct ata_host *host = ata_shost_to_host(shost);
>        unsigned long flags;
>        int devno, rc = 0;
>
> +       if (channel == SCAN_WILD_CARD)
> +               ap = host->ports[0];
> +       else {
> +               if (channel > host->n_ports)
> +                       return -EINVAL;
> +               ap = host->ports[channel];
> +       }
> +
>        if (!ap->ops->error_handler)
>                return -EOPNOTSUPP;
>
>        if (lun != SCAN_WILD_CARD && lun)
>                return -EINVAL;
>
> -       if (!sata_pmp_attached(ap)) {
> -               if (channel != SCAN_WILD_CARD && channel)
> -                       return -EINVAL;
> -               devno = id;
> -       } else {
> -               if (id != SCAN_WILD_CARD && id)
> -                       return -EINVAL;
> -               devno = channel;
> -       }
> +       if (id != SCAN_WILD_CARD && id)
> +               return -EINVAL;
> +       devno = id;
>
>        spin_lock_irqsave(ap->lock, flags);
>
> @@ -3749,7 +3760,7 @@ int ata_sas_queuecmd(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *),
>  {
>        int rc = 0;
>
> -       ata_scsi_dump_cdb(ap, cmd);
> +       ata_scsi_dump_cdb(cmd);
>
>        if (likely(ata_dev_enabled(ap->link.device)))
>                rc = __ata_scsi_queuecmd(cmd, done, ap->link.device);
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index 89a1e00..af890b4 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -205,4 +205,7 @@ extern u8 ata_irq_on(struct ata_port *ap);
>  extern void ata_pio_task(struct work_struct *work);
>  #endif /* CONFIG_ATA_SFF */
>
> +struct ata_device *
> +ata_scsi_find_dev(struct ata_host *host, const struct scsi_device *scsidev);
> +
>  #endif /* __LIBATA_H__ */
> diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
> index e155011..758df0b 100644
> --- a/drivers/scsi/libsas/sas_ata.c
> +++ b/drivers/scsi/libsas/sas_ata.c
> @@ -375,6 +375,7 @@ int sas_ata_init_host_and_port(struct domain_device *found_dev,
>                      ha->dev,
>                      sata_port_info.flags,
>                      &sas_sata_ops);
> +       found_dev->sata_dev.ata_host.scsi_host = shost;
>        ap = ata_sas_port_alloc(&found_dev->sata_dev.ata_host,
>                                &sata_port_info,
>                                shost);
> @@ -385,7 +386,6 @@ int sas_ata_init_host_and_port(struct domain_device *found_dev,
>
>        ap->private_data = found_dev;
>        ap->cbl = ATA_CBL_SATA;
> -       ap->scsi_host = shost;
>        found_dev->sata_dev.ap = ap;
>
>        return 0;
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 3d501db..2ea4dce 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -507,10 +507,12 @@ struct ata_ioports {
>  #endif /* CONFIG_ATA_SFF */
>
>  struct ata_host {
> +       struct Scsi_Host        *scsi_host; /* our co-allocated scsi host */
>        spinlock_t              lock;
>        struct device           *dev;
>        void __iomem * const    *iomap;
>        unsigned int            n_ports;
> +       unsigned int            print_id; /* user visible unique host ID */
>        void                    *private_data;
>        struct ata_port_operations *ops;
>        unsigned long           flags;
> @@ -690,7 +692,6 @@ struct ata_link {
>  };
>
>  struct ata_port {
> -       struct Scsi_Host        *scsi_host; /* our co-allocated scsi host */
>        struct ata_port_operations *ops;
>        spinlock_t              *lock;
>        /* Flags owned by the EH context. Only EH should touch these once the
> @@ -947,7 +948,7 @@ extern void ata_host_init(struct ata_host *, struct device *,
>  extern int ata_scsi_detect(struct scsi_host_template *sht);
>  extern int ata_scsi_ioctl(struct scsi_device *dev, int cmd, void __user *arg);
>  extern int ata_scsi_queuecmd(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *));
> -extern int ata_sas_scsi_ioctl(struct ata_port *ap, struct scsi_device *dev,
> +extern int ata_sas_scsi_ioctl(struct ata_host *, struct scsi_device *dev,
>                            int cmd, void __user *arg);
>  extern void ata_sas_port_destroy(struct ata_port *);
>  extern struct ata_port *ata_sas_port_alloc(struct ata_host *,
> @@ -1472,9 +1473,9 @@ static inline unsigned int __ac_err_mask(u8 status)
>        return mask;
>  }
>
> -static inline struct ata_port *ata_shost_to_port(struct Scsi_Host *host)
> +static inline struct ata_host *ata_shost_to_host(struct Scsi_Host *host)
>  {
> -       return *(struct ata_port **)&host->hostdata[0];
> +       return *(struct ata_host **)&host->hostdata[0];
>  }
>
>  static inline int ata_check_ready(u8 status)
>

2009-04-22 19:27:40

by James Bottomley

[permalink] [raw]
Subject: Re: [PATCH] libata: rewrite SCSI host scheme to be one per ATA host

On Wed, 2009-04-22 at 14:36 -0400, Jeff Garzik wrote:
> James Bottomley wrote:
> > On Wed, 2009-04-22 at 16:18 +0100, Alan Cox wrote:
> >>> Don't forget, there are ATAPI devices (some Sony CD burners and old
> >>> phase-changers come into mind) which *do* have multiple LUNs sitting
> >>> beyond the PATA port. I don't know if libata supports such setups (my
> >>> old OS/2 driver does) but one shouldn't hijack LUNs to emulate targets.
> >> It's a long time ago since I tested it but my 5 CD changer was correctly
> >> supported by libata (or more accurately by sr...).
> >
> > Well, sr supports the discovered CD/DVD; ch is the actual changer
> > manager. Usually ch attaches to one LUN and sr attaches to another.
>
> Like Alan's, if I understand him correctly, my PATA ATAPI CD changer
> simply presents a bunch of addressible LUNs. I never loaded, nor seemed
> to need, ch.

Yes, they both seem to be non standard changer implementations that just
present as many CDs as they could chage to.

The ch is actually a driver for devices which conform to the SCSI Media
Changer standard, which tends to present LUN zero as the smc device and
LUN1 as the selected CD/DVD

> I should boot a current libata and see how it behaves...

Might be fun.

James

2009-04-23 06:35:56

by Jens Axboe

[permalink] [raw]
Subject: Re: [PATCH] libata: rewrite SCSI host scheme to be one per ATA host

On Wed, Apr 22 2009, Jeff Garzik wrote:
> Jeff Garzik wrote:
>> Currently, libata creates a Scsi_Host per port. This was originally
>> done to leverage SCSI's infrastructure to arbitrate among master/slave
>> devices, but is not needed for most modern SATA controllers. And I
>> _think_ it is not needed for master/slave if done properly, either.
>
> BTW note the above, with regards to the libata SCSI->block conversion.
> libata currently relies on SCSI for some amount of generic device
> arbitration, in several situations (see ->qc_defer,
> SCSI_MLQUEUE_.*_BUSY). libata expects SCSI to be intelligent and not
> starve devices, etc.

Defer looks like internal policy, I don't see that functioning any
different in the block layer. SCSI_MLQUEUE_*_BUSY in SCSI is primarily
using the block layer functionality of BLKPREP_DEFER to begin with, so I
think we're pretty close to providing all that already.

--
Jens Axboe

2009-04-23 10:39:56

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] libata: rewrite SCSI host scheme to be one per ATA host

Jens Axboe wrote:
> On Wed, Apr 22 2009, Jeff Garzik wrote:
>> Jeff Garzik wrote:
>>> Currently, libata creates a Scsi_Host per port. This was originally
>>> done to leverage SCSI's infrastructure to arbitrate among master/slave
>>> devices, but is not needed for most modern SATA controllers. And I
>>> _think_ it is not needed for master/slave if done properly, either.
>> BTW note the above, with regards to the libata SCSI->block conversion.
>> libata currently relies on SCSI for some amount of generic device
>> arbitration, in several situations (see ->qc_defer,
>> SCSI_MLQUEUE_.*_BUSY). libata expects SCSI to be intelligent and not
>> starve devices, etc.
>
> Defer looks like internal policy, I don't see that functioning any
> different in the block layer. SCSI_MLQUEUE_*_BUSY in SCSI is primarily
> using the block layer functionality of BLKPREP_DEFER to begin with, so I
> think we're pretty close to providing all that already.

It's not quite that simple. I am referring mainly to arbitration across
multiple request_queue's. SCSI has useful code in place to deal with
target-busy and host-busy conditions, both of which could potentially be
blocking and unblocking multiple request queues.

mlqueue is much more than just a wrapper over block requeueing
functions. Read scsi_next_command() and scsi_run_queue(), and grep for
starved_list, host_{busy,blocked}, target_{busy,blocked},
device_{busy,blocked}.

In our master/slave case, we must choose between queue A and queue B,
making sure to starve neither. For simplex DMA, we potentially have
queues A, B, C and D serving requests across the "bus bottleneck," and
must ensure no starvation of A, B, C or D.


Although I have no code to back this up, my gut feeling is that a
"request queue group" object, with associated functions, that would be
the appropriate place for cross-queue or "host-wide" (as in, struct
Scsi_Host or struct ata_host) functionality.

Whatever the solution, libata definitely makes use of SCSI's
cross-request_queue arbitration, so any move to block will require
similar functionality.

Jeff


2009-04-23 10:43:22

by Jens Axboe

[permalink] [raw]
Subject: Re: [PATCH] libata: rewrite SCSI host scheme to be one per ATA host

On Thu, Apr 23 2009, Jeff Garzik wrote:
> Jens Axboe wrote:
>> On Wed, Apr 22 2009, Jeff Garzik wrote:
>>> Jeff Garzik wrote:
>>>> Currently, libata creates a Scsi_Host per port. This was originally
>>>> done to leverage SCSI's infrastructure to arbitrate among master/slave
>>>> devices, but is not needed for most modern SATA controllers. And I
>>>> _think_ it is not needed for master/slave if done properly, either.
>>> BTW note the above, with regards to the libata SCSI->block
>>> conversion. libata currently relies on SCSI for some amount of
>>> generic device arbitration, in several situations (see ->qc_defer,
>>> SCSI_MLQUEUE_.*_BUSY). libata expects SCSI to be intelligent and not
>>> starve devices, etc.
>>
>> Defer looks like internal policy, I don't see that functioning any
>> different in the block layer. SCSI_MLQUEUE_*_BUSY in SCSI is primarily
>> using the block layer functionality of BLKPREP_DEFER to begin with, so I
>> think we're pretty close to providing all that already.
>
> It's not quite that simple. I am referring mainly to arbitration across
> multiple request_queue's. SCSI has useful code in place to deal with
> target-busy and host-busy conditions, both of which could potentially be
> blocking and unblocking multiple request queues.
>
> mlqueue is much more than just a wrapper over block requeueing
> functions. Read scsi_next_command() and scsi_run_queue(), and grep for
> starved_list, host_{busy,blocked}, target_{busy,blocked},
> device_{busy,blocked}.
>
> In our master/slave case, we must choose between queue A and queue B,
> making sure to starve neither. For simplex DMA, we potentially have
> queues A, B, C and D serving requests across the "bus bottleneck," and
> must ensure no starvation of A, B, C or D.
>
>
> Although I have no code to back this up, my gut feeling is that a
> "request queue group" object, with associated functions, that would be
> the appropriate place for cross-queue or "host-wide" (as in, struct
> Scsi_Host or struct ata_host) functionality.
>
> Whatever the solution, libata definitely makes use of SCSI's
> cross-request_queue arbitration, so any move to block will require
> similar functionality.

Agree, I think we discussed this many years ago as well. I guess a
request queue grouping with fair arbitration would suffice. If you need
to defer for a device beyond that, a simple BLKPREP_DEFER would just
postpone service until the next round. Probably allow both "skip until
next round", or "defer the entire group, service me again next time
first".

--
Jens Axboe

2009-04-23 11:01:19

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] libata: rewrite SCSI host scheme to be one per ATA host

Grant Grundler wrote:
> On Wed, Apr 22, 2009 at 2:09 AM, Jeff Garzik <[email protected]> wrote:
>> Currently, libata creates a Scsi_Host per port. This was originally
>> done to leverage SCSI's infrastructure to arbitrate among master/slave
>> devices, but is not needed for most modern SATA controllers. And I
>> _think_ it is not needed for master/slave if done properly, either.
>>
>> The patch below converts libata such that there is now a 1:1
>> correspondence between struct Scsi_Host and struct ata_host. ATA ports
>> are represented as SCSI layer 'channels', which is more natural.
>
> Jeff,
> So far in reading this, the only reasons I gather for changing this
> mapping are "not needed" and "is more natural". Data Center
> environments (not just Google's) like to track disks in many different
> ways, including the SCSI identifiers since this one "key" for physical
> location. Breaking the current mappings is going to cause some people
> a world of pain since they will need to manually build (and integrate)
> old->new maps of the SCSI identifiers. Can you propose some real,
> tangible benefit to making this change? (e.g. enables some other
> feature)

Sure there are compat issues, just like there are compat issues with the
existing consensus goal of moving libata to the block layer -- part of
which implies that ATA disks would be served via a "native" block device
rather than drivers/scsi/sd.c.

So at least to me, it is axiomatic that these issues will be examined.

As to benefits, the phrase "more natural" means the code naturally
aligns with existing object topologies (ata_host becomes analagous to
Scsi_Host), which always has a long list of technical benefits.

- we get to remove all the ugly hacks currently in place that assume
ata_port is _the_ first class object.
- we get to remove all the workarounds where SCSI assumes it manipulates
all devices on a controller (not true in current libata)
- SCSI (soon block) host-wide busy, block etc functionality now works as
expected
- it makes the libata conversion from SCSI to block layer easier
- it makes integration with SAS+SATA devices such as mvsas or ipr easier
- the list goes on; that is just off the top of my head, before my
morning Mountain Dew

"more natural" also solves a longstanding user confusion/complaint about
libata: users expected that libata would export each ATA "channel"
(bus) as a SCSI channel.


> Mark already pointed out this might cause issues with Error Handling
> (forcing a review of all that code). So before triggering other
> developers (e.g. HW vendors) do that kind of work I'd like to hear
> what the reward is going to be at the end.

Are you aware that EH is already receiving a stream of updates, moving
it from SCSI to the block layer? This area has been in constant motion
since, well, Tejun arrived and started improving things! :)

Jeff



2009-04-23 17:59:37

by Grant Grundler

[permalink] [raw]
Subject: Re: [PATCH] libata: rewrite SCSI host scheme to be one per ATA host

On Thu, Apr 23, 2009 at 4:00 AM, Jeff Garzik <[email protected]> wrote:
...
> Sure there are compat issues, just like there are compat issues with the
> existing consensus goal of moving libata to the block layer -- part of which
> implies that ATA disks would be served via a "native" block device rather
> than drivers/scsi/sd.c.
>
> So at least to me, it is axiomatic that these issues will be examined.

agree

>
> As to benefits, the phrase "more natural" means the code naturally aligns
> with existing object topologies (ata_host becomes analagous to Scsi_Host),
> which always has a long list of technical benefits.
>
> - we get to remove all the ugly hacks currently in place that assume
> ata_port is _the_ first class object.
> - we get to remove all the workarounds where SCSI assumes it manipulates all
> devices on a controller (not true in current libata)
> - SCSI (soon block) host-wide busy, block etc functionality now works as
> expected
> - it makes the libata conversion from SCSI to block layer easier
> - it makes integration with SAS+SATA devices such as mvsas or ipr easier
> - the list goes on; that is just off the top of my head, before my morning
> Mountain Dew

Your initial list is good. In particular the issue around "SCSI Host-wide busy"
working as expected. Of all the things listed, this is the only one that *I* can
clearly identify as a user visible functional change.

I'm not familiar with the the "workarounds where SCSI assumes it manipulates
all devices on a controller" issue. The few SATA controllers I've looked at can
deal with each port independently - e.g. discovery and phy reset. Anyway, this
seems to be closely tied with "SCSI Host-wide Busy".

One reason I was thinking of NOT list above: "wide port" in SAS 2.0 controllers.
aka "port aggregation". E.g. http://www.pmc-sierra.com/products/details/pm8005/

To change port aggregation on the fly requires the SCSI host controller to be
manageable object. This should be a change in transport and not a change
in devices available....and there are some other problems with implementing
this but this is the main one I initially see.


> "more natural" also solves a longstanding user confusion/complaint about
> libata:  users expected that libata would export each ATA "channel" (bus) as
> a SCSI channel.

I haven't seen that complaint. And I'd be impressed by any "normal users" that
know the difference between "host" and "channel". This sounds like a developer
complaint to me. AFAICT, users just accepted the initial mapping.

If the mapping is going to change, it makes sense to address
outstanding complaints.
But it'd be helpful to attribute the complaint/problem report to a real person
and which HW they saw the issue on.

>> Mark already pointed out this might cause issues with Error Handling
>> (forcing a review of all that code). So before triggering other
>> developers (e.g. HW vendors) do that kind of work I'd like to hear
>> what the reward is going to be at the end.
>
> Are you aware that EH is already receiving a stream of updates, moving it
> from SCSI to the block layer?  This area has been in constant motion since,
> well, Tejun arrived and started improving things!  :)

Yes, I have been following (and in generally very impressed with) Tejun's work.
I've read through about 1/2 of his patches. I clearly don't understand the
libata/SCSI subsystems as well he does.

But Tejun isn't the only person working on SATA drivers. AFAICT, the HW vendors
are doing most of the work to support new SAS/SATA controllers. I'm
thinking of Marvell,
Intel, LSI, and Broadcom off the top of my head. I'm sure there are others
(Silicon Image?). Having a moving target will just make it harder for them
to focus on their core business (building/supporting controllers)
instead of trying
to keep up with kernel.org. I think has historically been a bigger problem.

thanks,
grant

2009-04-23 18:09:22

by James Bottomley

[permalink] [raw]
Subject: Re: [PATCH] libata: rewrite SCSI host scheme to be one per ATA host

On Thu, 2009-04-23 at 10:59 -0700, Grant Grundler wrote:
> > As to benefits, the phrase "more natural" means the code naturally aligns
> > with existing object topologies (ata_host becomes analagous to Scsi_Host),
> > which always has a long list of technical benefits.
> >
> > - we get to remove all the ugly hacks currently in place that assume
> > ata_port is _the_ first class object.
> > - we get to remove all the workarounds where SCSI assumes it manipulates all
> > devices on a controller (not true in current libata)
> > - SCSI (soon block) host-wide busy, block etc functionality now works as
> > expected
> > - it makes the libata conversion from SCSI to block layer easier
> > - it makes integration with SAS+SATA devices such as mvsas or ipr easier
> > - the list goes on; that is just off the top of my head, before my morning
> > Mountain Dew
>
> Your initial list is good. In particular the issue around "SCSI Host-wide busy"
> working as expected. Of all the things listed, this is the only one that *I* can
> clearly identify as a user visible functional change.
>
> I'm not familiar with the the "workarounds where SCSI assumes it manipulates
> all devices on a controller" issue. The few SATA controllers I've looked at can
> deal with each port independently - e.g. discovery and phy reset. Anyway, this
> seems to be closely tied with "SCSI Host-wide Busy".
>
> One reason I was thinking of NOT list above: "wide port" in SAS 2.0 controllers.
> aka "port aggregation". E.g. http://www.pmc-sierra.com/products/details/pm8005/
>
> To change port aggregation on the fly requires the SCSI host controller to be
> manageable object. This should be a change in transport and not a change
> in devices available....and there are some other problems with implementing
> this but this is the main one I initially see.

This isn't really a correct assessment. Wide ports are a SAS only
problem. So sas has phys and ports ... and it's the port (essentially a
virtual object) that communicates in the link diagram.

A wide port has two or more phys in it. You can see the handling of
this in libsas and the sas transport class today, but it's all handled
in a fashion completely invisible to the SCSI mid layer ... it's an
inessential abstraction, if you will.

James

2009-04-24 11:01:00

by Stefan Richter

[permalink] [raw]
Subject: Re: [PATCH] libata: rewrite SCSI host scheme to be one per ATA host

Grant Grundler wrote:
> Data Center
> environments (not just Google's) like to track disks in many different
> ways, including the SCSI identifiers since this one "key" for physical
> location. Breaking the current mappings is going to cause some people
> a world of pain

If by SCSI identifiers you mean the Linux SCSI core's h:c:i:l, then
remember that these are scsi-core internal artifacts without any meaning
to lower and upper layers whatsoever, including userspace. (Exception:
The c:i:l part has some significance with SCSI Parallel Interface
attached hardware.)

> since they will need to manually build (and integrate)
> old->new maps of the SCSI identifiers.

Stock udev already provides mapping according to actually useful
identifiers (if the respective transport provides them: persistent and
unique identifiers of targets and logical units).
--
Stefan Richter
-=====-==--= -=-- ==---
http://arcgraph.de/sr/