[PATCH] drm/i915: Allow NULL memory region

[Dan Carpenter]

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?

regards,
dan carpenter