Re: [PATCH 01/18] char: moxa: call disable_pci_device() if pci_probe() failed

Previous thread: Re: Attempted summary of suspend-blockers LKML thread by Alan Stern on Friday, August 6, 2010 - 12:44 pm. (14 messages)

Next thread: [PATCH 03/18] char: stallion: call disable_pci_device() if pci_probe() failed by Kulikov Vasiliy on Friday, August 6, 2010 - 12:49 pm. (1 message)
From: Kulikov Vasiliy
Date: Friday, August 6, 2010 - 12:49 pm

Driver should call disable_pci_device() if it returns from pci_probe()
with error. Also it must not be called if pci_request_region() fails as
it means that somebody uses device resources and rules the device.

Move pci_enable_device() after checking moxa_boards[] to remove senseless
pci_enable_device()/pci_disable_device().

Signed-off-by: Kulikov Vasiliy <segooon@gmail.com>
---
 drivers/char/moxa.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/char/moxa.c b/drivers/char/moxa.c
index 107b0bd..0ea5aeb 100644
--- a/drivers/char/moxa.c
+++ b/drivers/char/moxa.c
@@ -939,11 +939,6 @@ static int __devinit moxa_pci_probe(struct pci_dev *pdev,
 	int board_type = ent->driver_data;
 	int retval;
 
-	retval = pci_enable_device(pdev);
-	if (retval) {
-		dev_err(&pdev->dev, "can't enable pci device\n");
-		goto err;
-	}
 
 	for (i = 0; i < MAX_BOARDS; i++)
 		if (moxa_boards[i].basemem == NULL)
@@ -956,6 +951,12 @@ static int __devinit moxa_pci_probe(struct pci_dev *pdev,
 		goto err;
 	}
 
+	retval = pci_enable_device(pdev);
+	if (retval) {
+		dev_err(&pdev->dev, "can't enable pci device\n");
+		goto err;
+	}
+
 	board = &moxa_boards[i];
 
 	retval = pci_request_region(pdev, 2, "moxa-base");
@@ -1001,6 +1002,7 @@ err_base:
 	board->basemem = NULL;
 err_reg:
 	pci_release_region(pdev, 2);
+	pci_disable_device(pdev);
 err:
 	return retval;
 }
-- 
1.7.0.4

--

From: Vasiliy Kulikov
Date: Saturday, August 7, 2010 - 1:55 am

See this thread: http://lkml.org/lkml/2005/2/13/82

Specifically this mail:

    Date    Mon, 14 Feb 2005 14:51:26 -0500
    From    Jeff Garzik <>

    ...
    You also need to consider situations such as out-of-tree drivers 
    for the same hardware (might not use PCI API), and situations where you 
    have peer devices discovered and used (PCI API doesn't have "hey, <this> 
    device is associated with <current driver>, too" capability).
    ...

Searching for 'pci_disable_device() inurl:lkml' doesn't give me newer info
--

From: Dan Carpenter
Date: Saturday, August 7, 2010 - 2:58 am

That's ancient.  That's a couple months before the start of git.

git show v2.6.12:drivers/pci/pci.c 

In those days pci_enable/disable_device() were not nestable.  These days
we can just unwind normally so it's a big improvement.

regards,
dan carpenter
--

From: Vasiliy Kulikov
Date: Saturday, August 7, 2010 - 11:02 am

The thing is not in that pci_enable_device() is nestable, but in that
some buggy non-mainline drivers may not use PCI api, but use
request_*(). In this case PCI bus doesn't see this driver and tries to
call mainline driver probe(). It sees that resources are already
grabbed. The only reason can be another driver that rules this device
(or our kernel is crazy). As we are robust driver we don't want to break
this buggy third party driver :-)

--

From: Dan Carpenter
Date: Saturday, August 7, 2010 - 12:08 pm

What Jeff Garzik was saying in those days if two people called
pci_enable_device() it wasn't a problem, but if one person then called
pci_disable_device() the other person who still wanted it enabled was
screwed.  That model was "*first* person out of the building lock the
door."

That was half a decade ago though.  The fix that Jeff proposed wasn't
really a good one because there is no way people would get that right.
None of that applies any more.  These days we just call enable when we 
want it enabled and disable when we are done.  All the dependencies are
handled for us.

In fact, if we try to use the old hacks and work arounds, that will cause
bugs in the new code...

regards,
dan carpenter

--

Previous thread: Re: Attempted summary of suspend-blockers LKML thread by Alan Stern on Friday, August 6, 2010 - 12:44 pm. (14 messages)

Next thread: [PATCH 03/18] char: stallion: call disable_pci_device() if pci_probe() failed by Kulikov Vasiliy on Friday, August 6, 2010 - 12:49 pm. (1 message)