linux-kernel - Re: [PATCH] remoteproc: move rproc_da_to

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] [day] [month] [year] [list]

Message-ID: <YiuQAvnbBIdWhy2l@builder.lan>
Date:   Fri, 11 Mar 2022 12:08:02 -0600
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Drew Fustini <dfustini@...libre.com>
Cc:     Mathieu Poirier <mathieu.poirier@...aro.org>,
        linux-remoteproc@...r.kernel.org, linux-kernel@...r.kernel.org,
        Suman Anna <s-anna@...com>, Dave Gerlach <d-gerlach@...com>
Subject: Re: [PATCH] remoteproc: move rproc_da_to_va declaration to
 remoteproc.h

On Wed 09 Mar 11:04 CST 2022, Drew Fustini wrote:

> On Tue, Mar 08, 2022 at 10:16:54AM -0800, Bjorn Andersson wrote:
> > On Tue 08 Mar 09:25 PST 2022, Drew Fustini wrote:
> > 
> > > From: Suman Anna <s-anna@...com>
> > > 
> > > The rproc_da_to_va() API is an exported function, so move its
> > > declaration from the remoteproc local remoteproc_internal.h
> > > to the public remoteproc.h file.
> > > 
> > > This will allow drivers outside of the remoteproc folder to be
> > > able to use this API.
> > > 
> > 
> > Can you explain why drivers outside of the remoteproc folder should be
> > able to poke straight into the memory of the remoteproc?
> > 
> > Your reasoning makes sense, but we've on purpose kept it out of
> > remoteproc.h because no one has had a proper reason for it and I sense
> > that we might open the door for some new creative solutions...
> 
> rproc_da_to_va() is used in a patch for drivers/soc/ti/wkup_m3_ipc.c
> that adds support for i2c voltage scaling [1].
> 
> wkup_m3_copy_aux_data() will copy auxiliary data to special region of
> the Cortex M3 memory. It calls rproc_da_to_va() to get aux_data_addr
> which is then used as a memcpy destination.
> 

So in essence it's an essential part for the "communication protocol"
used to communicate with the remoteproc...

> Does that seem like a reasonable way to do it?
> 

I have a concern about the life cycle of the pointer acquired by this
"independent" driver. But this is an extension of my existing concern
where the wkup driver uses the remoteproc core as a "firmware loader",
but it's not a standalone remoteproc driver.

I think it would have been nicer to model the remoteproc driver as the
parent of the wkup device, so we probe/remove the wkup device based on
the state of the remoteproc.

This would remove concerns about races between the remoteproc
starting/stopping/restarting and the other driver and it would help
clarify that the life cycle of the pointer returned by rproc_da_to_va()
lives from start to stop of the remoteproc.

This does however not change the need for exporting the symbol, so I'm
merging this patch.

Regards,
Bjorn

> I was going to submit the i2c voltage scaling patches later. However,
> I could combine them into a series with this remoteproc patch if that
> helps to justify the remoteproc.h change.
> 
> Thanks,
> Drew
> 
> [1] https://lore.kernel.org/linux-omap/20220219215328.485660-9-dfustini@baylibre.com/