From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NStKB-0004V6-4U for qemu-devel@nongnu.org; Thu, 07 Jan 2010 09:26:55 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NStK6-0004KG-DH for qemu-devel@nongnu.org; Thu, 07 Jan 2010 09:26:54 -0500 Received: from [199.232.76.173] (port=50715 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NStK6-0004K4-0e for qemu-devel@nongnu.org; Thu, 07 Jan 2010 09:26:50 -0500 Received: from moutng.kundenserver.de ([212.227.17.8]:51354) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NStK5-0002Ry-Es for qemu-devel@nongnu.org; Thu, 07 Jan 2010 09:26:49 -0500 Message-ID: <4B45EF20.3010305@mail.berlios.de> Date: Thu, 07 Jan 2010 15:26:40 +0100 From: Stefan Weil MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 06/17] eepro100: symbolic names for pci registers References: <20091210181046.GG25707@redhat.com> <4B45C209.1030802@mail.berlios.de> <4B45E26B.5040401@codemonkey.ws> In-Reply-To: <4B45E26B.5040401@codemonkey.ws> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Paul Brook , qemu-devel@nongnu.org Anthony Liguori schrieb: > On 01/07/2010 05:14 AM, Stefan Weil wrote: >> Michael S. Tsirkin schrieb: >> >>> No functional changes. I verified that the generated binary >>> does not change in meaningful ways. Survived light usage >>> with linux guest. >>> >>> Signed-off-by: Michael S. Tsirkin >>> --- >>> hw/eepro100.c | 49 ++++++++++++++++++++++++++++++++----------------- >>> 1 files changed, 32 insertions(+), 17 deletions(-) >>> >>> diff --git a/hw/eepro100.c b/hw/eepro100.c >>> index 2a9e3b5..82e3766 100644 >>> --- a/hw/eepro100.c >>> +++ b/hw/eepro100.c >>> @@ -412,19 +412,24 @@ static void pci_reset(EEPRO100State * s) >>> pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL); >>> /* PCI Device ID depends on device and is set below. */ >>> /* PCI Command */ >>> + /* TODO: this is the default, do not override. */ >>> PCI_CONFIG_16(PCI_COMMAND, 0x0000); >>> /* PCI Status */ >>> - PCI_CONFIG_16(PCI_STATUS, 0x2800); >>> + /* TODO: this seems to make no sense. */ >>> + /* TODO: Value at RST# should be 0. */ >>> + PCI_CONFIG_16(PCI_STATUS, >>> + PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_SIG_TARGET_ABORT); >>> /* PCI Revision ID */ >>> >> Hi, >> >> this PCI status value is wrong. The correct value for PCI_STATUS is >> 0x0280 >> and was fixed in the maintainer version in 2007: >> http://repo.or.cz/w/qemu/ar7.git/commitdiff/9da3830d81948cc1f666fcf562699f165b029a79 >> >> >> It was also fixed in a patch sent to qemu-devel (which was never >> applied): >> http://patchwork.ozlabs.org/patch/33962/ >> >> I'll send a new patch which fixes the wrong status value. >> >> Antony, how can we speed up the synchronisation process for >> eepro100.c? Today, patches get lost without feedback. >> This is no wonder because you have to work on hundreds of patches. >> It was suggested that I should send patch series. >> My last patch serie with 3 patches is ready for integration >> since 2009-12-20. >> > > The big problem with eepro100 is that you're doing a bunch of work in > an external tree, and then trickling things slowly onto the list. > Take some time and send out one big series getting your internal tree > in sync. Of course, the end target must conform to CodingStyle which > is a problem right now in your tree. > >> Paul, if I had commit rights, I could integrate the missing >> parts myself and maintain eepro100.c in the future. Of course >> I'd send the single changes to the list before comitting them. >> Don't you think that would be the best solution for all of us? >> > > Pull requests work just as well as commit rights. However, pull > requests only work when the patches are ready to go and don't need > iteration. A bare minimum for that is going to be conforming to > CodingStyle which has been a problem with eepro100. > > But look, you send out patches during a holiday, and we're still > catching up. If it had been during a normal time period, that would > be one thing, but please exercise a bit of patience. > > Regards, > > Anthony Liguori Hello Antony, thank you for your fast feedback. I also think that coding style is important, so don't hesitate to tell me when there are problems. The only violations of coding style in eepro100.c I am aware of are C++ comments and data types using _t. They are relicts from the time when QEMU did not have a documented coding style, were addressed in previous patches and are also fixed in new patches. Are there other coding style issues which I did not see? Regards, Stefan Weil