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

Trent Piepho xyzzy at speakeasy.org
Mon Jan 21 08:41:01 CET 2008


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.



More information about the linux-dvb mailing list