set a base index for libsas based ata devices

Message ID CAF2xp_F3ea-n4R8jG8UpHEE3YmvKN7UWBAn36_QXMg-e34tY_w@mail.gmail.com
State New
Headers show

Commit Message

Peter Chang Dec. 20, 2016, 6:15 p.m.
we discovered this when futzing w/ the queue depth parameter for ata
disks behind the pm8006 controller. setting depth == 1 should disable
ncq, but the sysfs part silently fails and we continue sending the
fpdma command variants. no one else probably cares about the disabling
ncq path, but we do like to test.

anyway, adding both the ide and scsi lists because i'm not quite sure
there's a separate libsas list and a single commit seems better for
this.

\p

Comments

James Bottomley Dec. 20, 2016, 6:30 p.m. | #1
On Tue, 2016-12-20 at 10:15 -0800, Peter Chang wrote:
> we discovered this when futzing w/ the queue depth parameter for ata

> disks behind the pm8006 controller. setting depth == 1 should disable

> ncq, but the sysfs part silently fails and we continue sending the

> fpdma command variants. no one else probably cares about the 

> disabling ncq path, but we do like to test.


I'd actually disagree with this assertion; it's why tagging (what you
mean by ncq) and queue depth are separate.  Queue depth represents the
number of outstanding commands we sent on the wire; however, it often
excludes things like sense probes and error handling commands, so
tagged depth==1 is a different operating environment from untagged. 
 Some transports actually have no untagged variant nowadays, so it's
physically impossible to disable tagging.

James

> anyway, adding both the ide and scsi lists because i'm not quite sure

> there's a separate libsas list and a single commit seems better for

> this.




--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig Dec. 21, 2016, 8:03 a.m. | #2
On Tue, Dec 20, 2016 at 10:30:34AM -0800, James Bottomley wrote:
> I'd actually disagree with this assertion; it's why tagging (what you

> mean by ncq) and queue depth are separate.  Queue depth represents the

> number of outstanding commands we sent on the wire; however, it often

> excludes things like sense probes and error handling commands, so

> tagged depth==1 is a different operating environment from untagged. 

>  Some transports actually have no untagged variant nowadays, so it's

> physically impossible to disable tagging.


Yes.  We have the queue_type sysfs file that also used to be writeable
and allow changing the queue type, but it's never been used for
anything.

For debugging you can clear the tagged_supported flag in the driver,
but there should be no reason for doing that during normal operation
for a SAS HBA driver.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

From 6bdfcb35a074d9c58c180f8c512706faf7d6c7cb Mon Sep 17 00:00:00 2001
From: peter chang <dpf@google.com>
Date: Tue, 20 Dec 2016 09:48:45 -0800
Subject: [PATCH] set a base index for libsas based ata devices

libsas hosts allow multiple links, but when the controller
supports SATA devices control is handed to libata. this means
that an attached scsi device will be setup properly, but device
management requests and sysfs futzing don't get routed correctly
because the device lookup fails.

Tested:
- pre-patch:
jkgg70:~# ls -d /sys/block/sd*
/sys/block/sda
jkgg70:~# modprobe pm80xx
jkgg70:~# cat /sys/block/sdb/device/queue_depth
31
jkgg70:~# echo 1 > /sys/block/sdb/device/queue_depth
jkgg70:~# cat /sys/block/sdb/device/queue_depth
31
- post-patch:
jkgg70:~# modprobe pm80xx
jkgg70:~# cat /sys/block/sdb/device/queue_depth
31
jkgg70:~# echo 1 > /sys/block/sdb/device/queue_depth
jkgg70:~# cat /sys/block/sdb/device/queue_depth
1

Signed-off-by: peter chang <dpf@google.com>
---
 drivers/ata/libata-scsi.c           | 33 ++++++++++++++++++++++++++++++++-
 drivers/scsi/libsas/sas_scsi_host.c |  4 ++++
 include/linux/libata.h              |  3 +++
 3 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 1f863e7..340f144 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1088,7 +1088,7 @@  static void ata_to_sense_error(unsigned id, u8 drv_stat, u8 drv_err, u8 *sk,
  *	passthrough command, so we use the following sense data:
  *	sk = RECOVERED ERROR
  *	asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE
- *      
+ *
  *
  *	LOCKING:
  *	None.
@@ -3052,6 +3052,16 @@  static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
 
 static struct ata_device *ata_find_dev(struct ata_port *ap, int devno)
 {
+	/* adjust if this port is behind a libsas host rather than a
+	 * direct libata host. warn and fail if somehow we got out of
+	 * sync and we've a negative device number.
+	 */
+	devno -= ap->link.sas_host_base;
+	if (unlikely(devno < 0)) {
+		WARN_ON(devno < 0);
+		return NULL;
+	}
+
 	if (!sata_pmp_attached(ap)) {
 		if (likely(devno < ata_link_max_devices(&ap->link)))
 			return &ap->link.device[devno];
@@ -4332,6 +4342,27 @@  int ata_scsi_queuecmd(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
 }
 
 /**
+ *	ata_sas_set_link_base - set the libata link's 'base' because
+ *	libsas hosts have more ports / links.
+ *	@ap: ATA port to which the target is attached
+ *	@starget: SCSI target being attached
+ */
+void ata_sas_set_link_base(struct ata_port *ap, struct scsi_target *starget)
+{
+	unsigned int host_base;
+
+	if (!sata_pmp_attached(ap)) {
+		WARN_ON(starget->channel);
+		host_base = starget->id;
+	} else {
+		WARN_ON(starget->id);
+		host_base = starget->channel;
+	}
+	ap->link.sas_host_base = host_base;
+}
+EXPORT_SYMBOL_GPL(ata_sas_set_link_base);
+
+/**
  *	ata_scsi_simulate - simulate SCSI command on ATA device
  *	@dev: the target device
  *	@cmd: SCSI command being sent to device.
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index 519dac4..d8300bc 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -859,6 +859,10 @@  int sas_target_alloc(struct scsi_target *starget)
 
 	kref_get(&found_dev->kref);
 	starget->hostdata = found_dev;
+
+	if (dev_is_sata(found_dev))
+		ata_sas_set_link_base(found_dev->sata_dev.ap, starget);
+
 	return 0;
 }
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index c170be5..323811f 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -791,6 +791,7 @@  struct ata_acpi_gtm {
 struct ata_link {
 	struct ata_port		*ap;
 	int			pmp;		/* port multiplier port # */
+	int			sas_host_base;	/* host relative id */
 
 	struct device		tdev;
 	unsigned int		active_tag;	/* active tag on this link */
@@ -1130,6 +1131,8 @@  extern int ata_sas_port_start(struct ata_port *ap);
 extern void ata_sas_port_stop(struct ata_port *ap);
 extern int ata_sas_slave_configure(struct scsi_device *, struct ata_port *);
 extern int ata_sas_queuecmd(struct scsi_cmnd *cmd, struct ata_port *ap);
+extern void ata_sas_set_link_base(struct ata_port *ap,
+				  struct scsi_target *starget);
 extern int sata_scr_valid(struct ata_link *link);
 extern int sata_scr_read(struct ata_link *link, int reg, u32 *val);
 extern int sata_scr_write(struct ata_link *link, int reg, u32 val);
-- 
2.8.0.rc3.226.g39d4020