libsas: add host SMP processing

PATCH In review James Bottomley 2 participants 6 messages Dec 28, 2007 – Dec 30, 2007

Subsystems (4)

SCSI Block layer SCSI libsas BSG (block layer generic sg v4 driver)

lore

Share & tools

lore

James Bottomley

author maintainer

2007-12-28 23:36 UTC #0 lore raw

PATCH 1/1 libsas: add host SMP processing

This patch allows the sas host device to accept SMP commands. This brings the libsas attached hosts on to a par with the firmware based ones like mptsas.

James

drivers/scsi/libsas/Kconfig +9 -0

index c01a40d..18f33cd 100644
@@ -38,6 +38,15 @@ config SCSI_SAS_ATA
 		Builds in ATA support into libsas.  Will necessitate
 		the loading of libata along with libsas.
 
+config SCSI_SAS_HOST_SMP
+	bool "Support for SMP interpretation for SAS hosts"
+	default y
+	depends on SCSI_SAS_LIBSAS
+	help
+		Allows sas hosts to receive SMP frames.  Selecting this
+		option builds an SMP interpreter into libsas.  Say
+		N here if you want to save the few kb this consumes.
+
 config SCSI_SAS_LIBSAS_DEBUG
 	bool "Compile the SAS Domain Transport Attributes in debug mode"
 	default y

drivers/scsi/libsas/Makefile +1 -0

index fd387b9..60d6e93 100644
@@ -35,3 +35,4 @@ libsas-y +=  sas_init.o     \
 		sas_expander.o \
 		sas_scsi_host.o
 libsas-$(CONFIG_SCSI_SAS_ATA) +=	sas_ata.o
+libsas-$(CONFIG_SCSI_SAS_HOST_SMP) +=	sas_host_smp.o
\ No newline at end of file

drivers/scsi/libsas/sas_expander.c +3 -5

index 27674fe..76555b1 100644
@@ -1896,11 +1896,9 @@ int sas_smp_handler(struct Scsi_Host *shost, struct sas_rphy *rphy,
 	}
 
 	/* no rphy means no smp target support (ie aic94xx host) */
-	if (!rphy) {
-		printk("%s: can we send a smp request to a host?\n",
-		       __FUNCTION__);
-		return -EINVAL;
-	}
+	if (!rphy)
+		return sas_smp_host_handler(shost, req, rsp);
+
 	type = rphy->identify.device_type;
 
 	if (type != SAS_EDGE_EXPANDER_DEVICE &&

drivers/scsi/libsas/sas_host_smp.c new file +247 -0

247 lines not on this page · raw message

drivers/scsi/libsas/sas_internal.h +14 -0

index baa0666..b4f9368 100644
@@ -80,6 +80,20 @@ struct domain_device *sas_find_dev_by_rphy(struct sas_rphy *rphy);
 
 void sas_hae_reset(struct work_struct *work);
 
+#ifdef CONFIG_SCSI_SAS_HOST_SMP
+extern int sas_smp_host_handler(struct Scsi_Host *shost, struct request *req,
+				struct request *rsp);
+#else
+static inline int sas_smp_host_handler(struct Scsi_Host *shost,
+				       struct request *req,
+				       struct request *rsp)
+{
+	shost_printk(KERN_ERR, shost,
+		"Cannot send SMP to a sas host (not enabled in CONFIG)\n");
+	return -EINVAL;
+}
+#endif
+
 static inline void sas_queue_event(int event, spinlock_t *lock,
 				   unsigned long *pending,
 				   struct work_struct *work,

include/scsi/scsi_transport_sas.h +4 -4

index 007d929..09125fa 100644
@@ -11,10 +11,10 @@ struct sas_rphy;
 struct request;
 
 enum sas_device_type {
-	SAS_PHY_UNUSED,
-	SAS_END_DEVICE,
-	SAS_EDGE_EXPANDER_DEVICE,
-	SAS_FANOUT_EXPANDER_DEVICE,
+	SAS_PHY_UNUSED = 0,
+	SAS_END_DEVICE = 1,
+	SAS_EDGE_EXPANDER_DEVICE = 2,
+	SAS_FANOUT_EXPANDER_DEVICE = 3,
 };
 
 static inline int sas_protocol_ata(enum sas_protocol proto)

Currently, bsg doesn't return an error to user space since I had no idea how to convert errors such as EINVAL and ENOMEM into driver_status, transport_status, and device_status in struct sg_io_v4. I think that it's confusing that bsg don't return an error even if SMP requests aren't sent (e.g. devices are offline).

Do we need to map errors to the current error code in scsi/scsi.h (like DID_*) or define a new one for SMP?

James Bottomley

author maintainer

2007-12-29 15:44 UTC #2 lore raw

Quoted 18 lines · FUJITA Tomonori

From: James Bottomley <[email protected]>
Quoted 4 lines
drivers/scsi/libsas/sas_expander.c +0 -0
@@ -1896,11 +1896,9 @@ int sas_smp_handler(struct Scsi_Host *shost, struct sas_rphy *rphy,
 	}
drivers/scsi/libsas/sas_expander.c int sas_smp_handler(struct Scsi_Host *shost, struct sas_rphy *rphy, · line 1901
Quoted 10 lines
drivers/scsi/libsas/sas_expander.c +3 -5
 
 	/* no rphy means no smp target support (ie aic94xx host) */
-	if (!rphy) {
-		printk("%s: can we send a smp request to a host?\n",
-		       __FUNCTION__);
-		return -EINVAL;
-	}
+	if (!rphy)
+		return sas_smp_host_handler(shost, req, rsp);
+
I have one related question.

Quoted 8 lines

Currently, bsg doesn't return an error to user space since I had no idea how to convert errors such as EINVAL and ENOMEM into driver_status, transport_status, and device_status in struct sg_io_v4. I think that it's confusing that bsg don't return an error even if SMP requests aren't sent (e.g. devices are offline).
Do we need to map errors to the current error code in scsi/scsi.h (like DID_*) or define a new one for SMP?

Neither, I think ... the DID codes are only for things that actually pass through the SCSI stack. The way you implemented the smp functions in bsg, they're direct queue handlers themselves (Incidentally, that's another point about this: I think almost every use of bsg like this is going to be for SG_IO only, so it makes sense to move the actual queue handler into bsg, since they'll all share it).

The attached is the simplest patch that implements this. However, it unfortunately can't be applied yet ... the current SMP tools send receive buffers too large and libsas actually returns a data underrun error (which is now propagated).

I'll fix it with a series of patches transferring the protocol handler into bsg, adding error handling and correcting libsas.

James

block/bsg.c +3 -1

index 8e181ab..fbf0be3 100644
@@ -837,6 +837,7 @@ static long bsg_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct bsg_device *bd = file->private_data;
 	int __user *uarg = (int __user *) arg;
+	int ret;
 
 	switch (cmd) {
 		/*

@@ -889,12 +890,13 @@ static long bsg_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		if (rq->next_rq)
 			bidi_bio = rq->next_rq->bio;
 		blk_execute_rq(bd->queue, NULL, rq, 0);
+		ret = rq->errors;
 		blk_complete_sgv4_hdr_rq(rq, &hdr, bio, bidi_bio);
 
 		if (copy_to_user(uarg, &hdr, sizeof(hdr)))
 			return -EFAULT;
 
-		return 0;
+		return ret;
 	}
 	/*
 	 * block device ioctls

drivers/scsi/scsi_transport_sas.c +1 -0

index 87e786d..f2149d0 100644
@@ -173,6 +173,7 @@ static void sas_smp_request(struct request_queue *q, struct Scsi_Host *shost,
 
 		handler = to_sas_internal(shost->transportt)->f->smp_handler;
 		ret = handler(shost, rphy, req);
+		req->errors = ret;
 
 		spin_lock_irq(q->queue_lock);

FUJITA Tomonori

2007-12-29 16:06 UTC #3 lore raw

Quoted 35 lines · On Sat, 29 Dec 2007 09:44:32 -0600 James Bottomley…

Quoted 18 lines · FUJITA Tomonori
From: James Bottomley <[email protected]>
Quoted 4 lines
drivers/scsi/libsas/sas_expander.c +0 -0
@@ -1896,11 +1896,9 @@ int sas_smp_handler(struct Scsi_Host *shost, struct sas_rphy *rphy,
 	}
drivers/scsi/libsas/sas_expander.c int sas_smp_handler(struct Scsi_Host *shost, struct sas_rphy *rphy, · line 1901
Quoted 10 lines
drivers/scsi/libsas/sas_expander.c +3 -5
 
 	/* no rphy means no smp target support (ie aic94xx host) */
-	if (!rphy) {
-		printk("%s: can we send a smp request to a host?\n",
-		       __FUNCTION__);
-		return -EINVAL;
-	}
+	if (!rphy)
+		return sas_smp_host_handler(shost, req, rsp);
+
I have one related question.
Quoted 8 lines
Currently, bsg doesn't return an error to user space since I had no idea how to convert errors such as EINVAL and ENOMEM into driver_status, transport_status, and device_status in struct sg_io_v4. I think that it's confusing that bsg don't return an error even if SMP requests aren't sent (e.g. devices are offline).
Do we need to map errors to the current error code in scsi/scsi.h (like DID_*) or define a new one for SMP?
Neither, I think ... the DID codes are only for things that actually pass through the SCSI stack. The way you implemented the smp functions in bsg, they're direct queue handlers themselves (Incidentally, that's another point about this: I think almost every use of bsg like this is going to be for SG_IO only, so it makes sense to move the actual queue handler into bsg, since they'll all share it).

Quoted 4 lines

The attached is the simplest patch that implements this. However, it unfortunately can't be applied yet ... the current SMP tools send receive buffers too large and libsas actually returns a data underrun error (which is now propagated).

bsg read/write interface doens't return errors in this way (compatible with sg3 read/write interface). If we support only SG_IO for non SCSI request/response protocols, then that's fine.

James Bottomley

author maintainer

2007-12-29 17:00 UTC #4 lore raw

Quoted 38 lines · FUJITA Tomonori

Quoted 35 lines · On Sat, 29 Dec 2007 09:44:32 -0600 James Bottomley…
Quoted 18 lines · FUJITA Tomonori
From: James Bottomley <[email protected]>
Quoted 4 lines
drivers/scsi/libsas/sas_expander.c +0 -0
@@ -1896,11 +1896,9 @@ int sas_smp_handler(struct Scsi_Host *shost, struct sas_rphy *rphy,
 	}
drivers/scsi/libsas/sas_expander.c int sas_smp_handler(struct Scsi_Host *shost, struct sas_rphy *rphy, · line 1901
Quoted 10 lines
drivers/scsi/libsas/sas_expander.c +3 -5
 
 	/* no rphy means no smp target support (ie aic94xx host) */
-	if (!rphy) {
-		printk("%s: can we send a smp request to a host?\n",
-		       __FUNCTION__);
-		return -EINVAL;
-	}
+	if (!rphy)
+		return sas_smp_host_handler(shost, req, rsp);
+
I have one related question.
Quoted 8 lines
Currently, bsg doesn't return an error to user space since I had no idea how to convert errors such as EINVAL and ENOMEM into driver_status, transport_status, and device_status in struct sg_io_v4. I think that it's confusing that bsg don't return an error even if SMP requests aren't sent (e.g. devices are offline).
Do we need to map errors to the current error code in scsi/scsi.h (like DID_*) or define a new one for SMP?
Neither, I think ... the DID codes are only for things that actually pass through the SCSI stack. The way you implemented the smp functions in bsg, they're direct queue handlers themselves (Incidentally, that's another point about this: I think almost every use of bsg like this is going to be for SG_IO only, so it makes sense to move the actual queue handler into bsg, since they'll all share it).

Quoted 8 lines

Quoted 4 lines
The attached is the simplest patch that implements this. However, it unfortunately can't be applied yet ... the current SMP tools send receive buffers too large and libsas actually returns a data underrun error (which is now propagated).
bsg read/write interface doens't return errors in this way (compatible with sg3 read/write interface). If we support only SG_IO for non SCSI request/response protocols, then that's fine.

OK, guilty of disliking the read/write interface (and therefore not thinking about it). However, it's easy to fix. How about this? Note, I think returning errors when they occur is more important than preserving compatibility and swallowing the error somewhere, especially as the bsg v3 interface *only* dealt with SCSI, so this is more like an extension to non-scsi error returning. But if I'd had my way, bsg wouldn't have had a read/write interface, so I'm happy to do whatever people who actually use it want.

James

block/bsg.c +12 -2

index 8e181ab..69b0a9d 100644
@@ -445,6 +445,15 @@ static int blk_complete_sgv4_hdr_rq(struct request *rq, struct sg_io_v4 *hdr,
 	else
 		hdr->dout_resid = rq->data_len;
 
+	/*
+	 * If the request generated a negative error number, return it
+	 * (providing we aren't already returning an error); if it's
+	 * just a protocol response (i.e. non negative), that gets
+	 * processed above.
+	 */
+	if (!ret && rq->errors < 0)
+		ret = rq->errors;
+
 	blk_rq_unmap_user(bio);
 	blk_put_request(rq);

@@ -837,6 +846,7 @@ static long bsg_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct bsg_device *bd = file->private_data;
 	int __user *uarg = (int __user *) arg;
+	int ret;
 
 	switch (cmd) {
 		/*

@@ -889,12 +899,12 @@ static long bsg_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		if (rq->next_rq)
 			bidi_bio = rq->next_rq->bio;
 		blk_execute_rq(bd->queue, NULL, rq, 0);
-		blk_complete_sgv4_hdr_rq(rq, &hdr, bio, bidi_bio);
+		ret = blk_complete_sgv4_hdr_rq(rq, &hdr, bio, bidi_bio);
 
 		if (copy_to_user(uarg, &hdr, sizeof(hdr)))
 			return -EFAULT;
 
-		return 0;
+		return ret;
 	}
 	/*
 	 * block device ioctls

drivers/scsi/scsi_transport_sas.c +1 -0

index 87e786d..f2149d0 100644
@@ -173,6 +173,7 @@ static void sas_smp_request(struct request_queue *q, struct Scsi_Host *shost,
 
 		handler = to_sas_internal(shost->transportt)->f->smp_handler;
 		ret = handler(shost, rphy, req);
+		req->errors = ret;
 
 		spin_lock_irq(q->queue_lock);

FUJITA Tomonori

Acked-by

2007-12-30 06:10 UTC #5 lore raw

From: James Bottomley <[email protected]>
Subject: Re: [PATCH] libsas: add host SMP processing
Date: Sat, 29 Dec 2007 10:59:53 -0600

Quoted 49 lines · FUJITA Tomonori

Quoted 38 lines
Quoted 35 lines · On Sat, 29 Dec 2007 09:44:32 -0600 James Bottomley…
Quoted 18 lines · FUJITA Tomonori
From: James Bottomley <[email protected]>
Quoted 4 lines
drivers/scsi/libsas/sas_expander.c +0 -0
@@ -1896,11 +1896,9 @@ int sas_smp_handler(struct Scsi_Host *shost, struct sas_rphy *rphy,
 	}
drivers/scsi/libsas/sas_expander.c int sas_smp_handler(struct Scsi_Host *shost, struct sas_rphy *rphy, · line 1901
Quoted 10 lines
drivers/scsi/libsas/sas_expander.c +3 -5
 
 	/* no rphy means no smp target support (ie aic94xx host) */
-	if (!rphy) {
-		printk("%s: can we send a smp request to a host?\n",
-		       __FUNCTION__);
-		return -EINVAL;
-	}
+	if (!rphy)
+		return sas_smp_host_handler(shost, req, rsp);
+
I have one related question.
Quoted 8 lines
Currently, bsg doesn't return an error to user space since I had no idea how to convert errors such as EINVAL and ENOMEM into driver_status, transport_status, and device_status in struct sg_io_v4. I think that it's confusing that bsg don't return an error even if SMP requests aren't sent (e.g. devices are offline).
Do we need to map errors to the current error code in scsi/scsi.h (like DID_*) or define a new one for SMP?
Neither, I think ... the DID codes are only for things that actually pass through the SCSI stack. The way you implemented the smp functions in bsg, they're direct queue handlers themselves (Incidentally, that's another point about this: I think almost every use of bsg like this is going to be for SG_IO only, so it makes sense to move the actual queue handler into bsg, since they'll all share it).
Quoted 8 lines
Quoted 4 lines
The attached is the simplest patch that implements this. However, it unfortunately can't be applied yet ... the current SMP tools send receive buffers too large and libsas actually returns a data underrun error (which is now propagated).
bsg read/write interface doens't return errors in this way (compatible with sg3 read/write interface). If we support only SG_IO for non SCSI request/response protocols, then that's fine.

Quoted 6 lines

OK, guilty of disliking the read/write interface (and therefore not thinking about it). However, it's easy to fix. How about this? Note, I think returning errors when they occur is more important than preserving compatibility and swallowing the error somewhere, especially as the bsg v3 interface *only* dealt with SCSI, so this is more like an extension to non-scsi error returning.

Oops, I remember wrongly. I've just realized that sgv3 returns errors in this way (that is, returns negative values) though bsg doesn't (I guess Jens did that because bsg read/write interface handle multiple request/responses). There isn't the compatibility issue.

The patch looks fine.

Acked-by FUJITA Tomonori <[email protected]>

Quoted 3 lines

But if I'd had my way, bsg
wouldn't have had a read/write interface, so I'm happy to do whatever people who actually use it want.

I don't like the read/write interface much but OSD people need an efficient interface to perform SCSI commands (the interface should to handle multiple requests/response at a time and work asynchronously). Some of them seem to use bsg read/write interface. So I guess that we to invent a substitute to kill the read/write interface.