From: wrsadmin <[email protected]>
disable reglator when imx6_pcie_probe fails,
otherwise the following calltrace will appear
[ 3.785075] ------------[ cut here ]------------
[ 3.788142] Registering SWP/SWPB emulation handler
[ 3.789853] WARNING: CPU: 0 PID: 7 at drivers/regulator/core.c:2257 _regulator_put.part.0+0x1bc/0x1e0
[ 3.795680] Loading compiled-in X.509 certificates
[ 3.803947] Modules linked in:
[ 3.811922] CPU: 0 PID: 7 Comm: kworker/u8:0 Not tainted 5.16.0-10645-g3c750c7b6143-dirty #9
[ 3.820393] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[ 3.826945] Workqueue: events_unbound async_run_entry_fn
[ 3.832304] unwind_backtrace from show_stack+0x10/0x14
[ 3.837569] show_stack from dump_stack_lvl+0x58/0x70
[ 3.842663] dump_stack_lvl from __warn+0xd8/0x114
[ 3.847493] __warn from warn_slowpath_fmt+0x5c/0xc4
[ 3.852490] warn_slowpath_fmt from _regulator_put.part.0+0x1bc/0x1e0
[ 3.858968] _regulator_put.part.0 from regulator_put+0x2c/0x3c
[ 3.864918] regulator_put from release_nodes+0x50/0x178
[ 3.870270] release_nodes from devres_release_all+0x80/0xd0
[ 3.875968] devres_release_all from really_probe+0xdc/0x30c
[ 3.881661] really_probe from __driver_probe_device+0x80/0xe4
[ 3.887522] __driver_probe_device from driver_probe_device+0x30/0xd4
[ 3.893991] driver_probe_device from __driver_attach_async_helper+0x20/0x38
[ 3.901068] __driver_attach_async_helper from async_run_entry_fn+0x20/0xb4
[ 3.908059] async_run_entry_fn from process_one_work+0x298/0x7d0
[ 3.914188] process_one_work from worker_thread+0x30/0x510
[ 3.919792] worker_thread from kthread+0x128/0x14c
[ 3.924705] kthread from ret_from_fork+0x14/0x38
[ 3.929443] Exception stack(0xc20cbfb0 to 0xc20cbff8)
[ 3.934521] bfa0: 00000000 00000000 00000000 00000000
[ 3.942722] bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[ 3.950922] bfe0: 00000000 00000000 00000000 00000000 00000013 00000000
[ 3.957677] irq event stamp: 1207
[ 3.961024] hardirqs last enabled at (1215): [<c0198270>] __up_console_sem+0x50/0x60
[ 3.968974] hardirqs last disabled at (1224): [<c019825c>] __up_console_sem+0x3c/0x60
[ 3.976911] softirqs last enabled at (1206): [<c010150c>] __do_softirq+0x2ec/0x5a4
[ 3.984669] softirqs last disabled at (1197): [<c012ef08>] irq_exit+0x18c/0x20c
[ 3.992021] ---[ end trace 45a52c023bf8fb33 ]---
Signed-off-by: Xiaolei Wang <[email protected]>
---
drivers/pci/controller/dwc/pci-imx6.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 6974bd5aa116..f8279a15463b 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1216,7 +1216,7 @@ static int imx6_pcie_probe(struct platform_device *pdev)
ret = dw_pcie_host_init(&pci->pp);
if (ret < 0)
- return ret;
+ goto err_vpcie;
if (pci_msi_enabled()) {
u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
@@ -1226,6 +1226,11 @@ static int imx6_pcie_probe(struct platform_device *pdev)
}
return 0;
+
+err_vpcie:
+ regulator_disable(imx6_pcie->vpcie);
+
+ return ret;
}
static void imx6_pcie_shutdown(struct platform_device *pdev)
--
2.25.1
On Mon, Jan 17, 2022 at 06:21:37PM +0800, Xiaolei Wang wrote:
> From: wrsadmin <[email protected]>
I got two copies of this. This one changed:
- Signed-off-by: wrsadmin <[email protected]>
+ Signed-off-by: Xiaolei Wang <[email protected]>
This looks like a good change. You should also change the
"From: wrsadmin" line above because that is what "git am" uses in the
commit:
commit 119fdfb8fb1b ("pci: imx: disable reglator when imx6_pcie_probe fails")
Author: wrsadmin <[email protected]>
Date: Mon Jan 17 18:17:35 2022 +0800
Also, please run "git log --oneline drivers/pci/controller/dwc/pci-imx6.c"
and make your subject line match in capitalization and style.
Also, fix the "s/reglator/regulator" typo in subject and below and add
"()" after the imx6_pcie_probe() function name.
If you include a calltrace, remove timestamps and other non-relevant
stuff from it and indent quoted material two spaces. But in this
case, I don't think the calltrace is useful.
The reason to do this is not to avoid a warning and the related
calltrace. The reason is to fix the problem that *caused* the warning
in the first place. So we should describe the underlying problem and
the fix.
> disable reglator when imx6_pcie_probe fails,
> otherwise the following calltrace will appear
>
> [ 3.785075] ------------[ cut here ]------------
> [ 3.788142] Registering SWP/SWPB emulation handler
> [ 3.789853] WARNING: CPU: 0 PID: 7 at drivers/regulator/core.c:2257 _regulator_put.part.0+0x1bc/0x1e0
> [ 3.795680] Loading compiled-in X.509 certificates
> [ 3.803947] Modules linked in:
> [ 3.811922] CPU: 0 PID: 7 Comm: kworker/u8:0 Not tainted 5.16.0-10645-g3c750c7b6143-dirty #9
> [ 3.820393] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
> [ 3.826945] Workqueue: events_unbound async_run_entry_fn
> [ 3.832304] unwind_backtrace from show_stack+0x10/0x14
> [ 3.837569] show_stack from dump_stack_lvl+0x58/0x70
> [ 3.842663] dump_stack_lvl from __warn+0xd8/0x114
> [ 3.847493] __warn from warn_slowpath_fmt+0x5c/0xc4
> [ 3.852490] warn_slowpath_fmt from _regulator_put.part.0+0x1bc/0x1e0
> [ 3.858968] _regulator_put.part.0 from regulator_put+0x2c/0x3c
> [ 3.864918] regulator_put from release_nodes+0x50/0x178
> [ 3.870270] release_nodes from devres_release_all+0x80/0xd0
> [ 3.875968] devres_release_all from really_probe+0xdc/0x30c
> [ 3.881661] really_probe from __driver_probe_device+0x80/0xe4
> [ 3.887522] __driver_probe_device from driver_probe_device+0x30/0xd4
> [ 3.893991] driver_probe_device from __driver_attach_async_helper+0x20/0x38
> [ 3.901068] __driver_attach_async_helper from async_run_entry_fn+0x20/0xb4
> [ 3.908059] async_run_entry_fn from process_one_work+0x298/0x7d0
> [ 3.914188] process_one_work from worker_thread+0x30/0x510
> [ 3.919792] worker_thread from kthread+0x128/0x14c
> [ 3.924705] kthread from ret_from_fork+0x14/0x38
> [ 3.929443] Exception stack(0xc20cbfb0 to 0xc20cbff8)
> [ 3.934521] bfa0: 00000000 00000000 00000000 00000000
> [ 3.942722] bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [ 3.950922] bfe0: 00000000 00000000 00000000 00000000 00000013 00000000
> [ 3.957677] irq event stamp: 1207
> [ 3.961024] hardirqs last enabled at (1215): [<c0198270>] __up_console_sem+0x50/0x60
> [ 3.968974] hardirqs last disabled at (1224): [<c019825c>] __up_console_sem+0x3c/0x60
> [ 3.976911] softirqs last enabled at (1206): [<c010150c>] __do_softirq+0x2ec/0x5a4
> [ 3.984669] softirqs last disabled at (1197): [<c012ef08>] irq_exit+0x18c/0x20c
> [ 3.992021] ---[ end trace 45a52c023bf8fb33 ]---
>
> Signed-off-by: Xiaolei Wang <[email protected]>
> ---
> drivers/pci/controller/dwc/pci-imx6.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 6974bd5aa116..f8279a15463b 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -1216,7 +1216,7 @@ static int imx6_pcie_probe(struct platform_device *pdev)
>
> ret = dw_pcie_host_init(&pci->pp);
> if (ret < 0)
> - return ret;
> + goto err_vpcie;
>
> if (pci_msi_enabled()) {
> u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
> @@ -1226,6 +1226,11 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> }
>
> return 0;
> +
> +err_vpcie:
> + regulator_disable(imx6_pcie->vpcie);
I don't know the regulator code and I couldn't figure out how
devm_regulator_get_optional() works because it passes OPTIONAL_GET in
to _devm_regulator_get(), but nothing ever checks for that, and I
couldn't be bothered to figure out what magic makes it work.
But regulator_disable() definitely dereferences its argument, and I'm
pretty sure it's possible to get here with "imx6_pcie->vpcie == NULL".
Also, it looks like the same situation with imx6_pcie->vph, so why
don't you clean that one up, too?
> + return ret;
> }
>
> static void imx6_pcie_shutdown(struct platform_device *pdev)
> --
> 2.25.1
>
> -----Original Message-----
> From: Bjorn Helgaas <[email protected]>
> Sent: Tuesday, January 18, 2022 7:18 AM
> To: Xiaolei Wang <[email protected]>
> Cc: Hongxing Zhu <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; dl-linux-imx
> <[email protected]>; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH] pci: imx: disable reglator when imx6_pcie_probe
> fails
>
> On Mon, Jan 17, 2022 at 06:21:37PM +0800, Xiaolei Wang wrote:
> > From: wrsadmin <[email protected]>
>
> I got two copies of this. This one changed:
>
> - Signed-off-by: wrsadmin
> <[email protected]>
> + Signed-off-by: Xiaolei Wang <[email protected]>
>
> This looks like a good change. You should also change the
> "From: wrsadmin" line above because that is what "git am" uses in the
> commit:
>
> commit 119fdfb8fb1b ("pci: imx: disable reglator when
> imx6_pcie_probe fails")
> Author: wrsadmin <[email protected]>
> Date: Mon Jan 17 18:17:35 2022 +0800
>
> Also, please run "git log --oneline drivers/pci/controller/dwc/pci-imx6.c"
> and make your subject line match in capitalization and style.
>
> Also, fix the "s/reglator/regulator" typo in subject and below and add "()"
> after the imx6_pcie_probe() function name.
>
> If you include a calltrace, remove timestamps and other non-relevant
> stuff from it and indent quoted material two spaces. But in this case, I
> don't think the calltrace is useful.
>
> The reason to do this is not to avoid a warning and the related calltrace.
> The reason is to fix the problem that *caused* the warning in the first
> place. So we should describe the underlying problem and the fix.
>
> > disable reglator when imx6_pcie_probe fails, otherwise the following
> > calltrace will appear
> >
> > [ 3.785075] ------------[ cut here ]------------ [ 3.788142]
> > Registering SWP/SWPB emulation handler [ 3.789853] WARNING: CPU:
> 0
> > PID: 7 at drivers/regulator/core.c:2257
> > _regulator_put.part.0+0x1bc/0x1e0 [ 3.795680] Loading compiled-in
> > X.509 certificates [ 3.803947] Modules linked in:
> > [ 3.811922] CPU: 0 PID: 7 Comm: kworker/u8:0 Not tainted
> > 5.16.0-10645-g3c750c7b6143-dirty #9 [ 3.820393] Hardware name:
> > Freescale i.MX6 Quad/DualLite (Device Tree) [ 3.826945] Workqueue:
> > events_unbound async_run_entry_fn [ 3.832304] unwind_backtrace
> from
> > show_stack+0x10/0x14 [ 3.837569] show_stack from
> > dump_stack_lvl+0x58/0x70 [ 3.842663] dump_stack_lvl from
> > __warn+0xd8/0x114 [ 3.847493] __warn from
> warn_slowpath_fmt+0x5c/0xc4
> > [ 3.852490] warn_slowpath_fmt from
> _regulator_put.part.0+0x1bc/0x1e0 [
> > 3.858968] _regulator_put.part.0 from regulator_put+0x2c/0x3c [
> > 3.864918] regulator_put from release_nodes+0x50/0x178 [ 3.870270]
> > release_nodes from devres_release_all+0x80/0xd0 [ 3.875968]
> > devres_release_all from really_probe+0xdc/0x30c [ 3.881661]
> > really_probe from __driver_probe_device+0x80/0xe4 [ 3.887522]
> > __driver_probe_device from driver_probe_device+0x30/0xd4
> [ 3.893991]
> > driver_probe_device from __driver_attach_async_helper+0x20/0x38
> > [ 3.901068] __driver_attach_async_helper from
> > async_run_entry_fn+0x20/0xb4 [ 3.908059] async_run_entry_fn from
> > process_one_work+0x298/0x7d0 [ 3.914188] process_one_work from
> > worker_thread+0x30/0x510 [ 3.919792] worker_thread from
> > kthread+0x128/0x14c [ 3.924705] kthread from
> ret_from_fork+0x14/0x38 [
> > 3.929443] Exception stack(0xc20cbfb0 to 0xc20cbff8) [ 3.934521] bfa0:
> > 00000000 00000000 00000000 00000000 [ 3.942722] bfc0: 00000000
> > 00000000 00000000 00000000 00000000 00000000 00000000
> 00000000 [
> > 3.950922] bfe0: 00000000 00000000 00000000 00000000 00000013
> 00000000
> > [ 3.957677] irq event stamp: 1207 [ 3.961024] hardirqs last enabled at
> > (1215): [<c0198270>] __up_console_sem+0x50/0x60 [ 3.968974]
> hardirqs
> > last disabled at (1224): [<c019825c>] __up_console_sem+0x3c/0x60 [
> > 3.976911] softirqs last enabled at (1206): [<c010150c>]
> > __do_softirq+0x2ec/0x5a4 [ 3.984669] softirqs last disabled at (1197):
> > [<c012ef08>] irq_exit+0x18c/0x20c [ 3.992021] ---[ end trace
> > 45a52c023bf8fb33 ]---
> >
> > Signed-off-by: Xiaolei Wang <[email protected]>
[Richard Zhu] Thanks for Bjorn's kindly help firstly.
Hi Xiaolei:
Thanks for your patch.
There is already a patch issued by Fabio refer to this kernel dump problem.
https://patchwork.kernel.org/project/linux-pci/patch/[email protected]/
Then, I prefer to update the fix by the following codes refine
patch-set in next step.
https://patchwork.ozlabs.org/project/linux-pci/cover/[email protected]/
Best Regards
Richard
> > ---
> > drivers/pci/controller/dwc/pci-imx6.c | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 6974bd5aa116..f8279a15463b 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -1216,7 +1216,7 @@ static int imx6_pcie_probe(struct
> > platform_device *pdev)
> >
> > ret = dw_pcie_host_init(&pci->pp);
> > if (ret < 0)
> > - return ret;
> > + goto err_vpcie;
> >
> > if (pci_msi_enabled()) {
> > u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI); @@
> > -1226,6 +1226,11 @@ static int imx6_pcie_probe(struct
> platform_device *pdev)
> > }
> >
> > return 0;
> > +
> > +err_vpcie:
> > + regulator_disable(imx6_pcie->vpcie);
>
> I don't know the regulator code and I couldn't figure out how
> devm_regulator_get_optional() works because it passes OPTIONAL_GET
> in to _devm_regulator_get(), but nothing ever checks for that, and I
> couldn't be bothered to figure out what magic makes it work.
>
> But regulator_disable() definitely dereferences its argument, and I'm
> pretty sure it's possible to get here with "imx6_pcie->vpcie == NULL".
>
> Also, it looks like the same situation with imx6_pcie->vph, so why don't
> you clean that one up, too?
>
> > + return ret;
> > }
> >
> > static void imx6_pcie_shutdown(struct platform_device *pdev)
> > --
> > 2.25.1
> >