Re: [PATCH] drm/xe/i2c: Report i2c irq handler issue

[Raag Jadav <raag.jadav@intel.com>]

On Thu, Jun 04, 2026 at 07:52:56PM +0530, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Jadav, Raag <raag.jadav@intel.com> 
> > On Thu, Jun 04, 2026 at 04:10:07AM +0800, Jonathan Cavitt wrote:
> > > xe_i2c_irq_handler calls generic_handle_irq_safe, which can return an
> > 
> > Use '()' for functions.
> > 
> > > error value.  In all other cases, this is passed to a
> > 
> > Why the early wrapping? Looks like the next word can fit here.
> > 
> > > .*_err_ratelimited error message helper.  Add the missing error handler.
> > 
> > There's no change in the code path so it wasn't "missing".
> > I'd rephrase this as "Log the error".
> 
> It's supposed to be there, but it isn't.  That is, by definition, "missing",
> regardless of its effective impact.
> But I can at least s/handler/logger.

Yes, they are different in _both_ definition and impact ;)

> > > This issue was caught by static analysis.
> > 
> > This is redundant information but I'll leave it to you.
> > 
> > > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> > > Cc: Raag Jadav <raag.jadav@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_i2c.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_i2c.c b/drivers/gpu/drm/xe/xe_i2c.c
> > > index 706783863d07..acfc171483b2 100644
> > > --- a/drivers/gpu/drm/xe/xe_i2c.c
> > > +++ b/drivers/gpu/drm/xe/xe_i2c.c
> > > @@ -177,12 +177,15 @@ static bool xe_i2c_irq_present(struct xe_device *xe)
> > >  void xe_i2c_irq_handler(struct xe_device *xe, u32 master_ctl)
> > >  {
> > >  	struct xe_mmio *mmio = xe_root_tile_mmio(xe);
> > > +	int ret;
> > >  
> > >  	if (!(master_ctl & I2C_IRQ) || !xe_i2c_irq_present(xe))
> > >  		return;
> > >  
> > >  	/* Forward interrupt to I2C adapter */
> > > -	generic_handle_irq_safe(xe->i2c->adapter_irq);
> > > +	ret = generic_handle_irq_safe(xe->i2c->adapter_irq);
> > > +	if (ret)
> > > +		drm_err_ratelimited(&xe->drm, "error handling i2c irq: %d\n", ret);
> > 
> > drm_err_ratelimited() already logs "error" so I'd rephrase this as
> > "failed to handle i2c irq".
> 
> The current phrasing is consistent with all other uses of .*_err_ratelimited()
> with respect to generic_handle_irq_safe(), so I won't be changing it.

They are to be refactored, I just haven't got to it yet.

Raag

> > >  	/* Deassert after I2C adapter clears the interrupt */
> > >  	xe_mmio_rmw32(mmio, I2C_CONFIG_CMD, 0, PCI_COMMAND_INTX_DISABLE);
> > > -- 
> > > 2.53.0
> > > 
> >