linux-kernel - Re: [PATCH v3] misc: rtsx: add power saving function and bios guide options

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] [day] [month] [year] [list]

Date:   Wed, 2 Sep 2020 07:29:01 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     吳昊澄 Ricky <ricky_wu@...ltek.com>
Cc:     "arnd@...db.de" <arnd@...db.de>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "ulf.hansson@...aro.org" <ulf.hansson@...aro.org>,
        "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        "rui_feng@...lsil.com.cn" <rui_feng@...lsil.com.cn>,
        "vailbhavgupta40@...ail.com" <vailbhavgupta40@...ail.com>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "puranjay12@...il.com" <puranjay12@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] misc: rtsx: add power saving function and bios guide
 options

> > > From: Ricky Wu <ricky_wu@...ltek.com>
> > >
> > > Added rts5227 rts5249 rts5260 rts5228 power saving functions,
> > > added BIOS guide MMC funciton and U_d3_en register support and
> > > fixed rts5260 driving parameter
> > 
> > This should be split into small logical pieces.  I can't really tell
> > what those would be, but just based on the commit message, it could
> > be:
> > 
> >   1) Add rts5227 rts5249 rts5260 rts5228 power saving functions
> > 
> >   2) Add BIOS guide MMC function and U_d3_en register support
> > 
> >   3) Fix rts5260 driving parameter
> > 
> > s/funciton/function/
> > 
> > It looks like 1) *might* be just this:
> > 
> >   rts*_force_power_down()
> >   {
> >     ...
> >     rtsx_pci_write_register(pcr, pcr->reg_pm_ctrl3, D3_DELINK_MODE_EN,
> > 			    D3_DELINK_MODE_EN);
> > 
> > That should be a single patch by itself so it's obvious that it's
> > doing the same thing to several drivers.
> 
> Ok, I will have a extra patch for all xx_force_power_down()

Great, thanks!

> > Explain what "BIOS guide MMC function" means.  Mention the name of the
> > function this adds so we can connect the patch with the commit log.
> > 
> 
> "BIOS guide MMC function" means, via BIOS setting to know MMC card
> support or not 

It will be helpful if this is in a patch by itself.

As far as I know, there's no actual BIOS *call* here, so you must be
looking at some setting in the *device* itself, on the assumption that
it was done by the BIOS?

That sounds like it could become a problem if the device is ever reset
or put in a low-power state.  For resets, and possibly even a
low-power state, BIOS won't be involved, so the setting will be lost,
and the device may work differently after the reset than it did
before.  That sounds undesirable.

> > Explain what "U_d3_en" is; that doesn't even appear in the patch.
> 
> I am going to remove U_d3_en from patch description, this mean
> D3_DELINK_MODE_EN register 

OK.  Given the size of the patch, I think the commit log is too short
to describe what's going on.  More details would be helpful.

Bjorn