From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:60362) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ScGlK-0001DO-AH for qemu-devel@nongnu.org; Wed, 06 Jun 2012 09:59:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ScGlA-0005ZE-BB for qemu-devel@nongnu.org; Wed, 06 Jun 2012 09:59:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:9382) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ScGlA-0005Yz-3P for qemu-devel@nongnu.org; Wed, 06 Jun 2012 09:58:52 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q56DwoqZ031727 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 6 Jun 2012 09:58:50 -0400 Message-ID: <4FCF6217.2050907@redhat.com> Date: Wed, 06 Jun 2012 15:58:47 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <095b1f20091f8cf18565a3700372fa65736f0d0a.1338911144.git.phrdina@redhat.com> <4FCE2AC8.9080306@redhat.com> <4FCE2C32.1060309@redhat.com> <4FCE3508.8030009@redhat.com> <4FCF3282.2080408@redhat.com> <4FCF5E72.1020809@redhat.com> In-Reply-To: <4FCF5E72.1020809@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2] fdc: fix media change detection for windows List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pavel Hrdina Cc: mtosatti@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com Am 06.06.2012 15:43, schrieb Pavel Hrdina: >>> I'll move the code into the end of 'fd_seek' function, where a step i= s=20 >>> guaranteed. Internal seek should also reset the 'media_changed' bit i= f=20 >>> there is a media in the drive. I'll create the qtest tomorrow then I'= ll=20 >>> send another version of this patch. >> My point is that the success of a seek has nothing to do with the DSKC= HG >> signal. Even if you move the code around, DSKCHG may start to coincide >> with the condition you really want to check, but it's logically still >> not what should be checked. What's wrong with !bdrv_is_inserted() in t= he >> if condition? >> >> If something is wrong with the media_changed handling, then it's a >> separate bug that should be dealt with in a separate patch. >> >> Kevin > I don't think that is true. Please read this > http://wiki.osdev.org/Floppy_Disk_Controller#DIR_register.2C_Disk_Chang= e_bit > and http://wiki.osdev.org/Floppy_Disk_Controller#Seek . Directly it has > nothing to do with DSKCHG bit, but indirectly seek to different cylinde= r > fails if there is no media in drive on real FDC. Only successful seek t= o > different cylinder clears the DSKCHG bit. If you have the choice between a direct way (bdrv_is_inserted) and an indirect one (DSKCHG), you should always use the directly related one. It makes the intention of the code clearer and is less likely to break, because you actually said what you want and not just something that you believe happens at the same time. So your explanation confirms for me that the check should be whether a medium is in the drive, i.e. bdrv_is_inserted(). > That's why I do it this way, because in our FDC emulation we clears the > DSKCHG bit or not also if you seek to the same cylinder. The function > 'fd_seek' stands for implied seek which also clears the DSKCHG bit. That the DSKCHG bit is cleared even if the head doesn't move is a bug in our code. I accepted Herv=E9's patches because they were a big improvement, but they don't implement quite correct behaviour yet. Relying on buggy behaviour is obviously wrong. Anyway, I'm not arguing against clearing DSKCHG in this function. I just say that it addresses an unrelated problem and should be posted in a different patch. And even then the updated DSKCHG isn't directly related to the check you're introducing in this patch, so it still would have to stay bdrv_is_inserted() there. Kevin