Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756283AbZFRRoF (ORCPT ); Thu, 18 Jun 2009 13:44:05 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1753858AbZFRRnz (ORCPT ); Thu, 18 Jun 2009 13:43:55 -0400 Received: from perninha.conectiva.com.br ([200.140.247.100]:58246 "EHLO perninha.conectiva.com.br" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752723AbZFRRny convert rfc822-to-8bit (ORCPT ); Thu, 18 Jun 2009 13:43:54 -0400 From: Herton Ronaldo Krzesinski Organization: Mandriva To: Ian Abbott Subject: Re: staging driver s626 clashes with philips SAA7146 media/dvb based cards Date: Thu, 18 Jun 2009 14:43:56 -0300 User-Agent: KMail/1.11.90 (Linux/2.6.30-desktop-1mnb; KDE/4.2.90; i686; ; ) Cc: "fmhess@users.sourceforge.net" , Ian Abbott , Greg KH , LKML , Gianluca Palli , David Schleef References: <200906161701.45234.herton@mandriva.com.br> <200906181423.11245.herton@mandriva.com.br> <4A3A7A2F.3020800@mev.co.uk> In-Reply-To: <4A3A7A2F.3020800@mev.co.uk> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 8BIT Message-Id: <200906181443.57660.herton@mandriva.com.br> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7295 Lines: 180 Em Quinta-feira 18 Junho 2009, ?s 14:32:31, Ian Abbott escreveu: > On 18/06/09 18:23, Herton Ronaldo Krzesinski wrote: > > Let me know if there is any problem remaining with this version: > > > > Staging: comedi: s626: use subvendor:subdevice ids for SAA7146 board > > > > The current s626 comedi driver in staging conflicts with philips SAA7146 > > media/dvb based cards, because it claims the same vendor:device pci id > > for all subdevice/subvendor ids. What happens is that for people that have a > > philips SAA7146 media/dvb based card, s626 if available gets loaded by udev > > and makes system freeze (https://qa.mandriva.com/show_bug.cgi?id=51445). > > > > The s626 driver shouldn't claim all 1131:7146 devices. Fix this by > > specifying specific known subvendor:subdevice ids in its pci id table > > list. > > > > Also s626_attach is modified to use now pci_get_subsys instead of > > pci_get_device as reported by Ian Abbott, and now we loop over pci id > > table entries in case more ids are added in the future. > > > > Reference: http://lkml.org/lkml/2009/6/16/552 > > > > Signed-off-by: Herton Ronaldo Krzesinski > > > > diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c > > index 30dec9d..4210590 100644 > > --- a/drivers/staging/comedi/drivers/s626.c > > +++ b/drivers/staging/comedi/drivers/s626.c > > @@ -110,9 +110,13 @@ static const struct s626_board s626_boards[] = { > > #define PCI_VENDOR_ID_S626 0x1131 > > #define PCI_DEVICE_ID_S626 0x7146 > > > > +/* > > + * For devices with vendor:device id == 0x1131:0x7146 you must specify > > + * also subvendor:subdevice ids, because otherwise it will conflict with > > + * Philips SAA7146 media/dvb based cards. > > + */ > > 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,25 +502,26 @@ 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)) { > > - if (it->options[0] || it->options[1]) { > > - if (pdev->bus->number == it->options[0] && > > - PCI_SLOT(pdev->devfn) == it->options[1]) { > > + for (i = 0; i < ARRAY_SIZE(s626_pci_table) && !pdev; i++) { > > + ids = &s626_pci_table[i]; > > + do { > > + pdev = pci_get_subsys(ids->vendor, ids->device, ids->subvendor, > > + ids->subdevice, pdev); > > + > > + if ((it->options[0] || it->options[1]) && pdev) { > > /* matches requested bus/slot */ > > + if (pdev->bus->number == it->options[0] && > > + PCI_SLOT(pdev->devfn) == it->options[1]) > > + break; > > + } else > > break; > > - } > > - } else { > > - /* no bus/slot specified */ > > - break; > > - } > > + } while (1); > > } > > devpriv->pdev = pdev; > > The outer for loop iterates once too often - it doesn't need to iterate > over the sentinel at the end of the id table as that shouldn't match any > PCI device. Ouch, yep didn't saw that, new version: Staging: comedi: s626: use subvendor:subdevice ids for SAA7146 board The current s626 comedi driver in staging conflicts with philips SAA7146 media/dvb based cards, because it claims the same vendor:device pci id for all subdevice/subvendor ids. What happens is that for people that have a philips SAA7146 media/dvb based card, s626 if available gets loaded by udev and makes system freeze (https://qa.mandriva.com/show_bug.cgi?id=51445). The s626 driver shouldn't claim all 1131:7146 devices. Fix this by specifying specific known subvendor:subdevice ids in its pci id table list. Also s626_attach is modified to use now pci_get_subsys instead of pci_get_device as reported by Ian Abbott, and now we loop over pci id table entries in case more ids are added in the future. Reference: http://lkml.org/lkml/2009/6/16/552 Signed-off-by: Herton Ronaldo Krzesinski diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c index 30dec9d..7bf2a79 100644 --- a/drivers/staging/comedi/drivers/s626.c +++ b/drivers/staging/comedi/drivers/s626.c @@ -110,9 +110,13 @@ static const struct s626_board s626_boards[] = { #define PCI_VENDOR_ID_S626 0x1131 #define PCI_DEVICE_ID_S626 0x7146 +/* + * For devices with vendor:device id == 0x1131:0x7146 you must specify + * also subvendor:subdevice ids, because otherwise it will conflict with + * Philips SAA7146 media/dvb based cards. + */ 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,25 +502,26 @@ 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)) { - if (it->options[0] || it->options[1]) { - if (pdev->bus->number == it->options[0] && - PCI_SLOT(pdev->devfn) == it->options[1]) { + for (i = 0; i < (ARRAY_SIZE(s626_pci_table) - 1) && !pdev; i++) { + ids = &s626_pci_table[i]; + do { + pdev = pci_get_subsys(ids->vendor, ids->device, ids->subvendor, + ids->subdevice, pdev); + + if ((it->options[0] || it->options[1]) && pdev) { /* matches requested bus/slot */ + if (pdev->bus->number == it->options[0] && + PCI_SLOT(pdev->devfn) == it->options[1]) + break; + } else break; - } - } else { - /* no bus/slot specified */ - break; - } + } while (1); } devpriv->pdev = pdev; -- []'s Herton -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/