linux-kernel - Re: [PATCH 5.15.y 3/4] phy: qcom-qmp-combo: fix broken power on

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 13 Jan 2023 14:03:00 -0600
From:   Stephen Boyd <swboyd@...omium.org>
To:     Johan Hovold <johan@...nel.org>
Cc:     stable@...r.kernel.org, Johan Hovold <johan+linaro@...nel.org>,
        linux-kernel@...r.kernel.org, patches@...ts.linux.dev,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Vinod Koul <vkoul@...nel.org>
Subject: Re: [PATCH 5.15.y 3/4] phy: qcom-qmp-combo: fix broken power on

Quoting Johan Hovold (2023-01-13 00:07:32)
> On Thu, Jan 12, 2023 at 04:54:04PM -0800, Stephen Boyd wrote:
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > index c6f860ce3d99..9fda6d283f20 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > @@ -4620,13 +4621,13 @@ static int qcom_qmp_phy_com_init(struct qmp_phy *qphy)
> >               qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL],
> >                            SW_PWRDN);
> >       } else {
> > -             if (cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL])
> > -                     qphy_setbits(pcs,
> > -                                     cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
> > -                                     cfg->pwrdn_ctrl);
> > +             if (usb_phy->cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL])
> > +                     qphy_setbits(usb_phy->pcs,
> > +                                     usb_phy->cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
> > +                                     usb_phy->cfg->pwrdn_ctrl);
> >               else
> > -                     qphy_setbits(pcs, QPHY_POWER_DOWN_CONTROL,
> > -                                     cfg->pwrdn_ctrl);
> > +                     qphy_setbits(usb_phy->pcs, QPHY_POWER_DOWN_CONTROL,
> > +                                     usb_phy->cfg->pwrdn_ctrl);
>
> This bit looks like it requires some more work in order not to break
> PCIe and UFS PHYs, which lack the USB registers.

Good point. I don't have PCIe or UFS PHYs so I missed this.

>
> >       }
> >
> >       mutex_unlock(&qmp->phy_mutex);
> > @@ -5769,6 +5770,9 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev)
> >                       goto err_node_put;
> >               }
> >
> > +             if (cfg->type == PHY_TYPE_USB3)

I changed this to by 'cfg->type != PHY_TYPE_DP'

> > +                     qmp->usb_phy = qmp->phys[id];
> > +
> >               /*
> >                * Register the pipe clock provided by phy.
> >                * See function description to see details of this pipe clock.
> > @@ -5791,6 +5795,9 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev)
> >               id++;
> >       }
> >
> > +     if (!qmp->usb_phy)
> > +             return -EINVAL;
> > +
>
> This will break probe of PCIe and UFS PHYs unless you only check this
> for combo PHYs.
>
> Perhaps you can rename the usb_phy pointer to something more generic and
> set it also for PCIe and UFS so that it always points to the register
> block that should be used for QPHY_PCS_POWER_DOWN_CONTROL.

I worry that future backports will be complicated if they use 'usb_phy'.
Maybe I'll just leave this as is.