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

[Frank Mori Hess]

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: signature.asc
Description: This is a digitally signed message part.