From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:49064) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SUOhA-0002y2-FL for qemu-devel@nongnu.org; Tue, 15 May 2012 16:50:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SUOh8-0007S9-CA for qemu-devel@nongnu.org; Tue, 15 May 2012 16:50:12 -0400 Received: from smtp1-g21.free.fr ([212.27.42.1]:55300) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SUOh7-0007Ga-Qi for qemu-devel@nongnu.org; Tue, 15 May 2012 16:50:10 -0400 Message-ID: <4FB2C172.6020401@reactos.org> Date: Tue, 15 May 2012 22:49:54 +0200 From: =?ISO-8859-15?Q?Herv=E9_Poussineau?= MIME-Version: 1.0 References: <1337073445-9679-1-git-send-email-zhihuili@linux.vnet.ibm.com> <1337073445-9679-3-git-send-email-zhihuili@linux.vnet.ibm.com> <4FB22171.7050104@redhat.com> In-Reply-To: <4FB22171.7050104@redhat.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 2/3 v6] Replace bdrv_* to bdrv_aio_* functions in DMA mode in fdc.c List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Li Zhi Hui , qemu-devel@nongnu.org Hi, Paolo Bonzini a =E9crit : > Il 15/05/2012 11:17, Li Zhi Hui ha scritto: >> Signed-off-by: Paolo Bonzini >> Signed-off-by: Li Zhi Hui >> --- >> hw/fdc.c | 313 +++++++++++++++++++++++++++++++++++++++++------------= -------- >> 1 files changed, 210 insertions(+), 103 deletions(-) >=20 > To reviewers, this is obviously missing migration support. :) >=20 > It is also missing a good commit message. It should say that the > support in the existing code for scan commands has a lot of "weirdness"= , > for example: >=20 > - if ((ret < 0 && fdctrl->data_dir =3D=3D FD_DIR_SCANL) = || > - (ret > 0 && fdctrl->data_dir =3D=3D FD_DIR_SCANH))= { > - status2 =3D 0x00; > - goto end_transfer; >=20 > ... >=20 > - end_transfer: > - len =3D fdctrl->data_pos - start_pos; > - FLOPPY_DPRINTF("end transfer %d %d %d\n", > - fdctrl->data_pos, len, fdctrl->data_len); > - if (fdctrl->data_dir =3D=3D FD_DIR_SCANE || > - fdctrl->data_dir =3D=3D FD_DIR_SCANL || > - fdctrl->data_dir =3D=3D FD_DIR_SCANH) > - status2 =3D FD_SR2_SEH; >=20 > which blindly overwrites status2. Hence the new code was not written > based on it. However, the new code is untested as far as I know. >=20 >> + if (fdctrl->data_dir =3D=3D FD_DIR_SCANE || >> + fdctrl->data_dir =3D=3D FD_DIR_SCANL || >> + fdctrl->data_dir =3D=3D FD_DIR_SCANH) { >> + fdctrl->dma_status2 =3D FD_SR2_SEH; >> + } >=20 > This should be FD_SR2_SNS (Spec for the FDC is at > http://www.cepece.info/amstrad/docs/i8272/8272sp.htm: "If the condition= s > for scan are not met between the the starting sector (as specified by R= ) > and the last sector on the cylinder (EOT), then the FDC sets the SN > (Scan Not Satisfied) flag of Status Register 2 to a 1 (high), and > terminates the Scan Command"). >=20 > I'm not sure whether scan commands should be kept at all though. Herv=E9= , > do you know anything that uses them? No, I don't know anything which uses it. I will ack patches which remove the scan command implementations and=20 call the unimplemented command handler instead (like for=20 format_and_write command). Regards, Herv=E9