[Qemu-devel] [PATCH 1.0] scsi: fix fw path

[Qemu-devel] [PATCH 1.0] scsi: fix fw path

[Paolo Bonzini]

The pre-1.0 firmware path for SCSI devices already included the LUN
using the suffix argument to add_boot_device_path.  Avoid that it is
included twice, and convert the colons to commas for consistency with
other kinds of devices

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-bus.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 372fe7f..b4e6e29 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -1304,7 +1304,7 @@ static char *scsibus_get_fw_dev_path(DeviceState *dev)
     SCSIDevice *d = DO_UPCAST(SCSIDevice, qdev, dev);
     char path[100];
 
-    snprintf(path, sizeof(path), "%s@%d:%d:%d", qdev_fw_name(dev),
+    snprintf(path, sizeof(path), "%s@%d,%d,%d", qdev_fw_name(dev),
              d->channel, d->id, d->lun);
 
     return strdup(path);
-- 
1.7.7.1

[Qemu-devel] [PATCH 1.0 v2] scsi: fix fw path

[Paolo Bonzini]

The pre-1.0 firmware path for SCSI devices already included the LUN
using the suffix argument to add_boot_device_path.  I missed that when
making channel and LUN customizable.  Avoid that it is included twice, and
convert the colons to commas for consistency with other kinds of devices

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
	v1->v2: include scsi-disk hunk too

 hw/scsi-bus.c  |    2 +-
 hw/scsi-disk.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 372fe7f..b4e6e29 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -1304,7 +1304,7 @@ static char *scsibus_get_fw_dev_path(DeviceState *dev)
     SCSIDevice *d = DO_UPCAST(SCSIDevice, qdev, dev);
     char path[100];
 
-    snprintf(path, sizeof(path), "%s@%d:%d:%d", qdev_fw_name(dev),
+    snprintf(path, sizeof(path), "%s@%d,%d,%d", qdev_fw_name(dev),
              d->channel, d->id, d->lun);
 
     return strdup(path);
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 9da6d36..16a4714 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -1581,7 +1581,7 @@ static int scsi_initfn(SCSIDevice *dev)
     bdrv_set_buffer_alignment(s->qdev.conf.bs, s->qdev.blocksize);
 
     bdrv_iostatus_enable(s->qdev.conf.bs);
-    add_boot_device_path(s->qdev.conf.bootindex, &dev->qdev, ",0");
+    add_boot_device_path(s->qdev.conf.bootindex, &dev->qdev, NULL);
     return 0;
 }
 
-- 
1.7.7.1

Re: [Qemu-devel] [PATCH 1.0 v2] scsi: fix fw path

[Kevin Wolf]

Am 15.11.2011 17:36, schrieb Paolo Bonzini:
> The pre-1.0 firmware path for SCSI devices already included the LUN
> using the suffix argument to add_boot_device_path.  I missed that when
> making channel and LUN customizable.  Avoid that it is included twice, and
> convert the colons to commas for consistency with other kinds of devices
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> 	v1->v2: include scsi-disk hunk too

Thanks, applied to the block-stable branch (for 1.0)

Kevin

Re: [Qemu-devel] [PATCH 1.0 v2] scsi: fix fw path

[Andreas Färber]

Am 18.11.2011 13:35, schrieb Kevin Wolf:
> Am 15.11.2011 17:36, schrieb Paolo Bonzini:
>> The pre-1.0 firmware path for SCSI devices already included the LUN
>> using the suffix argument to add_boot_device_path.  I missed that when
>> making channel and LUN customizable.  Avoid that it is included twice, and
>> convert the colons to commas for consistency with other kinds of devices
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>> 	v1->v2: include scsi-disk hunk too
> 
> Thanks, applied to the block-stable branch (for 1.0)

Did you guys check the consistency part against OpenFirmware syntax? I
didn't get around to that yet.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1.0 v2] scsi: fix fw path

[Paolo Bonzini]

On 11/18/2011 02:36 PM, Andreas Färber wrote:
> Am 18.11.2011 13:35, schrieb Kevin Wolf:
>> Am 15.11.2011 17:36, schrieb Paolo Bonzini:
>>> The pre-1.0 firmware path for SCSI devices already included the LUN
>>> using the suffix argument to add_boot_device_path.  I missed that when
>>> making channel and LUN customizable.  Avoid that it is included twice, and
>>> convert the colons to commas for consistency with other kinds of devices
>>>
>>> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
>>> ---
>>> 	v1->v2: include scsi-disk hunk too
>>
>> Thanks, applied to the block-stable branch (for 1.0)
>
> Did you guys check the consistency part against OpenFirmware syntax? I
> didn't get around to that yet.

No, I wasn't aware about the existence of an OF spec for that (only that 
fw_dev_path design was roughly corresponding to OF).  Based on 
http://www.openfirmware.org/ofwg/practice/spi/spi1_0.ps it looks like 
this (followup) patch would be preferrable:

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index b4e6e29..4c33583 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -1304,7 +1304,7 @@ static char *scsibus_get_fw_dev_path(DeviceState *dev)
      SCSIDevice *d = DO_UPCAST(SCSIDevice, qdev, dev);
      char path[100];

-    snprintf(path, sizeof(path), "%s@%d,%d,%d", qdev_fw_name(dev),
+    snprintf(path, sizeof(path), "scsi@%x/%s@%x,%x", qdev_fw_name(dev),
               d->channel, d->id, d->lun);

      return strdup(path);

Kevin, how do you want to proceed?

Paolo

Re: [Qemu-devel] [PATCH 1.0 v2] scsi: fix fw path

[Paolo Bonzini]

On 11/18/2011 03:10 PM, Kevin Wolf wrote:
> Whatever you think is best. I already included the other patch in the
> pull request, so any other change would have to be on top.

Ok, I'll put together a formal patch today and test it with seabios.

Paolo

Re: [Qemu-devel] [PATCH 1.0 v2] scsi: fix fw path

[Kevin Wolf]

Am 18.11.2011 14:59, schrieb Paolo Bonzini:
> On 11/18/2011 02:36 PM, Andreas Färber wrote:
>> Am 18.11.2011 13:35, schrieb Kevin Wolf:
>>> Am 15.11.2011 17:36, schrieb Paolo Bonzini:
>>>> The pre-1.0 firmware path for SCSI devices already included the LUN
>>>> using the suffix argument to add_boot_device_path.  I missed that when
>>>> making channel and LUN customizable.  Avoid that it is included twice, and
>>>> convert the colons to commas for consistency with other kinds of devices
>>>>
>>>> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
>>>> ---
>>>> 	v1->v2: include scsi-disk hunk too
>>>
>>> Thanks, applied to the block-stable branch (for 1.0)
>>
>> Did you guys check the consistency part against OpenFirmware syntax? I
>> didn't get around to that yet.
> 
> No, I wasn't aware about the existence of an OF spec for that (only that 
> fw_dev_path design was roughly corresponding to OF).  Based on 
> http://www.openfirmware.org/ofwg/practice/spi/spi1_0.ps it looks like 
> this (followup) patch would be preferrable:
> 
> diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
> index b4e6e29..4c33583 100644
> --- a/hw/scsi-bus.c
> +++ b/hw/scsi-bus.c
> @@ -1304,7 +1304,7 @@ static char *scsibus_get_fw_dev_path(DeviceState *dev)
>       SCSIDevice *d = DO_UPCAST(SCSIDevice, qdev, dev);
>       char path[100];
> 
> -    snprintf(path, sizeof(path), "%s@%d,%d,%d", qdev_fw_name(dev),
> +    snprintf(path, sizeof(path), "scsi@%x/%s@%x,%x", qdev_fw_name(dev),
>                d->channel, d->id, d->lun);

The parameter order doesn't look right.

> 
>       return strdup(path);
> 
> Kevin, how do you want to proceed?

Whatever you think is best. I already included the other patch in the
pull request, so any other change would have to be on top.

Kevin