staging: comedi: adv_pci1723: move comedi_pci_enable() into the attach

Use pci_is_enabled() in the "find pci device" function to determine if the found pci device is not in use and move the comedi_pci_enable() call into the attach.

Signed-off-by H Hartley Sweeten <[email protected]>

Cc Ian Abbott <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

---
  drivers/staging/comedi/drivers/adv_pci1723.c | 10 +++++-----
  1 file changed, 5 insertions(+), 5 deletions(-)

drivers/staging/comedi/drivers/adv_pci1723.c +0 -0

index f561a2a..e971fa6 100644
@@ -302,11 +302,7 @@ static struct pci_dev *pci1723_find_pci_dev(struct comedi_device *dev,
  		}
  		if (pcidev->vendor != PCI_VENDOR_ID_ADVANTECH)

drivers/staging/comedi/drivers/adv_pci1723.c +4 -0

@@ -335,6 +331,10 @@ static int pci1723_attach(struct comedi_device *dev,
  	if (!devpriv->pcidev)
  		return -EIO;
 
+	ret = comedi_pci_enable(devpriv->pcidev, "adv_pci1723");
+	if (ret)
+		return ret;
+
  	dev->iobase = pci_resource_start(devpriv->pcidev, 2);
 
  	dev->board_name = this_board->name;

Quoted 23 lines · H Hartley Sweeten

Use pci_is_enabled() in the "find pci device" function to determine if the found pci device is not in use and move the comedi_pci_enable() call into the attach.

Signed-off-by H Hartley Sweeten <[email protected]>

Cc Ian Abbott <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

---
  drivers/staging/comedi/drivers/adv_pci1723.c | 10 +++++-----
  1 file changed, 5 insertions(+), 5 deletions(-)

drivers/staging/comedi/drivers/adv_pci1723.c +0 -4

index f561a2a..e971fa6 100644
@@ -302,11 +302,7 @@ static struct pci_dev *pci1723_find_pci_dev(struct comedi_device *dev,
  		}
  		if (pcidev->vendor != PCI_VENDOR_ID_ADVANTECH)
  			continue;
-		/*
-		 * Look for device that isn't in use.
-		 * Enable PCI device and request regions.
-		 */

drivers/staging/comedi/drivers/adv_pci1723.c static struct pci_dev *pci1723_find_pci_dev(struct comedi_device *dev, · line 308

Quoted 5 lines

drivers/staging/comedi/drivers/adv_pci1723.c +1 -1

-		if (comedi_pci_enable(pcidev, "adv_pci1723"))
+		if (pci_is_enabled(pcidev))
  			continue;
  		return pcidev;
  	}

Just because the device is enabled doesn't mean that it is in use, so this change could skip over a perfectly good unused device.

If you want to move the comedi_pci_enable() call, this is a change in behaviour for this particular driver, but is consistent with most of the other Comedi PCI drivers (and the bus/slot options can be used to select a particular device). This is probably a good thing, but you should take out the pci_is_enabled test.

Quoted 24 lines · Ian Abbott

Quoted 23 lines · H Hartley Sweeten

Use pci_is_enabled() in the "find pci device" function to determine if the found pci device is not in use and move the comedi_pci_enable() call into the attach.

Signed-off-by H Hartley Sweeten <[email protected]>

Cc Ian Abbott <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

---
  drivers/staging/comedi/drivers/adv_pci1723.c | 10 +++++-----
  1 file changed, 5 insertions(+), 5 deletions(-)

drivers/staging/comedi/drivers/adv_pci1723.c +0 -4

index f561a2a..e971fa6 100644
@@ -302,11 +302,7 @@ static struct pci_dev *pci1723_find_pci_dev(struct comedi_device *dev,
  		}
  		if (pcidev->vendor != PCI_VENDOR_ID_ADVANTECH)
  			continue;
-		/*
-		 * Look for device that isn't in use.
-		 * Enable PCI device and request regions.
-		 */

Quoted 8 lines

drivers/staging/comedi/drivers/adv_pci1723.c static struct pci_dev *pci1723_find_pci_dev(struct comedi_device *dev, · line 308

Quoted 5 lines

drivers/staging/comedi/drivers/adv_pci1723.c +1 -1

-		if (comedi_pci_enable(pcidev, "adv_pci1723"))
+		if (pci_is_enabled(pcidev))
  			continue;
  		return pcidev;
  	}

Just because the device is enabled doesn't mean that it is in use, so this change could skip over a perfectly good unused device.

Are you sure? According to Documentation/PCI/pci.txt, the first thing a driver "should" do when taking ownership of a PCI device is enable the device. The last thing it "should" do when being unloaded is disable the device.

It would seem that checking pci_is_enabled() would let us know that the pci_dev in question is being used.

Quoted 5 lines

If you want to move the comedi_pci_enable() call, this is a change in behaviour for this particular driver, but is consistent with most of the other Comedi PCI drivers (and the bus/slot options can be used to select a particular device). This is probably a good thing, but you should take out the pci_is_enabled test.

I was curious about this.

In the original driver, doing an 'attach' with bus/slot both = 0 would result in the pci bus being walked to find the first device that could be enabled (i.e. a "free" device) and using that device. This allows doing the 'attach' with more than one card installed and being able to attach to each one by simply:

comedi_config /dev/comedi0 adv_pci1723
comedi_config /dev/comedi1 adv_pci1723
etc.

The "normal" operation for the comedi pci drivers appears to require the bus/slot information when multiple devices are used. Or implement the 'attach_pci' callback in the comedi_driver and allow the core to simply pass the pci_dev directly to the driver.

What would you prefer?

I was planning on making a comedi_find_pci_dev() function that the drivers could call with a "match" callback. This would allow a common function for most of the boilerplate code and just require the drivers to check the the match against the boardinfo dev/id, etc. as required. Something like:

struct pci_dev *comedi_find_pci_dev(struct comedi_device *dev,
struct comedi_devconfig *it,
const void *(*match)(struct comedi_device *,
struct pci_dev *))

{
	struct pci_dev *pcidev = NULL;
	int bus = it->options[0];
	int slot = it->options[1];

	for_each_pci_dev(pcidev) {
		/* pci_is_enabled() test? */
		if ((bus && bus != pcidev->bus->number) ||
		    (slot && slot != PCI_SLOT(pcidev->devfn)))
			continue;
		dev->board_ptr = match(dev, pcidev);
		if (dev->board_ptr) {
			comedi_set_hw_dev(dev, &pcidev->dev);
			return pcidev;
		}
	}
	return NULL;
}

The "match" function for a driver would then just be something like:

const void *match_pci(struct comedi_device *dev, struct pci_dev *pcidev)

{
	const struct boardinfo *board;
	int i;

	for (i = 0; i < ARRAY_SIZE(boardinfo); i++) {
		board = &boardinfo[i];
		if (pcidev->vendor != board->ven_id ||
		    pcidev->device != board->dev_id)
			continue;
		return board;
	}
	return NULL;
}

This would require adding a dummy boardinfo to some of the drivers but I think it's cleaner.

Comments?

Quoted 46 lines · H Hartley Sweeten

I was planning on making a comedi_find_pci_dev() function that the drivers could call with a "match" callback. This would allow a common function for most of the boilerplate code and just require the drivers to check the the match against the boardinfo dev/id, etc. as required. Something like:

struct pci_dev *comedi_find_pci_dev(struct comedi_device *dev,
struct comedi_devconfig *it,
const void *(*match)(struct comedi_device *,
struct pci_dev *))

{
	struct pci_dev *pcidev = NULL;
	int bus = it->options[0];
	int slot = it->options[1];

	for_each_pci_dev(pcidev) {
		/* pci_is_enabled() test? */
		if ((bus && bus != pcidev->bus->number) ||
		    (slot && slot != PCI_SLOT(pcidev->devfn)))
			continue;
		dev->board_ptr = match(dev, pcidev);
		if (dev->board_ptr) {
			comedi_set_hw_dev(dev, &pcidev->dev);
			return pcidev;
		}
	}
	return NULL;
}

The "match" function for a driver would then just be something like:

const void *match_pci(struct comedi_device *dev, struct pci_dev *pcidev)

{
	const struct boardinfo *board;
	int i;

	for (i = 0; i < ARRAY_SIZE(boardinfo); i++) {
		board = &boardinfo[i];
		if (pcidev->vendor != board->ven_id ||
		    pcidev->device != board->dev_id)
			continue;
		return board;
	}
	return NULL;
}

Quoted 4 lines

This would require adding a dummy boardinfo to some of the drivers but I think it's cleaner.

Comments?

Ick. What ever happened to converting these drivers to use the PCI api correctly and not to search the PCI space for the device, but have the PCI core call them if the device is found?

It looks like most of these drivers have already been converted to that style, so these checks for "do we find a device" can all be removed entirely now, right? There's no way the functions would be called if the device wasn't found in the first place.

Or am I missing something here?

Quoted 56 lines · gregkh

Quoted 46 lines · H Hartley Sweeten

I was planning on making a comedi_find_pci_dev() function that the drivers could call with a "match" callback. This would allow a common function for most of the boilerplate code and just require the drivers to check the the match against the boardinfo dev/id, etc. as required. Something like:

struct pci_dev *comedi_find_pci_dev(struct comedi_device *dev,
struct comedi_devconfig *it,
const void *(*match)(struct comedi_device *,
struct pci_dev *))

{
	struct pci_dev *pcidev = NULL;
	int bus = it->options[0];
	int slot = it->options[1];

	for_each_pci_dev(pcidev) {
		/* pci_is_enabled() test? */
		if ((bus && bus != pcidev->bus->number) ||
		    (slot && slot != PCI_SLOT(pcidev->devfn)))
			continue;
		dev->board_ptr = match(dev, pcidev);
		if (dev->board_ptr) {
			comedi_set_hw_dev(dev, &pcidev->dev);
			return pcidev;
		}
	}
	return NULL;
}

The "match" function for a driver would then just be something like:

const void *match_pci(struct comedi_device *dev, struct pci_dev *pcidev)

{
	const struct boardinfo *board;
	int i;

	for (i = 0; i < ARRAY_SIZE(boardinfo); i++) {
		board = &boardinfo[i];
		if (pcidev->vendor != board->ven_id ||
		    pcidev->device != board->dev_id)
			continue;
		return board;
	}
	return NULL;
}

Quoted 4 lines

This would require adding a dummy boardinfo to some of the drivers but I think it's cleaner.

Comments?

Ick. What ever happened to converting these drivers to use the PCI api correctly and not to search the PCI space for the device, but have the PCI core call them if the device is found?

Quoted 6 lines

It looks like most of these drivers have already been converted to that style, so these checks for "do we find a device" can all be removed entirely now, right? There's no way the functions would be called if the device wasn't found in the first place.

Or am I missing something here?

If the comedi pci drivers have the "attach_pci" callback defined, the PCI api does correctly probe the driver. The comedi_pci_auto_config() then passes the pci_dev directly to the driver and the search of the PCI space for the device is not required.

If the "attach_pci" callback is not defined, the comedi_pci_auto_config() then falls back to passing the bus/slot information to the driver and uses the "attach" callback. In this case we could probably fast-track the search by using pci_get_slot() instead of doing the for_each_pci_dev() loop.

I think the problem is the COMEDI_DEVCONFIG ioctl. The userspace utility 'comedi_config' uses that ioctl to link a device node to a comedi driver. That utility allows passing the bus/slot information but it's not required. This means we have to search the PCI space for the pci_dev that matches the driver.

Quoted 10 lines · H Hartley Sweeten

If the comedi pci drivers have the "attach_pci" callback defined, the PCI api does correctly probe the driver. The comedi_pci_auto_config() then passes the pci_dev directly to the driver and the search of the PCI space for the device is not required.

If the "attach_pci" callback is not defined, the comedi_pci_auto_config() then falls back to passing the bus/slot information to the driver and uses the "attach" callback. In this case we could probably fast-track the search by using pci_get_slot() instead of doing the for_each_pci_dev() loop.

Quoted 5 lines

I think the problem is the COMEDI_DEVCONFIG ioctl. The userspace utility 'comedi_config' uses that ioctl to link a device node to a comedi driver. That utility allows passing the bus/slot information but it's not required. This means we have to search the PCI space for the pci_dev that matches the driver.

The ioctl shouldn't be needed anymore for PCI or USB devices, as the kernel handles the matching of the driver to the device. Even if it didn't, there are other more "standard" ways that you can bind devices to drivers (through sysfs.)

So, I'd really recommend ripping all of this logic out for PCI drivers as odds are, it's not used, and probably doesn't really work.

For old ISA cards, it makes sense, but the odds that anyone uses any ISA devices is pretty slim.

Quoted 17 lines · gregkh

Quoted 10 lines · H Hartley Sweeten

If the comedi pci drivers have the "attach_pci" callback defined, the PCI api does correctly probe the driver. The comedi_pci_auto_config() then passes the pci_dev directly to the driver and the search of the PCI space for the device is not required.

If the "attach_pci" callback is not defined, the comedi_pci_auto_config() then falls back to passing the bus/slot information to the driver and uses the "attach" callback. In this case we could probably fast-track the search by using pci_get_slot() instead of doing the for_each_pci_dev() loop.

Quoted 5 lines

I think the problem is the COMEDI_DEVCONFIG ioctl. The userspace utility 'comedi_config' uses that ioctl to link a device node to a comedi driver. That utility allows passing the bus/slot information but it's not required. This means we have to search the PCI space for the pci_dev that matches the driver.

Quoted 4 lines

The ioctl shouldn't be needed anymore for PCI or USB devices, as the kernel handles the matching of the driver to the device. Even if it didn't, there are other more "standard" ways that you can bind devices to drivers (through sysfs.)

I think it's still needed for some of the devices that require external firmware. The comedi_config utility allows the user to remove the driver binding and then reattach to it while passing the firmware blob into the driver.

Not saying any of this is valid... And yes, there probably is a more "standard" way to do this. I just need a hint of what that is... ;-)

Quoted 10 lines · H Hartley Sweeten

If the comedi pci drivers have the "attach_pci" callback defined, the PCI api does correctly probe the driver. The comedi_pci_auto_config() then passes the pci_dev directly to the driver and the search of the PCI space for the device is not required.

If the "attach_pci" callback is not defined, the comedi_pci_auto_config() then falls back to passing the bus/slot information to the driver and uses the "attach" callback. In this case we could probably fast-track the search by using pci_get_slot() instead of doing the for_each_pci_dev() loop.

Quoted 5 lines

I think the problem is the COMEDI_DEVCONFIG ioctl. The userspace utility 'comedi_config' uses that ioctl to link a device node to a comedi driver. That utility allows passing the bus/slot information but it's not required. This means we have to search the PCI space for the pci_dev that matches the driver.

The ioctl shouldn't be needed anymore for PCI or USB devices, as the kernel handles the matching of the driver to the device. Even if it didn't, there are other more "standard" ways that you can bind devices to drivers (through sysfs.)

So, I'd really recommend ripping all of this logic out for PCI drivers as odds are, it's not used, and probably doesn't really work.

I think there are a couple pci drivers that would still need the old "attach" in order to load firmware using comedi_config. We can address those later and possibly figure out a more "standard" way to load the firmware.