diff mbox series

drm/panel: novatek-nt35950: Improve error handling

Message ID 20230415-konrad-longbois-next-v1-1-ce695dc9df84@linaro.org
State New
Headers
Series drm/panel: novatek-nt35950: Improve error handling |

Commit Message

Konrad Dybcio April 15, 2023, 11 a.m. UTC 
  In a very peculiar case when probing and registering with the secondary
DSI host succeeds, but the OF backlight or DSI attachment fails, the
primary DSI device is automatically cleaned up, but the secondary one
is not, leading to -EEXIST when the driver core tries to handle
-EPROBE_DEFER.

Unregister the DSI1 device manually on failure to prevent that.

Fixes: 623a3531e9cf ("drm/panel: Add driver for Novatek NT35950 DSI DriverIC panels")
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/panel/panel-novatek-nt35950.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)


---
base-commit: 035ba5f9cf511b9299fd9c9d0688ef930d33c886
change-id: 20230415-konrad-longbois-next-847d57abb4d2

Best regards,

Comments

Neil Armstrong April 17, 2023, 7:25 a.m. UTC | #1 
On 15/04/2023 13:00, Konrad Dybcio wrote:
> In a very peculiar case when probing and registering with the secondary
> DSI host succeeds, but the OF backlight or DSI attachment fails, the
> primary DSI device is automatically cleaned up, but the secondary one
> is not, leading to -EEXIST when the driver core tries to handle
> -EPROBE_DEFER.
> 
> Unregister the DSI1 device manually on failure to prevent that.
> 
> Fixes: 623a3531e9cf ("drm/panel: Add driver for Novatek NT35950 DSI DriverIC panels")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/gpu/drm/panel/panel-novatek-nt35950.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35950.c b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
> index abf752b36a52..7498fc6258bb 100644
> --- a/drivers/gpu/drm/panel/panel-novatek-nt35950.c
> +++ b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
> @@ -585,8 +585,11 @@ static int nt35950_probe(struct mipi_dsi_device *dsi)
>   		       DRM_MODE_CONNECTOR_DSI);
>   
>   	ret = drm_panel_of_backlight(&nt->panel);
> -	if (ret)
> +	if (ret) {
> +		mipi_dsi_device_unregister(nt->dsi[1]);
> +
>   		return dev_err_probe(dev, ret, "Failed to get backlight\n");
> +	}
>   
>   	drm_panel_add(&nt->panel);
>   
> @@ -602,6 +605,9 @@ static int nt35950_probe(struct mipi_dsi_device *dsi)
>   
>   		ret = mipi_dsi_attach(nt->dsi[i]);
>   		if (ret < 0) {
> +			/* If we fail to attach to either host, we're done */
> +			mipi_dsi_device_unregister(nt->dsi[1]);
> +
>   			return dev_err_probe(dev, ret,
>   					     "Cannot attach to DSI%d host.\n", i);
>   		}
> 
> ---
> base-commit: 035ba5f9cf511b9299fd9c9d0688ef930d33c886
> change-id: 20230415-konrad-longbois-next-847d57abb4d2
> 
> Best regards,

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
Neil Armstrong April 17, 2023, 7:28 a.m. UTC | #2 
Hi,

On Sat, 15 Apr 2023 13:00:30 +0200, Konrad Dybcio wrote:
> In a very peculiar case when probing and registering with the secondary
> DSI host succeeds, but the OF backlight or DSI attachment fails, the
> primary DSI device is automatically cleaned up, but the secondary one
> is not, leading to -EEXIST when the driver core tries to handle
> -EPROBE_DEFER.
> 
> Unregister the DSI1 device manually on failure to prevent that.
> 
> [...]

Thanks, Applied to https://anongit.freedesktop.org/git/drm/drm-misc.git (drm-misc-next-fixes)

[1/1] drm/panel: novatek-nt35950: Improve error handling
      https://cgit.freedesktop.org/drm/drm-misc/commit/?id=5dd45b66742a1f3cfa9a92dc0ac8714c7708ee6c
AngeloGioacchino Del Regno April 17, 2023, 9:44 a.m. UTC | #3 
Il 17/04/23 09:25, Neil Armstrong ha scritto:
> On 15/04/2023 13:00, Konrad Dybcio wrote:
>> In a very peculiar case when probing and registering with the secondary
>> DSI host succeeds, but the OF backlight or DSI attachment fails, the
>> primary DSI device is automatically cleaned up, but the secondary one
>> is not, leading to -EEXIST when the driver core tries to handle
>> -EPROBE_DEFER.
>>
>> Unregister the DSI1 device manually on failure to prevent that.
>>
>> Fixes: 623a3531e9cf ("drm/panel: Add driver for Novatek NT35950 DSI DriverIC 
>> panels")
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/gpu/drm/panel/panel-novatek-nt35950.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35950.c 
>> b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
>> index abf752b36a52..7498fc6258bb 100644
>> --- a/drivers/gpu/drm/panel/panel-novatek-nt35950.c
>> +++ b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
>> @@ -585,8 +585,11 @@ static int nt35950_probe(struct mipi_dsi_device *dsi)
>>                  DRM_MODE_CONNECTOR_DSI);
>>       ret = drm_panel_of_backlight(&nt->panel);
>> -    if (ret)
>> +    if (ret) {

If this is not a dual-DSI case, nt->dsi[1] will be NULL: I agree it's still
fine as in the kernel won't crash in that case, but for logical correctness
I would still account for that:

		if (num_dsis == 2)
			mipi_dsi_device_unregister(nt->dsi[1]);

>> +        mipi_dsi_device_unregister(nt->dsi[1]);
>> +
>>           return dev_err_probe(dev, ret, "Failed to get backlight\n");
>> +    }
>>       drm_panel_add(&nt->panel);
>> @@ -602,6 +605,9 @@ static int nt35950_probe(struct mipi_dsi_device *dsi)
>>           ret = mipi_dsi_attach(nt->dsi[i]);
>>           if (ret < 0) {

Same here, please add a check for num_dsis.

Regards,
Angelo
AngeloGioacchino Del Regno April 17, 2023, 9:44 a.m. UTC | #4 
Il 17/04/23 11:44, AngeloGioacchino Del Regno ha scritto:
> Il 17/04/23 09:25, Neil Armstrong ha scritto:
>> On 15/04/2023 13:00, Konrad Dybcio wrote:
>>> In a very peculiar case when probing and registering with the secondary
>>> DSI host succeeds, but the OF backlight or DSI attachment fails, the
>>> primary DSI device is automatically cleaned up, but the secondary one
>>> is not, leading to -EEXIST when the driver core tries to handle
>>> -EPROBE_DEFER.
>>>
>>> Unregister the DSI1 device manually on failure to prevent that.
>>>
>>> Fixes: 623a3531e9cf ("drm/panel: Add driver for Novatek NT35950 DSI DriverIC 
>>> panels")
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> ---
>>>   drivers/gpu/drm/panel/panel-novatek-nt35950.c | 8 +++++++-
>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35950.c 
>>> b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
>>> index abf752b36a52..7498fc6258bb 100644
>>> --- a/drivers/gpu/drm/panel/panel-novatek-nt35950.c
>>> +++ b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
>>> @@ -585,8 +585,11 @@ static int nt35950_probe(struct mipi_dsi_device *dsi)
>>>                  DRM_MODE_CONNECTOR_DSI);
>>>       ret = drm_panel_of_backlight(&nt->panel);
>>> -    if (ret)
>>> +    if (ret) {
> 
> If this is not a dual-DSI case, nt->dsi[1] will be NULL: I agree it's still
> fine as in the kernel won't crash in that case, but for logical correctness
> I would still account for that:
> 
>          if (num_dsis == 2)
>              mipi_dsi_device_unregister(nt->dsi[1]);
> 
>>> +        mipi_dsi_device_unregister(nt->dsi[1]);
>>> +
>>>           return dev_err_probe(dev, ret, "Failed to get backlight\n");
>>> +    }
>>>       drm_panel_add(&nt->panel);
>>> @@ -602,6 +605,9 @@ static int nt35950_probe(struct mipi_dsi_device *dsi)
>>>           ret = mipi_dsi_attach(nt->dsi[i]);
>>>           if (ret < 0) {
> 
> Same here, please add a check for num_dsis.
> 
> Regards,
> Angelo

Eh, too late, I just noticed that this got picked already...
Konrad Dybcio April 17, 2023, 1:38 p.m. UTC | #5 
On 17.04.2023 11:44, AngeloGioacchino Del Regno wrote:
> Il 17/04/23 11:44, AngeloGioacchino Del Regno ha scritto:
>> Il 17/04/23 09:25, Neil Armstrong ha scritto:
>>> On 15/04/2023 13:00, Konrad Dybcio wrote:
>>>> In a very peculiar case when probing and registering with the secondary
>>>> DSI host succeeds, but the OF backlight or DSI attachment fails, the
>>>> primary DSI device is automatically cleaned up, but the secondary one
>>>> is not, leading to -EEXIST when the driver core tries to handle
>>>> -EPROBE_DEFER.
>>>>
>>>> Unregister the DSI1 device manually on failure to prevent that.
>>>>
>>>> Fixes: 623a3531e9cf ("drm/panel: Add driver for Novatek NT35950 DSI DriverIC panels")
>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>> ---
>>>>   drivers/gpu/drm/panel/panel-novatek-nt35950.c | 8 +++++++-
>>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35950.c b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
>>>> index abf752b36a52..7498fc6258bb 100644
>>>> --- a/drivers/gpu/drm/panel/panel-novatek-nt35950.c
>>>> +++ b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
>>>> @@ -585,8 +585,11 @@ static int nt35950_probe(struct mipi_dsi_device *dsi)
>>>>                  DRM_MODE_CONNECTOR_DSI);
>>>>       ret = drm_panel_of_backlight(&nt->panel);
>>>> -    if (ret)
>>>> +    if (ret) {
>>
>> If this is not a dual-DSI case, nt->dsi[1] will be NULL: I agree it's still
>> fine as in the kernel won't crash in that case, but for logical correctness
>> I would still account for that:
>>
>>          if (num_dsis == 2)
>>              mipi_dsi_device_unregister(nt->dsi[1]);
>>
>>>> +        mipi_dsi_device_unregister(nt->dsi[1]);
>>>> +
>>>>           return dev_err_probe(dev, ret, "Failed to get backlight\n");
>>>> +    }
>>>>       drm_panel_add(&nt->panel);
>>>> @@ -602,6 +605,9 @@ static int nt35950_probe(struct mipi_dsi_device *dsi)
>>>>           ret = mipi_dsi_attach(nt->dsi[i]);
>>>>           if (ret < 0) {
>>
>> Same here, please add a check for num_dsis.
Right, I'll send an incremental patch to fix the fix, thanks
for spotting this!

Konrad
>>
>> Regards,
>> Angelo
> 
> Eh, too late, I just noticed that this got picked already...
AngeloGioacchino Del Regno April 17, 2023, 2:16 p.m. UTC | #6 
Il 17/04/23 15:38, Konrad Dybcio ha scritto:
> 
> 
> On 17.04.2023 11:44, AngeloGioacchino Del Regno wrote:
>> Il 17/04/23 11:44, AngeloGioacchino Del Regno ha scritto:
>>> Il 17/04/23 09:25, Neil Armstrong ha scritto:
>>>> On 15/04/2023 13:00, Konrad Dybcio wrote:
>>>>> In a very peculiar case when probing and registering with the secondary
>>>>> DSI host succeeds, but the OF backlight or DSI attachment fails, the
>>>>> primary DSI device is automatically cleaned up, but the secondary one
>>>>> is not, leading to -EEXIST when the driver core tries to handle
>>>>> -EPROBE_DEFER.
>>>>>
>>>>> Unregister the DSI1 device manually on failure to prevent that.
>>>>>
>>>>> Fixes: 623a3531e9cf ("drm/panel: Add driver for Novatek NT35950 DSI DriverIC panels")
>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>> ---
>>>>>    drivers/gpu/drm/panel/panel-novatek-nt35950.c | 8 +++++++-
>>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35950.c b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
>>>>> index abf752b36a52..7498fc6258bb 100644
>>>>> --- a/drivers/gpu/drm/panel/panel-novatek-nt35950.c
>>>>> +++ b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
>>>>> @@ -585,8 +585,11 @@ static int nt35950_probe(struct mipi_dsi_device *dsi)
>>>>>                   DRM_MODE_CONNECTOR_DSI);
>>>>>        ret = drm_panel_of_backlight(&nt->panel);
>>>>> -    if (ret)
>>>>> +    if (ret) {
>>>
>>> If this is not a dual-DSI case, nt->dsi[1] will be NULL: I agree it's still
>>> fine as in the kernel won't crash in that case, but for logical correctness
>>> I would still account for that:
>>>
>>>           if (num_dsis == 2)
>>>               mipi_dsi_device_unregister(nt->dsi[1]);
>>>
>>>>> +        mipi_dsi_device_unregister(nt->dsi[1]);
>>>>> +
>>>>>            return dev_err_probe(dev, ret, "Failed to get backlight\n");
>>>>> +    }
>>>>>        drm_panel_add(&nt->panel);
>>>>> @@ -602,6 +605,9 @@ static int nt35950_probe(struct mipi_dsi_device *dsi)
>>>>>            ret = mipi_dsi_attach(nt->dsi[i]);
>>>>>            if (ret < 0) {
>>>
>>> Same here, please add a check for num_dsis.
> Right, I'll send an incremental patch to fix the fix, thanks
> for spotting this!
> 

You're welcome! Please make sure to Cc me so that I can timely give you a review!

Cheers,
Angelo
diff mbox series

Patch

diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35950.c b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
index abf752b36a52..7498fc6258bb 100644
--- a/drivers/gpu/drm/panel/panel-novatek-nt35950.c
+++ b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
@@ -585,8 +585,11 @@  static int nt35950_probe(struct mipi_dsi_device *dsi)
 		       DRM_MODE_CONNECTOR_DSI);
 
 	ret = drm_panel_of_backlight(&nt->panel);
-	if (ret)
+	if (ret) {
+		mipi_dsi_device_unregister(nt->dsi[1]);
+
 		return dev_err_probe(dev, ret, "Failed to get backlight\n");
+	}
 
 	drm_panel_add(&nt->panel);
 
@@ -602,6 +605,9 @@  static int nt35950_probe(struct mipi_dsi_device *dsi)
 
 		ret = mipi_dsi_attach(nt->dsi[i]);
 		if (ret < 0) {
+			/* If we fail to attach to either host, we're done */
+			mipi_dsi_device_unregister(nt->dsi[1]);
+
 			return dev_err_probe(dev, ret,
 					     "Cannot attach to DSI%d host.\n", i);
 		}