On Thu, Jul 30, 2015 at 7:04 PM, Kevin O'Connor <ke...@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 <ke...@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
