On Thu, 7 May 2026 11:02:26 +0200 Marcin Ślusarz marcin.slusarz@arm.com wrote:
On Tue, May 05, 2026 at 06:15:23PM +0200, Boris Brezillon wrote:
@@ -277,9 +286,21 @@ int panthor_device_init(struct panthor_device *ptdev) return ret; }
- /* If a protected heap name is specified but not found, defer the probe until created */
- if (protected_heap_name && strlen(protected_heap_name)) {
Do we really need this strlen() > 0? Won't dma_heap_find() fail is the name is "" already?
If dma_heap_find() will fail, then the whole probe with fail too. This check prevents that.
Yeah, that's also a questionable design choice. I mean, we can currently probe and boot the FW even though we never setup the protected FW sections, so why should we defer the probe here? Can't we just retry the next time a group with the protected bit is created and fail if we can find a protected heap?
I'm not sure why it's needed at all, but if it is really needed, then s/strlen(protected_heap_name)/protected_heap_name[0]/ would simplify this.
It's not so much about how you do the test, and more about the case you're trying to protect against. I guess here you assume that panthor.protected_heap_name="" means "I don't have a protected heap for you". If it's deemed acceptable, this should most certainly be described somewhere.
ptdev->protm.heap = dma_heap_find(protected_heap_name);if (!ptdev->protm.heap) {drm_warn(&ptdev->base,"Protected heap \'%s\' not (yet) available - deferring probe",protected_heap_name);ret = -EPROBE_DEFER;goto err_rpm_put;If you move the heap retrieval before the rpm enablement, you can get rid of this goto err_rpm_put.
}- }
- ret = panthor_hw_init(ptdev); if (ret)
goto err_rpm_put;
goto err_dma_heap_put;ret = panthor_pwr_init(ptdev); if (ret)
On Thu, May 07, 2026 at 01:53:56PM +0200, Boris Brezillon wrote:
On Thu, 7 May 2026 11:02:26 +0200 Marcin Ślusarz marcin.slusarz@arm.com wrote:
On Tue, May 05, 2026 at 06:15:23PM +0200, Boris Brezillon wrote:
@@ -277,9 +286,21 @@ int panthor_device_init(struct panthor_device *ptdev) return ret; }
- /* If a protected heap name is specified but not found, defer the probe until created */
- if (protected_heap_name && strlen(protected_heap_name)) {
Do we really need this strlen() > 0? Won't dma_heap_find() fail is the name is "" already?
If dma_heap_find() will fail, then the whole probe with fail too. This check prevents that.
Yeah, that's also a questionable design choice. I mean, we can currently probe and boot the FW even though we never setup the protected FW sections, so why should we defer the probe here? Can't we just retry the next time a group with the protected bit is created and fail if we can find a protected heap?
The problem we have with the current firmware is that it does a number of setup steps at "boot" time only. One of the steps is preparing its internal structures for when it enters protected mode and it stores them in the buffer passed in at firmware loading. We cannot later run the process when we have a group with protected mode set.
So unfortunately adding support for protected mode where the heap name is provided means we have to try our best to set it up at boot time, or otherwise disable protected mode support.
Best regards, Liviu
I'm not sure why it's needed at all, but if it is really needed, then s/strlen(protected_heap_name)/protected_heap_name[0]/ would simplify this.
It's not so much about how you do the test, and more about the case you're trying to protect against. I guess here you assume that panthor.protected_heap_name="" means "I don't have a protected heap for you". If it's deemed acceptable, this should most certainly be described somewhere.
ptdev->protm.heap = dma_heap_find(protected_heap_name);if (!ptdev->protm.heap) {drm_warn(&ptdev->base,"Protected heap \'%s\' not (yet) available - deferring probe",protected_heap_name);ret = -EPROBE_DEFER;goto err_rpm_put;If you move the heap retrieval before the rpm enablement, you can get rid of this goto err_rpm_put.
}- }
- ret = panthor_hw_init(ptdev); if (ret)
goto err_rpm_put;
goto err_dma_heap_put;ret = panthor_pwr_init(ptdev); if (ret)
On Tue, 12 May 2026 14:47:27 +0100 Liviu Dudau liviu.dudau@arm.com wrote:
On Thu, May 07, 2026 at 01:53:56PM +0200, Boris Brezillon wrote:
On Thu, 7 May 2026 11:02:26 +0200 Marcin Ślusarz marcin.slusarz@arm.com wrote:
On Tue, May 05, 2026 at 06:15:23PM +0200, Boris Brezillon wrote:
@@ -277,9 +286,21 @@ int panthor_device_init(struct panthor_device *ptdev) return ret; }
- /* If a protected heap name is specified but not found, defer the probe until created */
- if (protected_heap_name && strlen(protected_heap_name)) {
Do we really need this strlen() > 0? Won't dma_heap_find() fail is the name is "" already?
If dma_heap_find() will fail, then the whole probe with fail too. This check prevents that.
Yeah, that's also a questionable design choice. I mean, we can currently probe and boot the FW even though we never setup the protected FW sections, so why should we defer the probe here? Can't we just retry the next time a group with the protected bit is created and fail if we can find a protected heap?
The problem we have with the current firmware is that it does a number of setup steps at "boot" time only. One of the steps is preparing its internal structures for when it enters protected mode and it stores them in the buffer passed in at firmware loading. We cannot later run the process when we have a group with protected mode set.
No, but we can force a full/slow reset and have that thing re-initialized, can't we? I mean, that's basically what we do when a fast reset fails: we re-initialize all the sections and reset again, at which point the FW should start from a fresh state, and be able to properly initialize the protected-related stuff if protected sections are populated. Am I missing something?
On Tue, May 12, 2026 at 04:11:11PM +0200, Boris Brezillon wrote:
On Tue, 12 May 2026 14:47:27 +0100 Liviu Dudau liviu.dudau@arm.com wrote:
On Thu, May 07, 2026 at 01:53:56PM +0200, Boris Brezillon wrote:
On Thu, 7 May 2026 11:02:26 +0200 Marcin Ślusarz marcin.slusarz@arm.com wrote:
On Tue, May 05, 2026 at 06:15:23PM +0200, Boris Brezillon wrote:
@@ -277,9 +286,21 @@ int panthor_device_init(struct panthor_device *ptdev) return ret; }
- /* If a protected heap name is specified but not found, defer the probe until created */
- if (protected_heap_name && strlen(protected_heap_name)) {
Do we really need this strlen() > 0? Won't dma_heap_find() fail is the name is "" already?
If dma_heap_find() will fail, then the whole probe with fail too. This check prevents that.
Yeah, that's also a questionable design choice. I mean, we can currently probe and boot the FW even though we never setup the protected FW sections, so why should we defer the probe here? Can't we just retry the next time a group with the protected bit is created and fail if we can find a protected heap?
The problem we have with the current firmware is that it does a number of setup steps at "boot" time only. One of the steps is preparing its internal structures for when it enters protected mode and it stores them in the buffer passed in at firmware loading. We cannot later run the process when we have a group with protected mode set.
No, but we can force a full/slow reset and have that thing re-initialized, can't we? I mean, that's basically what we do when a fast reset fails: we re-initialize all the sections and reset again, at which point the FW should start from a fresh state, and be able to properly initialize the protected-related stuff if protected sections are populated. Am I missing something?
Right, we can do that. For some reason I keep associating the reset with the error handling and not with "normal" operations.
Best regards, Liviu
linaro-mm-sig@lists.linaro.org
-
Boris Brezillon -
Liviu Dudau