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 17:52:06 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Florian Fainelli <f.fainelli@...il.com>
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 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...  

> 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.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)