Date:   Wed, 25 Jan 2023 10:55:51 -0800
From:   Bjorn Andersson <quic_bjorande@quicinc.com>
To:     Chris Lew <quic_clew@quicinc.com>
CC:     Bjorn Andersson <andersson@kernel.org>,
        Mathieu Poirier <mathieu.poirier@linaro.org>,
        <linux-arm-msm@vger.kernel.org>,
        <linux-remoteproc@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/6] rpmsg: glink: smem: Wrap driver context
Message-ID: <20230125185551.GA2034155@hu-bjorande-lv.qualcomm.com>
References: <20230109224001.1706516-1-quic_bjorande@quicinc.com>
 <20230109224001.1706516-3-quic_bjorande@quicinc.com>
 <21302696-aadc-2005-bb64-ec807e737d1a@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <21302696-aadc-2005-bb64-ec807e737d1a@quicinc.com>
Precedence: bulk

On Tue, Jan 24, 2023 at 10:30:42PM -0800, Chris Lew wrote:
> 
> 
> On 1/9/2023 2:39 PM, Bjorn Andersson wrote:
> > The Glink SMEM driver allocates a struct device and hangs two
> > devres-allocated pipe objects thereon. To facilitate the move of
> > interrupt and mailbox handling to the driver, introduce a wrapper object
> > capturing the device, glink reference and remote processor id.
> > 
> > The type of the remoteproc reference is updated, as these are
> > specifically targetting the SMEM implementation.
> 
> s/targetting/targeting
> 

Thank you.

> > 
> > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > ---
> >   drivers/remoteproc/qcom_common.h |  3 +-
> >   drivers/rpmsg/qcom_glink_smem.c  | 76 ++++++++++++++++++++------------
> >   include/linux/rpmsg/qcom_glink.h | 12 ++---
> >   3 files changed, 55 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/qcom_common.h b/drivers/remoteproc/qcom_common.h
> > index c35adf730be0..2747c7d9ba44 100644
> > --- a/drivers/remoteproc/qcom_common.h
> > +++ b/drivers/remoteproc/qcom_common.h
> > @@ -6,6 +6,7 @@
> >   #include "remoteproc_internal.h"
> >   #include <linux/soc/qcom/qmi.h>
> > +struct qcom_glink_smem;
> >   struct qcom_sysmon;
> >   struct qcom_rproc_glink {
> > @@ -15,7 +16,7 @@ struct qcom_rproc_glink {
> >   	struct device *dev;
> >   	struct device_node *node;
> > -	struct qcom_glink *edge;
> > +	struct qcom_glink_smem *edge;
> >   };
> >   struct qcom_rproc_subdev {
> > diff --git a/drivers/rpmsg/qcom_glink_smem.c b/drivers/rpmsg/qcom_glink_smem.c
> > index 579bc4443f6d..703e63fa5a86 100644
> > --- a/drivers/rpmsg/qcom_glink_smem.c
> > +++ b/drivers/rpmsg/qcom_glink_smem.c
> > @@ -33,6 +33,14 @@
> >   #define SMEM_GLINK_NATIVE_XPRT_FIFO_0		479
> >   #define SMEM_GLINK_NATIVE_XPRT_FIFO_1		480
> > +struct qcom_glink_smem {
> > +	struct device dev;
> > +
> > +	struct qcom_glink *glink;
> > +
> > +	u32 remote_pid;
> > +};
> > +
> >   struct glink_smem_pipe {
> >   	struct qcom_glink_pipe native;
> > @@ -41,7 +49,7 @@ struct glink_smem_pipe {
> >   	void *fifo;
> > -	int remote_pid;
> > +	struct qcom_glink_smem *smem;
> >   };
> >   #define to_smem_pipe(p) container_of(p, struct glink_smem_pipe, native)
> > @@ -49,13 +57,14 @@ struct glink_smem_pipe {
> >   static size_t glink_smem_rx_avail(struct qcom_glink_pipe *np)
> >   {
> >   	struct glink_smem_pipe *pipe = to_smem_pipe(np);
> > +	struct qcom_glink_smem *smem = pipe->smem;
> >   	size_t len;
> >   	void *fifo;
> >   	u32 head;
> >   	u32 tail;
> >   	if (!pipe->fifo) {
> > -		fifo = qcom_smem_get(pipe->remote_pid,
> > +		fifo = qcom_smem_get(smem->remote_pid,
> >   				     SMEM_GLINK_NATIVE_XPRT_FIFO_1, &len);
> >   		if (IS_ERR(fifo)) {
> >   			pr_err("failed to acquire RX fifo handle: %ld\n",
> > @@ -179,45 +188,49 @@ static void glink_smem_tx_write(struct qcom_glink_pipe *glink_pipe,
> >   static void qcom_glink_smem_release(struct device *dev)
> >   {
> > -	kfree(dev);
> > +	struct qcom_glink_smem *smem = container_of(dev, struct qcom_glink_smem, dev);
> > +
> > +	kfree(smem);
> >   }
> > -struct qcom_glink *qcom_glink_smem_register(struct device *parent,
> > -					    struct device_node *node)
> > +struct qcom_glink_smem *qcom_glink_smem_register(struct device *parent,
> > +						 struct device_node *node)
> >   {
> >   	struct glink_smem_pipe *rx_pipe;
> >   	struct glink_smem_pipe *tx_pipe;
> >   	struct qcom_glink *glink;
> > -	struct device *dev;
> > +	struct qcom_glink_smem *smem;
> 
> I think we're following reverse christmas tree in this file
> 
> >   	u32 remote_pid;
> >   	__le32 *descs;
> >   	size_t size;
> >   	int ret;
> > -	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> > -	if (!dev)
> > +	smem = kzalloc(sizeof(*smem), GFP_KERNEL);
> > +	if (!smem)
> >   		return ERR_PTR(-ENOMEM);
> > 
> 
> Would it be proper to keep a pointer to dev and avoid all the changes to
> smem->dev use?
> 
> dev = &smem->dev;
> 

That seems reasonable. Will respin accordingly.

Thanks,
Bjorn