Received-SPF: pass (google.com: domain of linux-kernel+bounces-163266-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <be54f273-7bba-4db0-bc52-5ddbb3982d84@oss.nxp.com>
Date: Tue, 30 Apr 2024 10:32:58 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 3/3] media: amphion: Remove lock in s_ctrl callback
To: Andrzej Pietrasiewicz <andrzej.p@collabora.com>,
 Ming Qian <ming.qian@nxp.com>, mchehab@kernel.org, hverkuil-cisco@xs4all.nl
Cc: shawnguo@kernel.org, robh+dt@kernel.org, s.hauer@pengutronix.de,
 kernel@pengutronix.de, festevam@gmail.com, linux-imx@nxp.com,
 xiahong.bao@nxp.com, eagle.zhou@nxp.com, tao.jiang_2@nxp.com,
 imx@lists.linux.dev, linux-media@vger.kernel.org,
 linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org
References: <20240425065011.105915-1-ming.qian@nxp.com>
 <20240425065011.105915-3-ming.qian@nxp.com>
 <8c39b3c3-8146-4418-8835-6dbfe38a85ec@collabora.com>
Content-Language: en-US
From: ming qian <ming.qian@oss.nxp.com>
In-Reply-To: <8c39b3c3-8146-4418-8835-6dbfe38a85ec@collabora.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
X-MS-PublicTrafficType: Email
X-MS-TrafficTypeDiagnostic: PAXPR04MB8254:EE_|DUZPR04MB9983:EE_
X-MS-Office365-Filtering-Correlation-Id: 49640a1f-f84e-44cd-b1b7-08dc68bcf906
X-MS-Exchange-SharedMailbox-RoutingAgent-Processed: True
X-MS-Exchange-SenderADCheck: 1
X-MS-Exchange-AntiSpam-Relay: 0
X-Microsoft-Antispam: BCL:0;ARA:13230031|1800799015|7416005|366007|376005;
X-Microsoft-Antispam-Message-Info:
	=?utf-8?B?TFJYZDZBUGlsbDJKZVhLN05QdnorRDh3eE1lcTRRc2FXWnJ0UCtRSzR4dmc5?=
 =?utf-8?B?T3dOUWxnVDRUZEdaanZkOFdNRjBjQWhrN0JFaGlNUEMwcXZoTXNJTG8wcmJR?=
 =?utf-8?B?bVhEVVFHL2tmSWdBT1lLMXJ4d3VISjRGd0VoMjNEMkZsYzkrNjNNRzJXUWE3?=
 =?utf-8?B?Z1lBVjQ4Tlo3Q1ppdFpvQVlabU50dkJtN0hTZnB6eW1FQTM5YzBmbk50VVZD?=
 =?utf-8?B?MXlQei9MMG1EaWRwS1ZEajRKWWxXV1hXVTlLREhFNzh0TDZtYTVHaHJqZGtN?=
 =?utf-8?B?bFNiSlZHNEVuOFBVeGwyRjdQem5xM28wQnlOZC9MSzFaZTVhRXRJVkdWTlB6?=
 =?utf-8?B?c0FUc0lpRE02YmNRdUI1NjExb2cvWVd0V0VXQVVlbEZaNXFjcmVkbGVWSTBy?=
 =?utf-8?B?cU8vaTBmWUtTMFNqZURlbXFuL2NPVG41b0k2WjFsYzRDcXpNQUhMS3V1ZVpr?=
 =?utf-8?B?RzVpb2lUeWFVNDhlbU1rOUJ1a3AvZUxNL1B1TFF1bldIb2krekFKcDNLUVhD?=
 =?utf-8?B?N2lya1ErY0dlV2w2TXBiS0pVZWc0cFJkcEFibjJ5V1E4aUpYZjc4eXlpeEpk?=
 =?utf-8?B?aVlLUWxlOVQzdnJFK3lDaElEMy81NWtaNEZxRFlKMHc3eU5iZ0xheUMyclBT?=
 =?utf-8?B?R1diSmtnakFValFMMXRIZzNhWVloeHFzTzZhZm5UcEdITUYyUUplRVo1NHVQ?=
 =?utf-8?B?a0RNVEQ2cm9vV2RWWEtSekdJcE9za1JXN1k5VjlBYzd5MFI0cHFLUDRSSHl1?=
 =?utf-8?B?SXFRZW1WZmJTcC9QRE95bko1YnFXeVppLzBvbC85NklEcFpxankvYXFsZCs5?=
 =?utf-8?B?VFJpTFhrUjR5ZVVPRmU1aEtEcnFiUElwK3dPeWZQTDZFdDhSc2xUT1VPK05U?=
 =?utf-8?B?azFHalhYcUJKMXdiWEQrTmxrM2JOYTY1bTVNNG9RUHJXblJRQndTNHUvRTl3?=
 =?utf-8?B?cUdHSHY3TUhYZENpVjZZeEVWUU1VVWxFWmdpZ3c0bHFPMWhEeUdvVEh3NTd1?=
 =?utf-8?B?WlpFbG5DNWlDYTRUdG5QalVpaGwvR2RwcDN4MENFdDc4YU1qN2pocVFIdERS?=
 =?utf-8?B?azRBUGZxZnNiUEhjN2prUWJ4Wm9tU0RiOUcxa1g4T2V5UTNYTjNaUHpPU2t1?=
 =?utf-8?B?dWFSSGc5c2VtS1c0UGxWRzBhZ3lLL1JmV3JPNnh6Zy9PUVFhZUdVelNZRWNZ?=
 =?utf-8?B?cFp3dlk1Y1pleWVnNU1XaGJEQVU5OGgvdWJYRW5iSm9oRmt4RVU4OFMyUjhj?=
 =?utf-8?B?L3RKcDhvTXB4ZVdyd05LMlZnU2ZtQk1nTmJ0aDRVNG12SnBZMFA4KzVEYjJo?=
 =?utf-8?B?VUlMdVJpZ1FwUzNaQytJRUMyUU8za24vM09sbklBREtxVFpkNm44UVBJelBF?=
 =?utf-8?B?Q084U2FIankyU08zNE5zZ3dacSt5NEtMUFVrb0lPd2RPNGlNcEVxWmJHeEQy?=
 =?utf-8?B?NThSTENpMzlzNzgzaVFHZXIyalpTendaMG8rdU00SmxlcjlSLzU0Qzdudmd5?=
 =?utf-8?B?TFcvVDF6bGdzSC9EQ0hZQkJvcDVHc0N3SHdPODd6OXRPckl2RGxNQzFyRFlo?=
 =?utf-8?B?Z0hSZFo3OVg0Q2IxaTJFc3ZBajRnNXlLRFRkdC9NK0NhR1poRlBmUDgwK21M?=
 =?utf-8?B?SVVkWEl2eTRqM01UNi9JeTNZemg3ZDN5MFFBeFA1eDlOdytadENHVlBGbFYx?=
 =?utf-8?B?d0VOdDRIc3JqZTlqRUJvd0ZCazgyVm1XczNXYUtHSEczL1VKcEdwdVlnPT0=?=
X-Forefront-Antispam-Report:
	CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:PAXPR04MB8254.eurprd04.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230031)(1800799015)(7416005)(366007)(376005);DIR:OUT;SFP:1101;
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?OUhlaXJYaXlxUk5EUW1heUZOT0FQRjFnYmloZXdnNjZNNWRZUWl2czBBQ0J0?=
 =?utf-8?B?MTJ3aUpWb1haNzZ1NlZaajZscmlNSHl4N2pZWllaQzRYeHN4bVhmcTN2eVFR?=
 =?utf-8?B?LzNKMFRlOG5lc0dQaEV6TkVtSVF1aDE1TWM5QndGeWQ1S3pvekdwbHRLQ0sv?=
 =?utf-8?B?dTVqOXZ3S1JTbTRwNE9odUZVeUxoMnVacjhIMzFKWW5WUzFTM3VrRG9jWWtY?=
 =?utf-8?B?QndUanNId0E0M0NuWlFsNE14VVVBVVNlMzdwbXRTWFRZUkZtTmVsR3E3VG54?=
 =?utf-8?B?ZUoxWm1LV1NoZk0yZEVuQWg5akdpWE9ERkxzOG9ialhzZ0pLVmIzTHpiS1I1?=
 =?utf-8?B?QkxuY05YVG9RQlAzOUZPZGFyQlQ0NllKck9pTTd0WXBSOVViamhuU0gvTm1J?=
 =?utf-8?B?Mm9kQ1QySGlCVStZbG1RLzAyZU5LNUR5SXhrb0FGT1JaaDlPMFI0QXZtMjNr?=
 =?utf-8?B?RkdSaHF6NW5HWFl1Umo2OVRNR3VkY1BIL28vSmtZdXUrNkNOdFlOMVNBQ1ZX?=
 =?utf-8?B?Nk1IODZSSzFLeldHWmpITWFac1FIUElkUzNiL0VvZksyOEs1ZW1lKzJNQXNU?=
 =?utf-8?B?VzBHV1hZYXdBUWRjV0xISituZll4TVlRVVp6U0Y4K0pxNytRVFNNaWQxbnA4?=
 =?utf-8?B?OWxWR1FaUTVKSG9WbWZIU05rRFNqMGdNTEpKYUFlbDFLUk16cEt2VlUvcFV3?=
 =?utf-8?B?ZkhlaEtISEJSbTVGSXhrV1djWXBmb0JvVnJqQTZzTjZEM3JzWjlwNVFKZk11?=
 =?utf-8?B?eCtuQmhIT0dwcmJpRjBDN2haT241NUdacWhGMjFDWndsaWF2ci8xYnY5dVJJ?=
 =?utf-8?B?NitDazdsQlMrQkFYOC9xbXNCVG9YV2RjY01pcE93N0syQStuWW5NK01CbXJB?=
 =?utf-8?B?aU56RkwwRms5Z1kvWGVnTllnN25RbHZETVNSTDUrMW1lS0licXFrRWxhOGxi?=
 =?utf-8?B?ZkI2RUtjdkRQTnBoRU9uZGFRVkNJM2gyQ0hkRTVhdlVNTWVYam9wR0xGbVho?=
 =?utf-8?B?ZDc0RjZJZ2x2eVNxdEMrQ3Nld2J6aHFUUDJZSDQ0NXFJYm5FV3hNNDArUFpz?=
 =?utf-8?B?Mkp0c0hWR1NSV0wzYlRoaVNBRCtKblZDcFJmRzdlZGlIVy9XTWw3aUlLMDJt?=
 =?utf-8?B?dG8wdXpPeXVaOFdGTWVGWENMeGM0cTgzMHZqaThVdlJwajIwUlV3MnZ0TXQz?=
 =?utf-8?B?NWF3elJxSUU2RnV1c0Fubm9JWmlOT0lsQnFjNVRkUFNTSW5EVjhmd2hHdXN1?=
 =?utf-8?B?Y3laVFZDU3FnSkNQeG5uTGtEQ29lSnBsQk5GcnVTQUlUOWx4MkRUQ1lhbjBG?=
 =?utf-8?B?eGFaV2xiZmpmbEJQTURKVEdVd2tuL3hJbFgxT2pBbTRhcUNub3QvYzZrRloz?=
 =?utf-8?B?ems2TmZUOVpiYXp4RHh1dG40UHQ2cGpESHh0RHN2NkpYWDllNTEraVNzYTVD?=
 =?utf-8?B?QXgxZllXdlBaY0Ntam5jcGhlYkxGTDhqV21oN1hQNmU2bkZzSkNEaURZM0Y5?=
 =?utf-8?B?RXdXb0h3MGZCbnZLMnlzczkrUXFFUjVoTm5JeFhuUlljaUxCYUpHRjVNY3o2?=
 =?utf-8?B?Qkp3VDd2WkYrZm9yL3VGYUhjZHlabXlsUFQ3TTYweENXY1pBUEhUVGE0UVlx?=
 =?utf-8?B?d2dxUHh0WjdCSU9MeFUyekIrRjRNTG40SnBpVXhUalc2NjEwNDN2aDFPR1Bl?=
 =?utf-8?B?dk81cFBrMzlOeXlBSDBhUjRRblBNZzl4QytUc1ZtM3FMNktzQVlCUHdEQk1x?=
 =?utf-8?B?MkVPMjVxdS9GUXZnWGF1NThlbFdHUk1odms2SCt6RjdJTjlCTUcwcDlTeGRJ?=
 =?utf-8?B?Y096OTVNSXpmakZBQ01od0ZnSGZ6aUtITWNRbE14QW9YOXZodytaSllPUExU?=
 =?utf-8?B?OEFkeEMvWVorZEZuVXVXdGNrTC9ydWo4c2NhcndvUVBiWExqZVJXd2tLaXJr?=
 =?utf-8?B?M1VqUFVmNkF4ZGlEQ21PL1VPNjk3Y1RHL0YwcVRmcWx0WGF1UU93c3grc0ZW?=
 =?utf-8?B?WXlIV24vVmp1V0dlWk1BZS9Hdzh1dEdncXZOVGdIRllXUXdXbEhYRFh2VkpN?=
 =?utf-8?B?cHNpak8vOFRIaUU5emJDYzE0bXdrUkxNMHc2UlNVdzlkSXQ4bU51VFdzMlNX?=
 =?utf-8?Q?hN7sPB8DH+LBxbSrVaTbePXrL?=
X-OriginatorOrg: oss.nxp.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 49640a1f-f84e-44cd-b1b7-08dc68bcf906
X-MS-Exchange-CrossTenant-AuthSource: PAXPR04MB8254.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Apr 2024 02:26:42.2064
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 7KrQMr7DWq1hu4zolrpqTC8k95F0DaxtzV0p31CK3F3rbPz9I82VNfLTaFYZprTdO6aLZ/nks6unQOTzHdU0ZA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DUZPR04MB9983

Hi Andrzej,

> Hi Ming Qian,
> 
> W dniu 25.04.2024 o 08:50, Ming Qian pisze:
>> There is no need to add lock in s_ctrl callback, it has been
>> synchronized by the ctrl_handler's lock, otherwise it may led to
>> deadlock if driver call v4l2_ctrl_s_ctrl().
>>
>> Signed-off-by: Ming Qian <ming.qian@nxp.com>
>> ---
>>   drivers/media/platform/amphion/vdec.c | 2 --
>>   drivers/media/platform/amphion/venc.c | 2 --
>>   2 files changed, 4 deletions(-)
>>
>> diff --git a/drivers/media/platform/amphion/vdec.c 
>> b/drivers/media/platform/amphion/vdec.c
>> index a57f9f4f3b87..6a38a0fa0e2d 100644
>> --- a/drivers/media/platform/amphion/vdec.c
>> +++ b/drivers/media/platform/amphion/vdec.c
>> @@ -195,7 +195,6 @@ static int vdec_op_s_ctrl(struct v4l2_ctrl *ctrl)
>>       struct vdec_t *vdec = inst->priv;
>>       int ret = 0;
>> -    vpu_inst_lock(inst);
> 
> I assume that PATCH v2 2/3 might cause the said deadlock to happen?
> If so, maybe it would make more sense to make the current patch preceed
>   PATCH v2 2/3? Otherwise the kernel at PATCH v2 2/3 introduces a potential
> deadlock.
> 
> Regards,
> 
> Andrzej
> 

I actually discovered this problem when I was preparing the v2 2/3 patch.

But in the v2 2/3 patch, it tried to add a read-only ctrl, then I just
unset the s_ctrl callback for the new added ctrl, the potential deadlock
is caused by call the s_ctrl back in a locked environment, so after unset
the s_ctrl callback, the 2/3 patch won't trigger the deadlock even if
this patch is missing.

In order to avoid encountering similar problems in the future, that
driver may set or get some ctrl, I added this patch.

Best regards,
Ming

>>       switch (ctrl->id) {
>>       case V4L2_CID_MPEG_VIDEO_DEC_DISPLAY_DELAY_ENABLE:
>>           vdec->params.display_delay_enable = ctrl->val;
>> @@ -207,7 +206,6 @@ static int vdec_op_s_ctrl(struct v4l2_ctrl *ctrl)
>>           ret = -EINVAL;
>>           break;
>>       }
>> -    vpu_inst_unlock(inst);
>>       return ret;
>>   }
>> diff --git a/drivers/media/platform/amphion/venc.c 
>> b/drivers/media/platform/amphion/venc.c
>> index cdfaba9d107b..351b4edc8742 100644
>> --- a/drivers/media/platform/amphion/venc.c
>> +++ b/drivers/media/platform/amphion/venc.c
>> @@ -518,7 +518,6 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
>>       struct venc_t *venc = inst->priv;
>>       int ret = 0;
>> -    vpu_inst_lock(inst);
>>       switch (ctrl->id) {
>>       case V4L2_CID_MPEG_VIDEO_H264_PROFILE:
>>           venc->params.profile = ctrl->val;
>> @@ -579,7 +578,6 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
>>           ret = -EINVAL;
>>           break;
>>       }
>> -    vpu_inst_unlock(inst);
>>       return ret;
>>   }
>