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

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

[Herton Ronaldo Krzesinski]

Hi,

The 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 based card, s626 if available gets loaded by udev and makes system 
freeze (https://qa.mandriva.com/show_bug.cgi?id=51445).

Looks like s626 shouldn't claim all 1131:7146 devices, either by specifying 
specific subdevice/subvendor ids specific to s626 devices or doing additional 
checks in its probe/attach function.

--
[]'s
Herton

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

[Greg KH]

On Tue, Jun 16, 2009 at 05:01:44PM -0300, Herton Ronaldo Krzesinski wrote:
> Hi,
> 
> The 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 based card, s626 if available gets loaded by udev and makes system 
> freeze (https://qa.mandriva.com/show_bug.cgi?id=51445).

So a PCI device that does different things has the same device ids?
ick, stupid vendors...

> Looks like s626 shouldn't claim all 1131:7146 devices, either by specifying 
> specific subdevice/subvendor ids specific to s626 devices or doing additional 
> checks in its probe/attach function.

If you can propose the proper sub ids, or the needed checks, please send
a patch.

thanks,

greg k-h

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

[Herton Ronaldo Krzesinski]

Em Terça-feira 16 Junho 2009, às 17:51:21, Greg KH escreveu:
> On Tue, Jun 16, 2009 at 05:01:44PM -0300, Herton Ronaldo Krzesinski wrote:
> > Hi,
> >
> > The 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 based card, s626 if available gets loaded by udev
> > and makes system freeze (https://qa.mandriva.com/show_bug.cgi?id=51445).
>
> So a PCI device that does different things has the same device ids?
> ick, stupid vendors...
>
> > Looks like s626 shouldn't claim all 1131:7146 devices, either by
> > specifying specific subdevice/subvendor ids specific to s626 devices or
> > doing additional checks in its probe/attach function.
>
> If you can propose the proper sub ids, or the needed checks, please send
> a patch.

Can't propose proper sub ids here etc., as I don't know about/don't have s626 
device, s626 author is CC'ed here to check this. But I could send a patch to 
disable just the build of s626 if acceptable/desired for the moment.

>
> thanks,
>
> greg k-h

--
[]'s
Herton

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

[Ian Abbott]

Herton Ronaldo Krzesinski wrote:
> Em Terça-feira 16 Junho 2009, às 17:51:21, Greg KH escreveu:
>> On Tue, Jun 16, 2009 at 05:01:44PM -0300, Herton Ronaldo Krzesinski wrote:
>>> Hi,
>>>
>>> The 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 based card, s626 if available gets loaded by udev
>>> and makes system freeze (https://qa.mandriva.com/show_bug.cgi?id=51445).
>> So a PCI device that does different things has the same device ids?
>> ick, stupid vendors...
>>
>>> Looks like s626 shouldn't claim all 1131:7146 devices, either by
>>> specifying specific subdevice/subvendor ids specific to s626 devices or
>>> doing additional checks in its probe/attach function.
>> If you can propose the proper sub ids, or the needed checks, please send
>> a patch.
> 
> Can't propose proper sub ids here etc., as I don't know about/don't have s626 
> device, s626 author is CC'ed here to check this. But I could send a patch to 
> disable just the build of s626 if acceptable/desired for the moment.

The Windows driver (<http://www.sensoray.com/downloads/sdk626.zip>) has
this in the models section of the INF file:

%sx26.DeviceDesc%=sxdrv.Device,PCI\VEN_1131&DEV_7146&SUBSYS_02726000

And it looks like the correct device because this the strings section
contains:

sx26.DeviceDesc=   "Sensoray Model 626 Analog/Digital I/O"

Interpreting the above information gives us:

PCI Vendor ID = 0x1131
PCI Device ID = 0x7146
PCI Subvendor ID = 0x6000
PCI Subdevice ID = 0x0272 (626)

The Linux SDK for this board
(<http://www.sensoray.com/downloads/s626-1.0.1.tar.gz> has the same info
in the s626core.h file.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

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

[Herton Ronaldo Krzesinski]

Em Quarta-feira 17 Junho 2009, às 09:26:00, Ian Abbott escreveu:
> Herton Ronaldo Krzesinski wrote:
> > Em Terça-feira 16 Junho 2009, às 17:51:21, Greg KH escreveu:
> >> On Tue, Jun 16, 2009 at 05:01:44PM -0300, Herton Ronaldo Krzesinski wrote:
> >>> Hi,
> >>>
> >>> The 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 based card, s626 if available gets loaded by udev
> >>> and makes system freeze (https://qa.mandriva.com/show_bug.cgi?id=51445).
> >> So a PCI device that does different things has the same device ids?
> >> ick, stupid vendors...
> >>
> >>> Looks like s626 shouldn't claim all 1131:7146 devices, either by
> >>> specifying specific subdevice/subvendor ids specific to s626 devices or
> >>> doing additional checks in its probe/attach function.
> >> If you can propose the proper sub ids, or the needed checks, please send
> >> a patch.
> > 
> > Can't propose proper sub ids here etc., as I don't know about/don't have s626 
> > device, s626 author is CC'ed here to check this. But I could send a patch to 
> > disable just the build of s626 if acceptable/desired for the moment.
> 
> The Windows driver (<http://www.sensoray.com/downloads/sdk626.zip>) has
> this in the models section of the INF file:
> 
> %sx26.DeviceDesc%=sxdrv.Device,PCI\VEN_1131&DEV_7146&SUBSYS_02726000
> 
> And it looks like the correct device because this the strings section
> contains:
> 
> sx26.DeviceDesc=   "Sensoray Model 626 Analog/Digital I/O"
> 
> Interpreting the above information gives us:
> 
> PCI Vendor ID = 0x1131
> PCI Device ID = 0x7146
> PCI Subvendor ID = 0x6000
> PCI Subdevice ID = 0x0272 (626)
> 
> The Linux SDK for this board
> (<http://www.sensoray.com/downloads/s626-1.0.1.tar.gz> has the same info
> in the s626core.h file.

Ok thanks. So lets limit s626 by its subvendor:subdevice id, patch follows:

>From 6f4d2430959a378ab754c5dbd3903fdcf33abe36 Mon Sep 17 00:00:00 2001
From: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
Date: Wed, 17 Jun 2009 13:31:15 -0300
Subject: [PATCH] Staging: comedi: use subvendor:subdevice ids for its pci id device table

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 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.

Reference: http://lkml.org/lkml/2009/6/16/552

Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
---
 drivers/staging/comedi/drivers/s626.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c
index 30dec9d..b4b7713 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
 
+/*
+ * Note: always specify subvendor:subdevice ids in table below, to avoid
+ * clash with Philips SAA7146 media/dvb based cards which have same
+ * vendor:device ids as S626
+ */
 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}
 };
 
-- 
1.6.3.2

--
[]'s
Herton

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

[Ian Abbott]

Herton Ronaldo Krzesinski wrote:
> Em Quarta-feira 17 Junho 2009, às 09:26:00, Ian Abbott escreveu:
>> Herton Ronaldo Krzesinski wrote:
>>> Em Terça-feira 16 Junho 2009, às 17:51:21, Greg KH escreveu:
>>>> On Tue, Jun 16, 2009 at 05:01:44PM -0300, Herton Ronaldo Krzesinski wrote:
>>>>> Hi,
>>>>>
>>>>> The 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 based card, s626 if available gets loaded by udev
>>>>> and makes system freeze (https://qa.mandriva.com/show_bug.cgi?id=51445).
>>>> So a PCI device that does different things has the same device ids?
>>>> ick, stupid vendors...
>>>>
>>>>> Looks like s626 shouldn't claim all 1131:7146 devices, either by
>>>>> specifying specific subdevice/subvendor ids specific to s626 devices or
>>>>> doing additional checks in its probe/attach function.
>>>> If you can propose the proper sub ids, or the needed checks, please send
>>>> a patch.
>>> Can't propose proper sub ids here etc., as I don't know about/don't have s626
>>> device, s626 author is CC'ed here to check this. But I could send a patch to
>>> disable just the build of s626 if acceptable/desired for the moment.
>> The Windows driver (<http://www.sensoray.com/downloads/sdk626.zip>) has
>> this in the models section of the INF file:
>>
>> %sx26.DeviceDesc%=sxdrv.Device,PCI\VEN_1131&DEV_7146&SUBSYS_02726000
>>
>> And it looks like the correct device because this the strings section
>> contains:
>>
>> sx26.DeviceDesc=   "Sensoray Model 626 Analog/Digital I/O"
>>
>> Interpreting the above information gives us:
>>
>> PCI Vendor ID = 0x1131
>> PCI Device ID = 0x7146
>> PCI Subvendor ID = 0x6000
>> PCI Subdevice ID = 0x0272 (626)
>>
>> The Linux SDK for this board
>> (<http://www.sensoray.com/downloads/s626-1.0.1.tar.gz> has the same info
>> in the s626core.h file.
>
> Ok thanks. So lets limit s626 by its subvendor:subdevice id, patch follows:
>
> From 6f4d2430959a378ab754c5dbd3903fdcf33abe36 Mon Sep 17 00:00:00 2001
> From: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
> Date: Wed, 17 Jun 2009 13:31:15 -0300
> Subject: [PATCH] Staging: comedi: use subvendor:subdevice ids for its pci id device table
>
> 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 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.
>
> Reference: http://lkml.org/lkml/2009/6/16/552
>
> Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
> ---
>  drivers/staging/comedi/drivers/s626.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c
> index 30dec9d..b4b7713 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
>
> +/*
> + * Note: always specify subvendor:subdevice ids in table below, to avoid
> + * clash with Philips SAA7146 media/dvb based cards which have same
> + * vendor:device ids as S626
> + */
>  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}
>  };
>
> --
> 1.6.3.2

There are also a couple of calls to pci_get_device() that need changing
to pci_get_subsys() in the s626_attach() function (so it also might be
worth #define'ing the subsys numbers to avoid repeating these magic
numbers).

--
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>             )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587              )=-

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

[Herton Ronaldo Krzesinski]

Em Qua 17 Jun 2009, às 15:21:48, Ian Abbott escreveu:
> Herton Ronaldo Krzesinski wrote:
> > Em Quarta-feira 17 Junho 2009, às 09:26:00, Ian Abbott escreveu:
> >> Herton Ronaldo Krzesinski wrote:
> >>> Em Terça-feira 16 Junho 2009, às 17:51:21, Greg KH escreveu:
> >>>> On Tue, Jun 16, 2009 at 05:01:44PM -0300, Herton Ronaldo Krzesinski wrote:
> >>>>> Hi,
> >>>>>
> >>>>> The 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 based card, s626 if available gets loaded by udev
> >>>>> and makes system freeze (https://qa.mandriva.com/show_bug.cgi?id=51445).
> >>>> So a PCI device that does different things has the same device ids?
> >>>> ick, stupid vendors...
> >>>>
> >>>>> Looks like s626 shouldn't claim all 1131:7146 devices, either by
> >>>>> specifying specific subdevice/subvendor ids specific to s626 devices or
> >>>>> doing additional checks in its probe/attach function.
> >>>> If you can propose the proper sub ids, or the needed checks, please send
> >>>> a patch.
> >>> Can't propose proper sub ids here etc., as I don't know about/don't have s626
> >>> device, s626 author is CC'ed here to check this. But I could send a patch to
> >>> disable just the build of s626 if acceptable/desired for the moment.
> >> The Windows driver (<http://www.sensoray.com/downloads/sdk626.zip>) has
> >> this in the models section of the INF file:
> >>
> >> %sx26.DeviceDesc%=sxdrv.Device,PCI\VEN_1131&DEV_7146&SUBSYS_02726000
> >>
> >> And it looks like the correct device because this the strings section
> >> contains:
> >>
> >> sx26.DeviceDesc=   "Sensoray Model 626 Analog/Digital I/O"
> >>
> >> Interpreting the above information gives us:
> >>
> >> PCI Vendor ID = 0x1131
> >> PCI Device ID = 0x7146
> >> PCI Subvendor ID = 0x6000
> >> PCI Subdevice ID = 0x0272 (626)
> >>
> >> The Linux SDK for this board
> >> (<http://www.sensoray.com/downloads/s626-1.0.1.tar.gz> has the same info
> >> in the s626core.h file.
> >
> > Ok thanks. So lets limit s626 by its subvendor:subdevice id, patch follows:
> >
> > From 6f4d2430959a378ab754c5dbd3903fdcf33abe36 Mon Sep 17 00:00:00 2001
> > From: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
> > Date: Wed, 17 Jun 2009 13:31:15 -0300
> > Subject: [PATCH] Staging: comedi: use subvendor:subdevice ids for its pci id device table
> >
> > 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 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.
> >
> > Reference: http://lkml.org/lkml/2009/6/16/552
> >
> > Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
> > ---
> >  drivers/staging/comedi/drivers/s626.c |    8 ++++++--
> >  1 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c
> > index 30dec9d..b4b7713 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
> >
> > +/*
> > + * Note: always specify subvendor:subdevice ids in table below, to avoid
> > + * clash with Philips SAA7146 media/dvb based cards which have same
> > + * vendor:device ids as S626
> > + */
> >  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}
> >  };
> >
> > --
> > 1.6.3.2
> 
> There are also a couple of calls to pci_get_device() that need changing
> to pci_get_subsys() in the s626_attach() function (so it also might be
> worth #define'ing the subsys numbers to avoid repeating these magic
> numbers).

Indeed, using pci_get_device() would still cause problems if s626 is manually
modprobed. Check new patch below, it now uses pci_get_subsys and ensures we use
subvendor or subdevice. I didn't removed the define, may be there could be more
s626 cards available in future with same id? If not, it could be cleaned up
and may be logic at s626_attach() simplified.

>From 6c83b8665da770c2d60fe692819107e3d0bc329d Mon Sep 17 00:00:00 2001
From: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
Date: Wed, 17 Jun 2009 19:56:33 -0300
Subject: [PATCH v2] Staging: comedi (s626): always use subvendor:subdevice ids in pci id table

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 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, 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;
-- 
1.6.3.2

> 
> --
> -=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>             )=-
> -=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587              )=-
> 
> 
> 

--
[]'s
Herton

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

[Frank Mori Hess]

[-- Attachment #1: Type: text/plain, Size: 2802 bytes --]

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.


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

[Herton Ronaldo Krzesinski]

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

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

[Herton Ronaldo Krzesinski]

Em Qua 17 Jun 2009, às 21:05:56, Herton Ronaldo Krzesinski escreveu:
> 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.

Erm... forget that, you mean the "/* no bus/slot specified */" I removed. That
previous logic to me didn't made much sense, why you must always request a
particular bus/slot?

--
[]'s
Herton

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

[Herton Ronaldo Krzesinski]

Em Qua 17 Jun 2009, às 21:21:51, Frank Mori Hess escreveu:
> On Wednesday 17 June 2009, you wrote:
> > >
> > > 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.
> 
> Your patch breaks configuration of the board unless the bus and slot are 
> explicitly specified.  Just make a minimal patch that replaces 
> pci_get_device with pci_get_subsys and fixes the problem that was 
> reported.

Hmm that's not what the patch does, it doesn't break configuration, keeps
the same logic as before (I was wrong in my last email replying to myself),
check it, if it->options[0] and it->options[1] isn't specified, the pdev is
valid so the for loop exits (see !pdev check).

> 
> > 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.
> 
> Agreed, it's currently in a limbo between trying to support auto probing of 
> devices and supporting comedi's old way of configuring hardware.  But I 
> don't anticipate you are going to refactor the comedi core and all the 
> drivers, so just make a minimal patch that doesn't change any logic it 
> doesn't need to.
> 
> 

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

[Frank Mori Hess]

[-- Attachment #1: Type: text/plain, Size: 1002 bytes --]

On Wednesday 17 June 2009, Herton Ronaldo Krzesinski wrote:
> >
> > Your patch breaks configuration of the board unless the bus and slot
> > are explicitly specified.  Just make a minimal patch that replaces
> > pci_get_device with pci_get_subsys and fixes the problem that was
> > reported.
>
> Hmm that's not what the patch does, it doesn't break configuration,
> keeps the same logic as before (I was wrong in my last email replying to
> myself), check it, if it->options[0] and it->options[1] isn't specified,
> the pdev is valid so the for loop exits (see !pdev check).

Your right.  However, it also turns a loop over pci devices into a loop 
over pci ids, which appears to break the case of multiple s626 boards 
where the bus/slot of the second s626 board is specified.  If you're not 
willing to provide a minimal patch that just fixes the reported problem, 
just say so.  It would have been less effort for me to do it myself than 
analyze what your changes are breaking.


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

[Herton Ronaldo Krzesinski]

Em Quinta-feira 18 Junho 2009, às 04:28:04, Frank Mori Hess escreveu:
> On Wednesday 17 June 2009, Herton Ronaldo Krzesinski wrote:
> > >
> > > Your patch breaks configuration of the board unless the bus and slot
> > > are explicitly specified.  Just make a minimal patch that replaces
> > > pci_get_device with pci_get_subsys and fixes the problem that was
> > > reported.
> >
> > Hmm that's not what the patch does, it doesn't break configuration,
> > keeps the same logic as before (I was wrong in my last email replying to
> > myself), check it, if it->options[0] and it->options[1] isn't specified,
> > the pdev is valid so the for loop exits (see !pdev check).
> 
> Your right.  However, it also turns a loop over pci devices into a loop 
> over pci ids, which appears to break the case of multiple s626 boards 
> where the bus/slot of the second s626 board is specified.  If you're not 
> willing to provide a minimal patch that just fixes the reported problem, 
> just say so.  It would have been less effort for me to do it myself than 
> analyze what your changes are breaking.

That's part of reviewing process, I just wanted to enhance it in case more pci
ids are added in the future along with the switch to pci_get_subsys, I don't
know why you act like that, you don't want the code to be enhanced? Other
comedi driver loop over ids, for example gsc_hpdi

And indeed what you say it's true, there is a bug in the patch now that you
checked it out properly, it has a problem with this multiple s626 case, fixed
that now. Pointing out the problem instead just saying it's broken helps :)

I also removed the obligation to add sub ids and re-factored the patch, that
was too much and not good, just a simpler loop over pci id array is used now,
and a comment was added in pci id list telling people that they should add
subvendor:subdevice ids for boards with vendor:device == 0x1131:0x7146,
in case new boards with this id appear.

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@mandriva.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;
 


-- 
[]'s
Herton

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

[Ian Abbott]

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@mandriva.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.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

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

[Manu Abraham]

Hi Herton,

On Thu, Jun 18, 2009 at 9:23 PM, Herton Ronaldo
Krzesinski<herton@mandriva.com.br> wrote:
> Em Quinta-feira 18 Junho 2009, às 04:28:04, Frank Mori Hess escreveu:
>> On Wednesday 17 June 2009, Herton Ronaldo Krzesinski wrote:
>> > >
>> > > Your patch breaks configuration of the board unless the bus and slot
>> > > are explicitly specified.  Just make a minimal patch that replaces
>> > > pci_get_device with pci_get_subsys and fixes the problem that was
>> > > reported.
>> >
>> > Hmm that's not what the patch does, it doesn't break configuration,
>> > keeps the same logic as before (I was wrong in my last email replying to
>> > myself), check it, if it->options[0] and it->options[1] isn't specified,
>> > the pdev is valid so the for loop exits (see !pdev check).
>>
>> Your right.  However, it also turns a loop over pci devices into a loop
>> over pci ids, which appears to break the case of multiple s626 boards
>> where the bus/slot of the second s626 board is specified.  If you're not
>> willing to provide a minimal patch that just fixes the reported problem,
>> just say so.  It would have been less effort for me to do it myself than
>> analyze what your changes are breaking.
>
> That's part of reviewing process, I just wanted to enhance it in case more pci
> ids are added in the future along with the switch to pci_get_subsys, I don't
> know why you act like that, you don't want the code to be enhanced? Other
> comedi driver loop over ids, for example gsc_hpdi
>
> And indeed what you say it's true, there is a bug in the patch now that you
> checked it out properly, it has a problem with this multiple s626 case, fixed
> that now. Pointing out the problem instead just saying it's broken helps :)
>
> I also removed the obligation to add sub ids and re-factored the patch, that
> was too much and not good, just a simpler loop over pci id array is used now,
> and a comment was added in pci id list telling people that they should add
> subvendor:subdevice ids for boards with vendor:device == 0x1131:0x7146,
> in case new boards with this id appear.


Since there is valid subsystem information: why do you have to loop
over the ID's
manually in the driver ?

Can't the ID's be set into the PCI ID table and a normal pci_probe be
used instead ?
Not that i am familiar with the comedi stuff though ...

We use the probe method directly (rather than pci_get_ ) for most of
the multimedia drivers. As an example this is what i do:
http://jusst.de/hg/saa716x/file/59dd985d4473/linux/drivers/media/dvb/saa716x/saa716x_budget.c

Maybe it helps you in some way.

Regards,
Manu

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

[Herton Ronaldo Krzesinski]

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@mandriva.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@mandriva.com.br>

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

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

[Ian Abbott]

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@mandriva.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@mandriva.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@mev.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@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-