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

From: Herton Ronaldo Krzesinski
Date: Wed Jun 17 2009 - 20:06:16 EST


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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/