* Re: [ide] clean up error path in do_ide_setup_pci_device()
[not found] <200412310343.iBV3hqvd015595@hera.kernel.org>
@ 2005-01-03 18:42 ` Alan Cox
2005-01-03 22:22 ` Bartlomiej Zolnierkiewicz
0 siblings, 1 reply; 6+ messages in thread
From: Alan Cox @ 2005-01-03 18:42 UTC (permalink / raw)
To: Linux Kernel Mailing List, torvalds, Bartlomiej Zolnierkiewicz
This changeset will break support for several systems because the PCI
IDE controller uses some BARs on a multifunction PCI northbridge. The
old IDE code was extremely careful *NOT* to play pci_disable_device
games because of this.
Nothing in the IDE specification requires the PCI IDE controller be the
only use of that PCI function. The damage is probably minimal as it
deals with error paths but this change should be reverted (and will be
for -ac).
On Iau, 2004-12-30 at 19:08, Linux Kernel Mailing List wrote:
> ChangeSet 1.2034.118.8, 2004/12/30 20:08:53+01:00, bzolnier@trik.(none)
>
> [ide] clean up error path in do_ide_setup_pci_device()
>
> ide_setup_pci_controller() puts the device in a PCI enabled state.
> The patch adds a small helper to balance it when things go wrong.
>
> Actually the helper does not *exactly* balance the setup: if it can
> not do a better job, ide_setup_pci_controller() may only enable some
> BARS whereas the new counterpart will try to disable everything.
>
> Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
>
>
>
> setup-pci.c | 15 +++++++++++++--
> 1 files changed, 13 insertions(+), 2 deletions(-)
>
>
> diff -Nru a/drivers/ide/setup-pci.c b/drivers/ide/setup-pci.c
> --- a/drivers/ide/setup-pci.c 2004-12-30 19:44:05 -08:00
> +++ b/drivers/ide/setup-pci.c 2004-12-30 19:44:05 -08:00
> @@ -542,6 +542,13 @@
> return 0;
> }
>
> +static void ide_release_pci_controller(struct pci_dev *dev, ide_pci_device_t *d,
> + int noisy)
> +{
> + /* Balance ide_pci_enable() */
> + pci_disable_device(dev);
> +}
> +
> /**
> * ide_pci_setup_ports - configure ports/devices on PCI IDE
> * @dev: PCI device
> @@ -672,7 +679,7 @@
> */
> ret = d->init_chipset ? d->init_chipset(dev, d->name) : 0;
> if (ret < 0)
> - goto out;
> + goto err_release_pci_controller;
> pciirq = ret;
> } else if (tried_config) {
> if (noisy)
> @@ -687,7 +694,7 @@
> if (d->init_chipset) {
> ret = d->init_chipset(dev, d->name);
> if (ret < 0)
> - goto out;
> + goto err_release_pci_controller;
> }
> if (noisy)
> #ifdef __sparc__
> @@ -705,6 +712,10 @@
> ide_pci_setup_ports(dev, d, pciirq, index);
> out:
> return ret;
> +
> +err_release_pci_controller:
> + ide_release_pci_controller(dev, d, noisy);
> + goto out;
> }
>
> int ide_setup_pci_device(struct pci_dev *dev, ide_pci_device_t *d)
> -
> To unsubscribe from this list: send the line "unsubscribe bk-commits-head" 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: [ide] clean up error path in do_ide_setup_pci_device()
2005-01-03 22:22 ` Bartlomiej Zolnierkiewicz
@ 2005-01-03 21:44 ` Alan Cox
2005-01-04 0:14 ` Francois Romieu
0 siblings, 1 reply; 6+ messages in thread
From: Alan Cox @ 2005-01-03 21:44 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz; +Cc: Linux Kernel Mailing List, torvalds
On Llu, 2005-01-03 at 22:22, Bartlomiej Zolnierkiewicz wrote:
> > Nothing in the IDE specification requires the PCI IDE controller be the
> > only use of that PCI function. The damage is probably minimal as it
> > deals with error paths but this change should be reverted (and will be
> > for -ac).
>
> Different PCI functions should have different struct pci_dev instances
> so is this really a problem?
Different PCI functions are but nothing requires that the PCI function
that is the IDE controller is only the IDE controller. In some cases
other logic lives in the "spare" BAR register areas of the device.
One example where the weird design makes it obvious is the CS5520. Here
the 5520 bridge has the IDE in one BAR and all sorts of other logic
(including the xBUS virtual ISA environment) in the same PCI function.
On that chip a pci_disable_device on the IDE pci_dev turns off mundane
things like the timer chips keyboard and mouse 8).
Other vendors do equally evil things and providing the chip reports IDE
class and has the IDE BARs set up nobody else is any the wiser and
presumably gate count goes down.
Alan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [ide] clean up error path in do_ide_setup_pci_device()
2005-01-03 18:42 ` [ide] clean up error path in do_ide_setup_pci_device() Alan Cox
@ 2005-01-03 22:22 ` Bartlomiej Zolnierkiewicz
2005-01-03 21:44 ` Alan Cox
0 siblings, 1 reply; 6+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2005-01-03 22:22 UTC (permalink / raw)
To: Alan Cox; +Cc: Linux Kernel Mailing List, torvalds
On Mon, 03 Jan 2005 18:42:26 +0000, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> This changeset will break support for several systems because the PCI
> IDE controller uses some BARs on a multifunction PCI northbridge. The
> old IDE code was extremely careful *NOT* to play pci_disable_device
> games because of this.
Well, I supposed something like this but since pci_disable_device()
is called *only* in error paths I decided to take the risk. ;-)
> Nothing in the IDE specification requires the PCI IDE controller be the
> only use of that PCI function. The damage is probably minimal as it
> deals with error paths but this change should be reverted (and will be
> for -ac).
Different PCI functions should have different struct pci_dev instances
so is this really a problem?
> On Iau, 2004-12-30 at 19:08, Linux Kernel Mailing List wrote:
> > ChangeSet 1.2034.118.8, 2004/12/30 20:08:53+01:00, bzolnier@trik.(none)
> >
> > [ide] clean up error path in do_ide_setup_pci_device()
> >
> > ide_setup_pci_controller() puts the device in a PCI enabled state.
> > The patch adds a small helper to balance it when things go wrong.
> >
> > Actually the helper does not *exactly* balance the setup: if it can
> > not do a better job, ide_setup_pci_controller() may only enable some
> > BARS whereas the new counterpart will try to disable everything.
> >
> > Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> >
> >
> >
> > setup-pci.c | 15 +++++++++++++--
> > 1 files changed, 13 insertions(+), 2 deletions(-)
> >
> >
> > diff -Nru a/drivers/ide/setup-pci.c b/drivers/ide/setup-pci.c
> > --- a/drivers/ide/setup-pci.c 2004-12-30 19:44:05 -08:00
> > +++ b/drivers/ide/setup-pci.c 2004-12-30 19:44:05 -08:00
> > @@ -542,6 +542,13 @@
> > return 0;
> > }
> >
> > +static void ide_release_pci_controller(struct pci_dev *dev, ide_pci_device_t *d,
> > + int noisy)
> > +{
> > + /* Balance ide_pci_enable() */
> > + pci_disable_device(dev);
> > +}
> > +
> > /**
> > * ide_pci_setup_ports - configure ports/devices on PCI IDE
> > * @dev: PCI device
> > @@ -672,7 +679,7 @@
> > */
> > ret = d->init_chipset ? d->init_chipset(dev, d->name) : 0;
> > if (ret < 0)
> > - goto out;
> > + goto err_release_pci_controller;
> > pciirq = ret;
> > } else if (tried_config) {
> > if (noisy)
> > @@ -687,7 +694,7 @@
> > if (d->init_chipset) {
> > ret = d->init_chipset(dev, d->name);
> > if (ret < 0)
> > - goto out;
> > + goto err_release_pci_controller;
> > }
> > if (noisy)
> > #ifdef __sparc__
> > @@ -705,6 +712,10 @@
> > ide_pci_setup_ports(dev, d, pciirq, index);
> > out:
> > return ret;
> > +
> > +err_release_pci_controller:
> > + ide_release_pci_controller(dev, d, noisy);
> > + goto out;
> > }
> >
> > int ide_setup_pci_device(struct pci_dev *dev, ide_pci_device_t *d)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [ide] clean up error path in do_ide_setup_pci_device()
2005-01-03 21:44 ` Alan Cox
@ 2005-01-04 0:14 ` Francois Romieu
2005-01-04 22:02 ` Alan Cox
0 siblings, 1 reply; 6+ messages in thread
From: Francois Romieu @ 2005-01-04 0:14 UTC (permalink / raw)
To: Alan Cox; +Cc: Bartlomiej Zolnierkiewicz, Linux Kernel Mailing List, torvalds
Alan Cox <alan@lxorguk.ukuu.org.uk> :
[...]
> One example where the weird design makes it obvious is the CS5520. Here
> the 5520 bridge has the IDE in one BAR and all sorts of other logic
> (including the xBUS virtual ISA environment) in the same PCI function.
> On that chip a pci_disable_device on the IDE pci_dev turns off mundane
> things like the timer chips keyboard and mouse 8).
/me looks at the comments in drivers/ide/pci/cs5520.c
Is it worth the pain to remember if ide_setup_pci_device() did enable a
specific bar or not in order to balance it more accurately ?
--
Ueimor
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [ide] clean up error path in do_ide_setup_pci_device()
2005-01-04 0:14 ` Francois Romieu
@ 2005-01-04 22:02 ` Alan Cox
2005-01-07 2:20 ` Bartlomiej Zolnierkiewicz
0 siblings, 1 reply; 6+ messages in thread
From: Alan Cox @ 2005-01-04 22:02 UTC (permalink / raw)
To: Francois Romieu
Cc: Bartlomiej Zolnierkiewicz, Linux Kernel Mailing List, torvalds
On Maw, 2005-01-04 at 00:14, Francois Romieu wrote:
> /me looks at the comments in drivers/ide/pci/cs5520.c
>
> Is it worth the pain to remember if ide_setup_pci_device() did enable a
> specific bar or not in order to balance it more accurately ?
I'm not sure that tells you enough about the device to know how to
disable it or if it is safe to disable. It might be better to just check
if the device is currently enabled, if the enable bits were off then you
know you can disable it again.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [ide] clean up error path in do_ide_setup_pci_device()
2005-01-04 22:02 ` Alan Cox
@ 2005-01-07 2:20 ` Bartlomiej Zolnierkiewicz
0 siblings, 0 replies; 6+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2005-01-07 2:20 UTC (permalink / raw)
To: Alan Cox; +Cc: Francois Romieu, Linux Kernel Mailing List, torvalds
all pci_disable_device() calls removed for now
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2005-01-07 2:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <200412310343.iBV3hqvd015595@hera.kernel.org>
2005-01-03 18:42 ` [ide] clean up error path in do_ide_setup_pci_device() Alan Cox
2005-01-03 22:22 ` Bartlomiej Zolnierkiewicz
2005-01-03 21:44 ` Alan Cox
2005-01-04 0:14 ` Francois Romieu
2005-01-04 22:02 ` Alan Cox
2005-01-07 2:20 ` Bartlomiej Zolnierkiewicz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox