[linux-dvb] [v4l-dvb-maintainer] [PATCH 1/2] bt878: remove handcrafted PCI subsystem ID check

Manu Abraham abraham.manu at gmail.com
Sat Jan 19 20:48:11 CET 2008


Trent Piepho wrote:
> On Sun, 20 Jan 2008, Akinobu Mita wrote:
>>>> +static const char * __devinit card_name(const struct pci_device_id *id)
>>>> +{
>>>> +     if (id >= bt878_pci_tbl &&
>>>> +         id < bt878_pci_tbl + ARRAY_SIZE(bt878_pci_tbl) - 1)
>>>> +             return (const char *)id->driver_data;
>>> Are you sure this is safe?  I don't remember reading that the pci_device_id
>>> passed to the pci driver probe method must be from the device id table.
>>> Couldn't the device id be a local variable from whatever calls probe, as
>>> long as driver_data is set to the correct value?  I looked at the current
>>> pci bus code, and it does always pass a pointer into the driver's pci id
>>> table.  But that could change.
>> Yes, that could change. And then it always returns "Unknown".
> 
> Which would be wrong.

That doesn't matter, as the real identification isn't known to the PCI subsystem 
at all. It is always retreived from the ASIC's ROM (for the majorirt of the cards in 
there)rather than the EEPROM upload, eventually the naming as reported at 
PCI probe time is of no consequence and useless.

> 
>> The safe way is:
>>
>> static const char * __devinit card_name(const struct pci_device_id *id)
>> {
>>         int i;
>>
>>         for (i = 0; i < ARRAY_SIZE(bt878_pci_tbl) - 1; i++) {
>>                 if (bt878_pci_tbl[i].vendor = id->vendor &&
>>                     bt878_pci_tbl[i].device = id->device &&
>>                     bt878_pci_tbl[i].subvendor = id->subvendor &&
>>                     bt878_pci_tbl[i].subdevice = id->subdevice)
>>                         return (const char *)bt878_pci_tbl.driver_data;
>>         }
>>         return "Unknown";
>> }
> 
> The pci layer has a function to do this for you.  But it's unnecessary.
> 
>>> It seems like a better method would just be to check if driver_data is 0,
>>> which will be the case if the id was added dynamically.
>>>
>> driver_data can be non-zero value. we can set it through new_id.
>> But there are many possibilities to break kernel by writing
> 
> Nope, driver_data will always be 0.  Try it.  You must set
> dynids.use_driver_data to allow driver data to be passed via sysfs for just
> this reason.
> 
>> Acutually I don't have a special preference for any of which.
>> Because we can remove the function card_name() after submitting
>> all the card name information that bt878.c holds to PCI ID repository.
>> (http://pci-ids.ucw.cz/iii/?i=109e0878)
>>
>> Then we can get the card name from lspci instead of seeing dmesg.
>

lspci wouldn't show up the actual device names, for a lack of what i mentioned
above. (Just pointless)

Manu




More information about the linux-dvb mailing list