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;
Message-ID: <830db03c-ec6e-b4aa-834a-e67622e5a41f@amd.com>
Date:   Wed, 22 Mar 2023 10:34:57 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:111.0)
 Gecko/20100101 Thunderbird/111.0
Subject: Re: [PATCH v2 2/2] remoteproc: enhance rproc_put() for clusters
Content-Language: en-US
To:     Mathieu Poirier <mathieu.poirier@linaro.org>
Cc:     andersson@kernel.org, linux-remoteproc@vger.kernel.org,
        linux-kernel@vger.kernel.org, Tarak Reddy <tarak.reddy@amd.com>
References: <20230322040933.924813-1-tanmay.shah@amd.com>
 <20230322040933.924813-3-tanmay.shah@amd.com> <20230322160537.GB2821487@p14s>
From:   Tanmay Shah <tanmay.shah@amd.com>
In-Reply-To: <20230322160537.GB2821487@p14s>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Skhuc1lsMGJWWWlraVhHS0hTYzlMSjZWWG4rRlpRdGhnS1cvT3Q0bjEzaWpI?=
 =?utf-8?B?ZlhFTCtlcXJxdmxDVnh0YTZGTUQwY0w3TWRhWnZ5VUdqbm9nYkV5cFg3akEy?=
 =?utf-8?B?am9yYXVhUVk3QmxhZW9naTRjbmF0ZTI1VmVCQzROell3eElEYTBqZU94VWhH?=
 =?utf-8?B?aURZT0tYcFc2N0JmZWt5dHp3b0pqdUFzSk9QSTN1Uy9SYXdTQ2VTZDhZbFdT?=
 =?utf-8?B?RWhJQjkyUHVPZzI3NzZrYS9aRjVqU1RGdXVjU2VuODhLUmgwZXA4cG1XRkxJ?=
 =?utf-8?B?eDVKampCbVZBVThtNUZrenFkcXcyMnQxcTJyZEhLckM0SVprVlZSZHVTeDE2?=
 =?utf-8?B?SnV1RG9KSVBBb2wrNFNEVyswWDRSMGZES3B3azVXb2F5REw3ampIY1k1Y25o?=
 =?utf-8?B?b3hHMkdnRHMrdzBmWUpmanFNQWQ2T0Y0cDRBWlZibnNBZldOa3dLRmFBdkEx?=
 =?utf-8?B?bGc2aTIyVEV2ekRDcjhLUTVaUC9XMGpTd3BhYmdudTRyNm5OMHR3dklDTlRq?=
 =?utf-8?B?a2tjM00rbzliRHhydGUxRklnSUdZTjFHZkJCWk9hMU5RTGF0OTlObnhGTGVV?=
 =?utf-8?B?bFhDNXlxTWhiZ2xhK05lRVE4dFkwb3JaMlNPbkZ6aW1iK2pMUHBGb0Q5UTll?=
 =?utf-8?B?LzZiSWZpSk8vR0FBL1h1MUw0ZE5lbkJjT1VodS9kOUlsMW9Qb3hUUUhleCti?=
 =?utf-8?B?KzRZSUdtaTNMZStQcStCemk3QlcwNWFRRUw0eWtqQnBkemswbHlpNW9JZzRj?=
 =?utf-8?B?WXkrUVljNHFTOTZvSG5Pam40SXhaWVhPTkREYUwxNzZtcE9ZV0hSeXBkbXZs?=
 =?utf-8?B?U0RNdWNRM21nL3hMZ2tPOFBlQmhRd1J6SWJPYXhVYzlaV0dsMmFzTmZ3NVNR?=
 =?utf-8?B?NUpvUk1nbWlkckRDSnA1VTRHb0J0d1Q2OHd4OWxVS2JiMldLL01BRHdlYUFu?=
 =?utf-8?B?UTY0YTdTZmhyWTdDT0hhNk8vWkRHaHZiaFVrVnU2NTAzZStteUl0YUtHVk16?=
 =?utf-8?B?ZjVDQ3NPM1VqSi9hekVVcHZ5QVNRZWRzUkx2VWd3QlowSnNWd0pvZVRUNWJj?=
 =?utf-8?B?Uk00RDJNUkRIR2UxaG03N3F6MU5HaXA3b3VybjhyWGtwaGNxTC9pN3FZa2xZ?=
 =?utf-8?B?UVFEOFJ5ZWJhRkovK1Y2cVMvOU53cnpjekdtNDlqL09vUXRWT0NpTnFFZURL?=
 =?utf-8?B?andKN0EyL0RIdDZKNlBVaW9lb0VHM2JTK3RSdjFlMG5LU1BOWVUxeVFHWisw?=
 =?utf-8?B?ZkE3YUZDTmg1L2RWaVdjSEpSdmNodGp6aC90bDdxcFBBdnlwZlV3dWhYK0p0?=
 =?utf-8?B?Q0wxVm5leEJtbi8vNjFBVzZrWGpBR0VhenFHUmFsbnNNYlQ0QWFUZWxQUzA1?=
 =?utf-8?B?WFQxbVlKVk1jeTh1ZmdXWnhHYWRFTjFFMzI1bEwwY21KUTBZcXR4WlUvc0Nu?=
 =?utf-8?B?empiWjhBSFNpVVdhSGxsU1NRRXA5RjlhOWFvL0RjYjYxTUt5cmtKSVBPdExL?=
 =?utf-8?B?N1FRMlUrOWo1T0g0SXRDQ3haTWdZMnJ1SU4wa25PWGtsS3lZTzVpYU1IeFRq?=
 =?utf-8?B?ckJsVWRLeHF3dUE0cUhvcWh6UXhQOVBNSGtGMlF6bGY5TDJDUzZrYXpBbVZR?=
 =?utf-8?B?cllRSGMyN0tId3BySGs1LzdzeFFFT1hKSVIvSktNNldjcHB3Nk1Sc0lsdlYw?=
 =?utf-8?B?N3ZYcktRR2x6MHpveHlqVW5YdURIQlJCL1FRNDhRK0pYaWhsNmUxamdTL2Ey?=
 =?utf-8?B?MzRCamdlUzh0WG9tclY4ei9FTERxa2hRQU5uOGU2V0Z3eDVvaGVOUGh5Uklm?=
 =?utf-8?B?a3ZCdml6QWZ1dlRIcTh1eEpZZ3RYWlJYRU93cGJMNFFBVEQxM0QxWDI3UVBL?=
 =?utf-8?B?aEo4L1gxNGlrM3ZHbXlFZGIvTkRHUGY4OUFvanV6T0ZRNkRjMVVERWVUN2pH?=
 =?utf-8?B?NE80SXE3WGM1eW5oNEZWNlBFdHBHTXVPbjVRaVBYY1FLam9UeURkcXkwbDdj?=
 =?utf-8?B?UzIyZmVjNHN0RUJ5Q1NjdjFjclZQR0JPN2pDYlhqMGR5S0NyTStUcWVMblNm?=
 =?utf-8?B?VWxzSWNkRGM4LzFKOUMwNW9ZWFlBSnowaWptNC9iSis5TGV2cC9Dc3Y4SDNZ?=
 =?utf-8?Q?cLSPv7lAmR8SNDMFQoznuO2NJ?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 432d18c6-0370-4c77-47ff-08db2afbc28c
X-MS-Exchange-CrossTenant-AuthSource: BY5PR12MB3683.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Mar 2023 17:34:59.4219
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: tPWpFfHAq9Svz7pLpkc7dLLb0FriJz3svCXvIsJVgn/pxVp3BQLAe7qyKd/tXRYS
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR12MB4085
X-Spam-Status: No, score=-0.2 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
        DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,NICE_REPLY_A,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE,
        URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


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.


>
>> +	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.

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
>>