RE: [PATCH] pci: imx: disable reglator when imx6_pcie_probe fails

[Hongxing Zhu]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Sent: Tuesday, January 18, 2022 7:18 AM
> To: Xiaolei Wang <xiaolei.wang@xxxxxxxxxxxxx>
> Cc: Hongxing Zhu <hongxing.zhu@xxxxxxx>; l.stach@xxxxxxxxxxxxxx;
> lorenzo.pieralisi@xxxxxxx; robh@xxxxxxxxxx; kw@xxxxxxxxx;
> bhelgaas@xxxxxxxxxx; shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx;
> kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; dl-linux-imx
> <linux-imx@xxxxxxx>; linux-pci@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> 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 <wrsadmin@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
> 
> I got two copies of this.  This one changed:
> 
>   - Signed-off-by: wrsadmin
> <wrsadmin@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
>   + Signed-off-by: Xiaolei Wang <xiaolei.wang@xxxxxxxxxxxxx>
> 
> 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 <wrsadmin@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
>   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 <xiaolei.wang@xxxxxxxxxxxxx>
[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/20220106103645.2790803-1-festevam@xxxxxxxxx/
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/1641368602-20401-1-git-send-email-hongxing.zhu@xxxxxxx/

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
> >