Re: [PATCH v1] i2c: designware: amdisp: Fix null pointer dereference in runtime resume

["Nirujogi, Pratap" <pnirujog@amd.com>]

On 3/20/2026 3:59 AM, Andy Shevchenko wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On Thu, Mar 19, 2026 at 05:09:55PM -0400, Nirujogi, Pratap wrote:
>> On 3/9/2026 9:29 PM, Mario Limonciello wrote:
>>> On 3/9/2026 5:00 PM, Pratap Nirujogi wrote:
> 
> ...
> 
>>>> Fixes: 02c057ddefef ("ACPI: video: Convert the driver to a platform one")
>>>
>>> Is this the right commit that introduced the race?  Did it change the
>>> timing?
>>>
>>> Or was the race always there and we just got lucky until that commit
>>> went in?
>>>
>> My apologies for mixing up this change along with other changes that were
>> needed to fix the AMD ISP driver regressions in v7.0.
>>
>> I investigated further and found that this issue was exposed by the below
>> commit.
>>
>> https://github.com/torvalds/linux/commit/38fa29b01a6a295aedb69d1bbdad70acd7d204c6
> 
> Still it might be not a culprit as at that point the device should be in power
> on state.
> 
>> However, the resume‑probe race condition existed even earlier and had not
>> surfaced because device initialization in resume is done only when init
>> handle is valid.
> 
> That's what the initial cause, the AMD ISP driver initially relies on
> the broken (racy) behaviour. So, the original patch d6263c468a761 does not
> explain that bit. You need to go deeper and understand why the heck that
> condition is present there.
> 
>> Here is some background and context on this change:
> 
> Exactly, the below seems that you have nailed it down.
> 
>> The amdisp i2c device requires ISP to be in power on state for probe to
>> succeed. To meet this requirement, we added this device, which is a amdgpu
>> MFD child to the genpd, to control isp power using runtime PM. The
>> pm_runtime_get_sync() call before probe triggers a PM resume, which powers
>> on the ISP and also invokes the amdisp i2c runtime resume before the probe
>> completes resulting in this race condition.
>>
>> To fix this issue properly, I’m considering this change. Please review and
>> share your feedback on whether this makes sense.
>>
>> - Call dev_pm_genpd_resume() to Power ON ISP before probe
>> - Call dev_pm_genpd_suspend() to Power OFF ISP after probe
>> - Call pm_runtime_enable() after probe is successful to take care of system
>> suspend/resume.
> 
> I don't know the scope of dev_pm_genpd_*() usage, but the description of
> the proposed change sounds sane. And definitely it's much much better than
> previous attempt.
> 
Thanks, Andy, for the feedback and comments. I’ll send out v2 with the 
proposed changes.

Thanks,
Pratap

> --
> With Best Regards,
> Andy Shevchenko
> 
>