| Message ID | CAPcyv4gYG7aa0SvpfKNPGC-dNO+4gq6-uNrJ17fckJ4uX36=jQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Sat, 2015-11-07 at 13:20 -0800, Dan Williams wrote: > On Sat, Nov 7, 2015 at 10:57 AM, Dan Williams <dan.j.williams@intel.co > m> wrote: > > Rafael, awaiting your ack... > > > > Vishal, one thing I'll fix up on applying... > > > > On Tue, Oct 27, 2015 at 3:58 PM, Vishal Verma <vishal.l.verma@intel. > > com> wrote: > > > Add a .notify callback to the acpi_nfit_driver that gets called on > > > a > > > hotplug event. From this, evaluate the _FIT ACPI method which > > > returns > > > the updated NFIT with handles for the hot-plugged NVDIMM. > > > > > > Iterate over the new NFIT, and add any new tables found, and > > > register/enable the corresponding regions. > > > > > > In the nfit test framework, after normal initialization, update > > > the NFIT > > > with a new hot-plugged NVDIMM, and directly call into the driver > > > to > > > update its view of the available regions. > > > > > > Cc: Dan Williams <dan.j.williams@intel.com> > > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > Cc: Toshi Kani <toshi.kani@hpe.com> > > > Cc: Elliott, Robert <elliott@hpe.com> > > > Cc: Jeff Moyer <jmoyer@redhat.com> > > > Cc: <linux-acpi@vger.kernel.org> > > > Cc: <linux-nvdimm@lists.01.org> > > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > > [..] > > > > > +static int acpi_nfit_add(struct acpi_device *adev) > > > +{ > > > + struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL }; > > > + struct acpi_nfit_desc *acpi_desc; > > > + struct device *dev = &adev->dev; > > > + struct acpi_table_header *tbl; > > > + acpi_status status = AE_OK; > > > + acpi_size sz; > > > + int rc = 0; > > > + > > > + device_lock(dev); > > > > The device is already locked by the time we reach this point. The > > unit tests don't trip this path which might be why this wasn't seen > > earlier. > > The lockup call trace if you're interested: > > dracut:/# [ 376.030455] INFO: task systemd-udevd:278 blocked for more > than 120 seconds. > [ 376.032561] Tainted: G O 4.3.0-rc6+ #1747 > [ 376.034376] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" > disables this message. > [ 376.036704] systemd-udevd D 0000000000000000 0 278 262 > 0x00000004 > [ 376.039758] ffff880352a0ba60 0000000000000092 ffff880352a0ba88 > ffff880361e57b98 > [ 376.043779] ffff88035f150000 ffff880352ea5dc0 ffff880352a0c000 > 0000000000000246 > [ 376.047800] ffff880359a43148 ffff880352ea5dc0 00000000ffffffff > ffff880352a0ba78 > [ 376.051821] Call Trace: > [ 376.052882] [<ffffffff818e8b43>] schedule+0x33/0x80 > [ 376.054527] [<ffffffff818e8eae>] > schedule_preempt_disabled+0xe/0x10 > [ 376.056493] [<ffffffff818eaf2f>] mutex_lock_nested+0x14f/0x3a0 > [ 376.058361] [<ffffffffa003140e>] ? acpi_nfit_add+0x4e/0x150 [nfit] > [ 376.060310] [<ffffffffa003140e>] acpi_nfit_add+0x4e/0x150 [nfit] > [ 376.062283] [<ffffffff81584310>] ? > devices_kset_move_last+0x60/0x90 > [ 376.064250] [<ffffffff814d61c0>] acpi_device_probe+0x4f/0xf5 > [ 376.066076] [<ffffffff81588244>] driver_probe_device+0x224/0x480 > [ 376.067983] [<ffffffff81588528>] __driver_attach+0x88/0x90 > [ 376.069769] [<ffffffff815884a0>] ? driver_probe_device+0x480/0x480 > [ 376.071716] [<ffffffff81585d83>] bus_for_each_dev+0x73/0xc0 > [ 376.073522] [<ffffffff81587b9e>] driver_attach+0x1e/0x20 > [ 376.075267] [<ffffffff815876de>] bus_add_driver+0x1ee/0x280 > [ 376.077072] [<ffffffffa0038000>] ? 0xffffffffa0038000 > [ 376.078757] [<ffffffff81588f10>] driver_register+0x60/0xe0 > [ 376.080543] [<ffffffff814d6095>] > acpi_bus_register_driver+0x40/0x42 > [ 376.082511] [<ffffffffa00380ce>] nfit_init+0xce/0x1000 [nfit] > > Thanks for the fixup, Dan. The change looks good - I think I got too reliant on lockdep not complaining - should've taken a closer look.
On Mon, Nov 9, 2015 at 10:12 AM, Verma, Vishal L <vishal.l.verma@intel.com> wrote: > On Sat, 2015-11-07 at 13:20 -0800, Dan Williams wrote: >> On Sat, Nov 7, 2015 at 10:57 AM, Dan Williams <dan.j.williams@intel.co >> m> wrote: >> > Rafael, awaiting your ack... >> > >> > Vishal, one thing I'll fix up on applying... >> > >> > On Tue, Oct 27, 2015 at 3:58 PM, Vishal Verma <vishal.l.verma@intel. >> > com> wrote: >> > > Add a .notify callback to the acpi_nfit_driver that gets called on >> > > a >> > > hotplug event. From this, evaluate the _FIT ACPI method which >> > > returns >> > > the updated NFIT with handles for the hot-plugged NVDIMM. >> > > >> > > Iterate over the new NFIT, and add any new tables found, and >> > > register/enable the corresponding regions. >> > > >> > > In the nfit test framework, after normal initialization, update >> > > the NFIT >> > > with a new hot-plugged NVDIMM, and directly call into the driver >> > > to >> > > update its view of the available regions. >> > > >> > > Cc: Dan Williams <dan.j.williams@intel.com> >> > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> > > Cc: Toshi Kani <toshi.kani@hpe.com> >> > > Cc: Elliott, Robert <elliott@hpe.com> >> > > Cc: Jeff Moyer <jmoyer@redhat.com> >> > > Cc: <linux-acpi@vger.kernel.org> >> > > Cc: <linux-nvdimm@lists.01.org> >> > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> >> > [..] >> > >> > > +static int acpi_nfit_add(struct acpi_device *adev) >> > > +{ >> > > + struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL }; >> > > + struct acpi_nfit_desc *acpi_desc; >> > > + struct device *dev = &adev->dev; >> > > + struct acpi_table_header *tbl; >> > > + acpi_status status = AE_OK; >> > > + acpi_size sz; >> > > + int rc = 0; >> > > + >> > > + device_lock(dev); >> > >> > The device is already locked by the time we reach this point. The >> > unit tests don't trip this path which might be why this wasn't seen >> > earlier. >> >> The lockup call trace if you're interested: >> >> dracut:/# [ 376.030455] INFO: task systemd-udevd:278 blocked for more >> than 120 seconds. >> [ 376.032561] Tainted: G O 4.3.0-rc6+ #1747 >> [ 376.034376] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" >> disables this message. >> [ 376.036704] systemd-udevd D 0000000000000000 0 278 262 >> 0x00000004 >> [ 376.039758] ffff880352a0ba60 0000000000000092 ffff880352a0ba88 >> ffff880361e57b98 >> [ 376.043779] ffff88035f150000 ffff880352ea5dc0 ffff880352a0c000 >> 0000000000000246 >> [ 376.047800] ffff880359a43148 ffff880352ea5dc0 00000000ffffffff >> ffff880352a0ba78 >> [ 376.051821] Call Trace: >> [ 376.052882] [<ffffffff818e8b43>] schedule+0x33/0x80 >> [ 376.054527] [<ffffffff818e8eae>] >> schedule_preempt_disabled+0xe/0x10 >> [ 376.056493] [<ffffffff818eaf2f>] mutex_lock_nested+0x14f/0x3a0 >> [ 376.058361] [<ffffffffa003140e>] ? acpi_nfit_add+0x4e/0x150 [nfit] >> [ 376.060310] [<ffffffffa003140e>] acpi_nfit_add+0x4e/0x150 [nfit] >> [ 376.062283] [<ffffffff81584310>] ? >> devices_kset_move_last+0x60/0x90 >> [ 376.064250] [<ffffffff814d61c0>] acpi_device_probe+0x4f/0xf5 >> [ 376.066076] [<ffffffff81588244>] driver_probe_device+0x224/0x480 >> [ 376.067983] [<ffffffff81588528>] __driver_attach+0x88/0x90 >> [ 376.069769] [<ffffffff815884a0>] ? driver_probe_device+0x480/0x480 >> [ 376.071716] [<ffffffff81585d83>] bus_for_each_dev+0x73/0xc0 >> [ 376.073522] [<ffffffff81587b9e>] driver_attach+0x1e/0x20 >> [ 376.075267] [<ffffffff815876de>] bus_add_driver+0x1ee/0x280 >> [ 376.077072] [<ffffffffa0038000>] ? 0xffffffffa0038000 >> [ 376.078757] [<ffffffff81588f10>] driver_register+0x60/0xe0 >> [ 376.080543] [<ffffffff814d6095>] >> acpi_bus_register_driver+0x40/0x42 >> [ 376.082511] [<ffffffffa00380ce>] nfit_init+0xce/0x1000 [nfit] >> >> > > Thanks for the fixup, Dan. The change looks good - I think I got too > reliant on lockdep not complaining - should've taken a closer look. Unfortunately device_lock() has this by default in device_initialize(): void device_initialize(struct device *dev) { ... lockdep_set_novalidate_class(&dev->mutex); ... } ...so it hides these issues by default.
diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c index 869f279fde95..a2e99ccf0480 100644 --- a/drivers/acpi/nfit.c +++ b/drivers/acpi/nfit.c @@ -1732,22 +1732,19 @@ static int acpi_nfit_add(struct acpi_device *adev) struct acpi_table_header *tbl; acpi_status status = AE_OK; acpi_size sz; - int rc = 0; - device_lock(dev); status = acpi_get_table_with_size("NFIT", 0, &tbl, &sz); if (ACPI_FAILURE(status)) { /* This is ok, we could have an nvdimm hotplugged later */ dev_dbg(dev, "failed to find NFIT at startup\n"); - goto out_unlock; + return 0; } acpi_desc = acpi_nfit_desc_init(adev); if (IS_ERR(acpi_desc)) { dev_err(dev, "%s: error initializing acpi_desc: %ld\n", __func__, PTR_ERR(acpi_desc)); - rc = PTR_ERR(acpi_desc); - goto out_unlock; + return PTR_ERR(acpi_desc); } acpi_desc->nfit = (struct acpi_table_nfit *) tbl; @@ -1762,12 +1759,9 @@ static int acpi_nfit_add(struct acpi_device *adev) rc = acpi_nfit_init(acpi_desc, sz); if (rc) { nvdimm_bus_unregister(acpi_desc->nvdimm_bus); - goto out_unlock; + return rc; } - - out_unlock: - device_unlock(dev); - return rc; + return 0; } _______________________________________________ Linux-nvdimm mailing list