| 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
| ||
|
Message-ID: <20240812041051.GA2861@thinkpad>
Date: Mon, 12 Aug 2024 09:40:51 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Shawn Lin <shawn.lin@...k-chips.com>
Cc: Rob Herring <robh+dt@...nel.org>,
"James E . J . Bottomley" <James.Bottomley@...senPartnership.com>,
"Martin K . Petersen" <martin.petersen@...cle.com>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Heiko Stuebner <heiko@...ech.de>,
Alim Akhtar <alim.akhtar@...sung.com>,
Avri Altman <avri.altman@....com>,
Bart Van Assche <bvanassche@....org>,
YiFeng Zhao <zyf@...k-chips.com>, Liang Chen <cl@...k-chips.com>,
linux-scsi@...r.kernel.org, linux-rockchip@...ts.infradead.org,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v2 3/3] scsi: ufs: rockchip: init support for UFS
On Mon, Aug 12, 2024 at 09:28:26AM +0800, Shawn Lin wrote:
> JHi Mani,
>
> 在 2024/8/10 17:28, Manivannan Sadhasivam 写道:
> > On Fri, Aug 09, 2024 at 04:16:41PM +0800, Shawn Lin wrote:
> >
> > [...]
> >
> > > > > +static int ufs_rockchip_hce_enable_notify(struct ufs_hba *hba,
> > > > > + enum ufs_notify_change_status status)
> > > > > +{
> > > > > + int err = 0;
> > > > > +
> > > > > + if (status == PRE_CHANGE) {
> > > > > + int retry_outer = 3;
> > > > > + int retry_inner;
> > > > > +start:
> > > > > + if (ufshcd_is_hba_active(hba))
> > > > > + /* change controller state to "reset state" */
> > > > > + ufshcd_hba_stop(hba);
> > > > > +
> > > > > + /* UniPro link is disabled at this point */
> > > > > + ufshcd_set_link_off(hba);
> > > > > +
> > > > > + /* start controller initialization sequence */
> > > > > + ufshcd_writel(hba, CONTROLLER_ENABLE, REG_CONTROLLER_ENABLE);
> > > > > +
> > > > > + usleep_range(100, 200);
> > > > > +
> > > > > + /* wait for the host controller to complete initialization */
> > > > > + retry_inner = 50;
> > > > > + while (!ufshcd_is_hba_active(hba)) {
> > > > > + if (retry_inner) {
> > > > > + retry_inner--;
> > > > > + } else {
> > > > > + dev_err(hba->dev,
> > > > > + "Controller enable failed\n");
> > > > > + if (retry_outer) {
> > > > > + retry_outer--;
> > > > > + goto start;
> > > > > + }
> > > > > + return -EIO;
> > > > > + }
> > > > > + usleep_range(1000, 1100);
> > > > > + }
> > > >
> > > > You just duplicated ufshcd_hba_execute_hce() here. Why? This doesn't make sense.
> > >
> > > Since we set UFSHCI_QUIRK_BROKEN_HCE, and we also need to do someting
> > > which is very similar to ufshcd_hba_execute_hce(), before calling
> > > ufshcd_dme_reset(). Similar but not totally the same. I'll try to see if
> > > we can export ufshcd_hba_execute_hce() to make full use of it.
> > >
> >
> > But you are starting the controller using REG_CONTROLLER_ENABLE. Isn't that
> > supposed to be broken if you set UFSHCI_QUIRK_BROKEN_HCE? Or I am
> > misunderstanding the quirk?
> >
>
> Our controller doesn't work with exiting code, whether setting
> UFSHCI_QUIRK_BROKEN_HCE or not.
>
Okay. Then this means you do not need this quirk at all.
>
> For UFSHCI_QUIRK_BROKEN_HCE case, it calls ufshcd_dme_reset（）first,
> but we need to set REG_CONTROLLER_ENABLE first.
>
> For !UFSHCI_QUIRK_BROKEN_HCE case, namly ufshcd_hba_execute_hce, it
> sets REG_CONTROLLER_ENABLE first but never send DMA_RESET and calls
> ufshcd_dme_enable.
>
I don't see where ufshcd_dme_enable() is getting called for
!UFSHCI_QUIRK_BROKEN_HCE case.
> So the closet code path is to go through UFSHCI_QUIRK_BROKEN_HCE case,
> and set REG_CONTROLLER_ENABLE by adding hce_enable_notify hook.
>
No, that is abusing the quirk. But I'm confused about why your controller wants
resetting the unipro stack _after_ enabling the controller? Why can't it be
reset before?
> > > >
> > > > > + } else { /* POST_CHANGE */
> > > > > + err = ufshcd_vops_phy_initialization(hba);
> > > > > + }
> > > > > +
> > > > > + return err;
> > > > > +}
> > > > > +
[...]
> > > > > +static const struct dev_pm_ops ufs_rockchip_pm_ops = {
> > > > > + SET_SYSTEM_SLEEP_PM_OPS(ufs_rockchip_suspend, ufs_rockchip_resume)
> > > > > + SET_RUNTIME_PM_OPS(ufs_rockchip_runtime_suspend, ufs_rockchip_runtime_resume, NULL)
> > > >
> > > > Why can't you use ufshcd PM ops as like other vendor drivers?
> > >
> > > It doesn't work from the test. We have many use case to power down the
> > > controller and device, so there is no flow to recovery the link. Only
> > > when the first accessing to UFS fails, the ufshcd error handle recovery the
> > > link. This is not what we expect.
> > >
> >
> > What tests? The existing UFS controller drivers are used in production devices
> > and they never had a usecase to invent their own PM callbacks. So if your
> > controller is special, then you need to justify it more elaborately. If
> > something is missing in ufshcd callbacks, then we can add them.
> >
>
> All the register got lost each time as we power down both controller & PHY
> and devices in suspend.
Which suspend? runtime or system suspend? I believe system suspend.
> So we have to restore the necessary
> registers and link. I didn't see where the code recovery the controller
> settings in ufshcd_resume, except ufshcd_err_handler（）triggers that.
> Am I missing any thing?
Can you explain what is causing the powerdown of the controller and PHY?
Because, ufshcd_suspend() just turns off the clocks and regulators (if
UFSHCD_CAP_AGGR_POWER_COLLAPSE is set) and spm_lvl 3 set by this driver only
puts the device in sleep mode and link in hibern8 state.
- Mani
> Below is the dump we get if using
> SET_SYSTEM_SLEEP_PM_OPS(ufshcd_system_suspend, ufshcd_system_resume).
> It can work as ufshcd_err_handler () will fix the link, but we have to
> suffer from getting the error log each time. Moreover, we need to gate
> 26MHz refclk for device when RPM is called. So our own rpm callback is
> needed.
>
> [ 14.318444] <<GTP-INF>>[gt1x_wakeup_sleep:964] Wakeup by poweron
> [ 14.439723] ufshcd-rockchip 2a2d0000.ufs: Controller not ready to accept
> UIC commands
> [ 14.439730] ufshcd-rockchip 2a2d0000.ufs: pwr ctrl cmd 0x18 with mode 0x0
> uic error -5
> [ 14.439736] ufshcd-rockchip 2a2d0000.ufs: UFS Host state=1
> [ 14.439740] ufshcd-rockchip 2a2d0000.ufs: outstanding reqs=0x0 tasks=0x0
> [ 14.439744] ufshcd-rockchip 2a2d0000.ufs: saved_err=0x0,
> saved_uic_err=0x0
> [ 14.439748] ufshcd-rockchip 2a2d0000.ufs: Device power mode=2, UIC link
> state=2
> [ 14.439753] ufshcd-rockchip 2a2d0000.ufs: PM in progress=1, sys.
> suspended=1
> [ 14.439758] ufshcd-rockchip 2a2d0000.ufs: Auto BKOPS=0, Host self-block=0
> [ 14.439762] ufshcd-rockchip 2a2d0000.ufs: Clk gate=1
> [ 14.439766] ufshcd-rockchip 2a2d0000.ufs: last_hibern8_exit_tstamp at 0
> us, hibern8_exit_cnt=0
> [ 14.439770] ufshcd-rockchip 2a2d0000.ufs: last intr at 10807625 us, last
> intr status=0x440
> [ 14.439775] ufshcd-rockchip 2a2d0000.ufs: error handling flags=0x0, req.
> abort count=0
> [ 14.439779] ufshcd-rockchip 2a2d0000.ufs: hba->ufs_version=0x200, Host
> capabilities=0x187011f, caps=0x48c
> [ 14.439785] ufshcd-rockchip 2a2d0000.ufs: quirks=0x2100, dev. quirks=0xc4
> [ 14.439790] ufshcd-rockchip 2a2d0000.ufs: UFS dev info: SAMSUNG
> KLUDG2R1DE-B0F1 rev 0100
> [ 14.439796] ufshcd-rockchip 2a2d0000.ufs: clk: core, rate: 50000000
> [ 14.439822] host_regs: 00000000: 0187011f 00000000 00000200 00000000
> [ 14.439827] host_regs: 00000010: 00000000 000005e6 00000000 00000000
> [ 14.439831] host_regs: 00000020: 00000000 00000000 00000000 00000000
> [ 14.439835] host_regs: 00000030: 00000000 00000000 00000000 00000000
> [ 14.439839] host_regs: 00000040: 00000000 00000000 00000000 00000000
> [ 14.439843] host_regs: 00000050: 00000000 00000000 00000000 00000000
> [ 14.439847] host_regs: 00000060: 00000000 00000000 00000000 00000000
> [ 14.439851] host_regs: 00000070: 00000000 00000000 00000000 00000000
> [ 14.439855] host_regs: 00000080: 00000000 00000000 00000000 00000000
> [ 14.439859] host_regs: 00000090: 00000000 00000000 00000000 00000000
> [ 14.439863] ufshcd-rockchip 2a2d0000.ufs: No record of pa_err
> [ 14.439867] ufshcd-rockchip 2a2d0000.ufs: No record of dl_err
> [ 14.439871] ufshcd-rockchip 2a2d0000.ufs: No record of nl_err
> [ 14.439876] ufshcd-rockchip 2a2d0000.ufs: No record of tl_err
> [ 14.439880] ufshcd-rockchip 2a2d0000.ufs: No record of dme_err
> [ 14.439884] ufshcd-rockchip 2a2d0000.ufs: No record of auto_hibern8_err
> [ 14.439888] ufshcd-rockchip 2a2d0000.ufs: No record of fatal_err
> [ 14.439892] ufshcd-rockchip 2a2d0000.ufs: No record of link_startup_fail
> [ 14.439896] ufshcd-rockchip 2a2d0000.ufs: No record of resume_fail
> [ 14.439900] ufshcd-rockchip 2a2d0000.ufs: No record of suspend_fail
> [ 14.439905] ufshcd-rockchip 2a2d0000.ufs: dev_reset[0] = 0x0 at 1418763
> us
> [ 14.439910] ufshcd-rockchip 2a2d0000.ufs: dev_reset: total cnt=1
> [ 14.439914] ufshcd-rockchip 2a2d0000.ufs: No record of host_reset
> [ 14.439918] ufshcd-rockchip 2a2d0000.ufs: No record of task_abort
> [ 14.439930] ufshcd-rockchip 2a2d0000.ufs: ufshcd_uic_hibern8_exit:
> hibern8 exit failed. ret = -5
> [ 14.439935] ufshcd-rockchip 2a2d0000.ufs: __ufshcd_wl_resume: hibern8
> exit failed -5
> [ 14.439944] ufs_device_wlun 0:0:0:49488: ufshcd_wl_resume failed: -5
> [ 14.439950] ufs_device_wlun 0:0:0:49488: PM: dpm_run_callback():
> scsi_bus_resume+0x0/0xa8 returns -5
> [ 14.440003] ufshcd-rockchip 2a2d0000.ufs: ufshcd_err_handler started; HBA
> state eh_fatal; powered 1; shutting down 0; saved_err = 0; saved_uic_err =
> 0; force_reset = 0; link is broken
> [ 14.440017] ufs_device_wlun 0:0:0:49488: PM: failed to resume async:
> error -5
>
> > - Mani
> >
--
மணிவண்ணன் சதாசிவம்
Powered by blists - more mailing lists