Patchwork [4/4] scsi-disk: fix the block descriptor returned by the MODE SENSE command

login
register
mail settings
Submitter Bernhard Kohl
Date Aug. 2, 2010, 3:31 p.m.
Message ID <1280763089-11829-5-git-send-email-bernhard.kohl@nsn.com>
Download mbox | patch
Permalink /patch/60547/
State New
Headers show

Comments

Bernhard Kohl - Aug. 2, 2010, 3:31 p.m.
The block descriptor contains the number of blocks, not the highest LBA.
Real hard disks return 0 if the number of blocks exceed the maximum 0xFFFFFF.

SCSI-Spec:
http://ldkelley.com/SCSI2/SCSI2/SCSI2-08.html#8.3.3
The number of blocks field specifies the number of logical blocks on the medium
to which the density code and block length fields apply. A value of zero
indicates that all of the remaining logical blocks of the logical unit shall
have the medium characteristics specified.

Signed-off-by: Bernhard Kohl <bernhard.kohl@nsn.com>
---
 hw/scsi-disk.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
Kevin Wolf - Aug. 16, 2010, 5:34 p.m.
Am 02.08.2010 17:31, schrieb Bernhard Kohl:
> The block descriptor contains the number of blocks, not the highest LBA.
> Real hard disks return 0 if the number of blocks exceed the maximum 0xFFFFFF.
> 
> SCSI-Spec:
> http://ldkelley.com/SCSI2/SCSI2/SCSI2-08.html#8.3.3
> The number of blocks field specifies the number of logical blocks on the medium
> to which the density code and block length fields apply. A value of zero
> indicates that all of the remaining logical blocks of the logical unit shall
> have the medium characteristics specified.
> 
> Signed-off-by: Bernhard Kohl <bernhard.kohl@nsn.com>
> ---
>  hw/scsi-disk.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index 26f7345..519513c 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -642,9 +642,8 @@ static int scsi_disk_emulate_mode_sense(SCSIRequest *req, uint8_t *outbuf)
>          else /* MODE_SENSE_10 */
>              outbuf[7] = 8; /* Block descriptor length  */
>          nb_sectors /= s->cluster_size;
> -        nb_sectors--;
>          if (nb_sectors > 0xffffff)
> -            nb_sectors = 0xffffff;
> +            nb_sectors = 0;
>          p[0] = 0; /* media density code */
>          p[1] = (nb_sectors >> 16) & 0xff;
>          p[2] = (nb_sectors >> 8) & 0xff;

The patch itself looks okay. However, it made me wonder what this line
wants to tell us:

    if ((~dbd) & nb_sectors) {

Is it just me or doesn't this make any sense at all? dbd is a single
bit, 0x8 if set or 0x0 otherwise. nb_sectors is the number of sectors.
Can this operation have any meaningful result? I suppose it was meant to
be something like:

    if (!dbd && nb_sectors) {

Can you please check this and add a patch 5/4 if necessary?

Kevin
Bernhard Kohl - Aug. 27, 2010, 3:26 p.m.
Am 16.08.2010 19:34, schrieb ext Kevin Wolf:
>
> The patch itself looks okay. However, it made me wonder what this line
> wants to tell us:
>
>     if ((~dbd) & nb_sectors) {
>
> Is it just me or doesn't this make any sense at all? dbd is a single
> bit, 0x8 if set or 0x0 otherwise. nb_sectors is the number of sectors.
> Can this operation have any meaningful result? I suppose it was meant to
> be something like:
>
>     if (!dbd && nb_sectors) {
>
> Can you please check this and add a patch 5/4 if necessary?
>
For my opinion it is nonsense too. And it does not work. I tested it:

root@grml ~ # sg_modes /dev/sdb -6 -v -H -H -p 0x3f
     inquiry cdb: 12 00 00 00 24 00
     QEMU      QEMU HARDDISK     0.13   peripheral_type: disk [0x0]
     mode sense (6) cdb: 1a 00 3f 00 fc 00
     mode sense (6): requested 252 bytes but got 220 bytes
Mode parameter header from MODE SENSE(6):
  00     1f 00 00 08
   Mode data length=32, medium type=0x00, WP=0, DpoFua=0, longlba=0
   Block descriptor length=8
 > Direct access device block descriptors:
    Density code=0x0
  00     00 a0 00 00 00 00 02 00

 >> page_code=0x8, page_control=0
  00     08 12 00 00 00 00 00 00  00 00 00 00 00 00 00 00
  10     00 00 00 00
root@grml ~ # sg_modes /dev/sdb -6 -v -H -H -p 0x3f -d
     inquiry cdb: 12 00 00 00 24 00
     QEMU      QEMU HARDDISK     0.13   peripheral_type: disk [0x0]
     mode sense (6) cdb: 1a 08 3f 00 fc 00
     mode sense (6): requested 252 bytes but got 220 bytes
Mode parameter header from MODE SENSE(6):
  00     1f 00 00 08
   Mode data length=32, medium type=0x00, WP=0, DpoFua=0, longlba=0
   Block descriptor length=8
 > Direct access device block descriptors:
    Density code=0x0
  00     00 a0 00 00 00 00 02 00

 >> page_code=0x8, page_control=0
  00     08 12 00 00 00 00 00 00  00 00 00 00 00 00 00 00
  10     00 00 00 00
root@grml ~ #

I will add a patch 5/5 in v2 as you proposed.

Bernhard

Patch

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 26f7345..519513c 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -642,9 +642,8 @@  static int scsi_disk_emulate_mode_sense(SCSIRequest *req, uint8_t *outbuf)
         else /* MODE_SENSE_10 */
             outbuf[7] = 8; /* Block descriptor length  */
         nb_sectors /= s->cluster_size;
-        nb_sectors--;
         if (nb_sectors > 0xffffff)
-            nb_sectors = 0xffffff;
+            nb_sectors = 0;
         p[0] = 0; /* media density code */
         p[1] = (nb_sectors >> 16) & 0xff;
         p[2] = (nb_sectors >> 8) & 0xff;