mpt2sas: Enable TLR for SSP TAPE drives (Added SAS Transport APIs)

PATCH In review Kashyap, Desai 2 participants 5 messages Dec 16, 2009 – Jan 20, 2010 SCSI

lore

Share & tools

lore

Kashyap, Desai

author

2009-12-16 13:53 UTC #0 lore raw

PATCH 9/10 mpt2sas: Enable TLR for SSP TAPE drives (Added SAS Transport APIs)

Added tlr_enabled bit in sas_device structure, which will have boolean value 0 for disabled and 1 for enabled.

As suggested by James B, I have added sas_tlr_supported(),sas_enable_tlr(), sas_disable_tlr() functions at SAS transport layer so that any other Low layer driver can use those APIs.

SAS transport API sas_tlr_supported() will send vpd page 0x90, to check the TLR support. If TLR is supported for end device, MPT2SAS driver will enable the TLR bit in the SCSI_IO for every request. If there is a response with MPI2_SCSITASKMGMT_RSP_INVALID_FRAME, the driver will turn off the TLR logic.

Signed-off-by Kashyap Desai <kashyap.desai@xxxxxxx>

Reviewed-by Eric Moore <Eric.moore@xxxxxxx>

---

drivers/scsi/mpt2sas/mpt2sas_scsih.c +35 ~~-20~~

index b748893..d9492a9 100644
@@ -1309,7 +1309,6 @@ _scsih_slave_alloc(struct scsi_device *sdev)
 	struct MPT2SAS_DEVICE *sas_device_priv_data;
 	struct scsi_target *starget;
 	struct _raid_device *raid_device;
-	struct _sas_device *sas_device;
 	unsigned long flags;
 
 	sas_device_priv_data = kzalloc(sizeof(struct scsi_device), GFP_KERNEL);

@@ -1336,21 +1335,8 @@ _scsih_slave_alloc(struct scsi_device *sdev)
 		if (raid_device)
 			raid_device->sdev = sdev; /* raid is single lun */
 		spin_unlock_irqrestore(&ioc->raid_device_lock, flags);
-	} else {
-		/* set TLR bit for SSP devices */
-		if (!(ioc->facts.IOCCapabilities &
-		     MPI2_IOCFACTS_CAPABILITY_TLR))
-			goto out;
-		spin_lock_irqsave(&ioc->sas_device_lock, flags);
-		sas_device = mpt2sas_scsih_sas_device_find_by_sas_address(ioc,
-		   sas_device_priv_data->sas_target->sas_address);
-		spin_unlock_irqrestore(&ioc->sas_device_lock, flags);
-		if (sas_device && sas_device->device_info &
-		    MPI2_SAS_DEVICE_INFO_SSP_TARGET)
-			sas_device_priv_data->flags |= MPT_DEVICE_TLR_ON;
 	}
 
- out:
 	return 0;
 }

@@ -1617,6 +1603,32 @@ _scsih_get_volume_capabilities(struct MPT2SAS_ADAPTER *ioc,
 }
 
 /**
+ * _scsih_enable_tlr - setting TLR flags
+ * @ioc: per adapter object
+ * @sdev: scsi device struct
+ *
+ * Enabling Transaction Layer Retries for tape devices when
+ * vpd page 0x90 is present
+ *
+ */
+static void
+_scsih_enable_tlr(struct MPT2SAS_ADAPTER *ioc, struct scsi_device *sdev)
+{
+	/* only for TAPE */
+	if (sdev->type != TYPE_TAPE)
+		return;
+
+	if (!(ioc->facts.IOCCapabilities & MPI2_IOCFACTS_CAPABILITY_TLR))
+		return;
+
+	sas_enable_tlr(sdev);
+	sdev_printk(KERN_INFO, sdev, "TLR %s\n",
+	    sdev->tlr_enabled ? "Enabled" : "Disabled");
+	return;
+
+}
+
+/**
  * _scsih_slave_configure - device configure routine.
  * @sdev: scsi device struct
  *

@@ -1761,8 +1773,10 @@ _scsih_slave_configure(struct scsi_device *sdev)
 
 	_scsih_change_queue_depth(sdev, qdepth, SCSI_QDEPTH_DEFAULT);
 
-	if (ssp_target)
+	if (ssp_target) {
 		sas_read_port_mode_page(sdev);
+		_scsih_enable_tlr(ioc, sdev);
+	}
 	return 0;
 }

@@ -3049,7 +3063,7 @@ _scsih_qcmd(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd *))
 	} else
 		mpi_control |= MPI2_SCSIIO_CONTROL_SIMPLEQ;
 
-	if ((sas_device_priv_data->flags & MPT_DEVICE_TLR_ON))
+	if (scmd->device->tlr_enabled)
 		mpi_control |= MPI2_SCSIIO_CONTROL_TLR_ON;
 
 	smid = mpt2sas_base_get_smid_scsiio(ioc, ioc->scsi_io_cb_idx, scmd);

@@ -3438,10 +3452,11 @@ _scsih_io_done(struct MPT2SAS_ADAPTER *ioc, u16 smid, u8 msix_index, u32 reply)
 		    le32_to_cpu(mpi_reply->ResponseInfo) & 0xFF;
 	if (!sas_device_priv_data->tlr_snoop_check) {
 		sas_device_priv_data->tlr_snoop_check++;
-		if ((sas_device_priv_data->flags & MPT_DEVICE_TLR_ON) &&
-		    response_code == MPI2_SCSITASKMGMT_RSP_INVALID_FRAME)
-			sas_device_priv_data->flags &=
-			    ~MPT_DEVICE_TLR_ON;
+		if ((scmd->device->tlr_enabled) &&
+		    response_code == MPI2_SCSITASKMGMT_RSP_INVALID_FRAME) {
+			sas_disable_tlr(scmd->device);
+			sdev_printk(KERN_INFO, scmd->device, "TLR disabled\n");
+		}
 	}
 
 	xfer_cnt = le32_to_cpu(mpi_reply->TransferCount);

drivers/scsi/scsi_transport_sas.c +55 -0

61 lines not on this page · raw message

include/scsi/scsi_device.h +1 -0

index 68d185c..7c1361b 100644
@@ -112,6 +112,7 @@ struct scsi_device {
 				 * scsi_devinfo.[hc]. For now used only to
 				 * pass settings from slave_alloc to scsi
 				 * core. */
+	unsigned tlr_enabled:1;	/* set tlr flags */
 	unsigned writeable:1;
 	unsigned removable:1;
 	unsigned changed:1;	/* Data invalid due to media change */

include/scsi/scsi_transport_sas.h +4 -0

index 61ad359..2fb099f 100644
@@ -181,6 +181,10 @@ extern int sas_phy_add(struct sas_phy *);
 extern void sas_phy_delete(struct sas_phy *);
 extern int scsi_is_sas_phy(const struct device *);
 
+unsigned int sas_tlr_supported(struct scsi_device *);
+void sas_disable_tlr(struct scsi_device *);
+void sas_enable_tlr(struct scsi_device *);
+
 extern struct sas_rphy *sas_end_device_alloc(struct sas_port *);
 extern struct sas_rphy *sas_expander_alloc(struct sas_port *, enum sas_device_type);
 void sas_rphy_free(struct sas_rphy *);

James Bottomley

2010-01-18 16:15 UTC #1 lore raw

Quoted 7 lines · Desai

Added tlr_enabled bit in sas_device structure, which will have boolean value 0 for disabled and 1 for enabled.
As suggested by James B, I have added sas_tlr_supported(),sas_enable_tlr(), sas_disable_tlr() functions at SAS transport layer so that any other Low layer driver can use those APIs.

Quoted 8 lines

SAS transport API sas_tlr_supported() will send vpd page 0x90, to check the TLR support. If TLR is supported for end device, MPT2SAS driver will enable the TLR bit in the SCSI_IO for every request. If there is a response with MPI2_SCSITASKMGMT_RSP_INVALID_FRAME, the driver will turn off the TLR logic.
Signed-off-by Kashyap Desai <kashyap.desai@xxxxxxx>
Reviewed-by Eric Moore <Eric.moore@xxxxxxx>

What's with the xxxx? I've got better things to do than fix up changelog signoffs.

Quoted 17 lines

drivers/scsi/scsi_transport_sas.c +9 -0

index fd47cb1..41b8eab 100644
@@ -358,6 +358,61 @@ void sas_remove_host(struct Scsi_Host *shost)
 }
 EXPORT_SYMBOL(sas_remove_host);
 
+/**
+ * sas_tlr_supported - checking TLR bit in vpd 0x90
+ * @sdev: scsi device struct
+ *
+ * Check Transport Layer Retries are supported or not.
+ * If vpd page 0x90 is present, TRL is supported.
+ *
+ */
+unsigned int

drivers/scsi/scsi_transport_sas.c void sas_remove_host(struct Scsi_Host *shost) · line 379

Quoted 10 lines

drivers/scsi/scsi_transport_sas.c +10 -0

+sas_tlr_supported(struct scsi_device *sdev)
+{
+	char *buffer;
+
+	buffer = scsi_get_vpd_page(sdev, 0x90);
+	if (buffer == NULL)
+		return 0;
+
+	kfree(buffer);
+	return 1;

This can't be right. TLR supported is the lowest bit of the 8th byte of the port entry ... you can't condition this on whether the protocol page exists or not.

include/scsi/scsi_device.h struct scsi_device { · line 115

Quoted 9 lines

include/scsi/scsi_device.h +1 -0

index 68d185c..7c1361b 100644
@@ -112,6 +112,7 @@ struct scsi_device {
 				 * scsi_devinfo.[hc]. For now used only to
 				 * pass settings from slave_alloc to scsi
 				 * core. */
+	unsigned tlr_enabled:1;	/* set tlr flags */

And this is wrong: the tlr is SAS specific, it belongs in the end device flags like all the other SAS specific entries.

We also need to expose it to the user in case they want to change the flag.

How about separating the two patches and doing this for the transport class? If it looks OK, I'll add it to the patch sequence and alter this commit.

James

---

drivers/scsi/scsi_sas_internal.h +1 -1

index 998cb5b..6266a5d 100644
@@ -5,7 +5,7 @@
 #define SAS_PHY_ATTRS		17
 #define SAS_PORT_ATTRS		1
 #define SAS_RPORT_ATTRS		7
-#define SAS_END_DEV_ATTRS	3
+#define SAS_END_DEV_ATTRS	5
 #define SAS_EXPANDER_ATTRS	7
 
 struct sas_internal {

drivers/scsi/scsi_transport_sas.c +90 -6

129 lines not on this page · raw message

include/scsi/scsi_transport_sas.h +6 -0

index 61ad359..2988fcf 100644
@@ -107,6 +107,8 @@ struct sas_end_device {
 	struct sas_rphy		rphy;
 	/* flags */
 	unsigned		ready_led_meaning:1;
+	unsigned		tlr_supported:1;
+	unsigned		tlr_enabled:1;
 	/* parameters */
 	u16			I_T_nexus_loss_timeout;
 	u16			initiator_response_timeout;

@@ -181,6 +183,10 @@ extern int sas_phy_add(struct sas_phy *);
 extern void sas_phy_delete(struct sas_phy *);
 extern int scsi_is_sas_phy(const struct device *);
 
+unsigned int sas_tlr_supported(struct scsi_device *);
+void sas_disable_tlr(struct scsi_device *);
+void sas_enable_tlr(struct scsi_device *);
+
 extern struct sas_rphy *sas_end_device_alloc(struct sas_port *);
 extern struct sas_rphy *sas_expander_alloc(struct sas_port *, enum sas_device_type);
 void sas_rphy_free(struct sas_rphy *);

Desai, Kashyap

author

2010-01-19 18:27 UTC #2 lore raw

Quoted 26 lines

-----Original Message-----
From: James Bottomley [mailto:[email protected]]
Sent: Monday, January 18, 2010 9:45 PM
To: Desai, Kashyap
Cc: [email protected]; Moore, Eric; Prakash, Sathya
Subject: Re: [PATCH 9/10] mpt2sas: Enable TLR for SSP TAPE drives (Added SAS Transport APIs)
Quoted 1 line · Desai
Added tlr_enabled bit in sas_device structure, which will have
boolean
Quoted 3 lines
value 0 for disabled and 1 for enabled.
As suggested by James B, I have added
sas_tlr_supported(),sas_enable_tlr(),
Quoted 5 lines
sas_disable_tlr() functions at SAS transport layer so that any other Low layer driver can use those APIs.
SAS transport API sas_tlr_supported() will send vpd page 0x90, to check the TLR support. If TLR is supported for end device, MPT2SAS
driver
Quoted 1 line
will enable the TLR bit in the SCSI_IO for every request. If there is
a
Quoted 1 line
response with MPI2_SCSITASKMGMT_RSP_INVALID_FRAME, the driver will

Quoted 8 lines

turn off
Quoted 4 lines
the TLR logic.
Signed-off-by Kashyap Desai <kashyap.desai@xxxxxxx>
Reviewed-by Eric Moore <Eric.moore@xxxxxxx>
What's with the xxxx? I've got better things to do than fix up changelog signoffs.

Really Sorry about this gaffe.

Quoted 27 lines

Review

Quoted 2 lines

(quoted diff) +0 -0

b/drivers/scsi/scsi_transport_sas.c

Quoted 24 lines

index fd47cb1..41b8eab 100644

drivers/scsi/scsi_transport_sas.c +17 -0

@@ -358,6 +358,61 @@ void sas_remove_host(struct Scsi_Host *shost)
 }
 EXPORT_SYMBOL(sas_remove_host);
 
+/**
+ * sas_tlr_supported - checking TLR bit in vpd 0x90
+ * @sdev: scsi device struct
+ *
+ * Check Transport Layer Retries are supported or not.
+ * If vpd page 0x90 is present, TRL is supported.
+ *
+ */
+unsigned int
+sas_tlr_supported(struct scsi_device *sdev)
+{
+	char *buffer;
+
+	buffer = scsi_get_vpd_page(sdev, 0x90);
+	if (buffer == NULL)
+		return 0;
+

Quoted 8 lines

drivers/scsi/scsi_transport_sas.c void sas_remove_host(struct Scsi_Host *shost) · line 379
Quoted 2 lines
drivers/scsi/scsi_transport_sas.c +2 -0
+	kfree(buffer);
+	return 1;
This can't be right. TLR supported is the lowest bit of the 8th byte of
the port entry ... you can't condition this on whether the protocol page
exists or not.

Got this point. I will change accordingly.

Quoted 13 lines

include/scsi/scsi_device.h struct scsi_device { · line 115
Quoted 10 lines
include/scsi/scsi_device.h +1 -0
index 68d185c..7c1361b 100644
@@ -112,6 +112,7 @@ struct scsi_device {
 				 * scsi_devinfo.[hc]. For now used only to
 				 * pass settings from slave_alloc to scsi
 				 * core. */
+	unsigned tlr_enabled:1;	/* set tlr flags */
And this is wrong: the tlr is SAS specific, it belongs in the end device flags like all the other SAS specific entries.

I got it. I will move this into scsi_transport_sas.h in sas_end_device as flags.

Quoted 4 lines

We also need to expose it to the user in case they want to change the flag.

I am little confused on this point. Hope you don’t mind simplifying it.

Quoted 4 lines

How about separating the two patches and doing this for the transport class? If it looks OK, I'll add it to the patch sequence and alter this
commit.

James, What does this mean ?

Quoted 188 lines

James

---

drivers/scsi/scsi_sas_internal.h +1 -1

index 998cb5b..6266a5d 100644
@@ -5,7 +5,7 @@
 #define SAS_PHY_ATTRS		17
 #define SAS_PORT_ATTRS		1
 #define SAS_RPORT_ATTRS		7
-#define SAS_END_DEV_ATTRS	3
+#define SAS_END_DEV_ATTRS	5
 #define SAS_EXPANDER_ATTRS	7
 
 struct sas_internal {

drivers/scsi/scsi_transport_sas.c +11 -0

index f27e52d..8b30675 100644
@@ -155,6 +155,17 @@ static struct {
 sas_bitfield_name_search(linkspeed, sas_linkspeed_names)
 sas_bitfield_name_set(linkspeed, sas_linkspeed_names)
 
+static struct sas_end_device *sas_sdev_to_rdev(struct scsi_device
 *sdev)
+{
+	struct sas_rphy *rphy = target_to_rphy(sdev->sdev_target);
+	struct sas_end_device *rdev;
+
+	BUG_ON(rphy->identify.device_type != SAS_END_DEVICE);
+
+	rdev = rphy_to_end_device(rphy);
+	return rdev;
+}
+
 static void sas_smp_request(struct request_queue *q, struct Scsi_Host
 *shost,

drivers/scsi/scsi_transport_sas.c +77 -6

 			    struct sas_rphy *rphy)
 {

@@ -358,6 +369,78 @@ void sas_remove_host(struct Scsi_Host *shost)
 }
 EXPORT_SYMBOL(sas_remove_host);
 
+/**
+ * sas_tlr_supported - checking TLR bit in vpd 0x90
+ * @sdev: scsi device struct
+ *
+ * Check Transport Layer Retries are supported or not.
+ * If vpd page 0x90 is present, TRL is supported.
+ *
+ */
+unsigned int
+sas_tlr_supported(struct scsi_device *sdev)
+{
+	const int vpd_len = 32;
+	struct sas_end_device *rdev = sas_sdev_to_rdev(sdev);
+	char *buffer = kzalloc(vpd_len, GFP_KERNEL);
+	int ret = 0;
+
+	if (scsi_get_vpd_page(sdev, 0x90, buffer, vpd_len))
+		goto out;
+
+	/*
+	 * Magic numbers: the VPD Protocol page (0x90)
+	 * has a 4 byte header and then one entry per device port
+	 * the TLR bit is at offset 8 on each port entry
+	 * if we take the first port, that's at total offset 12
+	 */
+	ret = buffer[12] & 0x01;
+
+ out:
+	kfree(buffer);
+	rdev->tlr_supported = ret;
+	return ret;
+
+}
+EXPORT_SYMBOL(sas_tlr_supported);
+
+/**
+ * sas_disable_tlr - setting TLR flags
+ * @sdev: scsi device struct
+ *
+ * Seting tlr_enabled flag to 0.
+ *
+ */
+void
+sas_disable_tlr(struct scsi_device *sdev)
+{
+	struct sas_end_device *rdev = sas_sdev_to_rdev(sdev);
+
+	rdev->tlr_enabled = 0;
+}
+EXPORT_SYMBOL(sas_disable_tlr);
+
+/**
+ * sas_enable_tlr - setting TLR flags
+ * @sdev: scsi device struct
+ *
+ * Seting tlr_enabled flag 1.
+ *
+ */
+void sas_enable_tlr(struct scsi_device *sdev)
+{
+	unsigned int tlr_supported = 0;
+	tlr_supported  = sas_tlr_supported(sdev);
+
+	if (tlr_supported) {
+		struct sas_end_device *rdev = sas_sdev_to_rdev(sdev);
+
+		rdev->tlr_enabled = 1;
+	}
+
+	return;
+}
+EXPORT_SYMBOL(sas_enable_tlr);
 
 /*
  * SAS Phy attributes

@@ -1146,15 +1229,10 @@ sas_rphy_simple_attr(identify.phy_identifier,
 phy_identifier, "%d\n", u8);
 int sas_read_port_mode_page(struct scsi_device *sdev)
 {
 	char *buffer = kzalloc(BUF_SIZE, GFP_KERNEL), *msdata;
-	struct sas_rphy *rphy = target_to_rphy(sdev->sdev_target);
-	struct sas_end_device *rdev;
+	struct sas_end_device *rdev = sas_sdev_to_rdev(sdev);
 	struct scsi_mode_data mode_data;
 	int res, error;
 
-	BUG_ON(rphy->identify.device_type != SAS_END_DEVICE);
-
-	rdev = rphy_to_end_device(rphy);
-
 	if (!buffer)
 		return -ENOMEM;

@@ -1207,6 +1285,10 @@ sas_end_dev_simple_attr(I_T_nexus_loss_timeout,
 I_T_nexus_loss_timeout,
 			"%d\n", int);
 sas_end_dev_simple_attr(initiator_response_timeout,
 initiator_response_timeout,
 			"%d\n", int);
+sas_end_dev_simple_attr(tlr_supported, tlr_supported,
+			"%d\n", int);
+sas_end_dev_simple_attr(tlr_enabled, tlr_enabled,
+			"%d\n", int);

static DECLARE_TRANSPORT_CLASS(sas_expander_class,

drivers/scsi/scsi_transport_sas.c +2 -0

 			       "sas_expander", NULL, NULL, NULL);

@@ -1733,6 +1815,8 @@ sas_attach_transport(struct sas_function_template
 *ft)
 	SETUP_END_DEV_ATTRIBUTE(end_dev_ready_led_meaning);
 	SETUP_END_DEV_ATTRIBUTE(end_dev_I_T_nexus_loss_timeout);
 	SETUP_END_DEV_ATTRIBUTE(end_dev_initiator_response_timeout);
+	SETUP_END_DEV_ATTRIBUTE(end_dev_tlr_supported);
+	SETUP_END_DEV_ATTRIBUTE(end_dev_tlr_enabled);
 	i->end_dev_attrs[count] = NULL;

drivers/scsi/scsi_transport_sas.c +0 -0

 	count = 0;

include/scsi/scsi_transport_sas.h +6 -0

index 61ad359..2988fcf 100644
@@ -107,6 +107,8 @@ struct sas_end_device {
 	struct sas_rphy		rphy;
 	/* flags */
 	unsigned		ready_led_meaning:1;
+	unsigned		tlr_supported:1;
+	unsigned		tlr_enabled:1;
 	/* parameters */
 	u16			I_T_nexus_loss_timeout;
 	u16			initiator_response_timeout;

@@ -181,6 +183,10 @@ extern int sas_phy_add(struct sas_phy *);
 extern void sas_phy_delete(struct sas_phy *);
 extern int scsi_is_sas_phy(const struct device *);
 
+unsigned int sas_tlr_supported(struct scsi_device *);
+void sas_disable_tlr(struct scsi_device *);
+void sas_enable_tlr(struct scsi_device *);
+
 extern struct sas_rphy *sas_end_device_alloc(struct sas_port *);
 extern struct sas_rphy *sas_expander_alloc(struct sas_port *, enum
 sas_device_type);

Quoted 2 lines

void sas_rphy_free(struct sas_rphy *);

Thanks,
Kashyap

James Bottomley

2010-01-19 20:49 UTC #3 lore raw

Quoted 89 lines · Kashyap

Quoted 25 lines
-----Original Message-----
From: James Bottomley [mailto:[email protected]]
Sent: Monday, January 18, 2010 9:45 PM
To: Desai, Kashyap
Cc: [email protected]; Moore, Eric; Prakash, Sathya
Subject: Re: [PATCH 9/10] mpt2sas: Enable TLR for SSP TAPE drives (Added SAS Transport APIs)
Quoted 1 line · Desai
Added tlr_enabled bit in sas_device structure, which will have
boolean
Quoted 3 lines
value 0 for disabled and 1 for enabled.
As suggested by James B, I have added
sas_tlr_supported(),sas_enable_tlr(),
Quoted 5 lines
sas_disable_tlr() functions at SAS transport layer so that any other Low layer driver can use those APIs.
SAS transport API sas_tlr_supported() will send vpd page 0x90, to check the TLR support. If TLR is supported for end device, MPT2SAS
driver
Quoted 1 line
will enable the TLR bit in the SCSI_IO for every request. If there is
a
Quoted 1 line
response with MPI2_SCSITASKMGMT_RSP_INVALID_FRAME, the driver will
Quoted 8 lines
turn off
Quoted 4 lines
the TLR logic.
Signed-off-by Kashyap Desai <kashyap.desai@xxxxxxx>
Reviewed-by Eric Moore <Eric.moore@xxxxxxx>
What's with the xxxx? I've got better things to do than fix up changelog signoffs.
Really Sorry about this gaffe.
Quoted 27 lines
Review
Quoted 2 lines
(quoted diff) +0 -0
b/drivers/scsi/scsi_transport_sas.c
Quoted 24 lines
index fd47cb1..41b8eab 100644
drivers/scsi/scsi_transport_sas.c +17 -0
@@ -358,6 +358,61 @@ void sas_remove_host(struct Scsi_Host *shost)
 }
 EXPORT_SYMBOL(sas_remove_host);
 
+/**
+ * sas_tlr_supported - checking TLR bit in vpd 0x90
+ * @sdev: scsi device struct
+ *
+ * Check Transport Layer Retries are supported or not.
+ * If vpd page 0x90 is present, TRL is supported.
+ *
+ */
+unsigned int
+sas_tlr_supported(struct scsi_device *sdev)
+{
+	char *buffer;
+
+	buffer = scsi_get_vpd_page(sdev, 0x90);
+	if (buffer == NULL)
+		return 0;
+
Quoted 8 lines
drivers/scsi/scsi_transport_sas.c void sas_remove_host(struct Scsi_Host *shost) · line 379
Quoted 2 lines
drivers/scsi/scsi_transport_sas.c +2 -0
+	kfree(buffer);
+	return 1;
This can't be right. TLR supported is the lowest bit of the 8th byte of
the port entry ... you can't condition this on whether the protocol page
exists or not.
Got this point. I will change accordingly.
Quoted 13 lines
include/scsi/scsi_device.h struct scsi_device { · line 115
Quoted 10 lines
include/scsi/scsi_device.h +1 -0
index 68d185c..7c1361b 100644
@@ -112,6 +112,7 @@ struct scsi_device {
 				 * scsi_devinfo.[hc]. For now used only to
 				 * pass settings from slave_alloc to scsi
 				 * core. */
+	unsigned tlr_enabled:1;	/* set tlr flags */
And this is wrong: the tlr is SAS specific, it belongs in the end device flags like all the other SAS specific entries.
I got it. I will move this into scsi_transport_sas.h in sas_end_device as flags.
Quoted 4 lines
We also need to expose it to the user in case they want to change the flag.

Quoted 7 lines

I am little confused on this point. Hope you don’t mind simplifying it.
Quoted 4 lines
How about separating the two patches and doing this for the transport class? If it looks OK, I'll add it to the patch sequence and alter this
commit.
James, What does this mean ?

It means does the patch look OK to you? As in storing the data in the transport class and separating it into supported and enabled. The actual API you use for mpt2sas is unchanged.

James

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Desai, Kashyap

author

2010-01-20 06:36 UTC #4 lore raw

Quoted 114 lines

-----Original Message-----
From: James Bottomley [mailto:[email protected]]
Sent: Wednesday, January 20, 2010 12:48 AM
To: Desai, Kashyap
Cc: [email protected]; Moore, Eric; Prakash, Sathya
Subject: RE: [PATCH 9/10] mpt2sas: Enable TLR for SSP TAPE drives (Added SAS Transport APIs)
Quoted 9 lines · Kashyap
Quoted 9 lines
-----Original Message-----
From: James Bottomley [mailto:[email protected]]
Sent: Monday, January 18, 2010 9:45 PM
To: Desai, Kashyap
Cc: [email protected]; Moore, Eric; Prakash, Sathya
Subject: Re: [PATCH 9/10] mpt2sas: Enable TLR for SSP TAPE drives (Added SAS Transport APIs)
Quoted 8 lines
Quoted 8 lines · Desai
Quoted 1 line
Added tlr_enabled bit in sas_device structure, which will have
boolean
Quoted 3 lines
value 0 for disabled and 1 for enabled.
As suggested by James B, I have added
sas_tlr_supported(),sas_enable_tlr(),
Quoted 1 line
sas_disable_tlr() functions at SAS transport layer so that any
other
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Low layer driver can use those APIs.
SAS transport API sas_tlr_supported() will send vpd page 0x90, to check the TLR support. If TLR is supported for end device,
MPT2SAS
Quoted 2 lines
Quoted 2 lines
driver
Quoted 1 line
will enable the TLR bit in the SCSI_IO for every request. If
there is
Quoted 2 lines
Quoted 2 lines
a
Quoted 1 line
response with MPI2_SCSITASKMGMT_RSP_INVALID_FRAME, the driver
will
Quoted 9 lines
Quoted 8 lines
turn off
Quoted 4 lines
the TLR logic.
Signed-off-by Kashyap Desai <kashyap.desai@xxxxxxx>
Reviewed-by Eric Moore <Eric.moore@xxxxxxx>
What's with the xxxx? I've got better things to do than fix up changelog signoffs.
Quoted 8 lines
Really Sorry about this gaffe.
Quoted 7 lines
Review
Quoted 2 lines
(quoted diff) +0 -0
b/drivers/scsi/scsi_transport_sas.c
drivers/scsi/scsi_transport_sas.c void sas_remove_host(struct Scsi_Host
Quoted 4 lines
index fd47cb1..41b8eab 100644
drivers/scsi/scsi_transport_sas.c +0 -0
@@ -358,6 +358,61 @@ void sas_remove_host(struct Scsi_Host
*shost)
Quoted 16 lines
Quoted 16 lines
Quoted 16 lines
drivers/scsi/scsi_transport_sas.c +13 -0
 }
 EXPORT_SYMBOL(sas_remove_host);
 
+/**
+ * sas_tlr_supported - checking TLR bit in vpd 0x90
+ * @sdev: scsi device struct
+ *
+ * Check Transport Layer Retries are supported or not.
+ * If vpd page 0x90 is present, TRL is supported.
+ *
+ */
+unsigned int
+sas_tlr_supported(struct scsi_device *sdev)
+{
+	char *buffer;
+
Quoted 8 lines
Quoted 8 lines
drivers/scsi/scsi_transport_sas.c void sas_remove_host(struct Scsi_Host · line 379
Quoted 6 lines
drivers/scsi/scsi_transport_sas.c +6 -0
+	buffer = scsi_get_vpd_page(sdev, 0x90);
+	if (buffer == NULL)
+		return 0;
+
+	kfree(buffer);
+	return 1;
This can't be right. TLR supported is the lowest bit of the 8th
byte
Quoted 7 lines
Quoted 4 lines
of
the port entry ... you can't condition this on whether the protocol page
exists or not.
Got this point. I will change accordingly.
Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +0 -0
b/include/scsi/scsi_device.h
Quoted 5 lines
Quoted 5 lines
include/scsi/scsi_device.h struct scsi_device { · line 112
Quoted 5 lines
index 68d185c..7c1361b 100644
include/scsi/scsi_device.h +0 -0
@@ -112,6 +112,7 @@ struct scsi_device {
 				 * scsi_devinfo.[hc]. For now used only
to
Quoted 7 lines
Quoted 6 lines
include/scsi/scsi_device.h struct scsi_device { · line 115
Quoted 3 lines
include/scsi/scsi_device.h +1 -0
 				 * pass settings from slave_alloc to scsi
 				 * core. */
+	unsigned tlr_enabled:1;	/* set tlr flags */
And this is wrong: the tlr is SAS specific, it belongs in the end device flags like all the other SAS specific entries.
I got it. I will move this into scsi_transport_sas.h in
sas_end_device as flags.
Quoted 2 lines
Quoted 2 lines
We also need to expose it to the user in case they want to change
the
Quoted 3 lines
Quoted 2 lines
flag.
I am little confused on this point. Hope you don’t mind simplifying
it.
Quoted 2 lines
Quoted 2 lines
How about separating the two patches and doing this for the

Quoted 8 lines

transport
Quoted 4 lines
Quoted 3 lines
class? If it looks OK, I'll add it to the patch sequence and alter this
commit.
James, What does this mean ?
It means does the patch look OK to you? As in storing the data in the transport class and separating it into supported and enabled. The

Thanks James. Now I understood your change.
This looks OK to me.

- Kashyap

Quoted 4 lines

actual API you use for mpt2sas is unchanged.
James