Re: [Qemu-devel] [PATCH 06/17] eepro100: symbolic names for pci registers

[Stefan Weil <weil@mail.berlios.de>]

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<mst@redhat.com>
>>> ---
>>> 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