linux-kernel - Re: [PATCH] spi: bcm2835: do not unregister controller in shutdown handler

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:   Mon, 4 Oct 2021 09:57:15 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Mark Brown <broonie@...nel.org>
Cc:     Jason Gunthorpe <jgg@...pe.ca>,
        Lino Sanfilippo <LinoSanfilippo@....de>, rjui@...adcom.com,
        sbranden@...adcom.com, bcm-kernel-feedback-list@...adcom.com,
        nsaenz@...nel.org, linux-spi@...r.kernel.org,
        linux-rpi-kernel@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        p.rosenberger@...bus.com, linux-integrity@...r.kernel.org,
        stable@...r.kernel.org
Subject: Re: [PATCH] spi: bcm2835: do not unregister controller in shutdown
 handler

On 10/4/21 9:52 AM, Mark Brown wrote:
> On Mon, Oct 04, 2021 at 09:36:37AM -0700, Florian Fainelli wrote:
>> On 10/4/21 9:31 AM, Mark Brown wrote:
> 
>>> an issue, someone could press a button or whatever.  Frankly for SPI the
>>> quiescing part doesn't seem like logic that should be implemented in
>>> drivers, it's a subsystem level thing since there's nothing driver
>>> specific about it.
> 
>> Surely the SPI subsystem can help avoid queuing new transfers towards
>> the SPI controller while the controller can shut down the resources that
>> only it knows about.
> 
> Yes, that's what I was saying.
> 
>>> In the case of this specific driver I'm still not clear that the best
>>> thing isn't just to delete the shutdown callback and let any ongoing
>>> transfers complete, though I guess there'd be issues in kexec cases with
>>> long enough tansfers.
> 
>> No please don't, I should have arguably justified the reasons why
>> better, but the main reason is that one of the platforms on which this
>> driver is used has received extensive power management analysis and
>> changes, and shutting down every bit of hardware, including something as
>> small as a SPI controller, and its clock (and its PLL) helped meet
>> stringent power targets.
> 
> OK, so it's similar to a lot of the other embedded cases where it's for
> a power down that doesn't cut as much power as would be desirable -
> that's reasonable.  Like you say you didn't mention it at all in the
> changelog.  Ideally the hardware would just cut all power to the SoC in
> shutdown but then IIRC those boards don't have a PMIC so...  

Yes, that's is what we do on other types of SoCs, this particular one
however only has a single power domain and so software must come to the
rescue to shut down as much as it can. Newer boards do have a PMIC that
can help us with that, but not with everything, still.

> 
>> TBH, I still wonder why we have .shutdown() and we simply don't use
>> .remove() which would reduce the amount of work that people have to do
>> validate that the hardware is put in a low power state and would also
>> reduce the amount of burden on the various subsystems.
> 
> Yeah, it does seem a bit odd - I'd figured it was for speed reasons.
> 


-- 
Florian