On 29/07/2024 15:59, Cavitt, Jonathan wrote:
-----Original Message-----
From: Tvrtko Ursulin <tursulin@xxxxxxxxxxx>
Sent: Monday, July 29, 2024 1:21 AM
To: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
Cc: Cavitt, Jonathan <jonathan.cavitt@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Gupta, saurabhg <saurabhg.gupta@xxxxxxxxx>; chris.p.wilson@xxxxxxxxxxxxxxx
Subject: Re: [PATCH] drm/i915: Allow NULL memory region
On 26/07/2024 18:00, Dan Carpenter wrote:
On Fri, Jul 26, 2024 at 09:17:20AM +0100, Tvrtko Ursulin wrote:
On 25/07/2024 16:58, Dan Carpenter wrote:
On Thu, Jul 25, 2024 at 08:48:35AM +0100, Tvrtko Ursulin wrote:
Hi,
On 12/07/2024 22:41, Jonathan Cavitt wrote:
Prevent a NULL pointer access in intel_memory_regions_hw_probe.
For future reference please include some impact assessment in patches tagged
as fixes. Makes maintainers job, and even anyone's who tries to backport
stuff to stable at some future date, much easier if it is known how
important is the fix and in what circumstances can the problem it is fixing
trigger.
As someone doing backport work, I think this patch is fine. Everyone
knows the impact of a NULL dereference in probe().
I guess with patches that add NULL dereferences, the trick is
understanding when people are adding NULL checks to make a static
checker happy or when it's a real bug. But the fault lies with the
people adding NULL checks just to make the tools happy. Some of these
pointless NULL checks end up in stable, but it's fine, extra NULL checks
never hurt anyone. If the maintainer wants to be extra safe by adding
NULL checks then who are we to say otherwise.
In other words normal patches shouldn't have to say. "I'm not lying" at
the end. It should be the pointless patches which say, "I'm doing a
pointless thing. Don't bother backporting."
Most stable patch backports are done automatically and people have
various tools and scripts to do that. If the tools don't handle this
patch automatically then they are defective.
Right, and every few releases maintainers and authors get a bunch of emails
for patches which did not apply to some stable tree.
I believe these emails are only sent for commits that are tagged for
stable. For AUTOSEL patches, the backporting is done on a best effort
basis. On the other hand, hopefully this patch would have been tagged
for stable if we hadn't fixed the bug so quickly.
In which case someone has to do manual work and then it is good to know how
important it is to backport something. For cases when it is not trivial. It
does not apply to this patch, but as a _best practice_ it is good if the
commit message explains the impacted platforms and scenarios.
In this case I can follow the Fixes: tag and see the fix that this patches
fixes is only about ATS-M. Which if it was a more complicated patch might be
a reason to not need bother backporting past some kernel version where
platform X wasn't even supported.
Therefore I think my point is that best practice is to include this the
commit text, so any future maintainer/backporter does not have to re-do
detective work, stands.
This is a really elaborate hypothetical. Are there kernels which are
affected by this bug but don't support ATS-M?
I am not sure why are we arguing against the value of putting a bit more
info in commit messages.
When I was writing up the drm-intel-next-fixes pull request I already
had to follow the Fixes: chain for this one to understand the impact.
This patch is already in and all but from my point of view best practice
still is for commit messages to be a bit more verbose than "fix null
pointer deref". At least when fixes are coming from inside Intel I think
we can assume people have enough info to asses and document.
For future reference, what kind of additional information would you have
preferred been added to this patch that was not originally provided, and in
what location should that information have been added (as a part of the
commit message itself, after the Fixes tag, etc.)?
In this particular case something as simple as below would have made my
job a little bit easier:
drm/i915: Allow NULL memory region
Prevent a NULL pointer access in intel_memory_regions_hw_probe which
can happen on some ATS-M machines with specific BIOS configurations.
(And it may be wrong what I added but hey-ho, that's kind of the point
of getting the information direct from the source instead of having to
figure it out when writing pull requests.)
Regards,
Tvrtko
