Re: [PATCH] scsi-sg: pass flag to inhibit setting LUN

Douglas Gilbert <dgilbert@xxxxxxxxxxxx> · Fri, 03 Jan 2014 15:10:36 -0500

On 14-01-02 08:19 AM, Jiří Pinkava wrote:
Hi,

This patch implements support for inhibiting setting LUN number
for SCSI custom command send via /dev/sgX with ioctl(.., SG_IO, ...) call.

This solves problems with some devices which claim support of
SCSI v1/v2, but for some special purposes use custom commands
which does not conform with standard message format.
(Like mine Pentax digital single-lens reflex camera)
It does not affect devices with SCSI v3 or latter.

For this purpose was earlier introduced
SG_FLAG_LUN_INHIBIT / SG_FLAG_UNUSED_LUN_INHIBIT (glibc/kernel) flag,
but the implementation was lost in some earlier code refactor.

The only possible issue I see is current implementation supports only
SG driver but there is few more code paths where the similar ioctl can
be invoked
(eg. anny SCSI device?). I'm not sure about, feel free to say anything
about it.

Hi,
SG_FLAG_LUN_INHIBIT was added to the sg driver around 15
years ago. The code to centralize the masking in scsi.c
and remove the "LUN inhibit" capability arrived about
10 years ago. A handful of people have complained about
it since. An you are the first to do something about it!

I have a few comments about the code (see below) but the
logic seems okay to me. That said I would be very surprised
if this is allowed back in the kernel. If you manage to do
that then I'll be studying your technique (because many of my
patches to the sg driver are not accepted).

So you can takes that as an "ack" with comments and good luck.
I'll let others judge.

---
  drivers/scsi/scsi.c       | 3 ++-
  drivers/scsi/sg.c         | 3 +++
  include/linux/blk_types.h | 2 ++
  3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index fe0bcb1..f6d5fc9 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -693,7 +693,8 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)
       * If SCSI-2 or lower, store the LUN value in cmnd.
       */
      if (cmd->device->scsi_level <= SCSI_2 &&
-        cmd->device->scsi_level != SCSI_UNKNOWN) {
+        cmd->device->scsi_level != SCSI_UNKNOWN &&
+        !(cmd->request->cmd_flags & REQ_LUN_INHIBIT)) {
          cmd->cmnd[1] = (cmd->cmnd[1] & 0x1f) |
                     (cmd->device->lun << 5 & 0xe0);
      }

The above being time sensitive code I was thinking a switch
might make it clearer and give the compiler more chances to
optimize:
      switch(cmd->device->scsi_level) {
      case SCSI_2:
      case SCSI_1_CCS:
      case SCSI_1:
              if (!(cmd->request->cmd_flags & REQ_LUN_INHIBIT)) {
                      cmd->cmnd[1] = (cmd->cmnd[1] & 0x1f) |
                                     (cmd->device->lun << 5 & 0xe0);
              }
              break;
      default:
              ;
      }

Hmm, can a switch's default be made "likely"?

More generally folks, with SAS SSDs available that can read at
1100 MB/sec we may want to spend time looking at the fast
paths through the SCSI subsystem.


diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index df5e961..8e09015 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1663,6 +1663,9 @@ static int sg_start_req(Sg_request *srp, unsigned
char *cmd)
      rq->sense = srp->sense_b;
      rq->retries = SG_DEFAULT_RETRIES;

+    if (hp->flags & SG_FLAG_UNUSED_LUN_INHIBIT)
+        rq->cmd_flags |=  REQ_LUN_INHIBIT;
+
      if ((dxfer_len <= 0) || (dxfer_dir == SG_DXFER_NONE))
          return 0;

diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 238ef0e..35d436b 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -178,6 +178,7 @@ enum rq_flag_bits {
      __REQ_MIXED_MERGE,    /* merge of different types, fail separately */
      __REQ_KERNEL,         /* direct IO to kernel pages */
      __REQ_PM,        /* runtime pm request */
+    __REQ_LUN_INHIBIT,    /* pass through for
SG_FLAG_UNUSED_LUN_INHIBIT flag */
      __REQ_END,        /* last of chain of requests */
      __REQ_NR_BITS,        /* stops here */
  };
@@ -230,6 +231,7 @@ enum rq_flag_bits {
  #define REQ_SECURE        (1ULL << __REQ_SECURE)
  #define REQ_KERNEL        (1ULL << __REQ_KERNEL)
  #define REQ_PM            (1ULL << __REQ_PM)
+#define REQ_LUN_INHIBIT    (1ULL << __REQ_LUN_INHIBIT)
  #define REQ_END            (1ULL << __REQ_END)

  #endif /* __LINUX_BLK_TYPES_H */


And I think the SG_FLAG_LUN_INHIBIT define should be re-instated
to sg.h . For backward compatibility (for the last 10 years)
please leave the SG_FLAG_UNUSED_LUN_INHIBIT define there. Both
defines should have the same value.

Doug Gilbert

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







