Patchwork [05/14] ide: Remove redundant IDEState member conf

login
register
mail settings
Submitter Markus Armbruster
Date May 28, 2010, 1:38 p.m.
Message ID <1275053935-10627-6-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/53898/
State New
Headers show

Comments

Markus Armbruster - May 28, 2010, 1:38 p.m.
Commit 428c149b added IDEState member conf to let commit 0009baf1 find
the BlockConf from there.  It exists only for qdev drives, created via
ide_drive_initfn(), not for drives created via ide_init2().

But for a qdev drive, we can just as well reach its IDEDevice, which
contains the BlockConf.  Do that, and revert the parts of commit
428c149b that add IDEState member conf.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/core.c     |   16 +++++++---------
 hw/ide/internal.h |    4 +---
 hw/ide/qdev.c     |    3 +--
 3 files changed, 9 insertions(+), 14 deletions(-)
Kevin Wolf - May 31, 2010, 2:56 p.m.
Am 28.05.2010 15:38, schrieb Markus Armbruster:
> Commit 428c149b added IDEState member conf to let commit 0009baf1 find
> the BlockConf from there.  It exists only for qdev drives, created via
> ide_drive_initfn(), not for drives created via ide_init2().
> 
> But for a qdev drive, we can just as well reach its IDEDevice, which
> contains the BlockConf.  Do that, and revert the parts of commit
> 428c149b that add IDEState member conf.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/ide/core.c     |   16 +++++++---------
>  hw/ide/internal.h |    4 +---
>  hw/ide/qdev.c     |    3 +--
>  3 files changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 066fecb..c3334b1 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -98,6 +98,7 @@ static void ide_identify(IDEState *s)
>  {
>      uint16_t *p;
>      unsigned int oldsize;
> +    IDEDevice *dev;
>  
>      if (s->identify_set) {
>  	memcpy(s->io_buffer, s->identify_data, sizeof(s->identify_data));
> @@ -165,8 +166,9 @@ static void ide_identify(IDEState *s)
>      put_le16(p + 101, s->nb_sectors >> 16);
>      put_le16(p + 102, s->nb_sectors >> 32);
>      put_le16(p + 103, s->nb_sectors >> 48);
> -    if (s->conf && s->conf->physical_block_size)
> -        put_le16(p + 106, 0x6000 | get_physical_block_exp(s->conf));
> +    dev = s->unit ? s->bus->slave : s->bus->master;
> +    if (dev && dev->conf.physical_block_size)

If this is meant to be the same condition it would be dev->conf && ...

I can't really tell right now if it can happen that either dev or
dev->conf can be NULL here. In a quick attempt I couldn't get either one.

So, which version is correct, or should the first part be dropped
completely?

Kevin
Markus Armbruster - May 31, 2010, 3:47 p.m.
Kevin Wolf <kwolf@redhat.com> writes:

> Am 28.05.2010 15:38, schrieb Markus Armbruster:
>> Commit 428c149b added IDEState member conf to let commit 0009baf1 find
>> the BlockConf from there.  It exists only for qdev drives, created via
>> ide_drive_initfn(), not for drives created via ide_init2().
>> 
>> But for a qdev drive, we can just as well reach its IDEDevice, which
>> contains the BlockConf.  Do that, and revert the parts of commit
>> 428c149b that add IDEState member conf.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/ide/core.c     |   16 +++++++---------
>>  hw/ide/internal.h |    4 +---
>>  hw/ide/qdev.c     |    3 +--
>>  3 files changed, 9 insertions(+), 14 deletions(-)
>> 
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 066fecb..c3334b1 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -98,6 +98,7 @@ static void ide_identify(IDEState *s)
>>  {
>>      uint16_t *p;
>>      unsigned int oldsize;
>> +    IDEDevice *dev;
>>  
>>      if (s->identify_set) {
>>  	memcpy(s->io_buffer, s->identify_data, sizeof(s->identify_data));
>> @@ -165,8 +166,9 @@ static void ide_identify(IDEState *s)
>>      put_le16(p + 101, s->nb_sectors >> 16);
>>      put_le16(p + 102, s->nb_sectors >> 32);
>>      put_le16(p + 103, s->nb_sectors >> 48);
>> -    if (s->conf && s->conf->physical_block_size)
>> -        put_le16(p + 106, 0x6000 | get_physical_block_exp(s->conf));
>> +    dev = s->unit ? s->bus->slave : s->bus->master;
>> +    if (dev && dev->conf.physical_block_size)
>
> If this is meant to be the same condition it would be dev->conf && ...
>
> I can't really tell right now if it can happen that either dev or
> dev->conf can be NULL here. In a quick attempt I couldn't get either one.
>
> So, which version is correct, or should the first part be dropped
> completely?

dev->conf can't ever be null:

struct IDEDevice {
    DeviceState qdev;
    uint32_t unit;
    BlockConf conf;
    char *version;
    char *serial;
};

If s->bus was set up by ide_qdev_init(), then dev cannot be null.  I
*think* it can be null for non-qdev IDE.  Three places still use that.
Maybe it's time to accidentally break them and wait for the screams ;)

Patch

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 066fecb..c3334b1 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -98,6 +98,7 @@  static void ide_identify(IDEState *s)
 {
     uint16_t *p;
     unsigned int oldsize;
+    IDEDevice *dev;
 
     if (s->identify_set) {
 	memcpy(s->io_buffer, s->identify_data, sizeof(s->identify_data));
@@ -165,8 +166,9 @@  static void ide_identify(IDEState *s)
     put_le16(p + 101, s->nb_sectors >> 16);
     put_le16(p + 102, s->nb_sectors >> 32);
     put_le16(p + 103, s->nb_sectors >> 48);
-    if (s->conf && s->conf->physical_block_size)
-        put_le16(p + 106, 0x6000 | get_physical_block_exp(s->conf));
+    dev = s->unit ? s->bus->slave : s->bus->master;
+    if (dev && dev->conf.physical_block_size)
+        put_le16(p + 106, 0x6000 | get_physical_block_exp(&dev->conf));
 
     memcpy(s->identify_data, p, sizeof(s->identify_data));
     s->identify_set = 1;
@@ -2594,8 +2596,7 @@  void ide_bus_reset(IDEBus *bus)
     ide_clear_hob(bus);
 }
 
-void ide_init_drive(IDEState *s, DriveInfo *dinfo, BlockConf *conf,
-        const char *version)
+void ide_init_drive(IDEState *s, DriveInfo *dinfo, const char *version)
 {
     int cylinders, heads, secs;
     uint64_t nb_sectors;
@@ -2620,9 +2621,6 @@  void ide_init_drive(IDEState *s, DriveInfo *dinfo, BlockConf *conf,
         }
         strncpy(s->drive_serial_str, drive_get_serial(s->bs),
                 sizeof(s->drive_serial_str));
-        if (conf) {
-            s->conf = conf;
-        }
     }
     if (strlen(s->drive_serial_str) == 0)
         snprintf(s->drive_serial_str, sizeof(s->drive_serial_str),
@@ -2653,9 +2651,9 @@  void ide_init2(IDEBus *bus, DriveInfo *hd0, DriveInfo *hd1,
         s->sector_write_timer = qemu_new_timer(vm_clock,
                                                ide_sector_write_timer_cb, s);
         if (i == 0)
-            ide_init_drive(s, hd0, NULL, NULL);
+            ide_init_drive(s, hd0, NULL);
         if (i == 1)
-            ide_init_drive(s, hd1, NULL, NULL);
+            ide_init_drive(s, hd1, NULL);
     }
     bus->irq = irq;
 }
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index b4554ce..cf71019 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -398,7 +398,6 @@  struct IDEState {
     /* set for lba48 access */
     uint8_t lba48;
     BlockDriverState *bs;
-    BlockConf *conf;
     char version[9];
     /* ATAPI specific */
     uint8_t sense_key;
@@ -555,8 +554,7 @@  uint32_t ide_data_readw(void *opaque, uint32_t addr);
 void ide_data_writel(void *opaque, uint32_t addr, uint32_t val);
 uint32_t ide_data_readl(void *opaque, uint32_t addr);
 
-void ide_init_drive(IDEState *s, DriveInfo *dinfo, BlockConf *conf,
-    const char *version);
+void ide_init_drive(IDEState *s, DriveInfo *dinfo, const char *version);
 void ide_init2(IDEBus *bus, DriveInfo *hd0, DriveInfo *hd1,
                qemu_irq irq);
 void ide_init_ioport(IDEBus *bus, int iobase, int iobase2);
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index b18693d..9ebb906 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -99,8 +99,7 @@  typedef struct IDEDrive {
 static int ide_drive_initfn(IDEDevice *dev)
 {
     IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
-    ide_init_drive(bus->ifs + dev->unit, dev->conf.dinfo, &dev->conf,
-                   dev->version);
+    ide_init_drive(bus->ifs + dev->unit, dev->conf.dinfo, dev->version);
     return 0;
 }