| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4A3A8687.6090000@mev.co.uk>
Date: Thu, 18 Jun 2009 19:25:11 +0100
From: Ian Abbott <abbotti@....co.uk>
To: Herton Ronaldo Krzesinski <herton@...driva.com.br>
CC: Ian Abbott <ian.abbott@....co.uk>,
"fmhess@...rs.sourceforge.net" <fmhess@...rs.sourceforge.net>,
Greg KH <gregkh@...e.de>, LKML <linux-kernel@...r.kernel.org>,
Gianluca Palli <gpalli@...s.unibo.it>,
David Schleef <ds@...leef.org>
Subject: Re: staging driver s626 clashes with philips SAA7146 media/dvb based
cards
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 <herton@...driva.com.br>
>>>
>>> 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 <herton@...driva.com.br>
This looks fine now, though I think it's unlikely this driver will
support more than one subdevice ID.
Signed-off-by: Ian Abbott <abbotti@....co.uk>
>
> 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: <abbotti@....co.uk> )=-
-=( 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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists