From: Sui Jingfeng <sui.jingfeng@linux.dev>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Neil Armstrong <neil.armstrong@linaro.org>,
Randy Dunlap <rdunlap@infradead.org>,
dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
Jessica Zhang <quic_jesszhan@quicinc.com>,
Sam Ravnborg <sam@ravnborg.org>,
Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
Maxime Ripard <mripard@kernel.org>,
Thomas Zimmermann <tzimmermann@suse.de>,
David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>
Subject: Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs
Date: Wed, 1 May 2024 01:24:09 +0800 [thread overview]
Message-ID: <960adb98-9f66-4d62-88b2-3e512601459f@linux.dev> (raw)
In-Reply-To: <ZjEBBRK7eoY4I0Gg@smile.fi.intel.com>
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
next prev parent reply other threads:[~2024-04-30 17:24 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-25 14:26 [PATCH v1 0/3] drm/panel: ili9341: Obvious fixes Andy Shevchenko
2024-04-25 14:26 ` [PATCH v1 1/3] drm/panel: ili9341: Correct use of device property APIs Andy Shevchenko
2024-04-25 16:00 ` Neil Armstrong
2024-04-25 18:08 ` [v1,1/3] " Sui Jingfeng
2024-04-25 18:53 ` Sui Jingfeng
2024-04-25 19:12 ` Andy Shevchenko
2024-04-25 21:13 ` Sui Jingfeng
2024-04-30 14:13 ` Andy Shevchenko
2024-04-30 16:27 ` Sui Jingfeng
2024-05-02 8:32 ` Andy Shevchenko
2024-05-02 16:25 ` Sui Jingfeng
2024-05-02 17:28 ` Andy Shevchenko
2024-05-03 4:57 ` Sui Jingfeng
2024-05-03 15:46 ` Andy Shevchenko
2024-04-25 19:10 ` Andy Shevchenko
2024-04-25 20:43 ` Sui Jingfeng
2024-04-25 21:27 ` Sui Jingfeng
2024-04-26 6:23 ` Maxime Ripard
2024-04-26 10:11 ` Sui Jingfeng
2024-04-27 5:57 ` Sui Jingfeng
2024-04-29 11:55 ` Maxime Ripard
2024-04-29 16:54 ` Sui Jingfeng
2024-04-30 9:34 ` Maxime Ripard
2024-05-02 7:34 ` Neil Armstrong
2024-05-02 8:33 ` Andy Shevchenko
2024-05-02 17:28 ` Sui Jingfeng
2024-05-02 17:38 ` Andy Shevchenko
2024-04-30 14:32 ` Andy Shevchenko
2024-04-30 17:24 ` Sui Jingfeng [this message]
2024-04-30 10:19 ` [PATCH v1 1/3] " Dmitry Baryshkov
2024-04-25 14:26 ` [PATCH v1 2/3] drm/panel: ili9341: Respect deferred probe Andy Shevchenko
2024-04-25 16:00 ` Neil Armstrong
2024-04-30 10:21 ` Dmitry Baryshkov
2024-04-30 16:50 ` [v1,2/3] " Sui Jingfeng
2024-04-25 14:26 ` [PATCH v1 3/3] drm/panel: ili9341: Use predefined error codes Andy Shevchenko
2024-04-25 16:00 ` Neil Armstrong
2024-04-30 16:54 ` [v1,3/3] " Sui Jingfeng
2024-04-25 15:08 ` [PATCH v1 0/3] drm/panel: ili9341: Obvious fixes Andy Shevchenko
2024-05-02 7:43 ` Neil Armstrong
2024-05-02 8:33 ` Andy Shevchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=960adb98-9f66-4d62-88b2-3e512601459f@linux.dev \
--to=sui.jingfeng@linux.dev \
--cc=airlied@gmail.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maarten.lankhorst@linux.intel.com \
--cc=mripard@kernel.org \
--cc=neil.armstrong@linaro.org \
--cc=quic_jesszhan@quicinc.com \
--cc=rdunlap@infradead.org \
--cc=sam@ravnborg.org \
--cc=tzimmermann@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).