Re: [ide] clean up error path in do_ide_setup_pci_device()

Re: [ide] clean up error path in do_ide_setup_pci_device()

[Alan Cox]

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

Re: [ide] clean up error path in do_ide_setup_pci_device()

[Bartlomiej Zolnierkiewicz]

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)

Re: [ide] clean up error path in do_ide_setup_pci_device()

[Alan Cox]

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

Re: [ide] clean up error path in do_ide_setup_pci_device()

[Francois Romieu]

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

Re: [ide] clean up error path in do_ide_setup_pci_device()

[Alan Cox]

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.

Re: [ide] clean up error path in do_ide_setup_pci_device()

[Bartlomiej Zolnierkiewicz]

all pci_disable_device() calls removed for now