* [PATCH 01/18] char: moxa: call disable_pci_device() if pci_probe() failed @ 2010-08-06 19:49 Kulikov Vasiliy 2010-08-07 7:22 ` [PATCH 01/18] char: moxa: call disable_pci_device() if Dan Carpenter 0 siblings, 1 reply; 6+ messages in thread From: Kulikov Vasiliy @ 2010-08-06 19:49 UTC (permalink / raw) To: kernel-janitors Cc: Greg Kroah-Hartman, Alan Cox, Andrew Morton, Rakib Mullick, Ben Hutchings, linux-kernel 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 01/18] char: moxa: call disable_pci_device() if 2010-08-06 19:49 [PATCH 01/18] char: moxa: call disable_pci_device() if pci_probe() failed Kulikov Vasiliy @ 2010-08-07 7:22 ` Dan Carpenter 2010-08-07 8:55 ` Vasiliy Kulikov 0 siblings, 1 reply; 6+ messages in thread From: Dan Carpenter @ 2010-08-07 7:22 UTC (permalink / raw) To: Kulikov Vasiliy Cc: kernel-janitors, Greg Kroah-Hartman, Alan Cox, Andrew Morton, Rakib Mullick, Ben Hutchings, linux-kernel On Fri, Aug 06, 2010 at 11:49:10PM +0400, Kulikov Vasiliy wrote: > 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. > I think we should disable it actually. The comments on pci_enable_device() and pci_disable_device() say that only the first and last callers actually enable and disable it. The others just increment or decrement a counter. regards, dan carpenter > 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 > > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 01/18] char: moxa: call disable_pci_device() if 2010-08-07 7:22 ` [PATCH 01/18] char: moxa: call disable_pci_device() if Dan Carpenter @ 2010-08-07 8:55 ` Vasiliy Kulikov 2010-08-07 9:58 ` Dan Carpenter 0 siblings, 1 reply; 6+ messages in thread From: Vasiliy Kulikov @ 2010-08-07 8:55 UTC (permalink / raw) To: Dan Carpenter, kernel-janitors, Greg Kroah-Hartman, Alan Cox, Andrew Morton, Rakib Mullick, Ben Hutchings, linux-kernel On Sat, Aug 07, 2010 at 09:22 +0200, Dan Carpenter wrote: > On Fri, Aug 06, 2010 at 11:49:10PM +0400, Kulikov Vasiliy wrote: > > 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. > > > > I think we should disable it actually. The comments on > pci_enable_device() and pci_disable_device() say that only the first and > last callers actually enable and disable it. The others just increment > or decrement a counter. 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 aboud this problem, so I think it's better to play safe. > > regards, > dan carpenter > > > 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 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 01/18] char: moxa: call disable_pci_device() if 2010-08-07 8:55 ` Vasiliy Kulikov @ 2010-08-07 9:58 ` Dan Carpenter 2010-08-07 18:02 ` Vasiliy Kulikov 0 siblings, 1 reply; 6+ messages in thread From: Dan Carpenter @ 2010-08-07 9:58 UTC (permalink / raw) To: Vasiliy Kulikov Cc: kernel-janitors, Greg Kroah-Hartman, Alan Cox, Andrew Morton, Rakib Mullick, Ben Hutchings, linux-kernel On Sat, Aug 07, 2010 at 12:55:12PM +0400, Vasiliy Kulikov wrote: > On Sat, Aug 07, 2010 at 09:22 +0200, Dan Carpenter wrote: > > On Fri, Aug 06, 2010 at 11:49:10PM +0400, Kulikov Vasiliy wrote: > > > 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. > > > > > > > I think we should disable it actually. The comments on > > pci_enable_device() and pci_disable_device() say that only the first and > > last callers actually enable and disable it. The others just increment > > or decrement a counter. > > 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 > aboud this problem, so I think it's better to play safe. > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 01/18] char: moxa: call disable_pci_device() if 2010-08-07 9:58 ` Dan Carpenter @ 2010-08-07 18:02 ` Vasiliy Kulikov 2010-08-07 19:08 ` Dan Carpenter 0 siblings, 1 reply; 6+ messages in thread From: Vasiliy Kulikov @ 2010-08-07 18:02 UTC (permalink / raw) To: Dan Carpenter, kernel-janitors, Greg Kroah-Hartman, Alan Cox, Andrew Morton, Rakib Mullick, Ben Hutchings, linux-kernel On Sat, Aug 07, 2010 at 11:58 +0200, Dan Carpenter wrote: > On Sat, Aug 07, 2010 at 12:55:12PM +0400, Vasiliy Kulikov wrote: > > On Sat, Aug 07, 2010 at 09:22 +0200, Dan Carpenter wrote: > > > On Fri, Aug 06, 2010 at 11:49:10PM +0400, Kulikov Vasiliy wrote: > > > > 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. > > > > > > > > > > I think we should disable it actually. The comments on > > > pci_enable_device() and pci_disable_device() say that only the first and > > > last callers actually enable and disable it. The others just increment > > > or decrement a counter. > > > > 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 > > aboud this problem, so I think it's better to play safe. > > > > That's ancient. That's a couple months before the start of git. Yes, but I can't find more recent answer to this question. > > 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. 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 :-) If you know better solution please tell me :-) > > regards, > dan carpenter ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 01/18] char: moxa: call disable_pci_device() if 2010-08-07 18:02 ` Vasiliy Kulikov @ 2010-08-07 19:08 ` Dan Carpenter 0 siblings, 0 replies; 6+ messages in thread From: Dan Carpenter @ 2010-08-07 19:08 UTC (permalink / raw) To: Vasiliy Kulikov Cc: kernel-janitors, Greg Kroah-Hartman, Alan Cox, Andrew Morton, Rakib Mullick, Ben Hutchings, linux-kernel On Sat, Aug 07, 2010 at 10:02:52PM +0400, Vasiliy Kulikov wrote: > > 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 :-) > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-08-07 19:08 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-08-06 19:49 [PATCH 01/18] char: moxa: call disable_pci_device() if pci_probe() failed Kulikov Vasiliy 2010-08-07 7:22 ` [PATCH 01/18] char: moxa: call disable_pci_device() if Dan Carpenter 2010-08-07 8:55 ` Vasiliy Kulikov 2010-08-07 9:58 ` Dan Carpenter 2010-08-07 18:02 ` Vasiliy Kulikov 2010-08-07 19:08 ` Dan Carpenter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).