Re: [Qemu-devel] scsi-bus: Add support for SCSI scanners

2016-06-30 Thread Jarkko Lavinen
On Thu, Jun 30, 2016 at 05:53:37PM +0200, Paolo Bonzini wrote:
> > The extra sense bit seems to be a kind of internal busy/film holder
> > moving status. After having this sense bit available scanning programs 
> > have been very stable and every feature seems to be working.
> 
> Thanks!  Out of curiosity what HBA are you using on the host?

I did the testing with Adaptec 29320LPE. I have also 29160N on a PCI slot, but 
it does not support MSI. Initially I was trying to use PCI pass-through with 
IOMMU which requires MSI. I have also seen one kernel crash when probing / 
initializing 29160N but that was with Xen and IOMMU enabled.

Multi Pros have also a Firewire port. These are known issue over time when 
scanner refuses to initialize if set to use Firewire port but still work with 
SCSI port.

Jarkko



Re: [Qemu-devel] scsi-bus: Add support for SCSI scanners

2016-06-30 Thread Paolo Bonzini


On 29/06/2016 19:36, Jarkko Lavinen wrote:
> On Tue, Jun 28, 2016 at 07:14:55PM +0200, Paolo Bonzini wrote:
>> This is wrong, because INQUIRY's byte 3 is defined to be part of the
>> length in modern SCSI standards.
> 
>> This is wrong, because INQUIRY's byte 3 is defined to be part of the
>> length in modern SCSI standards.
> Ok. I was using outdated ANSI spec too pedantically. The first patch is
> not needed at all.
> 
>> SCAN conflicts with START_STOP.  Add a comment please saying that
>> START_STOP has cmd->xfer set to 0 in scsi_req_xfer for non-scanner
>> devices.
> Done.
> 
> I also removed this:
> 
> case GET_DATA_BUFFER_STATUS:
> cmd->xfer = buf[8] | (buf[7] << 8);
> 
> Since it is again unneedes and handled by scsi_cdb_xfer.
> 
> I added longer sense buffer in Request Sense command for scanners.
> Multi Pro has at least one sense bit in byte 18 and Minolta's own SW
> uses 20 bytes as an allocation size in Request Sense command.
> 
> The extra sense bit seems to be a kind of internal busy/film holder
> moving status. After having this sense bit available scanning programs 
> have been very stable and every feature seems to be working.

Thanks!  Out of curiosity what HBA are you using on the host?

Paolo



Re: [Qemu-devel] scsi-bus: Add support for SCSI scanners

2016-06-29 Thread Jarkko Lavinen
On Tue, Jun 28, 2016 at 07:14:55PM +0200, Paolo Bonzini wrote:
> This is wrong, because INQUIRY's byte 3 is defined to be part of the
> length in modern SCSI standards.

> This is wrong, because INQUIRY's byte 3 is defined to be part of the
> length in modern SCSI standards.
Ok. I was using outdated ANSI spec too pedantically. The first patch is
not needed at all.

> SCAN conflicts with START_STOP.  Add a comment please saying that
> START_STOP has cmd->xfer set to 0 in scsi_req_xfer for non-scanner
> devices.
Done.

I also removed this:

case GET_DATA_BUFFER_STATUS:
cmd->xfer = buf[8] | (buf[7] << 8);

Since it is again unneedes and handled by scsi_cdb_xfer.

I added longer sense buffer in Request Sense command for scanners.
Multi Pro has at least one sense bit in byte 18 and Minolta's own SW
uses 20 bytes as an allocation size in Request Sense command.

The extra sense bit seems to be a kind of internal busy/film holder
moving status. After having this sense bit available scanning programs 
have been very stable and every feature seems to be working.

Jarkko
>From 0eb0f4ef920973f62ad3029f5b9ac726cfe610cf Mon Sep 17 00:00:00 2001
From: Jarkko Lavinen 
Date: Tue, 28 Jun 2016 21:51:52 +0300
Subject: [PATCH 1/2] scsi-bus: Add SCSI scanner support

Add support for missing scanner specific SCSI commands and their xfer
lenghts as per ANSI spec section 15.

Signed-off-by: Jarkko Lavinen 
---
 hw/scsi/scsi-bus.c   | 30 ++
 include/block/scsi.h |  6 ++
 2 files changed, 36 insertions(+)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index ad6f398..e0fbaee 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -1132,6 +1132,31 @@ static int scsi_req_medium_changer_xfer(SCSICommand *cmd, SCSIDevice *dev, uint8
 return 0;
 }
 
+static int scsi_req_scanner_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
+{
+switch (buf[0]) {
+/* Scanner commands */
+case OBJECT_POSITION:
+cmd->xfer = 0;
+break;
+case SCAN:
+/* SCAN conflicts with START_STOP which has cmd->xfer set to 0 for
+   non-scanner devices */
+cmd->xfer = buf[4];
+break;
+case READ_10:
+case SEND:
+case GET_WINDOW:
+case SET_WINDOW:
+cmd->xfer = buf[8] | (buf[7] << 8) | (buf[6] << 16);
+break;
+default:
+/* GET_DATA_BUFFER_STATUS xfer handled by scsi_req_xfer */
+return scsi_req_xfer(cmd, dev, buf);
+}
+
+return 0;
+}
 
 static void scsi_cmd_xfer_mode(SCSICommand *cmd)
 {
@@ -1178,6 +1203,8 @@ static void scsi_cmd_xfer_mode(SCSICommand *cmd)
 case SEND_DVD_STRUCTURE:
 case PERSISTENT_RESERVE_OUT:
 case MAINTENANCE_OUT:
+case SET_WINDOW:
+case SCAN:
 cmd->mode = SCSI_XFER_TO_DEV;
 break;
 case ATA_PASSTHROUGH_12:
@@ -1258,6 +1285,9 @@ int scsi_req_parse_cdb(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf)
 case TYPE_MEDIUM_CHANGER:
 rc = scsi_req_medium_changer_xfer(cmd, dev, buf);
 break;
+case TYPE_SCANNER:
+rc = scsi_req_scanner_length(cmd, dev, buf);
+break;
 default:
 rc = scsi_req_xfer(cmd, dev, buf);
 break;
diff --git a/include/block/scsi.h b/include/block/scsi.h
index a311341..8b966d7 100644
--- a/include/block/scsi.h
+++ b/include/block/scsi.h
@@ -48,13 +48,17 @@
 #define ERASE 0x19
 #define MODE_SENSE0x1a
 #define LOAD_UNLOAD   0x1b
+#define SCAN  0x1b
 #define START_STOP0x1b
 #define RECEIVE_DIAGNOSTIC0x1c
 #define SEND_DIAGNOSTIC   0x1d
 #define ALLOW_MEDIUM_REMOVAL  0x1e
+#define SET_WINDOW0x24
 #define READ_CAPACITY_10  0x25
+#define GET_WINDOW0x25
 #define READ_10   0x28
 #define WRITE_10  0x2a
+#define SEND  0x2a
 #define SEEK_10   0x2b
 #define LOCATE_10 0x2b
 #define POSITION_TO_ELEMENT   0x2b
@@ -62,10 +66,12 @@
 #define VERIFY_10 0x2f
 #define SEARCH_HIGH   0x30
 #define SEARCH_EQUAL  0x31
+#define OBJECT_POSITION   0x31
 #define SEARCH_LOW0x32
 #define SET_LIMITS0x33
 #define PRE_FETCH 0x34
 #define READ_POSITION 0x34
+#define GET_DATA_BUFFER_STATUS 0x34
 #define SYNCHRONIZE_CACHE 0x35
 #define LOCK_UNLOCK_CACHE 0x36
 #define INITIALIZE_ELEMENT_STATUS_WITH_RANGE 0x37
-- 
2.1.4

>From dd85d2aac375db97e860a0908e5bfd5192d0dc61 Mon Sep 17 00:00:00 2001
From: Jarkko Lavinen 
Date: Wed, 29 Jun 2016 03:11:46 +0300
Subject: [PATCH 2/2] scsi-bus: Use longer sense buffer with scanners

Scanners can provide additional sense bytes beyond 18 bytes.
VueScan uses 32 bytes alloc length with Request Sense command.

Signed-off-by: Jarkko Lavinen 
---
 hw/scsi/scsi-bus.c | 10 +-
 include/hw/scsi/scsi.h |  1 +
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 

Re: [Qemu-devel] scsi-bus: Add support for SCSI scanners

2016-06-28 Thread Paolo Bonzini
> +case INQUIRY:

This is wrong, because INQUIRY's byte 3 is defined to be part of the
length in modern SCSI standards.  Perhaps you can move it to
scsi_req_scanner_length if really necessary?

>  case MODE_SENSE:
> +case MODE_SELECT:
> +case REQUEST_SENSE:
> +cmd->xfer = buf[4];
> +break;

This is unnecessary, because scsi_cdb_xfer should get it right (the
commands are respectively 15h for MODE SELECT and 03h for REQUEST SENSE;
the existing entry for MODE SENSE is also unnecessary).

> +case MODE_SENSE_10:
> +case MODE_SELECT_10:
> +cmd->xfer = (buf[7] << 8) | buf[8];
>  break;

Same here, scsi_cdb_xfer should handle it.

> 
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index 045594a..2f8da49 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -1139,6 +1139,32 @@ static int scsi_req_medium_changer_xfer(SCSICommand 
> *cmd, SCSIDevice *dev, uint8
>  return 0;
>  }
>  
> +static int scsi_req_scanner_length(SCSICommand *cmd, SCSIDevice *dev, 
> uint8_t *buf)
> +{
> +switch (buf[0]) {
> +/* Scanner commands */
> +case OBJECT_POSITION:
> +cmd->xfer = 0;
> +break;
> +case GET_DATA_BUFFER_STATUS:
> +cmd->xfer = buf[8] | (buf[7] << 8);
> +break;
> +case SCAN:
> +cmd->xfer = buf[4];
> +break;
> +case READ_10:
> +case SEND:
> +case GET_WINDOW:
> +case SET_WINDOW:
> +cmd->xfer = buf[8] | (buf[7] << 8) | (buf[6] << 16);
> +break;
> +default:
> +return scsi_req_xfer(cmd, dev, buf);
> +}

Looks good.

> +return 0;
> +}
> +
>  static void scsi_cmd_xfer_mode(SCSICommand *cmd)
>  {
>  if (!cmd->xfer) {
> @@ -1184,6 +1210,8 @@ static void scsi_cmd_xfer_mode(SCSICommand *cmd)
>  case SEND_DVD_STRUCTURE:
>  case PERSISTENT_RESERVE_OUT:
>  case MAINTENANCE_OUT:
> +case SET_WINDOW:
> +case SCAN:

SCAN conflicts with START_STOP.  Add a comment please saying that
START_STOP has cmd->xfer set to 0 in scsi_req_xfer for non-scanner devices.

Everything else looks good.

Thanks,

Paolo

>  cmd->mode = SCSI_XFER_TO_DEV;
>  break;
>  case ATA_PASSTHROUGH_12:
> @@ -1264,6 +1292,9 @@ int scsi_req_parse_cdb(SCSIDevice *dev, SCSICommand 
> *cmd, uint8_t *buf)
>  case TYPE_MEDIUM_CHANGER:
>  rc = scsi_req_medium_changer_xfer(cmd, dev, buf);
>  break;
> +case TYPE_SCANNER:
> +rc = scsi_req_scanner_length(cmd, dev, buf);
> +break;
>  default:
>  rc = scsi_req_xfer(cmd, dev, buf);
>  break;
> diff --git a/include/block/scsi.h b/include/block/scsi.h
> index a311341..8b966d7 100644
> --- a/include/block/scsi.h
> +++ b/include/block/scsi.h
> @@ -48,13 +48,17 @@
>  #define ERASE 0x19
>  #define MODE_SENSE0x1a
>  #define LOAD_UNLOAD   0x1b
> +#define SCAN  0x1b
>  #define START_STOP0x1b
>  #define RECEIVE_DIAGNOSTIC0x1c
>  #define SEND_DIAGNOSTIC   0x1d
>  #define ALLOW_MEDIUM_REMOVAL  0x1e
> +#define SET_WINDOW0x24
>  #define READ_CAPACITY_10  0x25
> +#define GET_WINDOW0x25
>  #define READ_10   0x28
>  #define WRITE_10  0x2a
> +#define SEND  0x2a
>  #define SEEK_10   0x2b
>  #define LOCATE_10 0x2b
>  #define POSITION_TO_ELEMENT   0x2b
> @@ -62,10 +66,12 @@
>  #define VERIFY_10 0x2f
>  #define SEARCH_HIGH   0x30
>  #define SEARCH_EQUAL  0x31
> +#define OBJECT_POSITION   0x31
>  #define SEARCH_LOW0x32
>  #define SET_LIMITS0x33
>  #define PRE_FETCH 0x34
>  #define READ_POSITION 0x34
> +#define GET_DATA_BUFFER_STATUS 0x34
>  #define SYNCHRONIZE_CACHE 0x35
>  #define LOCK_UNLOCK_CACHE 0x36
>  #define INITIALIZE_ELEMENT_STATUS_WITH_RANGE 0x37
> -- 
> 2.1.4
>