Message-ID: <960adb98-9f66-4d62-88b2-3e512601459f@linux.dev>
Date: Wed, 1 May 2024 01:24:09 +0800
From: Sui Jingfeng <sui.jingfeng@...ux.dev>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Neil Armstrong <neil.armstrong@...aro.org>,
 Randy Dunlap <rdunlap@...radead.org>, dri-devel@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org, Jessica Zhang <quic_jesszhan@...cinc.com>,
 Sam Ravnborg <sam@...nborg.org>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>
Subject: Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs

Hi,


On 2024/4/30 22:32, Andy Shevchenko wrote:
> On Fri, Apr 26, 2024 at 04:43:18AM +0800, Sui Jingfeng wrote:
>> On 2024/4/26 03:10, Andy Shevchenko wrote:
>>> On Fri, Apr 26, 2024 at 02:08:16AM +0800, Sui Jingfeng wrote:
>>>> On 2024/4/25 22:26, Andy Shevchenko wrote:
>>>>> It seems driver missed the point of proper use of device property APIs.
>>>>> Correct this by updating headers and calls respectively.
>>>> You are using the 'seems' here exactly saying that you are not 100% sure.
> To add here, "seems" is used to show that I have no knowledge on what was
> the idea behind this implementation by the original author. Plus my knowledge
> in the firmware node / device property APIs and use cases in Linux kernel.
>
>>>> Please allow me to tell you the truth: This patch again has ZERO effect.
>>>> It fix nothing. And this patch is has the risks to be wrong.
>>> Huh?! Really, stop commenting the stuff you do not understand.
>> I'm actually a professional display drivers developer at the downstream
>> in the past, despite my contribution to upstream is less. But I believe
>> that all panel driver developers know what I'm talking about. So please
>> have take a look at my replies.
> Okay.
>
>>>> Simple because the "ili9341_probe() ---> ili9341_dbi_prob()" code path
>>>> is DT dependent.
>>>>
>>>> First of all, the devm_of_find_backlight() is called in ili9341_dbi_probe()
>>>> under *non-DT* environment, devm_of_find_backlight() is just a just a
>>>> no-op and will return NULL. NULL is not an error code, so ili9341_dbi_probe()
>>>> won't rage quit. But the several side effect is that the backlight will
>>>> NOT works at all.
>>> Is it a problem?
>> Yes, it is.
>>
>> The core problem is that the driver you are modifying has *implicit* *dependency* on DT.
>> The implicit dependency is due to the calling of devm_of_find_backlight(). This function
>> is a no-op under non-DT systems.
> Okay.
>
>> Therefore, before the devm_of_find_backlight() and
>> the device_get_match_data() function can truly DT independent.
> True for the first part, not true for the second.
>
>> Removing the "OF" dependency just let the tigers run out from the jail.
>>
>> It is not really meant to targeting at you, but I thinks, all of drm_panel drivers
>> that has the devm_of_find_backlight() invoked will suffer such concerns.
>> In short, the reason is that the *implicit* *dependency* populates and
>> the undefined behavior gets triggered.
> Still no problem statement. My hardware works nicely on non-DT environment.
> (And since it's Arduino-based one, I assume it will work on DT environments
>   the very same way.)
>
>> I'm sure you know that device_get_match_data() is same with of_device_get_match_data()
>> for DT based systems. For non DT based systems, device_get_match_data() is just *undefined*
>> Note that ACPI is not in the scope of the discussion here, as all of the drm bridges and
>> panels driver under drivers/gpu/drm/ hasn't the ACPI support yet.
> This patch shows exactly how to bring back the ACPI support to one of them
> (as it's done for tinyDRM cases).
>
>> Therefore, at present,
>> it safe to say that device_get_match_data() is *undefined* under no-DT environment.
> This is not true.
>
>> Removing the "OF" dependency hints to us that it allows the driver to be probed as a
>> pure SPI device under non DT systems. When device_get_match_data() is called, it returns
>> NULL to us now. As a result, the drm driver being modified will tears down.
>>
>> See bellow code snippet extracted frompanel-ilitek-ili9341.c:
>>
>>
>> ```
>> 	ili->conf = of_device_get_match_data(dev);
>> 	if (!ili->conf) {
>> 		dev_err(dev, "missing device configuration\n");
>> 		return -ENODEV;
>> 	}
>> ```
>>
>>>> It is actually considered as fatal bug for *panels* if the backlight of
>>>> it is not light up, at least the brightness of *won't* be able to adjust.
>>>> What's worse, if there is no sane platform setup code at the firmware
>>>> or boot loader stage to set a proper initial state. The screen is complete
>>>> dark. Even though the itself panel is refreshing framebuffers, it can not
>>>> be seen by human's eye. Simple because of no backlight.
>>> Can you imagine that I may have different hardware that considered
>>> this is non-fatal error?
>>>
>> Yes, I can imagine.
>>
>> I believe you have the hardware which make you patch correct to run
>> in 99.9% of all cases. But as long as there one bug happened, you patch
>> are going to be blamed.
>>
>> Because its your patch that open the door, both from the perceptive of
>> practice and from the perceptive of the concept (static analysis).
>>
>>>> Second, the ili9341_dbi_probe() requires additional device properties to
>>>> be able to works very well on the rotation screen case. See the calling
>>>> of "device_property_read_u32(dev, "rotation", &rotation)" in
>>>> ili9341_dbi_probe() function.
>>> Yes, exactly, and how does it object the purpose of this patch?
>> Because under *non-DT* environment, your commit message do not give a
>> valid description, how does the additional device property can be acquired
>> is not demonstrated.
>>
>> And it is exactly your patch open the non-DT code path (way or possibility).
>> It isn't has such risks before your patch is applied. In other words,
>> previously, the driver has the 'OF' dependency as the guard, all of the
>> potential risk(or problem) are suppressed. It is a extremely safe policy,
>> and it is also a extremely perfect defend.
>>
>> And suddenly, you patch release the dangerous tiger from the cage.
>> So I think you can imagine...
> No, I can't, sorry. I don't see how dangerous will be the use of DRM panel
> in a wrong configuration. The same can very well happen on improperly working
> hardware (backlight part) or simply when somebody didn't correctly set a DT
> or manually use it when it should not be. But again I see *no* problem
> statement, only some worries.
>
> And on top of that I made tinyDRM drivers to be accessible on ACPI platforms
> and so far I have none complains about the tigers that left the cage.
>
>>>> Combine with those two factors, it is actually can conclude that the
>>>> panel-ilitek-ili9394 driver has the *implicit* dependency on 'OF'.
>>>> Removing the 'OF' dependency from its Kconfig just trigger the
>>>> leakage of such risks.
>>> What?!
>>>
>> Posting a patch is actually doing the defensive works, such a saying
>> may not sound fair for you, but this is just the hash cruel reality.
>> Sorry for saying that. :(
> So, the summary of your message is that:
> - there's no understanding how ACPI (or any other non-DT fwnode based
>    environment) can utilise the driver
> - there's a worry about some problems which can't be stated clearly
> - there's a neglecting of the previous successful cases specific for DRM
>    (tinyDRM drivers)
>
> As a result of the false input, the non-constructive conclusion was given.
>
> And note, I converted dozens if not hundredth of drivers that used to be
> OF-only and haven't heart any negative feedback before this case. Maybe
> we (reviewers of my patches and maintainers who applied them and end users)
> miss a BIG DEAL here? Please, elaborate how dropping OF dependency can be
> dangerous as a free walking tiger.
>
>>>> My software node related patches can help to reduce part of the potential
>>>> risks, but it still need some extra work. And it is not landed yet.
>>> Your patch has nothing to do with this series.
> I am not going to repeat the above.
>
>> With my patch applied, this is way to meet the gap under non-DT systems.
>> Users of this driver could managed to attach(complete) absent properties
>> to the SPI device with software node properties. Register the swnode
>> properties group into the system prior the panel driver is probed. There
>> may need some quirk. But at the least there has a way to go.  When there
>> has a way to go, things become self-consistent. Viewed from both the
>> practice of viewpoint and the concept of viewpoint.
>>
>> And the dangerous tiger will steer its way to the direction of "ACPI
>> support is missing". But both of will be safe then.


I have no obvious opinions then, code inside this patch seems no obvious problem
for majority applications. Sorry about the noise and thanks for reply.


-- 
Best regards,
Sui

Powered by blists - more mailing lists