[Qemu-devel] Re: [PATCH 05/20] eepro100: Add all supported devices to pci.c

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

Michael S. Tsirkin schrieb:
> On Wed, Mar 03, 2010 at 02:31:53PM +0100, Gerd Hoffmann wrote:
>> On 03/03/10 12:51, Michael S. Tsirkin wrote:
>>> On Sun, Feb 21, 2010 at 10:08:49PM +0100, Stefan Weil wrote:
>>>>>> static const char * const pci_nic_models[] = {
>>>>>> "ne2k_pci",
>>>>>> + "i82550",
>>>>>> "i82551",
>>>>>> + "i82557a",
>>>>>> "i82557b",
>>>>>> + "i82557c",
>>>>>> + "i82558a",
>>>>>> + "i82558b",
>>>>>> + "i82559a",
>>>>>> + "i82559b",
>>>>>> + "i82559c",
>>>>>> "i82559er",
>>>>>> + "i82562",
>>>>>> "rtl8139",
>>>>>> "e1000",
>>>>>> "pcnet",
>>>>> One wonders: would it be cleaner to have a single eepro100 device
>>>>> with specific model as qdev option?
>> No. I think it would be cleaner to use qdev even more. For that
>> specific driver it would make sense to create a private Info struct,
>> i.e. something like this:
>>
>> E100PCIDeviceInfo {
>> PCIDeviceInfo pci;
>> [ model specific stuff such as has_extended_tcb_support here ]
>> };
>>
>> Then go
>>
>> static E100PCIDeviceInfo eepro100_info[] = {
>> {
>> .pci.qdev.name = "i82550",
>> [ usual qdev stuff here ]
>> has_extended_tcb_support = 0 | 1;
>> [ more e100 device description bits here ]
>> },
>> [ ... ]
>> };
>>
>> Then you can simply zap all the one-liner wrapper functions around
>> nic_init(). nic_init() can get a pointer using something like this:
>>
>> E100PCIDeviceInfo *einfo = DO_UPCAST(E100PCIDeviceInfo,
>> pci.qdev, pci_dev->qdev.info);
>>
>> and setup the device accordingly.

This would indeed simplify the code.

Although I don't like tricky programming like DO_UPCAST
(because I saw too much code where tricky programming
was faulty programming), I'll consider to modify the code
as Gerd proposed.

>>>> I prefer the "official" device names which are also
>>>> used in the Intel documentation. eepro100 or e100
>>>> are marketing names (more of them exist).
>> qdev has aliases. You can add .qdev.alias = "eepro100" to one of the
>> variants, then the eepro100 name will work as well.

Good idea. To keep the number of aliases small, I plan to
add only one or two aliases: e100 (eepro100 only if a second
alias is possible).

Why? e100 is the current linux driver name.
eepro100 was the former linux driver name.

>>
>> Adding the marketing name to the .desc test is probably a good idea too
>> because alot of people know the marketing name only.
>
> That's my thinking as well.

Neither eepro100 nor e100 are marketing names
(I was wrong when I wrote that in an earlier mail).

The marketing name was Intel EtherExpress Pro 100.
I agree that it would help people to see this name,
so I suggest adding it to qemu-doc.texi.

As far as I know, only insiders (and the old linux driver)
used the abbreviations eepro100 and e100.

Look at your favorite search machine:
nobody sells / sold eepro100 cards.

If you want a less technical name than i82559c, e100 is the
better choice - like e1000 for the successor.

>
>>>> Please note that this patch was marked as optional.
>>>> The arrays pci_nic_names and pci_nic_models are
>>>> not really needed, and as soon as the table of available
>>>> nics is automatically derived from the device information,
>>>> all variants of i825xx are supported automatically.
>>> Gerd, any input on this patch?
>> Looks fine to me. When we switch over to walking the list of registered
>> devices instead of having a hard-coded list of pci nic drivers all the
>> eepro100 variants will show up in the list anyway.
>>
>> cheers,
>> Gerd