From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55134) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ya2qm-00065k-Ps for qemu-devel@nongnu.org; Mon, 23 Mar 2015 09:57:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ya2qJ-0001nV-TN for qemu-devel@nongnu.org; Mon, 23 Mar 2015 09:57:04 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52045) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ya2qJ-0001mt-MO for qemu-devel@nongnu.org; Mon, 23 Mar 2015 09:56:35 -0400 Message-ID: <55101B7C.9040902@redhat.com> Date: Mon, 23 Mar 2015 14:56:12 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1426953265-19940-1-git-send-email-pbonzini@redhat.com> <87twxct6yq.fsf@blackfin.pond.sub.org> <55100193.5000701@redhat.com> <87wq27n8ed.fsf@blackfin.pond.sub.org> In-Reply-To: <87wq27n8ed.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH for-2.3] sdhci: add "drive" property List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: "Edgar E. Iglesias" , Peter Crosthwaite , qemu-devel@nongnu.org, Peter Maydell On 23/03/2015 14:35, Markus Armbruster wrote: >>> From 30,000ft, this looks a bit like the floppy case: BB's dev points to >>> the block device, and BB's dev_opaque points within the device. >>> >>> If you descend a bit, it looks a lot more like the usb-storage hack that >>> has caused us nothing but grief: two separate device models attaching to >>> the same BlockBackend. >>> >>> What is the usb-storage hack? Device model usb-storage pretends to be a >>> block device, but really is a SCSI controller that can serve just one >>> SCSI device, which it creates automatically, in its realize() method. >>> Since the automatically created device isn't accessible at -device / >>> device_add level, we need to stick the drive property for it into >>> usb-storage. Before the realize() method creates the SCSI device, it >>> carefully detaches the usb-storage device: >>> >>> /* >>> * Hack alert: this pretends to be a block device, but it's really >>> * a SCSI bus that can serve only a single device, which it >>> * creates automatically. But first it needs to detach from its >>> * blockdev, or else scsi_bus_legacy_add_drive() dies when it >>> * attaches again. >>> * >>> * The hack is probably a bad idea. >>> */ >>> blk_detach_dev(blk, &s->dev.qdev); >>> s->conf.blk = NULL; >>> >>> Bad idea, but ABI. >>> >>> Before we make another bad idea ABI, let's stop and think. >>> >>> I believe the proper solution for your problem is qdevifying the SD >>> card. >> >> The question is whether there is a use for qdevifying the SD card. > > Okay, that's a fair question. > >> Each SD/MMC controller will have exactly zero or one SD cards, but the >> hw/sd/sd.c interface already treats "BlockBackend ejected" as "zero SD >> cards": >> >> if (!sd->blk || !blk_is_inserted(sd->blk) || !sd->enable) { >> return 0; >> } >> >> Unlike SCSI, the SD card code: >> >> 1) doesn't need multiplexing (a la scsi-hd/scsi-cd/scsi-generic) >> >> 2) doesn't have a bus to talk on (real-world SD cards are just connected >> with GPIO pins; hw/sd/sd.c abstracts the bitbanging protocol but still >> there is only one device to talk to) >> >> So in the end I think it's easier to treat hw/sd/sd.c as the common code >> for all hw/sd/* devices, like e.g. hw/display/vga.c. > > To pick a block device precedent: like floppy. I picked VGA because you have devices doing fancy stuff on top of the common code, but floppy and AHCI are similar too. > I don't like that the floppy controller and its drives are fused. > However, the fusing has been *much* less grief than the usb-storage > hack: basically just a weird user interface to configure the drives, > namely --global instead of --device. Right, which we don't even have in the case of sdhci-pci. > If sd.c is common code rather than a device model in its own right, > perhaps SDState should be unboxed in SDHCIState, just like the FDrive > are unboxed in FDCtrl. The "drive" property can then be connected > straight to SDState member blk. Yes, of course not 2.3 work. Added to BiteSizedTasks. > Perhaps split sd_init() into two parts: an inner, "common code" part, > and an outer, "independend non-qdevified device" part. I think I'm just going to move blk_attach_dev_nofail to the callers, so that then this patch can just remove the call in sdhci.c. Paolo