Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754031AbZFRSZZ (ORCPT ); Thu, 18 Jun 2009 14:25:25 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752541AbZFRSZM (ORCPT ); Thu, 18 Jun 2009 14:25:12 -0400 Received: from mail.mev.co.uk ([62.49.15.74]:51994 "EHLO mail.mev.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752499AbZFRSZL (ORCPT ); Thu, 18 Jun 2009 14:25:11 -0400 Message-ID: <4A3A8687.6090000@mev.co.uk> Date: Thu, 18 Jun 2009 19:25:11 +0100 From: Ian Abbott User-Agent: Thunderbird 2.0.0.21 (X11/20090502) MIME-Version: 1.0 To: Herton Ronaldo Krzesinski CC: Ian Abbott , "fmhess@users.sourceforge.net" , Greg KH , LKML , Gianluca Palli , David Schleef Subject: Re: staging driver s626 clashes with philips SAA7146 media/dvb based cards References: <200906161701.45234.herton@mandriva.com.br> <200906181423.11245.herton@mandriva.com.br> <4A3A7A2F.3020800@mev.co.uk> <200906181443.57660.herton@mandriva.com.br> In-Reply-To: <200906181443.57660.herton@mandriva.com.br> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8383 Lines: 191 On 18/06/09 18:43, Herton Ronaldo Krzesinski wrote: > 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 This looks fine now, though I think it's unlikely this driver will support more than one subdevice ID. Signed-off-by: Ian Abbott > > 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 -- -=( Ian Abbott @ MEV Ltd. E-mail: )=- -=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=- -- 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/