| Message ID | 20180731143246.16915-1-stefanha@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | device-dax: avoid hang on error before devm_memremap_pages() | expand |
On 7/31/2018 7:32 AM, Stefan Hajnoczi wrote: > dax_pmem_percpu_exit() waits for dax_pmem_percpu_release() to invoke the > dax_pmem->cmp completion. Unfortunately this approach to cleaning up > the percpu_ref only works after devm_memremap_pages() was successful. > > If devm_add_action_or_reset() or devm_memremap_pages() fails, > dax_pmem_percpu_release() is not invoked. Therefore > dax_pmem_percpu_exit() hangs waiting for the completion: > > rc = devm_add_action_or_reset(dev, dax_pmem_percpu_exit, > &dax_pmem->ref); > if (rc) > return rc; > > dax_pmem->pgmap.ref = &dax_pmem->ref; > addr = devm_memremap_pages(dev, &dax_pmem->pgmap); > > Avoid the hang by calling percpu_ref_exit() in the error paths instead > of going through dax_pmem_percpu_exit(). > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Applied > --- > Found by code inspection. Compile-tested only. > --- > drivers/dax/pmem.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/dax/pmem.c b/drivers/dax/pmem.c > index fd49b24fd6af..99e2aace8078 100644 > --- a/drivers/dax/pmem.c > +++ b/drivers/dax/pmem.c > @@ -105,15 +105,19 @@ static int dax_pmem_probe(struct device *dev) > if (rc) > return rc; > > - rc = devm_add_action_or_reset(dev, dax_pmem_percpu_exit, > - &dax_pmem->ref); > - if (rc) > + rc = devm_add_action(dev, dax_pmem_percpu_exit, &dax_pmem->ref); > + if (rc) { > + percpu_ref_exit(&dax_pmem->ref); > return rc; > + } > > dax_pmem->pgmap.ref = &dax_pmem->ref; > addr = devm_memremap_pages(dev, &dax_pmem->pgmap); > - if (IS_ERR(addr)) > + if (IS_ERR(addr)) { > + devm_remove_action(dev, dax_pmem_percpu_exit, &dax_pmem->ref); > + percpu_ref_exit(&dax_pmem->ref); > return PTR_ERR(addr); > + } > > rc = devm_add_action_or_reset(dev, dax_pmem_percpu_kill, > &dax_pmem->ref);
On Tue, Jul 31, 2018 at 5:26 PM, Dave Jiang <dave.jiang@intel.com> wrote: > > > On 7/31/2018 7:32 AM, Stefan Hajnoczi wrote: >> >> dax_pmem_percpu_exit() waits for dax_pmem_percpu_release() to invoke the >> dax_pmem->cmp completion. Unfortunately this approach to cleaning up >> the percpu_ref only works after devm_memremap_pages() was successful. >> >> If devm_add_action_or_reset() or devm_memremap_pages() fails, >> dax_pmem_percpu_release() is not invoked. Therefore >> dax_pmem_percpu_exit() hangs waiting for the completion: >> >> rc = devm_add_action_or_reset(dev, dax_pmem_percpu_exit, >> &dax_pmem->ref); >> if (rc) >> return rc; >> >> dax_pmem->pgmap.ref = &dax_pmem->ref; >> addr = devm_memremap_pages(dev, &dax_pmem->pgmap); >> >> Avoid the hang by calling percpu_ref_exit() in the error paths instead >> of going through dax_pmem_percpu_exit(). >> >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > > Applied > A similar problem exists in other devm_memremap_pages() users. I had a more comprehensive fix in-flight that I will rebase on top of this change. https://lore.kernel.org/patchwork/patch/961576/
diff --git a/drivers/dax/pmem.c b/drivers/dax/pmem.c index fd49b24fd6af..99e2aace8078 100644 --- a/drivers/dax/pmem.c +++ b/drivers/dax/pmem.c @@ -105,15 +105,19 @@ static int dax_pmem_probe(struct device *dev) if (rc) return rc; - rc = devm_add_action_or_reset(dev, dax_pmem_percpu_exit, - &dax_pmem->ref); - if (rc) + rc = devm_add_action(dev, dax_pmem_percpu_exit, &dax_pmem->ref); + if (rc) { + percpu_ref_exit(&dax_pmem->ref); return rc; + } dax_pmem->pgmap.ref = &dax_pmem->ref; addr = devm_memremap_pages(dev, &dax_pmem->pgmap); - if (IS_ERR(addr)) + if (IS_ERR(addr)) { + devm_remove_action(dev, dax_pmem_percpu_exit, &dax_pmem->ref); + percpu_ref_exit(&dax_pmem->ref); return PTR_ERR(addr); + } rc = devm_add_action_or_reset(dev, dax_pmem_percpu_kill, &dax_pmem->ref);
dax_pmem_percpu_exit() waits for dax_pmem_percpu_release() to invoke the dax_pmem->cmp completion. Unfortunately this approach to cleaning up the percpu_ref only works after devm_memremap_pages() was successful. If devm_add_action_or_reset() or devm_memremap_pages() fails, dax_pmem_percpu_release() is not invoked. Therefore dax_pmem_percpu_exit() hangs waiting for the completion: rc = devm_add_action_or_reset(dev, dax_pmem_percpu_exit, &dax_pmem->ref); if (rc) return rc; dax_pmem->pgmap.ref = &dax_pmem->ref; addr = devm_memremap_pages(dev, &dax_pmem->pgmap); Avoid the hang by calling percpu_ref_exit() in the error paths instead of going through dax_pmem_percpu_exit(). Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- Found by code inspection. Compile-tested only. --- drivers/dax/pmem.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)