From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Sui Jingfeng <sui.jingfeng@linux.dev>
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: Tue, 30 Apr 2024 17:32:37 +0300 [thread overview]
Message-ID: <ZjEBBRK7eoY4I0Gg@smile.fi.intel.com> (raw)
In-Reply-To: <2599705c-0a64-4742-b1d7-330e9fde6e7a@linux.dev>
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.
--
With Best Regards,
Andy Shevchenko
next prev parent reply other threads:[~2024-04-30 14:32 UTC|newest]
Thread overview: 39+ 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-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 [this message]
2024-04-30 17:24 ` Sui Jingfeng
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=ZjEBBRK7eoY4I0Gg@smile.fi.intel.com \
--to=andriy.shevchenko@linux.intel.com \
--cc=airlied@gmail.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=sui.jingfeng@linux.dev \
--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).