Re: [bug report] PCI: brcmstb: Add control of subdevice voltage regulators

Re: [bug report] PCI: brcmstb: Add control of subdevice voltage regulators

[Lorenzo Pieralisi]

[+Bjorn, linux-pci]

On Tue, Jan 11, 2022 at 08:37:59AM -0800, Florian Fainelli wrote:
> +Lorenzo,
> 
> On 1/11/2022 8:02 AM, Jim Quinlan wrote:
> > On Tue, Jan 11, 2022 at 2:33 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > 
> > > Hello Jim Quinlan,
> > > 
> > > This is a semi-automatic email about new static checker warnings.
> > > 
> > > The patch 31d8fedc2827: "PCI: brcmstb: Add control of subdevice
> > > voltage regulators" from Jan 6, 2022, leads to the following Smatch
> > > complaint:
> > > 
> > >      drivers/pci/controller/pcie-brcmstb.c:1404 brcm_pcie_resume()
> > >      error: we previously assumed 'pcie->sr' could be null (see line 1350)
> > > 
> > > drivers/pci/controller/pcie-brcmstb.c
> > >    1349
> > >    1350          if (pcie->sr) {
> > >                      ^^^^^^^^
> > > Check for NULL
> > > 
> > >    1351                  if (pcie->ep_wakeup_capable) {
> > >    1352                          /*
> > >    1353                           * We are resuming from a suspend.  In the suspend we
> > >    1354                           * did not disable the power supplies, so there is
> > >    1355                           * no need to enable them (and falsely increase their
> > >    1356                           * usage count).
> > >    1357                           */
> > >    1358                          pcie->ep_wakeup_capable = false;
> > >    1359                  } else {
> > >    1360                          ret = regulator_bulk_enable(pcie->sr->num_supplies,
> > >    1361                                                      pcie->sr->supplies);
> > >    1362                          if (ret) {
> > >    1363                                  dev_err(dev, "Could not turn on regulators\n");
> > >    1364                                  goto err_disable_clk;
> > >    1365                          }
> > >    1366                  }
> > >    1367          }
> > >    1368
> > >    1369          ret = reset_control_reset(pcie->rescal);
> > >    1370          if (ret)
> > >    1371                  goto err_regulator;
> > >    1372
> > >    1373          ret = brcm_phy_start(pcie);
> > >    1374          if (ret)
> > >    1375                  goto err_reset;
> > >    1376
> > >    1377          /* Take bridge out of reset so we can access the SERDES reg */
> > >    1378          pcie->bridge_sw_init_set(pcie, 0);
> > >    1379
> > >    1380          /* SERDES_IDDQ = 0 */
> > >    1381          tmp = readl(base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
> > >    1382          u32p_replace_bits(&tmp, 0, PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK);
> > >    1383          writel(tmp, base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
> > >    1384
> > >    1385          /* wait for serdes to be stable */
> > >    1386          udelay(100);
> > >    1387
> > >    1388          ret = brcm_pcie_setup(pcie);
> > >    1389          if (ret)
> > >    1390                  goto err_reset;
> > >    1391
> > >    1392          ret = brcm_pcie_linkup(pcie);
> > >    1393          if (ret)
> > >    1394                  goto err_reset;
> > >    1395
> > >    1396          if (pcie->msi)
> > >    1397                  brcm_msi_set_regs(pcie->msi);
> > >    1398
> > >    1399          return 0;
> > >    1400
> > >    1401  err_reset:
> > >    1402          reset_control_rearm(pcie->rescal);
> > >    1403  err_regulator:
> > >    1404          regulator_bulk_disable(pcie->sr->num_supplies, pcie->- sr->supplies);
> > >                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > Unchecked dereferences.  I think we do need to add a check?
> > Absolutely, "if (pcie->sr)".  What do I need to do - send a fixup or a
> > new full pullreq?  I'm not quite sure if this was accepted yet....
> 
> Your patch series has been queued up in Lorenzo's tree here:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/log/?h=pci/brcmstb
> 
> so I suppose you could submit an incremental patch to fix the error
> reported by Dan and Lorenzo can either squash it or just leave it as a
> fix on top?

Please send a fix to Bjorn and CC linux-pci as soon as possible, he may
be able to squash that in before sending a PR.

Thanks,
Lorenzo