[PATCH v2] drm/ast: Fix soft lockup

[PATCH v2] drm/ast: Fix soft lockup

[Jammy Huang]

There is a while-loop in ast_dp_set_on_off() that could lead to
infinite-loop. This is because the register, VGACRI-Dx, checked in
this API is a scratch register actually controlled by a MCU, named
DPMCU, in BMC.

These scratch registers are protected by scu-lock. If suc-lock is not
off, DPMCU can not update these registers and then host will have soft
lockup due to never updated status.

DPMCU is used to control DP and relative registers to handshake with
host's VGA driver. Even the most time-consuming task, DP's link
training, is less than 100ms. 200ms should be enough.

Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com>
Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 drivers/gpu/drm/ast/ast_dp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c
index ebb6d8ebd44e..1e9259416980 100644
--- a/drivers/gpu/drm/ast/ast_dp.c
+++ b/drivers/gpu/drm/ast/ast_dp.c
@@ -180,6 +180,7 @@ void ast_dp_set_on_off(struct drm_device *dev, bool on)
 {
 	struct ast_device *ast = to_ast_device(dev);
 	u8 video_on_off = on;
+	u32 i = 0;
 
 	// Video On/Off
 	ast_set_index_reg_mask(ast, AST_IO_VGACRI, 0xE3, (u8) ~AST_DP_VIDEO_ENABLE, on);
@@ -192,6 +193,8 @@ void ast_dp_set_on_off(struct drm_device *dev, bool on)
 						ASTDP_MIRROR_VIDEO_ENABLE) != video_on_off) {
 			// wait 1 ms
 			mdelay(1);
+			if (++i > 200)
+				break;
 		}
 	}
 }

base-commit: b0546776ad3f332e215cebc0b063ba4351971cca
-- 
2.25.1

Re: [PATCH v2] drm/ast: Fix soft lockup

[Thomas Zimmermann]

Am 03.04.24 um 11:02 schrieb Jammy Huang:
> There is a while-loop in ast_dp_set_on_off() that could lead to
> infinite-loop. This is because the register, VGACRI-Dx, checked in
> this API is a scratch register actually controlled by a MCU, named
> DPMCU, in BMC.
>
> These scratch registers are protected by scu-lock. If suc-lock is not
> off, DPMCU can not update these registers and then host will have soft
> lockup due to never updated status.
>
> DPMCU is used to control DP and relative registers to handshake with
> host's VGA driver. Even the most time-consuming task, DP's link
> training, is less than 100ms. 200ms should be enough.
>
> Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com>
> Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/gpu/drm/ast/ast_dp.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c
> index ebb6d8ebd44e..1e9259416980 100644
> --- a/drivers/gpu/drm/ast/ast_dp.c
> +++ b/drivers/gpu/drm/ast/ast_dp.c
> @@ -180,6 +180,7 @@ void ast_dp_set_on_off(struct drm_device *dev, bool on)
>   {
>   	struct ast_device *ast = to_ast_device(dev);
>   	u8 video_on_off = on;
> +	u32 i = 0;
>   
>   	// Video On/Off
>   	ast_set_index_reg_mask(ast, AST_IO_VGACRI, 0xE3, (u8) ~AST_DP_VIDEO_ENABLE, on);
> @@ -192,6 +193,8 @@ void ast_dp_set_on_off(struct drm_device *dev, bool on)
>   						ASTDP_MIRROR_VIDEO_ENABLE) != video_on_off) {
>   			// wait 1 ms
>   			mdelay(1);
> +			if (++i > 200)
> +				break;
>   		}
>   	}
>   }
>
> base-commit: b0546776ad3f332e215cebc0b063ba4351971cca

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH v2] drm/ast: Fix soft lockup

[Thomas Zimmermann]

Hi,

I've added a Fixes tag and pushed to patch into drm-misc-fixes.

Best regards
Thomas

Am 03.04.24 um 11:02 schrieb Jammy Huang:
> There is a while-loop in ast_dp_set_on_off() that could lead to
> infinite-loop. This is because the register, VGACRI-Dx, checked in
> this API is a scratch register actually controlled by a MCU, named
> DPMCU, in BMC.
>
> These scratch registers are protected by scu-lock. If suc-lock is not
> off, DPMCU can not update these registers and then host will have soft
> lockup due to never updated status.
>
> DPMCU is used to control DP and relative registers to handshake with
> host's VGA driver. Even the most time-consuming task, DP's link
> training, is less than 100ms. 200ms should be enough.
>
> Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com>
> Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
> ---
>   drivers/gpu/drm/ast/ast_dp.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c
> index ebb6d8ebd44e..1e9259416980 100644
> --- a/drivers/gpu/drm/ast/ast_dp.c
> +++ b/drivers/gpu/drm/ast/ast_dp.c
> @@ -180,6 +180,7 @@ void ast_dp_set_on_off(struct drm_device *dev, bool on)
>   {
>   	struct ast_device *ast = to_ast_device(dev);
>   	u8 video_on_off = on;
> +	u32 i = 0;
>   
>   	// Video On/Off
>   	ast_set_index_reg_mask(ast, AST_IO_VGACRI, 0xE3, (u8) ~AST_DP_VIDEO_ENABLE, on);
> @@ -192,6 +193,8 @@ void ast_dp_set_on_off(struct drm_device *dev, bool on)
>   						ASTDP_MIRROR_VIDEO_ENABLE) != video_on_off) {
>   			// wait 1 ms
>   			mdelay(1);
> +			if (++i > 200)
> +				break;
>   		}
>   	}
>   }
>
> base-commit: b0546776ad3f332e215cebc0b063ba4351971cca

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

RE: [PATCH v2] drm/ast: Fix soft lockup

[Jammy Huang]

Hi Thomas,

Thank you.

Regards,
Jammy Huang

> 
> Hi,
> 
> I've added a Fixes tag and pushed to patch into drm-misc-fixes.
> 
> Best regards
> Thomas
> 
> Am 03.04.24 um 11:02 schrieb Jammy Huang:
> > There is a while-loop in ast_dp_set_on_off() that could lead to
> > infinite-loop. This is because the register, VGACRI-Dx, checked in
> > this API is a scratch register actually controlled by a MCU, named
> > DPMCU, in BMC.
> >
> > These scratch registers are protected by scu-lock. If suc-lock is not
> > off, DPMCU can not update these registers and then host will have soft
> > lockup due to never updated status.
> >
> > DPMCU is used to control DP and relative registers to handshake with
> > host's VGA driver. Even the most time-consuming task, DP's link
> > training, is less than 100ms. 200ms should be enough.
> >
> > Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com>
> > Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
> > ---
> >   drivers/gpu/drm/ast/ast_dp.c | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/ast/ast_dp.c
> > b/drivers/gpu/drm/ast/ast_dp.c index ebb6d8ebd44e..1e9259416980 100644
> > --- a/drivers/gpu/drm/ast/ast_dp.c
> > +++ b/drivers/gpu/drm/ast/ast_dp.c
> > @@ -180,6 +180,7 @@ void ast_dp_set_on_off(struct drm_device *dev, bool
> on)
> >   {
> >   	struct ast_device *ast = to_ast_device(dev);
> >   	u8 video_on_off = on;
> > +	u32 i = 0;
> >
> >   	// Video On/Off
> >   	ast_set_index_reg_mask(ast, AST_IO_VGACRI, 0xE3, (u8)
> > ~AST_DP_VIDEO_ENABLE, on); @@ -192,6 +193,8 @@ void
> ast_dp_set_on_off(struct drm_device *dev, bool on)
> >   						ASTDP_MIRROR_VIDEO_ENABLE) !=
> video_on_off) {
> >   			// wait 1 ms
> >   			mdelay(1);
> > +			if (++i > 200)
> > +				break;
> >   		}
> >   	}
> >   }
> >
> > base-commit: b0546776ad3f332e215cebc0b063ba4351971cca
> 
> --
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstrasse 146, 90461 Nuernberg, Germany
> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB
> 36809 (AG Nuernberg)