On Mon, 21 Jan 2008, Manu Abraham wrote:
> Trent Piepho wrote:
> > The DVB driver used to load for cards with sub-vendor/device 0x0000,0x0000,
> > but a previous patch changed that. An effect that that _should_ have been
> > mentioned in the description of the patch responsible, but was not.
>
> This doesn't have anything to do with accessing the wrong PCI function
> as what
> you have been trying to explain, as PCI function is based on Device ID
> and not
> SubDevice ID .. (pointless)
I never said anything about access the wrong pci function. That's not a
problem. The problem is that there are different mutually exclusive
drivers for the same pci function.
> > This patch changes the DVB driver to not auto-load for all bt878 chips. A
> > rather significant change that should be described in the patch
> > description, but is not.
>
> A noise out for nothing, that shouldn't have mattered ? But anyhow, the
> patch a
> while back changed that behaviour, the driver being loaded for all the
> devices.
> So how come this patch needs to have that description ?
No, the previous patch changed a different behavior. You don't seem to be
aware there is a difference between what the pci driver's probe function
will pass, and what PCI aliases are encoded into the module for
auto-loading purposes. An older applied patch had the *undocumented
side-effect* of changing the pci probe function to not drive devices with
the sub-vendor and sub-device of 0. This patch has the undocumented
side-effect of changing the pci aliases embedded in the module so that it
will not be auto-loaded for all sub-vendors/devices.
> >>>> +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.
> >>>
> >>> 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.
> >> This data can't be changed by pci core, since driver_data is a priv element. It
> >> is used as a pointer on some drivers, while others use it as an integer value to
> >> an array (like most V4L/DVB and ALSA drivers). The above code should work
> >> properly. If pci touches on this value, it would break the support for the rest
> >> of v4l/dvb drivers, so I think we don't need to worry about this.
> >
> > I don't think you understand. That code checks if the pci_device_id
> > pointer passed to the probe function points to a spot in the driver's pci
> > id table. It has nothing to do with the driver_data field being modified.
>
> what's the harm in it ?
If the pci layer ever changes certain internal implementation details it
will stop working. AFAIK, there is no guarantee that the pci_device_id
must point to in the driver's table. Only that is points to the correct id
for the pci_device that was passed and that the driver_data is correct.
It's poor programming to assume otherwise. It's obviously better to just
check if driver_data is NULL. That makes no unnecessary assumptions about
internal details and is simpler to boot.
_______________________________________________
linux-dvb mailing list
linux-dvb@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
