Re: kern/58027: ehci failed attach stops suspend/resume

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

The following reply was made to PR kern/58027; it has been noted by GNATS.

From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
To: mrg%NetBSD.org@localhost
Cc: gnats-bugs%NetBSD.org@localhost, netbsd-bugs%NetBSD.org@localhost
Subject: Re: kern/58027: ehci failed attach stops suspend/resume
Date: Mon, 11 Mar 2024 10:57:27 +0000

 >  	if (sc->sc_ih) {
 >  		pci_intr_disestablish(sc->sc_pc, sc->sc_ih);
 > +		pci_intr_release(sc->sc_pc, sc->sc_pihp, 1);
 >  		sc->sc_ih = NULL;
 >  	}
 
 This part seems wrong -- I think it should be a separate branch:
 
  	if (sc->sc_ih) {
  		pci_intr_disestablish(sc->sc_pc, sc->sc_ih);
  		sc->sc_ih = NULL;
  	}
 	if (sc->sc_pihp) {
 		pci_intr_release(sc->sc_pc, sc->sc_pihp, 1);
 		sc->sc_pihp = NULL;
 	}
 
 and then we can delete some more redundant code in ehci_pci_attach's
 pci_intr_establish_xname error branch.
 
 > @@ -262,6 +263,7 @@ ehci_pci_attach(device_t parent, device_
 >  	int err = ehci_init(&sc->sc);
 >  	if (err) {
 >  		aprint_error_dev(self, "init failed, error=%d\n", err);
 > +		ehci_release_ownership(&sc->sc, sc->sc_pc, sc->sc_tag);
 >  		goto fail;
 >  	}
 > ...
 >  	if (sc->sc.sc_size) {
 > -		ehci_release_ownership(&sc->sc, sc->sc_pc, sc->sc_tag);
 >  		bus_space_unmap(sc->sc.iot, sc->sc.ioh, sc->sc.sc_size);
 >  		sc->sc.sc_size = 0;
 >  	}
 > ...
 > @@ -302,27 +317,17 @@ ehci_pci_detach(device_t self, int flags
 > ...
 > -	if (sc->sc.sc_size) {
 >  		ehci_release_ownership(&sc->sc, sc->sc_pc, sc->sc_tag);
 > -		bus_space_unmap(sc->sc.iot, sc->sc.ioh, sc->sc.sc_size);
 > -		sc->sc.sc_size = 0;
 >  	}
 
 Why did the ehci_release_ownership part move?  I think we should
 generally have less logic in the `if (err) goto fail' branches
 themselves, and more logic shared in the common cleanup path.