Dne 3.1.2014 21:10, Douglas Gilbert napsal(a): > 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"?
In switch can be used only __branch_check__ (E, C) or __builtin_expect(E, C), which have too many '_' in name and I'm too afraid to use them. switch (__builtin_expect(var, const)) { ... if there is such a pressure for speed, unlikely(cmd->device->scsi_level <= SCSI_2) might be used, but i does not have statistics about how much modern high-speed devices report itself as scsi_level <= SCSI_2 or scsi_level == SCSI_UNKNOWN. (suppose the rate is pretty low). The switch solution look more elegant. > > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html