Re: staging driver s626 clashes with philips SAA7146 media/dvb based cards

[Herton Ronaldo Krzesinski <herton@mandriva.com.br>]

Em Qua 17 Jun 2009, às 20:35:13, Frank Mori Hess escreveu:
> On Wednesday 17 June 2009, Herton Ronaldo Krzesinski wrote:
> > Also s626_attach is modified to use now pci_get_subsys instead of
> > pci_get_device as reported by Ian Abbott, additionaly ensuring that
> > subvendor or subdevice id is set in pci id table entries.
> >
> > Reference: http://lkml.org/lkml/2009/6/16/552
> > ---
> >  drivers/staging/comedi/drivers/s626.c |   35
> > ++++++++++++++++++++------------ 1 files changed, 22 insertions(+), 13
> > deletions(-)
> >
> > diff --git a/drivers/staging/comedi/drivers/s626.c
> > b/drivers/staging/comedi/drivers/s626.c index 30dec9d..3ec4407 100644
> > --- a/drivers/staging/comedi/drivers/s626.c
> > +++ b/drivers/staging/comedi/drivers/s626.c
> > @@ -110,9 +110,9 @@ static const struct s626_board s626_boards[] = {
> >  #define PCI_VENDOR_ID_S626 0x1131
> >  #define PCI_DEVICE_ID_S626 0x7146
> >
> > +/* sub pci id must be specified, see s626_attach for more details */
> >  static DEFINE_PCI_DEVICE_TABLE(s626_pci_table) = {
> > -	{PCI_VENDOR_ID_S626, PCI_DEVICE_ID_S626, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> > -		0},
> > +	{PCI_VENDOR_ID_S626, PCI_DEVICE_ID_S626, 0x6000, 0x0272, 0, 0, 0},
> >  	{0}
> >  };
> >
> > @@ -498,24 +498,33 @@ static int s626_attach(struct comedi_device *dev,
> > struct comedi_devconfig *it) resource_size_t resourceStart;
> >  	dma_addr_t appdma;
> >  	struct comedi_subdevice *s;
> > -	struct pci_dev *pdev;
> > +	const struct pci_device_id *ids;
> > +	struct pci_dev *pdev = NULL;
> >
> >  	if (alloc_private(dev, sizeof(struct s626_private)) < 0)
> >  		return -ENOMEM;
> >
> > -	for (pdev = pci_get_device(PCI_VENDOR_ID_S626, PCI_DEVICE_ID_S626,
> > -			NULL); pdev != NULL;
> > -		pdev = pci_get_device(PCI_VENDOR_ID_S626,
> > -			PCI_DEVICE_ID_S626, pdev)) {
> > +	/*
> > +	 * Require also one of sub pci ids to be defined (see check below),
> > +	 * otherwise there will be a clash with Philips SAA7146 media/dvb
> > +	 * based cards (they have same vendor:device == 0x1131:0x7146 pair
> > +	 * as main S626 cards)
> > +	 */
> > +	for (ids = s626_pci_table;
> > +	     (ids->vendor && (ids->subvendor || ids->subdevice)) && !pdev;
> > +	     ids++) {
> > +		pdev = pci_get_subsys(ids->vendor, ids->device, ids->subvendor,
> > +				      ids->subdevice, NULL);
> > +		if (!pdev)
> > +			continue;
> > +
> >  		if (it->options[0] || it->options[1]) {
> > +			/* matches requested bus/slot */
> >  			if (pdev->bus->number == it->options[0] &&
> > -				PCI_SLOT(pdev->devfn) == it->options[1]) {
> > -				/* matches requested bus/slot */
> > +			    PCI_SLOT(pdev->devfn) == it->options[1])
> >  				break;
> > -			}
> > -		} else {
> > -			/* no bus/slot specified */
> > -			break;
> > +			pci_dev_put(pdev);
> > +			pdev = NULL;
> >  		}
> >  	}
> >  	devpriv->pdev = pdev;
> 
> 
> This patch looks buggy.  It's changing the logic beyond just checking for 
> subvendor/subdevice ids.
> 

That's the intention here, so that it avoids someone adding a new pci id
without specifying either subvendor or subdevice id for 0x1131:0x7146 boards,
but yes there will be a problem if boards with vendor:id not equal to
0x1131:0x7146 appear in future, as you will be obliged to add
subvendor:subdevice id even if not needed.

If not wanted and it gone too far, I can revert to use the same logic as
pci_match_id, or just simplify this in case it's unlikely more s626 boards
appear.

The current situation is ugly, comedi subsystem could have a better way to deal
with hotplug and probe of devices, without you having to reimplement what pci
subsystem functions already does.

--
[]'s
Herton