Re: [PATCH 4/8] drm/panthor: Add support for protected memory allocation in panthor

[Boris Brezillon <boris.brezillon@collabora.com>]

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)