Re: [PATCH] powerpc/eeh: Fix race with driver un/bind

[Michael Neuling <mikey@neuling.org>]

On Fri, 2018-03-23 at 17:33 +1100, Benjamin Herrenschmidt wrote:
> On Fri, 2018-03-23 at 16:44 +1100, Michael Neuling wrote:
>=20
>  .../...
>=20
> > This fixes the problem in the same way the generic PCIe AER code (in
> > drivers/pci/pcie/aer/aerdrv_core.c) does. It makes the EEH code hold
> > the device_lock() before performing the driver EEH callbacks. This
> > ensures either the callbacks are no longer register, or if they are
> > registered the driver will not be removed from underneath us.
> >=20
> > Signed-off-by: Michael Neuling <mikey@neuling.org>
>=20
> Generally ok, minor nits though and do we want a CC stable ?

ok, I'll cc stable.

>=20
> > ---
> >  arch/powerpc/kernel/eeh_driver.c | 67 ++++++++++++++++++++++++--------=
-----
> > ---
> >  1 file changed, 41 insertions(+), 26 deletions(-)
> >=20
> > diff --git a/arch/powerpc/kernel/eeh_driver.c
> > b/arch/powerpc/kernel/eeh_driver.c
> > index 0c0b66fc5b..7cf946ae9a 100644
> > --- a/arch/powerpc/kernel/eeh_driver.c
> > +++ b/arch/powerpc/kernel/eeh_driver.c
> > @@ -207,18 +207,18 @@ static void *eeh_report_error(void *data, void
> > *userdata)
> > =20
> >  	if (!dev || eeh_dev_removed(edev) || eeh_pe_passed(edev->pe))
> >  		return NULL;
> > +
> > +	device_lock(&dev->dev);
> >  	dev->error_state =3D pci_channel_io_frozen;
> > =20
> >  	driver =3D eeh_pcid_get(dev);
> > -	if (!driver) return NULL;
> > +	if (!driver) goto out2;
>=20
> I don't like out1/out2, why not call them out_nodev and out_no_handler
> ? (same comment for the other ones).

OK, will change.

> > =20
> >  	eeh_disable_irq(dev);
> > =20
> >  	if (!driver->err_handler ||
> > -	    !driver->err_handler->error_detected) {
> > -		eeh_pcid_put(dev);
> > -		return NULL;
> > -	}
> > +	    !driver->err_handler->error_detected)
> > +		goto out1;
> > =20
> >  	rc =3D driver->err_handler->error_detected(dev,
> > pci_channel_io_frozen);
> > =20
> > @@ -227,8 +227,11 @@ static void *eeh_report_error(void *data, void
> > *userdata)
> >  	if (*res =3D=3D PCI_ERS_RESULT_NONE) *res =3D rc;
> > =20
> >  	edev->in_error =3D true;
> > -	eeh_pcid_put(dev);
> >  	pci_uevent_ers(dev, PCI_ERS_RESULT_NONE);
> > +out1:
> > +	eeh_pcid_put(dev);
> > +out2:
>=20
> This also changes doing the uevent while holding a reference and the
> the device lock, is that ok ? (I guess a reference is a good thing, the
> device lock, not sure... I hope so but you should at least document it
> as a chance in the cset comment).

The AER code does this, so it should be ok. See report_error_detected().

Mikey