| 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
| ||
|
Date: Fri, 14 Jan 2022 08:51:18 +0000
From: Ricky WU <ricky_wu@...ltek.com>
To: Kai-Heng Feng <kai.heng.feng@...onical.com>
CC: "arnd@...db.de" <arnd@...db.de>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"christophe.jaillet@...adoo.fr" <christophe.jaillet@...adoo.fr>,
"yang.lee@...ux.alibaba.com" <yang.lee@...ux.alibaba.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] misc: rtsx: modify rtd3 flow
> -----Original Message-----
> From: Kai-Heng Feng <kai.heng.feng@...onical.com>
> Sent: Friday, January 14, 2022 3:29 PM
> To: Ricky WU <ricky_wu@...ltek.com>
> Cc: arnd@...db.de; gregkh@...uxfoundation.org;
> christophe.jaillet@...adoo.fr; yang.lee@...ux.alibaba.com;
> linux-kernel@...r.kernel.org
> Subject: Re: [PATCH] misc: rtsx: modify rtd3 flow
>
> On Fri, Jan 14, 2022 at 2:56 PM Ricky WU <ricky_wu@...ltek.com> wrote:
> >
> > > -----Original Message-----
> > > From: Kai-Heng Feng <kai.heng.feng@...onical.com>
> > > Sent: Thursday, January 13, 2022 7:33 PM
> > > To: Ricky WU <ricky_wu@...ltek.com>
> > > Cc: arnd@...db.de; gregkh@...uxfoundation.org;
> > > christophe.jaillet@...adoo.fr; yang.lee@...ux.alibaba.com;
> > > linux-kernel@...r.kernel.org
> > > Subject: Re: [PATCH] misc: rtsx: modify rtd3 flow
> > >
> > > On Thu, Jan 13, 2022 at 6:50 PM Ricky WU <ricky_wu@...ltek.com>
> wrote:
> > > >
> > > > move pm_runtime_get() to _runtime_resume when System enter S3, do
> > > > not have sd_request and do not call start_run to pm_runtime_get()
> > > > cause is_runtime_suspended status not correct
> > > >
> > > > set more register in power_down flow to make plugin or unplug card
> > > > do not wake up system when system is at S3
> > > >
> > > > Signed-off-by: Ricky Wu <ricky_wu@...ltek.com>
> > > > ---
> > > > drivers/misc/cardreader/rts5249.c | 31
> > > > ++++++++++++++++++++++++++++-- drivers/misc/cardreader/rtsx_pcr.c
> > > > ++++++++++++++++++++++++++++|
> > > > 17 ++++++++-------- drivers/misc/cardreader/rtsx_pcr.h | 1 +
> > > > 3 files changed, 38 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/drivers/misc/cardreader/rts5249.c
> > > > b/drivers/misc/cardreader/rts5249.c
> > > > index 53f3a1f45c4a..69e32f075ca9 100644
> > > > --- a/drivers/misc/cardreader/rts5249.c
> > > > +++ b/drivers/misc/cardreader/rts5249.c
> > > > @@ -74,7 +74,8 @@ static void
> > > > rtsx_base_fetch_vendor_settings(struct
> > > rtsx_pcr *pcr)
> > > > pci_read_config_dword(pdev, PCR_SETTING_REG2, ®);
> > > > pcr_dbg(pcr, "Cfg 0x%x: 0x%x\n", PCR_SETTING_REG2, reg);
> > > >
> > > > - pcr->rtd3_en = rtsx_reg_to_rtd3_uhsii(reg);
> > > > + if (CHK_PCI_PID(pcr, PID_524A) || CHK_PCI_PID(pcr,
> PID_525A))
> > > > + pcr->rtd3_en = rtsx_reg_to_rtd3_uhsii(reg);
> > > >
> > > > if (rtsx_check_mmc_support(reg))
> > > > pcr->extra_caps |= EXTRA_CAPS_NO_MMC; @@
> -143,6
> > > > +144,27 @@ static int rts5249_init_from_hw(struct rtsx_pcr *pcr)
> > > > return 0;
> > > > }
> > > >
> > > > +static void rts52xa_force_power_down(struct rtsx_pcr *pcr, u8
> > > > +pm_state) {
> > > > + /* Set relink_time to 0 */
> > > > + rtsx_pci_write_register(pcr, AUTOLOAD_CFG_BASE + 1,
> > > MASK_8_BIT_DEF, 0);
> > > > + rtsx_pci_write_register(pcr, AUTOLOAD_CFG_BASE + 2,
> > > MASK_8_BIT_DEF, 0);
> > > > + rtsx_pci_write_register(pcr, AUTOLOAD_CFG_BASE + 3,
> > > > + RELINK_TIME_MASK, 0);
> > > > +
> > > > + rtsx_pci_write_register(pcr, RTS524A_PM_CTRL3,
> > > > + D3_DELINK_MODE_EN,
> > > D3_DELINK_MODE_EN);
> > > > +
> > > > + if (!pcr->is_runtime_suspended) {
> > > > + rtsx_pci_write_register(pcr,
> RTS524A_AUTOLOAD_CFG1,
> > > > + CD_RESUME_EN_MASK, 0);
> > > > + rtsx_pci_write_register(pcr, RTS524A_PM_CTRL3,
> > > > + 0x01,
> > > 0x00);
> > > > + rtsx_pci_write_register(pcr,
> > > > + RTS524A_PME_FORCE_CTL,
> > > 0x30, 0x20);
> > > > + }
> > > > +
> > > > + rtsx_pci_write_register(pcr, FPDCTL, ALL_POWER_DOWN,
> > > > +ALL_POWER_DOWN); }
> > > > +
> > > > static void rts52xa_save_content_from_efuse(struct rtsx_pcr *pcr) {
> > > > u8 cnt, sv;
> > > > @@ -281,8 +303,11 @@ static int rts5249_extra_init_hw(struct
> > > > rtsx_pcr
> > > > *pcr)
> > > >
> > > > rtsx_pci_send_cmd(pcr, CMD_TIMEOUT_DEF);
> > > >
> > > > - if (CHK_PCI_PID(pcr, PID_524A) || CHK_PCI_PID(pcr,
> PID_525A))
> > > > + if (CHK_PCI_PID(pcr, PID_524A) || CHK_PCI_PID(pcr,
> > > > + PID_525A)) {
> > > > rtsx_pci_write_register(pcr, REG_VREF,
> > > PWD_SUSPND_EN,
> > > > PWD_SUSPND_EN);
> > > > + rtsx_pci_write_register(pcr,
> RTS524A_AUTOLOAD_CFG1,
> > > > + CD_RESUME_EN_MASK,
> > > CD_RESUME_EN_MASK);
> > > > + }
> > > >
> > > > if (pcr->rtd3_en) {
> > > > if (CHK_PCI_PID(pcr, PID_524A) || CHK_PCI_PID(pcr,
> > > > PID_525A)) { @@ -724,6 +749,7 @@ static const struct pcr_ops
> > > rts524a_pcr_ops = {
> > > > .card_power_on = rtsx_base_card_power_on,
> > > > .card_power_off = rtsx_base_card_power_off,
> > > > .switch_output_voltage = rtsx_base_switch_output_voltage,
> > > > + .force_power_down = rts52xa_force_power_down,
> > > > .set_l1off_cfg_sub_d0 = rts5250_set_l1off_cfg_sub_d0, };
> > > >
> > > > @@ -841,6 +867,7 @@ static const struct pcr_ops rts525a_pcr_ops = {
> > > > .card_power_on = rts525a_card_power_on,
> > > > .card_power_off = rtsx_base_card_power_off,
> > > > .switch_output_voltage = rts525a_switch_output_voltage,
> > > > + .force_power_down = rts52xa_force_power_down,
> > > > .set_l1off_cfg_sub_d0 = rts5250_set_l1off_cfg_sub_d0, };
> > > >
> > > > diff --git a/drivers/misc/cardreader/rtsx_pcr.c
> > > > b/drivers/misc/cardreader/rtsx_pcr.c
> > > > index 6ac509c1821c..a83adfb122dc 100644
> > > > --- a/drivers/misc/cardreader/rtsx_pcr.c
> > > > +++ b/drivers/misc/cardreader/rtsx_pcr.c
> > > > @@ -152,12 +152,6 @@ void rtsx_pci_start_run(struct rtsx_pcr *pcr)
> > > > if (pcr->remove_pci)
> > > > return;
> > > >
> > > > - if (pcr->rtd3_en)
> > > > - if (pcr->is_runtime_suspended) {
> > > > - pm_runtime_get(&(pcr->pci->dev));
> > > > - pcr->is_runtime_suspended = false;
> > > > - }
> > > > -
> > > > if (pcr->state != PDEV_STAT_RUN) {
> > > > pcr->state = PDEV_STAT_RUN;
> > > > if (pcr->ops->enable_auto_blink) @@ -1597,6
> > > > +1591,7
> > > @@
> > > > static int rtsx_pci_probe(struct pci_dev *pcidev,
> > > > pcr->host_sg_tbl_addr = pcr->rtsx_resv_buf_addr +
> > > HOST_CMDS_BUF_LEN;
> > > > pcr->card_inserted = 0;
> > > > pcr->card_removed = 0;
> > > > + pcr->rtd3_en = 0;
> > > > INIT_DELAYED_WORK(&pcr->carddet_work,
> > > rtsx_pci_card_detect);
> > > > INIT_DELAYED_WORK(&pcr->idle_work, rtsx_pci_idle_work);
> > > >
> > > > @@ -1796,17 +1791,16 @@ static int rtsx_pci_runtime_suspend(struct
> > > device *device)
> > > > pcr = handle->pcr;
> > > > dev_dbg(&(pcidev->dev), "--> %s\n", __func__);
> > > >
> > > > + pcr->is_runtime_suspended = true;
> > > > +
> > > > cancel_delayed_work(&pcr->carddet_work);
> > > > cancel_delayed_work(&pcr->rtd3_work);
> > > > cancel_delayed_work(&pcr->idle_work);
> > > >
> > > > mutex_lock(&pcr->pcr_mutex);
> > > > rtsx_pci_power_off(pcr, HOST_ENTER_S3);
> > > > -
> > > > mutex_unlock(&pcr->pcr_mutex);
> > > >
> > > > - pcr->is_runtime_suspended = true;
> > > > -
> > > > return 0;
> > > > }
> > > >
> > > > @@ -1820,6 +1814,11 @@ static int rtsx_pci_runtime_resume(struct
> > > > device
> > > *device)
> > > > pcr = handle->pcr;
> > > > dev_dbg(&(pcidev->dev), "--> %s\n", __func__);
> > > >
> > > > + if (pcr->is_runtime_suspended) {
> > > > + pm_runtime_get(&(pcr->pci->dev));
> > > > + pcr->is_runtime_suspended = false;
> > > > + }
> > >
> > > If the runtime resume routine is called for system wide suspend, the
> > > runtime suspend isn't allowed during the period.
> > > So I don't quite understand what this patch is for.
> > >
> >
> > We don’t want to entry D3 frequently
> > So we need to call pm_runtime_get() at start And call pm_runtime_put()
> > in delay-work (rtd3_work)
>
> Maybe use 'cancel_delayed_work(&pcr->rtd3_work)' like what
> rtsx_pci_runtime_suspend() does?
> And for this case maybe cancel_delayed_work_sync() is more preferred.
>
I think you misunderstand what I means
This delay_work() is for not enter D3 <-> D0 frequently
That delay is we need, we don’t want to power_on and power_off frequently on our device
This patch want to solve pcr->is_runtime_suspended this value
because we need to set more register at power_down flow when Device support D3 and System going to S3
> >
> > But we found If we keep this if statement in start_run if
> > (pcr->is_runtime_suspended) {
> > pm_runtime_get(&(pcr->pci->dev));
> > pcr->is_runtime_suspended = false;
> > }
> > pcr->is_runtime_suspended this status are not correct when enter S3
> > because enter S3 not call start_run()
>
> Maybe because the driver is trying to trick the runtime PM core on its real
> power status?
> I.e. the driver is maintaining its own PM state machine. Fully cooperating the
> driver with PM core should solve the issue.
>
System not call start_run() because do not have any sd_request at that time,
so we need to update value(pcr->is_runtime_suspended) at rtsx_pci_runtime_resume
but if we only update value here not to call pm_runtime_get(), the if-statement always be FALSE at start_run()
that is why we move this if-statement from start_run() to rtsx_pci_runtime_resume()
> > >
> > > > +
> > > > mutex_lock(&pcr->pcr_mutex);
> > > >
> > > > rtsx_pci_write_register(pcr, HOST_SLEEP_STATE, 0x03,
> > > > 0x00); diff --git a/drivers/misc/cardreader/rtsx_pcr.h
> > > > b/drivers/misc/cardreader/rtsx_pcr.h
> > > > index daf057c4eea6..b93975268e6d 100644
> > > > --- a/drivers/misc/cardreader/rtsx_pcr.h
> > > > +++ b/drivers/misc/cardreader/rtsx_pcr.h
> > > > @@ -25,6 +25,7 @@
> > > > #define REG_EFUSE_POWEROFF 0x00
> > > > #define RTS5250_CLK_CFG3 0xFF79
> > > > #define RTS525A_CFG_MEM_PD 0xF0
> > > > +#define RTS524A_AUTOLOAD_CFG1 0xFF7C
> > > > #define RTS524A_PM_CTRL3 0xFF7E
> > > > #define RTS525A_BIOS_CFG 0xFF2D
> > > > #define RTS525A_LOAD_BIOS_FLAG 0x01
> > > > --
> > > > 2.25.1
> > > ------Please consider the environment before printing this e-mail.
Powered by blists - more mailing lists