[PATCH] drm/i915: Allow NULL memory region

[Tvrtko Ursulin]

On 29/07/2024 15:59, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Tvrtko Ursulin <tursulin at ursulin.net>
> Sent: Monday, July 29, 2024 1:21 AM
> To: Dan Carpenter <dan.carpenter at linaro.org>
> Cc: Cavitt, Jonathan <jonathan.cavitt at intel.com>; intel-gfx at lists.freedesktop.org; Gupta, saurabhg <saurabhg.gupta at intel.com>; chris.p.wilson at linux.intel.com
> 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