Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 23 Mar 2023 16:45:32 -0600
From:   Mathieu Poirier <mathieu.poirier@linaro.org>
To:     Tanmay Shah <tanmay.shah@amd.com>
Cc:     andersson@kernel.org, linux-remoteproc@vger.kernel.org,
        linux-kernel@vger.kernel.org, Tarak Reddy <tarak.reddy@amd.com>
Subject: Re: [PATCH v2 2/2] remoteproc: enhance rproc_put() for clusters
Message-ID: <20230323224532.GB2905636@p14s>
References: <20230322040933.924813-1-tanmay.shah@amd.com>
 <20230322040933.924813-3-tanmay.shah@amd.com>
 <20230322160537.GB2821487@p14s>
 <830db03c-ec6e-b4aa-834a-e67622e5a41f@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <830db03c-ec6e-b4aa-834a-e67622e5a41f@amd.com>
Precedence: bulk

On Wed, Mar 22, 2023 at 10:34:57AM -0700, Tanmay Shah wrote:
> 
> On 3/22/23 9:05 AM, Mathieu Poirier wrote:
> > Hi Tanmay,
> > 
> > On Tue, Mar 21, 2023 at 09:09:36PM -0700, Tanmay Shah wrote:
> > > This patch enhances rproc_put() to support remoteproc clusters
> > > with multiple child nodes as in rproc_get_by_phandle().
> > > 
> > > Signed-off-by: Tarak Reddy <tarak.reddy@amd.com>
> > > Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> > > ---
> > >   drivers/remoteproc/remoteproc_core.c | 13 +++++++++++++
> > >   1 file changed, 13 insertions(+)
> > > 
> > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > > index a3e7c8798381..e7e451012615 100644
> > > --- a/drivers/remoteproc/remoteproc_core.c
> > > +++ b/drivers/remoteproc/remoteproc_core.c
> > > @@ -2560,6 +2560,19 @@ EXPORT_SYMBOL(rproc_free);
> > >   void rproc_put(struct rproc *rproc)
> > >   {
> > >   	module_put(rproc->dev.parent->driver->owner);
> > There is something wrong here - this should have been removed.
> 
> 
> Thanks Mathieu. Sure this needs to be fixed.
> 
> This is result of manually picking up patch from my side.
> 
> I will try to find better automated way to pick-up patches not available on
> mailing list.
>

That would certainly help avoid problems such as this one.

> 
> > 
> > > +	struct platform_device *cluster_pdev;
> > > +
> > > +	if (rproc->dev.parent) {
> > This condition is not needed, please remove.
> Ack.
> > 
> > > +		if (rproc->dev.parent->driver) {
> > > +			module_put(rproc->dev.parent->driver->owner);
> > > +		} else {
> > > +			cluster_pdev = of_find_device_by_node(rproc->dev.parent->of_node->parent);
> > > +			if (cluster_pdev) {
> > > +				module_put(cluster_pdev->dev.driver->owner);
> > > +				put_device(&cluster_pdev->dev);
> 
> I am not sure if cluster_pdev->dev should be dropped here.
>

It needs to be done here.

> Should we drop it in platform driver after rproc_free() ?
> 
> > > +			}
> > > +		}
> > > +	}
> > Some in-lined documentation, the way I did in patch 1/2 would be appreciated.
> > Otherwize I think the above enhancement make sense.
> Ack I will document in next revision.
> > 
> > Thanks,
> > Mathieu
> > 
> > >   	put_device(&rproc->dev);
> 
> 
> Also, if we decide to drop cluster->dev here? then,
> 
> should we drop reference of rproc->dev before cluster->dev ?
> 
> 
> > >   }
> > >   EXPORT_SYMBOL(rproc_put);
> > > -- 
> > > 2.25.1
> > >