From: Mark Brown <broonie@kernel.org> Date: 2021-09-28 20:09:35
On Tue, Sep 28, 2021 at 09:56:57PM +0200, Lino Sanfilippo wrote:
Do not unregister the SPI controller in the shutdown handler. The reason
to avoid this is that controller unregistration results in the slave
devices remove() handler being called which may be unexpected for slave
drivers at system shutdown.
One example is if the BCM2835 driver is used together with the TPM SPI
driver:
At system shutdown first the TPM chip devices (pre) shutdown handler
(tpm_class_shutdown) is called, stopping the chip and setting an operations
pointer to NULL.
Then since the BCM2835 shutdown handler unregisters the SPI controller the
TPM SPI remove function (tpm_tis_spi_remove) is also called. In case of
TPM 2 this function accesses the now nullified operations pointer,
resulting in the following NULL pointer access:
[ 174.078277] 8<--- cut here ---
[ 174.078288] Unable to handle kernel NULL pointer dereference at virtual address 00000034
[ 174.078293] pgd = 557a5fc9
[ 174.078300] [00000034] *pgd=031cf003, *pmd=00000000
[ 174.078317] Internal error: Oops: 206 [#1] SMP ARM
[ 174.078323] Modules linked in: tpm_tis_spi tpm_tis_core tpm spidev gpio_pca953x mcp320x rtc_pcf2127 industrialio regmap_i2c regmap_spi 8021q garp stp llc ftdi_sio6
Please think hard before including complete backtraces in upstream
reports, they are very large and contain almost no useful information
relative to their size so often obscure the relevant content in your
message. If part of the backtrace is usefully illustrative (it often is
for search engines if nothing else) then it's usually better to pull out
the relevant sections.
Gesendet: Dienstag, 28. September 2021 um 22:08 Uhr
Von: "Mark Brown" [off-list ref]
An: "Lino Sanfilippo" [off-list ref]
Cc: f.fainelli@gmail.com, rjui@broadcom.com, sbranden@broadcom.com, bcm-kernel-feedback-list@broadcom.com, nsaenz@kernel.org, linux-spi@vger.kernel.org, linux-rpi-kernel@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, jgg@ziepe.ca, p.rosenberger@kunbus.com, linux-integrity@vger.kernel.org, stable@vger.kernel.org
Betreff: Re: [PATCH] spi: bcm2835: do not unregister controller in shutdown handler
On Tue, Sep 28, 2021 at 09:56:57PM +0200, Lino Sanfilippo wrote:
quoted
Do not unregister the SPI controller in the shutdown handler. The reason
to avoid this is that controller unregistration results in the slave
devices remove() handler being called which may be unexpected for slave
drivers at system shutdown.
One example is if the BCM2835 driver is used together with the TPM SPI
driver:
At system shutdown first the TPM chip devices (pre) shutdown handler
(tpm_class_shutdown) is called, stopping the chip and setting an operations
pointer to NULL.
Then since the BCM2835 shutdown handler unregisters the SPI controller the
TPM SPI remove function (tpm_tis_spi_remove) is also called. In case of
TPM 2 this function accesses the now nullified operations pointer,
resulting in the following NULL pointer access:
[ 174.078277] 8<--- cut here ---
[ 174.078288] Unable to handle kernel NULL pointer dereference at virtual address 00000034
[ 174.078293] pgd = 557a5fc9
[ 174.078300] [00000034] *pgd=031cf003, *pmd=00000000
[ 174.078317] Internal error: Oops: 206 [#1] SMP ARM
[ 174.078323] Modules linked in: tpm_tis_spi tpm_tis_core tpm spidev gpio_pca953x mcp320x rtc_pcf2127 industrialio regmap_i2c regmap_spi 8021q garp stp llc ftdi_sio6
Please think hard before including complete backtraces in upstream
reports, they are very large and contain almost no useful information
relative to their size so often obscure the relevant content in your
message. If part of the backtrace is usefully illustrative (it often is
for search engines if nothing else) then it's usually better to pull out
the relevant sections.
Thank you for the feedback, I will omit the stack trace in the next version.
Regards,
Lino
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Mark Brown <broonie@kernel.org> Date: 2021-10-01 17:55:15
On Tue, Sep 28, 2021 at 09:56:57PM +0200, Lino Sanfilippo wrote:
One example is if the BCM2835 driver is used together with the TPM SPI
driver:
At system shutdown first the TPM chip devices (pre) shutdown handler
(tpm_class_shutdown) is called, stopping the chip and setting an operations
pointer to NULL.
Then since the BCM2835 shutdown handler unregisters the SPI controller the
TPM SPI remove function (tpm_tis_spi_remove) is also called. In case of
TPM 2 this function accesses the now nullified operations pointer,
resulting in the following NULL pointer access:
This is a bug in that driver, it should be able to cope with a race
between a removal (which might be triggered for some other reason) and a
shutdown. Obviously this is actively triggered by this code path but it
could happen via some other mechanism.
It is not at all clear to me that it is safe to deallocate the DMA
resources the controller is using without first releasing the
controller, I don't see what's stopping something coming along and
submitting new transactions which could in turn try to start doing
DMA.
On Tue, Sep 28, 2021 at 09:56:57PM +0200, Lino Sanfilippo wrote:
quoted
One example is if the BCM2835 driver is used together with the TPM SPI
driver:
At system shutdown first the TPM chip devices (pre) shutdown handler
(tpm_class_shutdown) is called, stopping the chip and setting an operations
pointer to NULL.
Then since the BCM2835 shutdown handler unregisters the SPI controller the
TPM SPI remove function (tpm_tis_spi_remove) is also called. In case of
TPM 2 this function accesses the now nullified operations pointer,
resulting in the following NULL pointer access:
This is a bug in that driver, it should be able to cope with a race
between a removal (which might be triggered for some other reason) and a
shutdown. Obviously this is actively triggered by this code path but it
could happen via some other mechanism.
It is not at all clear to me that it is safe to deallocate the DMA
resources the controller is using without first releasing the
controller, I don't see what's stopping something coming along and
submitting new transactions which could in turn try to start doing
DMA.
I see your point here. So what about narrowing down the shutdown handler
to only disable the hardware:
static void bcm2835_spi_shutdown(struct platform_device *pdev)
{
struct spi_controller *ctlr = platform_get_drvdata(pdev);
struct bcm2835_spi *bs = spi_controller_get_devdata(ctlr);
if (ctlr->dma_tx)
dmaengine_terminate_sync(ctlr->dma_tx);
if (ctlr->dma_rx)
dmaengine_terminate_sync(ctlr->dma_rx);
/* Clear FIFOs, and disable the HW block */
bcm2835_wr(bs, BCM2835_SPI_CS,
BCM2835_SPI_CS_CLEAR_RX | BCM2835_SPI_CS_CLEAR_TX);
clk_disable_unprepare(bs->clk);
}
Regards,
Lino
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Mark Brown <broonie@kernel.org> Date: 2021-10-04 12:49:28
On Sun, Oct 03, 2021 at 05:25:47PM +0200, Lino Sanfilippo wrote:
I see your point here. So what about narrowing down the shutdown handler
to only disable the hardware:
static void bcm2835_spi_shutdown(struct platform_device *pdev)
{
struct spi_controller *ctlr = platform_get_drvdata(pdev);
struct bcm2835_spi *bs = spi_controller_get_devdata(ctlr);
if (ctlr->dma_tx)
dmaengine_terminate_sync(ctlr->dma_tx);
if (ctlr->dma_rx)
dmaengine_terminate_sync(ctlr->dma_rx);
/* Clear FIFOs, and disable the HW block */
bcm2835_wr(bs, BCM2835_SPI_CS,
BCM2835_SPI_CS_CLEAR_RX | BCM2835_SPI_CS_CLEAR_TX);
clk_disable_unprepare(bs->clk);
}
This still leaves a potential race where something (eg, an interrupt
handler) could come in and try to schedule more SPI transfers on the
shut down hardware. I'm really not sure we can do something that's
totally robust here without also ensuring that all the client drivers
also have effective shutdown implementations (which seems ambitious) or
doing what we have now and unregistering the clients. I am, however,
wondering if we really need the shutdown callback at all - the commit
adding it just describes what it's doing, it doesn't explain why it's
particularly needed. I guess there might be an issue on reboot with
reset not completely resetting the hardware?
From: Jason Gunthorpe <jgg@ziepe.ca> Date: 2021-10-04 13:48:30
On Mon, Oct 04, 2021 at 01:49:21PM +0100, Mark Brown wrote:
This still leaves a potential race where something (eg, an interrupt
handler) could come in and try to schedule more SPI transfers on the
shut down hardware. I'm really not sure we can do something that's
totally robust here without also ensuring that all the client drivers
also have effective shutdown implementations (which seems ambitious) or
doing what we have now and unregistering the clients. I am, however,
wondering if we really need the shutdown callback at all - the commit
adding it just describes what it's doing, it doesn't explain why it's
particularly needed. I guess there might be an issue on reboot with
reset not completely resetting the hardware?
Shutdown is supposed to quiet the HW so it is not doing DMAs any
more. This is basically an 'emergency' kind of path, the HW should be
violently stopped if available - ie clearing the bus master bits on
PCI, for instance.
When something like kexec happens we need the machine to be in a state
where random DMA's are not corrupting memory.
Due to the emergency sort of nature it is not appropriate to do
locking complicated sorts of things like struct device unregistrations
here.
Jason
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Mark Brown <broonie@kernel.org> Date: 2021-10-04 14:12:26
On Mon, Oct 04, 2021 at 10:17:56AM -0300, Jason Gunthorpe wrote:
Shutdown is supposed to quiet the HW so it is not doing DMAs any
more. This is basically an 'emergency' kind of path, the HW should be
violently stopped if available - ie clearing the bus master bits on
PCI, for instance.
When something like kexec happens we need the machine to be in a state
where random DMA's are not corrupting memory.
That's all well and good but there's no point in implementing something
half baked that's opening up a whole bunch of opportunities to crash the
system if more work comes in after it's half broken the device setup.
Due to the emergency sort of nature it is not appropriate to do
locking complicated sorts of things like struct device unregistrations
here.
That's just not what's actually implemented in a bunch of places, nor
something one would infer from the documentation ("Called at shut-down
to quiesce the device", no mention of emergency cases which I'd guess
would just be kdump) - there's a bunch of locks in shutdown paths, and
drivers on sleeping buses with shutdown callbacks. Never mind the few
of them that use a shutdown callback to power the system down, though
that's a different thing and definitely abusing the API. I would guess
that a good proportion of people implementing it are more worried about
clean system shutdown than they are about kdump.
From: Jason Gunthorpe <jgg@ziepe.ca> Date: 2021-10-04 15:44:41
On Mon, Oct 04, 2021 at 03:12:20PM +0100, Mark Brown wrote:
On Mon, Oct 04, 2021 at 10:17:56AM -0300, Jason Gunthorpe wrote:
quoted
Shutdown is supposed to quiet the HW so it is not doing DMAs any
more. This is basically an 'emergency' kind of path, the HW should be
violently stopped if available - ie clearing the bus master bits on
PCI, for instance.
quoted
When something like kexec happens we need the machine to be in a state
where random DMA's are not corrupting memory.
That's all well and good but there's no point in implementing something
half baked that's opening up a whole bunch of opportunities to crash the
system if more work comes in after it's half broken the device setup.
Well, that is up to the driver implementing this. It looks like device
shutdown is called before the userspace is all nuked so yes,
concurrency with userspace is a possible concern here.
quoted
Due to the emergency sort of nature it is not appropriate to do
locking complicated sorts of things like struct device unregistrations
here.
That's just not what's actually implemented in a bunch of places, nor
something one would infer from the documentation ("Called at shut-down
to quiesce the device", no mention of emergency cases which I'd guess
would just be kdump) -
Drivers mis understanding stuff is not new..
that's a different thing and definitely abusing the API. I would guess
that a good proportion of people implementing it are more worried about
clean system shutdown than they are about kdump.
The other important case is to get the device cleaned up enough to
pass back to firmware for platforms that use a firmware
shutdown/reboot path.
Jason
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Mark Brown <broonie@kernel.org> Date: 2021-10-04 16:31:26
On Mon, Oct 04, 2021 at 12:44:36PM -0300, Jason Gunthorpe wrote:
On Mon, Oct 04, 2021 at 03:12:20PM +0100, Mark Brown wrote:
quoted
On Mon, Oct 04, 2021 at 10:17:56AM -0300, Jason Gunthorpe wrote:
quoted
quoted
When something like kexec happens we need the machine to be in a state
where random DMA's are not corrupting memory.
quoted
That's all well and good but there's no point in implementing something
half baked that's opening up a whole bunch of opportunities to crash the
system if more work comes in after it's half broken the device setup.
Well, that is up to the driver implementing this. It looks like device
shutdown is called before the userspace is all nuked so yes,
concurrency with userspace is a possible concern here.
It's not just userspace that can initiate things - interrupts are also
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.
quoted
quoted
Due to the emergency sort of nature it is not appropriate to do
locking complicated sorts of things like struct device unregistrations
here.
quoted
That's just not what's actually implemented in a bunch of places, nor
something one would infer from the documentation ("Called at shut-down
to quiesce the device", no mention of emergency cases which I'd guess
would just be kdump) -
Drivers mis understanding stuff is not new..
Not just drivers, entire subsystems. And like I say given the
documentation I'd be hard pressed to say that it's a misunderstanding.
quoted
that's a different thing and definitely abusing the API. I would guess
that a good proportion of people implementing it are more worried about
clean system shutdown than they are about kdump.
The other important case is to get the device cleaned up enough to
pass back to firmware for platforms that use a firmware
shutdown/reboot path.
Right, so the other cases I'm aware of are doing pretty much that -
bringing things down to a state where the system can reboot cleanly.
That can definitely include things like blocking for some hardware, and
you're going to need some concurrency handling which means a combination
of locking and infrequently tested lockless code paths.
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.
On Mon, Oct 04, 2021 at 12:44:36PM -0300, Jason Gunthorpe wrote:
quoted
On Mon, Oct 04, 2021 at 03:12:20PM +0100, Mark Brown wrote:
quoted
On Mon, Oct 04, 2021 at 10:17:56AM -0300, Jason Gunthorpe wrote:
quoted
quoted
quoted
When something like kexec happens we need the machine to be in a state
where random DMA's are not corrupting memory.
quoted
quoted
That's all well and good but there's no point in implementing something
half baked that's opening up a whole bunch of opportunities to crash the
system if more work comes in after it's half broken the device setup.
quoted
Well, that is up to the driver implementing this. It looks like device
shutdown is called before the userspace is all nuked so yes,
concurrency with userspace is a possible concern here.
It's not just userspace that can initiate things - interrupts are also
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.
quoted
quoted
quoted
Due to the emergency sort of nature it is not appropriate to do
locking complicated sorts of things like struct device unregistrations
here.
quoted
quoted
That's just not what's actually implemented in a bunch of places, nor
something one would infer from the documentation ("Called at shut-down
to quiesce the device", no mention of emergency cases which I'd guess
would just be kdump) -
quoted
Drivers mis understanding stuff is not new..
Not just drivers, entire subsystems. And like I say given the
documentation I'd be hard pressed to say that it's a misunderstanding.
quoted
quoted
that's a different thing and definitely abusing the API. I would guess
that a good proportion of people implementing it are more worried about
clean system shutdown than they are about kdump.
quoted
The other important case is to get the device cleaned up enough to
pass back to firmware for platforms that use a firmware
shutdown/reboot path.
Right, so the other cases I'm aware of are doing pretty much that -
bringing things down to a state where the system can reboot cleanly.
That can definitely include things like blocking for some hardware, and
you're going to need some concurrency handling which means a combination
of locking and infrequently tested lockless code paths.
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.
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.
--
Florian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Jason Gunthorpe <jgg@ziepe.ca> Date: 2021-10-04 16:51:33
On Mon, Oct 04, 2021 at 09:36:37AM -0700, Florian Fainelli wrote:
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.
Huh? for device shutdown? What would this matter if the next step is
reboot or power off?
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.
The difference between remove and shutdown really is that 'emergency'
sense that shutdown is something that must complete in bounded time
and thus only has to concern itself with quieting hardware to a safe
state for the next step in the shutdown/reboot/kexec/kdump sequence.
Many remove handlers happily block until, eg all user files are closed
or something to allow a graceful module unload.
Jason
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Mark Brown <broonie@kernel.org> Date: 2021-10-04 16:52:11
On Mon, Oct 04, 2021 at 09:36:37AM -0700, Florian Fainelli wrote:
On 10/4/21 9:31 AM, Mark Brown wrote:
quoted
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.
quoted
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.
On Mon, Oct 04, 2021 at 09:36:37AM -0700, Florian Fainelli wrote:
quoted
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.
Huh? for device shutdown? What would this matter if the next step is
reboot or power off?
Power off, the device is put into a low power state (equivalent to ACPI
S5) and then a remote control key press, or a GPIO could wake-up the
device again. While it is in that mode, it consumes less than 0.5W(AC).
Imagine your stick/cast/broom behind your TV falling in that category.
quoted
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.
The difference between remove and shutdown really is that 'emergency'
sense that shutdown is something that must complete in bounded time
and thus only has to concern itself with quieting hardware to a safe
state for the next step in the shutdown/reboot/kexec/kdump sequence.
I am fairly sure that no driver write knows about the being bound in
time aspect.
Many remove handlers happily block until, eg all user files are closed
or something to allow a graceful module unload.
On Mon, Oct 04, 2021 at 09:36:37AM -0700, Florian Fainelli wrote:
quoted
On 10/4/21 9:31 AM, Mark Brown wrote:
quoted
quoted
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.
quoted
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.
quoted
quoted
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.
quoted
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.
quoted
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.
From: Mark Brown <broonie@kernel.org> Date: 2021-10-04 17:05:31
On Mon, Oct 04, 2021 at 01:51:27PM -0300, Jason Gunthorpe wrote:
On Mon, Oct 04, 2021 at 09:36:37AM -0700, Florian Fainelli wrote:
quoted
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.
Huh? for device shutdown? What would this matter if the next step is
reboot or power off?
On some embedded systems, especially ultra low cost ones, the system
power off state might not actually involve removing all the physical
power supplies for the all the chips in the system so any residual
leakages or active functions will continue to consume power.
Ideally the system power on/off will be triggered by a PMIC which is
able to physically remove power to most other parts of the system which
avoids this issue (much like the PSU in a server) but that's not always
the case.
From: Jason Gunthorpe <jgg@ziepe.ca> Date: 2021-10-04 17:13:06
On Mon, Oct 04, 2021 at 09:55:32AM -0700, Florian Fainelli wrote:
On 10/4/21 9:51 AM, Jason Gunthorpe wrote:
quoted
On Mon, Oct 04, 2021 at 09:36:37AM -0700, Florian Fainelli wrote:
quoted
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.
Huh? for device shutdown? What would this matter if the next step is
reboot or power off?
Power off, the device is put into a low power state (equivalent to ACPI
S5) and then a remote control key press, or a GPIO could wake-up the
device again. While it is in that mode, it consumes less than 0.5W(AC).
Imagine your stick/cast/broom behind your TV falling in that category.
So really this is more of a very deep sleep that cannot be recovered
from than what other platforms would call a shutdown - eg the
powerdomain of the device under driver control will not loose
power.
I'm kind of surprised a scheme like this didn't involve a FW call
after Linux is done with the CPUs to quiet all the HW and let it
sleep, I've built things that way before at least.
I am fairly sure that no driver write knows about the being bound in
time aspect.
Well, it is a logical consequence. The system is shutting down, no
driver should be designed to deadlock the shutdown forever.
I suppose this is why I've occasionally seen Linux just hang at a
black screen and no power off when told to shutdown :)
Jason
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Mark Brown <broonie@kernel.org> Date: 2021-10-04 17:27:34
On Mon, Oct 04, 2021 at 02:13:01PM -0300, Jason Gunthorpe wrote:
I'm kind of surprised a scheme like this didn't involve a FW call
after Linux is done with the CPUs to quiet all the HW and let it
sleep, I've built things that way before at least.
That's a *lot* of code to put in firmware if you can't physically power
most of the system down.
From: Jason Gunthorpe <jgg@ziepe.ca> Date: 2021-10-04 17:35:36
On Mon, Oct 04, 2021 at 06:27:29PM +0100, Mark Brown wrote:
On Mon, Oct 04, 2021 at 02:13:01PM -0300, Jason Gunthorpe wrote:
quoted
I'm kind of surprised a scheme like this didn't involve a FW call
after Linux is done with the CPUs to quiet all the HW and let it
sleep, I've built things that way before at least.
That's a *lot* of code to put in firmware if you can't physically power
most of the system down.
Maybe? The chip I worked on we just made a list of register/value
pairs that covered all the functional blocks and the FW ran down the
list.
Mind you the chip was designed that poking ABC to reg 123 turned the
unit off no matter what. It didn't have a complex interactive shutdown
sequence.
Jason
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, Oct 04, 2021 at 02:13:01PM -0300, Jason Gunthorpe wrote:
quoted
I'm kind of surprised a scheme like this didn't involve a FW call
after Linux is done with the CPUs to quiet all the HW and let it
sleep, I've built things that way before at least.
That's a *lot* of code to put in firmware if you can't physically power
most of the system down.
Indeed, and that also assume it may be possible for firmware to have the
last say, which is not necessarily possible (though that ought to be a
system design issue that would need fixing). It seems reasonable to me
to delegate the powering off of the hardware to the respective Linux
drivers since they ought to be in the best position to make appropriate
decisions for the hardware they control.
Anyway, we are divergin slightly here, how do we go about fixing
.shutdown here?
--
Florian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Mark Brown <broonie@kernel.org> Date: 2021-10-04 17:56:09
On Mon, Oct 04, 2021 at 10:44:34AM -0700, Florian Fainelli wrote:
Anyway, we are divergin slightly here, how do we go about fixing
.shutdown here?
Implement something in the core which will stop any new operations being
requested and flush existing ones then update the driver to just do
whatever is needed to turn off the hardware.
Well, that is up to the driver implementing this. It looks like device
shutdown is called before the userspace is all nuked so yes,
concurrency with userspace is a possible concern here.
So the TPM driver has to handle remove() after shutdown() anyway, right?
Because even if not caused by the BCM2835 drivers controller unregistration
something else could unload the module and the problem (NULL pointer access)
would be the same.
Regards,
Lino
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Jason Gunthorpe <jgg@ziepe.ca> Date: 2021-10-04 18:37:51
On Mon, Oct 04, 2021 at 08:30:52PM +0200, Lino Sanfilippo wrote:
On 04.10.21 at 17:44, Jason Gunthorpe wrote:
quoted
Well, that is up to the driver implementing this. It looks like device
shutdown is called before the userspace is all nuked so yes,
concurrency with userspace is a possible concern here.
So the TPM driver has to handle remove() after shutdown() anyway, right?
Because even if not caused by the BCM2835 drivers controller unregistration
something else could unload the module and the problem (NULL pointer access)
would be the same.
Technically yes, remove shouldn't crash in this ordering - but it
should be difficult for remove to be called after shutdown in any
normal system.
Jason
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel