[PATCH next] drm: zynqmp_dp: Unlock on error in zynqmp_dp_bridge_atomic_enable()

[PATCH next] drm: zynqmp_dp: Unlock on error in zynqmp_dp_bridge_atomic_enable()

[Dan Carpenter]

We added some locking to this function, but accidentally forgot to unlock
if zynqmp_dp_mode_configure() failed.  Use a guard lock to fix it.

Fixes: a7d5eeaa57d7 ("drm: zynqmp_dp: Add locking")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/gpu/drm/xlnx/zynqmp_dp.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 25c5dc61ee88..0bea908b281e 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -1537,7 +1537,7 @@ static void zynqmp_dp_bridge_atomic_enable(struct drm_bridge *bridge,
 
 	pm_runtime_get_sync(dp->dev);
 
-	mutex_lock(&dp->lock);
+	guard(mutex)(&dp->lock);
 	zynqmp_dp_disp_enable(dp, old_bridge_state);
 
 	/*
@@ -1598,7 +1598,6 @@ static void zynqmp_dp_bridge_atomic_enable(struct drm_bridge *bridge,
 	zynqmp_dp_write(dp, ZYNQMP_DP_SOFTWARE_RESET,
 			ZYNQMP_DP_SOFTWARE_RESET_ALL);
 	zynqmp_dp_write(dp, ZYNQMP_DP_MAIN_STREAM_ENABLE, 1);
-	mutex_unlock(&dp->lock);
 }
 
 static void zynqmp_dp_bridge_atomic_disable(struct drm_bridge *bridge,
-- 
2.45.2

Re: [PATCH next] drm: zynqmp_dp: Unlock on error in zynqmp_dp_bridge_atomic_enable()

[Sean Anderson]

On 11/11/24 04:06, Dan Carpenter wrote:
> We added some locking to this function, but accidentally forgot to unlock
> if zynqmp_dp_mode_configure() failed.  Use a guard lock to fix it.
> 
> Fixes: a7d5eeaa57d7 ("drm: zynqmp_dp: Add locking")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  drivers/gpu/drm/xlnx/zynqmp_dp.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 25c5dc61ee88..0bea908b281e 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1537,7 +1537,7 @@ static void zynqmp_dp_bridge_atomic_enable(struct drm_bridge *bridge,
>  
>  	pm_runtime_get_sync(dp->dev);
>  
> -	mutex_lock(&dp->lock);
> +	guard(mutex)(&dp->lock);
>  	zynqmp_dp_disp_enable(dp, old_bridge_state);
>  
>  	/*
> @@ -1598,7 +1598,6 @@ static void zynqmp_dp_bridge_atomic_enable(struct drm_bridge *bridge,
>  	zynqmp_dp_write(dp, ZYNQMP_DP_SOFTWARE_RESET,
>  			ZYNQMP_DP_SOFTWARE_RESET_ALL);
>  	zynqmp_dp_write(dp, ZYNQMP_DP_MAIN_STREAM_ENABLE, 1);
> -	mutex_unlock(&dp->lock);
>  }
>  
>  static void zynqmp_dp_bridge_atomic_disable(struct drm_bridge *bridge,

Reviewed-by: Sean Anderson <sean.anderson@linux.dev>

Although this reverses the order of pm_runtime_put and mutex_unlock in
the error case, I don't think it matters.

Re: [PATCH next] drm: zynqmp_dp: Unlock on error in zynqmp_dp_bridge_atomic_enable()

[Laurent Pinchart]

Hi Dan,

Thank you for the patch.

On Mon, Nov 11, 2024 at 12:06:10PM +0300, Dan Carpenter wrote:
> We added some locking to this function, but accidentally forgot to unlock
> if zynqmp_dp_mode_configure() failed.  Use a guard lock to fix it.
> 
> Fixes: a7d5eeaa57d7 ("drm: zynqmp_dp: Add locking")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Sean, how about replacing all the mutex_lock()/mutex_unlock() calls
you've added in a7d5eeaa57d7 with guards ?

> ---
>  drivers/gpu/drm/xlnx/zynqmp_dp.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 25c5dc61ee88..0bea908b281e 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1537,7 +1537,7 @@ static void zynqmp_dp_bridge_atomic_enable(struct drm_bridge *bridge,
>  
>  	pm_runtime_get_sync(dp->dev);
>  
> -	mutex_lock(&dp->lock);
> +	guard(mutex)(&dp->lock);
>  	zynqmp_dp_disp_enable(dp, old_bridge_state);
>  
>  	/*
> @@ -1598,7 +1598,6 @@ static void zynqmp_dp_bridge_atomic_enable(struct drm_bridge *bridge,
>  	zynqmp_dp_write(dp, ZYNQMP_DP_SOFTWARE_RESET,
>  			ZYNQMP_DP_SOFTWARE_RESET_ALL);
>  	zynqmp_dp_write(dp, ZYNQMP_DP_MAIN_STREAM_ENABLE, 1);
> -	mutex_unlock(&dp->lock);
>  }
>  
>  static void zynqmp_dp_bridge_atomic_disable(struct drm_bridge *bridge,

-- 
Regards,

Laurent Pinchart

Re: [PATCH next] drm: zynqmp_dp: Unlock on error in zynqmp_dp_bridge_atomic_enable()

[Sean Anderson]

On 11/12/24 00:27, Laurent Pinchart wrote:
> Hi Dan,
> 
> Thank you for the patch.
> 
> On Mon, Nov 11, 2024 at 12:06:10PM +0300, Dan Carpenter wrote:
>> We added some locking to this function, but accidentally forgot to unlock
>> if zynqmp_dp_mode_configure() failed.  Use a guard lock to fix it.
>> 
>> Fixes: a7d5eeaa57d7 ("drm: zynqmp_dp: Add locking")
>> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Sean, how about replacing all the mutex_lock()/mutex_unlock() calls
> you've added in a7d5eeaa57d7 with guards ?

I have no objection to that.

--Sean

Re: [PATCH next] drm: zynqmp_dp: Unlock on error in zynqmp_dp_bridge_atomic_enable()

[Laurent Pinchart]

On Tue, Nov 12, 2024 at 09:41:58AM -0500, Sean Anderson wrote:
> On 11/12/24 00:27, Laurent Pinchart wrote:
> > Hi Dan,
> > 
> > Thank you for the patch.
> > 
> > On Mon, Nov 11, 2024 at 12:06:10PM +0300, Dan Carpenter wrote:
> >> We added some locking to this function, but accidentally forgot to unlock
> >> if zynqmp_dp_mode_configure() failed.  Use a guard lock to fix it.
> >> 
> >> Fixes: a7d5eeaa57d7 ("drm: zynqmp_dp: Add locking")
> >> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > Sean, how about replacing all the mutex_lock()/mutex_unlock() calls
> > you've added in a7d5eeaa57d7 with guards ?
> 
> I have no objection to that.

Would you send a patch ? Otherwise I can do it.

-- 
Regards,

Laurent Pinchart

Re: [PATCH next] drm: zynqmp_dp: Unlock on error in zynqmp_dp_bridge_atomic_enable()

[Sean Anderson]

On 11/12/24 11:43, Laurent Pinchart wrote:
> On Tue, Nov 12, 2024 at 09:41:58AM -0500, Sean Anderson wrote:
>> On 11/12/24 00:27, Laurent Pinchart wrote:
>> > Hi Dan,
>> > 
>> > Thank you for the patch.
>> > 
>> > On Mon, Nov 11, 2024 at 12:06:10PM +0300, Dan Carpenter wrote:
>> >> We added some locking to this function, but accidentally forgot to unlock
>> >> if zynqmp_dp_mode_configure() failed.  Use a guard lock to fix it.
>> >> 
>> >> Fixes: a7d5eeaa57d7 ("drm: zynqmp_dp: Add locking")
>> >> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
>> > 
>> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> > 
>> > Sean, how about replacing all the mutex_lock()/mutex_unlock() calls
>> > you've added in a7d5eeaa57d7 with guards ?
>> 
>> I have no objection to that.
> 
> Would you send a patch ? Otherwise I can do it.
> 

I can send a patch, but it will not be for a week or two.

--Sean

Re: [PATCH next] drm: zynqmp_dp: Unlock on error in zynqmp_dp_bridge_atomic_enable()

[Sean Anderson]

On 11/12/24 12:22, Sean Anderson wrote:
> On 11/12/24 11:43, Laurent Pinchart wrote:
>> On Tue, Nov 12, 2024 at 09:41:58AM -0500, Sean Anderson wrote:
>>> On 11/12/24 00:27, Laurent Pinchart wrote:
>>> > Hi Dan,
>>> > 
>>> > Thank you for the patch.
>>> > 
>>> > On Mon, Nov 11, 2024 at 12:06:10PM +0300, Dan Carpenter wrote:
>>> >> We added some locking to this function, but accidentally forgot to unlock
>>> >> if zynqmp_dp_mode_configure() failed.  Use a guard lock to fix it.
>>> >> 
>>> >> Fixes: a7d5eeaa57d7 ("drm: zynqmp_dp: Add locking")
>>> >> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
>>> > 
>>> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> > 
>>> > Sean, how about replacing all the mutex_lock()/mutex_unlock() calls
>>> > you've added in a7d5eeaa57d7 with guards ?
>>> 
>>> I have no objection to that.
>> 
>> Would you send a patch ? Otherwise I can do it.
>> 
> 
> I can send a patch, but it will not be for a week or two.
> 
> --Sean

Just following up on this; will the above patched be merged? I would
prefer to keep the bugfix and the conversion separate.

--Sean

Re: [PATCH next] drm: zynqmp_dp: Unlock on error in zynqmp_dp_bridge_atomic_enable()

[Tomi Valkeinen]

Hi,

On 24/01/2025 01:56, Sean Anderson wrote:
> On 11/12/24 12:22, Sean Anderson wrote:
>> On 11/12/24 11:43, Laurent Pinchart wrote:
>>> On Tue, Nov 12, 2024 at 09:41:58AM -0500, Sean Anderson wrote:
>>>> On 11/12/24 00:27, Laurent Pinchart wrote:
>>>>> Hi Dan,
>>>>>
>>>>> Thank you for the patch.
>>>>>
>>>>> On Mon, Nov 11, 2024 at 12:06:10PM +0300, Dan Carpenter wrote:
>>>>>> We added some locking to this function, but accidentally forgot to unlock
>>>>>> if zynqmp_dp_mode_configure() failed.  Use a guard lock to fix it.
>>>>>>
>>>>>> Fixes: a7d5eeaa57d7 ("drm: zynqmp_dp: Add locking")
>>>>>> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
>>>>>
>>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>>
>>>>> Sean, how about replacing all the mutex_lock()/mutex_unlock() calls
>>>>> you've added in a7d5eeaa57d7 with guards ?
>>>>
>>>> I have no objection to that.
>>>
>>> Would you send a patch ? Otherwise I can do it.
>>>
>>
>> I can send a patch, but it will not be for a week or two.
>>
>> --Sean
> 
> Just following up on this; will the above patched be merged? I would
> prefer to keep the bugfix and the conversion separate.

I'll push to drm-misc-fixes.

  Tomi