[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v7 09/17] hw/sd/sdcard: Special case the -ENOMEDIUM error
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH v7 09/17] hw/sd/sdcard: Special case the -ENOMEDIUM error |
Date: |
Sun, 5 Jul 2020 00:18:59 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 |
On 7/5/20 12:10 AM, Philippe Mathieu-Daudé wrote:
> On 7/4/20 1:42 AM, Philippe Mathieu-Daudé wrote:
>> On 7/3/20 5:16 PM, Philippe Mathieu-Daudé wrote:
>>> On 7/3/20 3:23 PM, Peter Maydell wrote:
>>>> On Tue, 30 Jun 2020 at 14:39, Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> wrote:
>>>>>
>>>>> As we have no interest in the underlying block geometry,
>>>>> directly call blk_getlength(). We have to care about machines
>>>>> creating SD card with not drive attached (probably incorrect
>>>>> API use). Simply emit a warning when such Frankenstein cards
>>>>> of zero size are reset.
>>>>
>>>> Which machines create SD cards without a backing block device?
>>>
>>> The Aspeed machines:
>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg718116.html
>
> Also all boards using:
>
> hw/sd/milkymist-memcard.c:278: /* FIXME use a qdev drive property
> instead of drive_get_next() */
> hw/sd/pl181.c:506: /* FIXME use a qdev drive property instead of
> drive_get_next() */
> hw/sd/ssi-sd.c:253: /* FIXME use a qdev drive property instead of
> drive_get_next() */
>
> I.e.:
>
> static void pl181_realize(DeviceState *dev, Error **errp)
> {
> PL181State *s = PL181(dev);
> DriveInfo *dinfo;
>
> /* FIXME use a qdev drive property instead of drive_get_next() */
> dinfo = drive_get_next(IF_SD);
> s->card = sd_init(dinfo ? blk_by_legacy_dinfo(dinfo) : NULL, false);
> if (s->card == NULL) {
> error_setg(errp, "sd_init failed");
> }
> }
Doh I was pretty sure this series was merged:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg514645.html
Time to respin I guess, addressing your comment...
https://www.mail-archive.com/qemu-devel@nongnu.org/msg515866.html
>
>>>
>>>> I have a feeling that also the monitor "change" and "eject"
>>>> commands can remove the backing block device from the SD card
>>>> object.
>>>
>>> This is what I wanted to talk about on IRC. This seems wrong to me,
>>> we should eject the card and destroy it, and recreate a new card
>>> when plugging in another backing block device.
>>>
>>> Keep the reparenting on the bus layer, not on the card.
>>
>> I was wrong, the current code is correct:
>>
>> void sdbus_reparent_card(SDBus *from, SDBus *to)
>> {
>> SDState *card = get_card(from);
>> SDCardClass *sc;
>> bool readonly;
>>
>> /* We directly reparent the card object rather than implementing this
>> * as a hotpluggable connection because we don't want to expose SD cards
>> * to users as being hotpluggable, and we can get away with it in this
>> * limited use case. This could perhaps be implemented more cleanly in
>> * future by adding support to the hotplug infrastructure for "device
>> * can be hotplugged only via code, not by user".
>> */
>>
>> if (!card) {
>> return;
>> }
>>
>> sc = SD_CARD_GET_CLASS(card);
>> readonly = sc->get_readonly(card);
>>
>> sdbus_set_inserted(from, false);
>> qdev_set_parent_bus(DEVICE(card), &to->qbus);
>> sdbus_set_inserted(to, true);
>> sdbus_set_readonly(to, readonly);
>> }
>>
>> What I don't understand is why create a sdcard with no block backend.
>>
>> Maybe this is old code before the null-co block backend existed? I
>> haven't checked the git history yet.
>>
>> I'll try to restrict sdcard with only block backend and see if
>> something break (I doubt) at least it simplifies the code.
>> But I need to update the Aspeed machines first.
>>
>> The problem when not using block backend, is the size is 0,
>> so the next patch abort in sd_reset() due to:
>>
>> static uint64_t sd_addr_to_wpnum(SDState *sd, uint64_t addr)
>> {
>> assert(addr < sd->size);
>>
>
>
Re: [PATCH v7 09/17] hw/sd/sdcard: Special case the -ENOMEDIUM error, Markus Armbruster, 2020/07/06