| Message ID | 1561026716-140537-5-git-send-email-john.garry@huawei.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Fixes for HiSilicon LPC driver and logical PIO code | expand |
[+cc Rafael, linux-acpi] On Thu, Jun 20, 2019 at 06:31:55PM +0800, John Garry wrote: > The original driver author seemed to be under the impression that a driver > cannot be removed if it does not have a .remove method. Or maybe if it is > a built-in platform driver. > > This is not true. This crash can be created: > > root@ubuntu:/sys/bus/platform/drivers/hisi-lpc# echo HISI0191\:00 > unbind > root@ubuntu:/sys/bus/platform/drivers/hisi-lpc# ipmitool raw 6 1 > Unable to handle kernel paging request at virtual address ffff000010035010 > ... > The problem here is that the host goes away but the associated logical PIO > region remains registered, as do the child devices. > > Fix by adding a .remove method to tidy-up by removing the child devices > and unregistering the logical PIO region. > > Fixes: adf38bb0b595 ("HISI LPC: Support the LPC host on Hip06/Hip07 with DT bindings") > Signed-off-by: John Garry <john.garry@huawei.com> > --- > drivers/bus/hisi_lpc.c | 34 ++++++++++++++++++++++++++++++++-- > 1 file changed, 32 insertions(+), 2 deletions(-) > > diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c > index 6d301aafcad2..0e9f1f141c93 100644 > --- a/drivers/bus/hisi_lpc.c > +++ b/drivers/bus/hisi_lpc.c > @@ -456,6 +456,17 @@ struct hisi_lpc_acpi_cell { > size_t pdata_size; > }; > > +static void hisi_lpc_acpi_remove(struct device *hostdev) > +{ > + struct acpi_device *adev = ACPI_COMPANION(hostdev); > + struct acpi_device *child; > + > + device_for_each_child(hostdev, NULL, hisi_lpc_acpi_remove_subdev); > + > + list_for_each_entry(child, &adev->children, node) > + acpi_device_clear_enumerated(child); There are only two other non-ACPI core callers of acpi_device_clear_enumerated() (i2c and spi). That always makes me wonder if we're getting too deep in ACPI internals. > +} > + > /* > * hisi_lpc_acpi_probe - probe children for ACPI FW > * @hostdev: LPC host device pointer > @@ -555,8 +566,7 @@ static int hisi_lpc_acpi_probe(struct device *hostdev) > return 0; > > fail: > - device_for_each_child(hostdev, NULL, > - hisi_lpc_acpi_remove_subdev); > + hisi_lpc_acpi_remove(hostdev); > return ret; > } > > @@ -626,6 +636,8 @@ static int hisi_lpc_probe(struct platform_device *pdev) > return ret; > } > > + dev_set_drvdata(dev, lpcdev); > + > io_end = lpcdev->io_host->io_start + lpcdev->io_host->size; > dev_info(dev, "registered range [%pa - %pa]\n", > &lpcdev->io_host->io_start, &io_end); > @@ -633,6 +645,23 @@ static int hisi_lpc_probe(struct platform_device *pdev) > return ret; > } > > +static int hisi_lpc_remove(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct acpi_device *acpi_device = ACPI_COMPANION(dev); > + struct hisi_lpc_dev *lpcdev = dev_get_drvdata(dev); > + struct logic_pio_hwaddr *range = lpcdev->io_host; > + > + if (acpi_device) > + hisi_lpc_acpi_remove(dev); > + else > + of_platform_depopulate(dev); > + > + logic_pio_unregister_range(range); > + > + return 0; > +} > + > static const struct of_device_id hisi_lpc_of_match[] = { > { .compatible = "hisilicon,hip06-lpc", }, > { .compatible = "hisilicon,hip07-lpc", }, > @@ -646,5 +675,6 @@ static struct platform_driver hisi_lpc_driver = { > .acpi_match_table = ACPI_PTR(hisi_lpc_acpi_match), > }, > .probe = hisi_lpc_probe, > + .remove = hisi_lpc_remove, > }; > builtin_platform_driver(hisi_lpc_driver); > -- > 2.17.1 >
On 21/06/2019 14:56, Bjorn Helgaas wrote: >> >> > +static void hisi_lpc_acpi_remove(struct device *hostdev) >> > +{ >> > + struct acpi_device *adev = ACPI_COMPANION(hostdev); >> > + struct acpi_device *child; >> > + >> > + device_for_each_child(hostdev, NULL, hisi_lpc_acpi_remove_subdev); >> > + >> > + list_for_each_entry(child, &adev->children, node) Hi Bjorn, >> > + acpi_device_clear_enumerated(child); > There are only two other non-ACPI core callers of > acpi_device_clear_enumerated() (i2c and spi). That always makes me > wonder if we're getting too deep in ACPI internals. It's no coincidence that i2c and spi are the only other two non-ACPI core callers. For getting ACPI support for the hisi-lpc driver, we modeled the driver to have the same ACPI enumeration method as i2c and spi hosts. That is, allow the host driver to enumerate the child devices. You can check drivers/acpi/scan.c::acpi_device_enumeration_by_parent() for where we make the check on the host and how it is used. Thanks, John > >> > +} >> > + >> > /* >> > * hisi_lpc_acpi_probe - probe children for ACPI FW >> > * @hostdev: LPC host device pointer >> > @@ -555,8 +566,7 @@ static int hisi_lpc_acpi_probe(struct device *hostdev) >> > return 0; >> > >> > fail: >> > - device_for_each_child(hostdev, NULL, >> > - hisi_lpc_acpi_remove_subdev); >> > + hisi_lpc_acpi_remove(hostdev); >> > return ret;
diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c index 6d301aafcad2..0e9f1f141c93 100644 --- a/drivers/bus/hisi_lpc.c +++ b/drivers/bus/hisi_lpc.c @@ -456,6 +456,17 @@ struct hisi_lpc_acpi_cell { size_t pdata_size; }; +static void hisi_lpc_acpi_remove(struct device *hostdev) +{ + struct acpi_device *adev = ACPI_COMPANION(hostdev); + struct acpi_device *child; + + device_for_each_child(hostdev, NULL, hisi_lpc_acpi_remove_subdev); + + list_for_each_entry(child, &adev->children, node) + acpi_device_clear_enumerated(child); +} + /* * hisi_lpc_acpi_probe - probe children for ACPI FW * @hostdev: LPC host device pointer @@ -555,8 +566,7 @@ static int hisi_lpc_acpi_probe(struct device *hostdev) return 0; fail: - device_for_each_child(hostdev, NULL, - hisi_lpc_acpi_remove_subdev); + hisi_lpc_acpi_remove(hostdev); return ret; } @@ -626,6 +636,8 @@ static int hisi_lpc_probe(struct platform_device *pdev) return ret; } + dev_set_drvdata(dev, lpcdev); + io_end = lpcdev->io_host->io_start + lpcdev->io_host->size; dev_info(dev, "registered range [%pa - %pa]\n", &lpcdev->io_host->io_start, &io_end); @@ -633,6 +645,23 @@ static int hisi_lpc_probe(struct platform_device *pdev) return ret; } +static int hisi_lpc_remove(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct acpi_device *acpi_device = ACPI_COMPANION(dev); + struct hisi_lpc_dev *lpcdev = dev_get_drvdata(dev); + struct logic_pio_hwaddr *range = lpcdev->io_host; + + if (acpi_device) + hisi_lpc_acpi_remove(dev); + else + of_platform_depopulate(dev); + + logic_pio_unregister_range(range); + + return 0; +} + static const struct of_device_id hisi_lpc_of_match[] = { { .compatible = "hisilicon,hip06-lpc", }, { .compatible = "hisilicon,hip07-lpc", }, @@ -646,5 +675,6 @@ static struct platform_driver hisi_lpc_driver = { .acpi_match_table = ACPI_PTR(hisi_lpc_acpi_match), }, .probe = hisi_lpc_probe, + .remove = hisi_lpc_remove, }; builtin_platform_driver(hisi_lpc_driver);
The original driver author seemed to be under the impression that a driver cannot be removed if it does not have a .remove method. Or maybe if it is a built-in platform driver. This is not true. This crash can be created: root@ubuntu:/sys/bus/platform/drivers/hisi-lpc# echo HISI0191\:00 > unbind root@ubuntu:/sys/bus/platform/drivers/hisi-lpc# ipmitool raw 6 1 Unable to handle kernel paging request at virtual address ffff000010035010 Mem abort info: ESR = 0x96000047 Exception class = DABT (current EL), IL = 32 bits SET = 0, FnV = 0 EA = 0, S1PTW = 0 Data abort info: ISV = 0, ISS = 0x00000047 CM = 0, WnR = 1 swapper pgtable: 4k pages, 48-bit VAs, pgdp=000000000118b000 [ffff000010035010] pgd=0000041ffbfff003, pud=0000041ffbffe003, pmd=0000041ffbffd003, pte=0000000000000000 Internal error: Oops: 96000047 [#1] PREEMPT SMP Modules linked in: CPU: 17 PID: 1473 Comm: ipmitool Not tainted 5.2.0-rc5-00003-gf68c53b414a3-dirty #198 Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon D05 IT21 Nemo 2.0 RC0 04/18/2018 pstate: 20000085 (nzCv daIf -PAN -UAO) pc : hisi_lpc_target_in+0x7c/0x120 lr : hisi_lpc_target_in+0x70/0x120 sp : ffff00001efe3930 x29: ffff00001efe3930 x28: ffff841f9f599200 x27: 0000000000000002 x26: 0000000000000000 x25: 0000000000000080 x24: 00000000000000e4 x23: 0000000000000000 x22: 0000000000000064 x21: ffff801fb667d280 x20: 0000000000000001 x19: ffff00001efe39ac x18: 0000000000000000 x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000 x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000 x11: 0000000000000000 x10: 0000000000000000 x9 : 0000000000000000 x8 : ffff841febe60340 x7 : ffff801fb55c52e8 x6 : 0000000000000000 x5 : 0000000000ffc0e3 x4 : 0000000000000001 x3 : ffff801fb667d280 x2 : 0000000000000001 x1 : ffff000010035010 x0 : ffff000010035000 Call trace: hisi_lpc_target_in+0x7c/0x120 hisi_lpc_comm_in+0x88/0x98 logic_inb+0x5c/0xb8 port_inb+0x18/0x20 bt_event+0x38/0x808 smi_event_handler+0x4c/0x5a0 check_start_timer_thread.part.4+0x40/0x58 sender+0x78/0x88 smi_send.isra.6+0x94/0x108 i_ipmi_request+0x2c4/0x8f8 ipmi_request_settime+0x124/0x160 handle_send_req+0x19c/0x208 ipmi_ioctl+0x2c0/0x990 do_vfs_ioctl+0xb8/0x8f8 ksys_ioctl+0x80/0xb8 __arm64_sys_ioctl+0x1c/0x28 el0_svc_common.constprop.0+0x64/0x160 el0_svc_handler+0x28/0x78 el0_svc+0x8/0xc Code: 941d1511 aa0003f9 f94006a0 91004001 (b9000034) ---[ end trace aa842b86af7069e4 ]--- The problem here is that the host goes away but the associated logical PIO region remains registered, as do the child devices. Fix by adding a .remove method to tidy-up by removing the child devices and unregistering the logical PIO region. Fixes: adf38bb0b595 ("HISI LPC: Support the LPC host on Hip06/Hip07 with DT bindings") Signed-off-by: John Garry <john.garry@huawei.com> --- drivers/bus/hisi_lpc.c | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-)