diff mbox

sdhci: Pass drive parameter to sdhci-pci via qdev property

Message ID 20150730180429.GA11818@morn.localdomain
State New
Headers show

Commit Message

Kevin O'Connor July 30, 2015, 6:04 p.m. UTC 
On Wed, Jul 29, 2015 at 10:01:03AM +0100, Stefan Hajnoczi wrote:
> On Tue, Jul 28, 2015 at 12:22:43PM -0400, Kevin O'Connor wrote:
> > Commit 19109131 disabled the sdhci-pci support because it used
> > drive_get_next().  This patch reenables sdhci-pci and changes it to
> > pass the drive via a qdev property - for example:
> >  -device sdhci-pci,drive=drive0 -drive id=drive0,if=sd,file=myimage
> > 
> > Signed-off-by: Kevin O'Connor <kevin@koconnor.net>
> > ---
> > 
> > This patch only changes the SDHCI PCI code - the sysbus code continues
> > to use drive_get_next().
> > 
> > The call to blk_detach_dev() is suspicious - I added it because
> > sd.c:sd_init() calls blk_attach_dev_nofail() and that causes a crash
> > if the drive is already attached to the PCI device.  It's not clear to
> > me how this should be wired up though.
> 
> Ugly bits:
> 
> hw/core/qdev-properties-system.c:release_drive() will call
> blk_detach_dev(*ptr, dev) and assert(blk->dev == dev) will fail.

Thanks for reviewing and catching the above.

> The SD card (SDState) isn't a DeviceState, it's a plain old C struct.
> So it doesn't have the qdev machinery for its own drive property.
> 
> There is no detach call paired with sd_init() attach, so that's ugly
> too.
> 
> A solution:
> 
> Add an sd_init() flag argument that skips the attach/set_dev_ops calls.
> Make sd_cardchange() externally visible and then call blk_set_dev_ops()
> from sdhci.c instead.
> 
> That way, existing users can rely on the semi-broken but convenient
> sd_init() behavior.  sdhci can forward the .change_media_cb() to
> sd_cardchange() keep itself as the blk->dev pointer.

I can do that.  However, another solution seems to be to just change
the blk_attach_dev_nofail() call in sd.c:sd_init() to blk_attach_dev()
and ignore any errors.  (Example patch below.)

I'm confused by what blk_attach_dev() attempts to accomplish.  The
BlockBackend->dev field is only ever used as a boolean flag.  (There
are four users of it - blk_dev_has_removable_media,
drive_check_orphaned, hmp_drive_del, pci_piix3_xen_ide_unplug - the
first three use it only as a non-NULL check and the fourth only uses
it to make blk_detach_dev() succeed.)  To the SD code, it doesn't
matter if it sets the "attached flag" or if something else has already
set that flag.  (Is it even important to set the flag at all?)

Thoughts?

-Kevin

Comments

Stefan Hajnoczi July 31, 2015, 8:29 a.m. UTC | #1 
On Thu, Jul 30, 2015 at 7:04 PM, Kevin O'Connor <kevin@koconnor.net> wrote:
> On Wed, Jul 29, 2015 at 10:01:03AM +0100, Stefan Hajnoczi wrote:
>> On Tue, Jul 28, 2015 at 12:22:43PM -0400, Kevin O'Connor wrote:
>> > Commit 19109131 disabled the sdhci-pci support because it used
>> > drive_get_next().  This patch reenables sdhci-pci and changes it to
>> > pass the drive via a qdev property - for example:
>> >  -device sdhci-pci,drive=drive0 -drive id=drive0,if=sd,file=myimage
>> >
>> > Signed-off-by: Kevin O'Connor <kevin@koconnor.net>
>> > ---
>> >
>> > This patch only changes the SDHCI PCI code - the sysbus code continues
>> > to use drive_get_next().
>> >
>> > The call to blk_detach_dev() is suspicious - I added it because
>> > sd.c:sd_init() calls blk_attach_dev_nofail() and that causes a crash
>> > if the drive is already attached to the PCI device.  It's not clear to
>> > me how this should be wired up though.
>>
>> Ugly bits:
>>
>> hw/core/qdev-properties-system.c:release_drive() will call
>> blk_detach_dev(*ptr, dev) and assert(blk->dev == dev) will fail.
>
> Thanks for reviewing and catching the above.
>
>> The SD card (SDState) isn't a DeviceState, it's a plain old C struct.
>> So it doesn't have the qdev machinery for its own drive property.
>>
>> There is no detach call paired with sd_init() attach, so that's ugly
>> too.
>>
>> A solution:
>>
>> Add an sd_init() flag argument that skips the attach/set_dev_ops calls.
>> Make sd_cardchange() externally visible and then call blk_set_dev_ops()
>> from sdhci.c instead.
>>
>> That way, existing users can rely on the semi-broken but convenient
>> sd_init() behavior.  sdhci can forward the .change_media_cb() to
>> sd_cardchange() keep itself as the blk->dev pointer.
>
> I can do that.  However, another solution seems to be to just change
> the blk_attach_dev_nofail() call in sd.c:sd_init() to blk_attach_dev()
> and ignore any errors.  (Example patch below.)
>
> I'm confused by what blk_attach_dev() attempts to accomplish.  The
> BlockBackend->dev field is only ever used as a boolean flag.  (There
> are four users of it - blk_dev_has_removable_media,
> drive_check_orphaned, hmp_drive_del, pci_piix3_xen_ide_unplug - the
> first three use it only as a non-NULL check and the fourth only uses
> it to make blk_detach_dev() succeed.)  To the SD code, it doesn't
> matter if it sets the "attached flag" or if something else has already
> set that flag.  (Is it even important to set the flag at all?)
>
> Thoughts?

That looks good.  I suggest adding a comment to let the reader know
that blk_attach_dev() is anticipated to silently fail if the storage
controller is using a drive property.

Stefan
diff mbox

Patch

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index a1ff465..29cd22c 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -493,7 +493,7 @@  SDState *sd_init(BlockBackend *blk, bool is_spi)
     sd->blk = blk;
     sd_reset(sd);
     if (sd->blk) {
-        blk_attach_dev_nofail(sd->blk, sd);
+        blk_attach_dev(sd->blk, sd);
         blk_set_dev_ops(sd->blk, &sd_block_ops, sd);
     }
     vmstate_register(NULL, -1, &sd_vmstate, sd);
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index e63367b..6e01de7 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1142,13 +1142,9 @@  static inline unsigned int sdhci_get_fifolen(SDHCIState *s)
     }
 }
 
-static void sdhci_initfn(SDHCIState *s)
+static void sdhci_initfn(SDHCIState *s, BlockBackend *blk)
 {
-    DriveInfo *di;
-
-    /* FIXME use a qdev drive property instead of drive_get_next() */
-    di = drive_get_next(IF_SD);
-    s->card = sd_init(di ? blk_by_legacy_dinfo(di) : NULL, false);
+    s->card = sd_init(blk, false);
     if (s->card == NULL) {
         exit(1);
     }
@@ -1214,7 +1210,8 @@  const VMStateDescription sdhci_vmstate = {
 
 /* Capabilities registers provide information on supported features of this
  * specific host controller implementation */
-static Property sdhci_properties[] = {
+static Property sdhci_pci_properties[] = {
+    DEFINE_BLOCK_PROPERTIES(SDHCIState, conf),
     DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
             SDHC_CAPAB_REG_DEFAULT),
     DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
@@ -1226,7 +1223,7 @@  static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
     SDHCIState *s = PCI_SDHCI(dev);
     dev->config[PCI_CLASS_PROG] = 0x01; /* Standard Host supported DMA */
     dev->config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin A */
-    sdhci_initfn(s);
+    sdhci_initfn(s, s->conf.blk);
     s->buf_maxsz = sdhci_get_fifolen(s);
     s->fifo_buffer = g_malloc0(s->buf_maxsz);
     s->irq = pci_allocate_irq(dev);
@@ -1253,9 +1250,7 @@  static void sdhci_pci_class_init(ObjectClass *klass, void *data)
     k->class_id = PCI_CLASS_SYSTEM_SDHCI;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
     dc->vmsd = &sdhci_vmstate;
-    dc->props = sdhci_properties;
-    /* Reason: realize() method uses drive_get_next() */
-    dc->cannot_instantiate_with_device_add_yet = true;
+    dc->props = sdhci_pci_properties;
 }
 
 static const TypeInfo sdhci_pci_info = {
@@ -1265,10 +1260,21 @@  static const TypeInfo sdhci_pci_info = {
     .class_init = sdhci_pci_class_init,
 };
 
+static Property sdhci_sysbus_properties[] = {
+    DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
+            SDHC_CAPAB_REG_DEFAULT),
+    DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void sdhci_sysbus_init(Object *obj)
 {
     SDHCIState *s = SYSBUS_SDHCI(obj);
-    sdhci_initfn(s);
+    DriveInfo *di;
+
+    /* FIXME use a qdev drive property instead of drive_get_next() */
+    di = drive_get_next(IF_SD);
+    sdhci_initfn(s, di ? blk_by_legacy_dinfo(di) : NULL);
 }
 
 static void sdhci_sysbus_finalize(Object *obj)
@@ -1295,7 +1301,7 @@  static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->vmsd = &sdhci_vmstate;
-    dc->props = sdhci_properties;
+    dc->props = sdhci_sysbus_properties;
     dc->realize = sdhci_sysbus_realize;
     /* Reason: instance_init() method uses drive_get_next() */
     dc->cannot_instantiate_with_device_add_yet = true;
diff --git a/hw/sd/sdhci.h b/hw/sd/sdhci.h
index 3352d23..e2de92d 100644
--- a/hw/sd/sdhci.h
+++ b/hw/sd/sdhci.h
@@ -26,6 +26,7 @@ 
 #define SDHCI_H
 
 #include "qemu-common.h"
+#include "hw/block/block.h"
 #include "hw/pci/pci.h"
 #include "hw/sysbus.h"
 #include "hw/sd.h"
@@ -239,6 +240,7 @@  typedef struct SDHCIState {
     };
     SDState *card;
     MemoryRegion iomem;
+    BlockConf conf;
 
     QEMUTimer *insert_timer;       /* timer for 'changing' sd card. */
     QEMUTimer *transfer_timer;