Re: [Qemu-devel] [PATCH] sdhci: Pass drive parameter to sdhci-pci via qdev property

[Stefan Hajnoczi]

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