Re: [PATCH v7 1/2] i2c: qcom-geni: Handle runtime PM disabled state during early resume

[Praveen Talari]

Hi Mukesh

On 24-07-2026 12:05, Mukesh Savaliya wrote:
> On 7/24/2026 10:12 AM, Praveen Talari wrote:
> > Hi
> >
> > On 23-07-2026 17:14, Mukesh Savaliya wrote:
> > > On 7/23/2026 4:24 PM, Praveen Talari wrote:
> > > > Hi mukesh
> > > >
> > > > On 23-07-2026 15:47, Mukesh Savaliya wrote:
> > > > > Hi Praveen,
> > > > >
> > > > > On 7/23/2026 3:32 PM, Praveen Talari wrote:
> > > > > > Hi Mukesh
> > > > > [...]
> > > > >
> > > > > > > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/ 
> > > > > > > busses/i2c-qcom-geni.c
> > > > > > > index 96dbf04138be..4bc00922cd97 100644
> > > > > > > --- a/drivers/i2c/busses/i2c-qcom-geni.c
> > > > > > > +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> > > > > > > @@ -917,6 +917,10 @@ static int geni_i2c_xfer(struct i2c_adapter 
> > > > > > > *adap,
> > > > > > >       gi2c->err = 0;
> > > > > > >       reinit_completion(&gi2c->done);
> > > > > > >       ret = pm_runtime_get_sync(gi2c->se.dev);
> > > > > > > +    if (ret == -EACCES) {
> > > > > > > +        dev_warn(gi2c->se.dev, "Runtime PM is disabled:%d\n", 
> > > > > > > ret);
> > > > > > > +        ret = 0;
> > > > > >
> > > > > > Here it is clear that pm_runtime_get_sync() is returning -EACCES 
> > > > > > from the runtime PM framework,
> > > > > >
> > > > > > which indicates that the request was rejected before the runtime 
> > > > > > resume path could enable any resources.
> > > > > >
> > > > > >  From the rpm_resume() implementation:
> > > > > >
> > > > > > static int rpm_resume(struct device *dev, int rpmflags) 
> > > > > > __releases(&dev-  >power.lock) __acquires(&dev->power.lock) { 
> > > > > > [...] repeat: if (dev-  >power.runtime_error) { retval = -EINVAL; 
> > > > > > } else if (dev-  >power.disable_depth > 0) { [...] else retval = 
> > > > > > - EACCES; } if (retval) goto out; [...] out: if (parent && !dev- 
> > > > > > >power.irq_safe) { spin_unlock_irq(&dev->power.lock); 
> > > > > > pm_runtime_put(parent); spin_lock_irq(&dev->power.lock); } 
> > > > > > trace_rpm_return_int(dev, _THIS_IP_, retval); return retval; }
> > > > > >
> > > > > > The -EACCES error is returned when runtime PM is disabled 
> > > > > > (disable_depth  > 0),
> > > > > >
> > > > > > causing the function to exit without invoking the device's 
> > > > > > runtime resume callback.
> > > > > >
> > > > > > As a result, no resource enablement is performed by the PM 
> > > > > > framework.
> > > > > >
> > > > > >
> > > > > > In your change, the error is effectively converted to ret = 0 and 
> > > > > > execution continues.
> > > > > >
> > > > > > How can we guarantee that the required resources have been 
> > > > > > enabled in this scenario?
> > > > > >
> > > > > > More importantly, where are those resources expected to be 
> > > > > > enabled if the runtime PM resume path was never executed?
> > > > >
> > > > > This was answered in V6 already on 7/3 exactly at same place.
> > > > > Pasting below.
> > > > >
> > > > > ==
> > > > > > Why we are checking specific error code here? Why can't we use 
> > > > > the below error check directly?
> > > > > > if get sync itself is failed with pm runtime disabled then why 
> > > > > we are going ahead by making ret = 0 here? How you will make sure 
> > > > > resources are enabled?
> > > > >
> > > > > This reason is also mentioned in the commit log. we surely get - 
> > > > > EACCESS as runtime PM is disabled during no_irq resume phase. We 
> > > > > still need to serve the transfer, hence we need to override it.
> > > >
> > > > We should not get it and not go further since you are already 
> > > > enabled in system_resume_no_irq().
> > > >
> > > > I just need to understand why we need to proceed when we 
> > > > get -EACCESS as runtime PM .
> > >
> > > ret = 0 was added because during early system resume, -EACCES from 
> > > pm_runtime_get_sync() indicates runtime PM is disabled, not that the 
> > > controller is unusable; treating it as an error would incorrectly 
> > > reject valid I2C transfers occurring during resume.
> >
> >
> > What is the purpose of enabling runtime PM during system resume? It 
> > is not clear how this helps, given that -EACCES can still be returned.
>
> I think you didn't get noirq_resume() change. if you review code, we 
> do force_resume(), but that doesn't invoke transfer time 
> runtime_resume() via pm_*_get_sync() call because device has runtime 
> PM disabled.
>
> Hence we need to call pm_runtime_enable(). In my testing, have seen 
> that driver returning with -EACCESS if not overridden with ret = 0. 
> That's the whole point. We need force resume and transfer to serve 
> early resume transfer. Usecase demands this transfer without failure.
I have gone through  noirq_resume() change.

force_resume will be executed only if force_suspend executed otherwise 
it won't execute.

IMO, If PM runtime enabled for device, it won't return -EACCESS.

What is the purpose of enabling runtime PM in the system resume 
callback? We still appear to encounter -EACCES despite runtime PM being 
enabled there.

> > > Other than early PM time, we are not going to get -EACESS. So i 
> > > don't see any case where this will be a problem.
> > > > > Review geni_i2c_resume_noirq() to know what all we do to enable 
> > > > > resources. That's guaranteed and tested with system suspend/resume 
> > > > > test back to back and PCIe could do i2c transfer successfully.
> > > > > ==
> > > > > > Thanks,
> > > > > >
> > > > > > Praveen Talari
> > > > > Do not keep this thanks/regards here. only leave the comments.
> > > > >
> > > > > > > +    }
> > > > > [...]