Re: [PATCH v3] drm/xe: Handle unreliable MMIO reads during forcewake

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Oct 17, 2024 at 10:25:46PM +0000, Lin, Shuicheng wrote:
> 
> On Thu, Oct 17, 2024 10:51 AM Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com> wrote:
> > On 17-10-2024 22:00, Shuicheng Lin wrote:
> > > In some cases, when the driver attempts to read an MMIO register, the
> > > hardware may return 0xFFFFFFFF. The current force wake path code
> > > treats this as a valid response, as it only checks the BIT.
> > > However, 0xFFFFFFFF should be considered an invalid value, indicating
> > > a potential issue. To address this, we should add a log entry to
> > > highlight this condition and return failure.
> > > The force wake failure log level is changed from notice to err to
> > > match the failure return value.
> > >
> > > v2 (Matt Brost):
> > >    - set ret value (-EIO) to kick the error to upper layers
> > > v3 (Rodrigo):
> > >    - add commit message for the log level promotion from notice to err
> > 
> > Feel free to retain RB on this version.
> > 
> > As a general practice, creating a new version solely for commit message or
> > changelog updates is avoided, as this triggers a full CI run. Making changes
> > before pushing and running checkpatch should be sufficient.
> > 
> > BR
> > Himal
> 
> Himal, Thanks very much for your experience sharing. It is really good for me as I am still new with the process.
> Yes. Maintainer could help update the commit message during push stage, but it will cost extra effort for the maintainer.
> Unless maintainer explicitly offer it, I still prefer to correct the commit message by myself.
> For the RB, I forgot to update it when I update this patch. Let me add it and send a new one. :(
> I will try my best to minimize the patch version number, and it is still a long way to reach it.

Well, 3 comments:

1. The versioning in the commit message itself is a questionable procedure.
Most of maintainers in Linux ask to not add it and only mention after '---'.
We here opt of keeping the whole history together.

2. We also try to minimize the changes in the patch when applying the patch.
Otherwise the history is lost and we won't be 100% sure if the CI results is
exactly for that patch. So, resending was a good thing.

3. Patch pushed. Thank you!

> 
> Shuicheng
> > 
> > >
> > > Suggested-by: Alex Zuo <alex.zuo@intel.com>
> > > Signed-off-by: Shuicheng Lin <shuicheng.lin@intel.com>
> > > Cc: Matthew Brost <matthew.brost@intel.com>
> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >   drivers/gpu/drm/xe/xe_force_wake.c | 12 +++++++++---
> > >   1 file changed, 9 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_force_wake.c
> > > b/drivers/gpu/drm/xe/xe_force_wake.c
> > > index a64c14757c84..08621717b14b 100644
> > > --- a/drivers/gpu/drm/xe/xe_force_wake.c
> > > +++ b/drivers/gpu/drm/xe/xe_force_wake.c
> > > @@ -115,9 +115,15 @@ static int __domain_wait(struct xe_gt *gt, struct
> > xe_force_wake_domain *domain,
> > >   			     XE_FORCE_WAKE_ACK_TIMEOUT_MS *
> > USEC_PER_MSEC,
> > >   			     &value, true);
> > >   	if (ret)
> > > -		xe_gt_notice(gt, "Force wake domain %d failed to ack %s
> > (%pe) reg[%#x] = %#x\n",
> > > -			     domain->id, str_wake_sleep(wake), ERR_PTR(ret),
> > > -			     domain->reg_ack.addr, value);
> > > +		xe_gt_err(gt, "Force wake domain %d failed to ack %s (%pe)
> > reg[%#x] = %#x\n",
> > > +			  domain->id, str_wake_sleep(wake), ERR_PTR(ret),
> > > +			  domain->reg_ack.addr, value);
> > > +	if (value == ~0) {
> > > +		xe_gt_err(gt,
> > > +			  "Force wake domain %d: %s. MMIO unreliable
> > (forcewake register returns 0xFFFFFFFF)!\n",
> > > +			  domain->id, str_wake_sleep(wake));
> > > +		ret = -EIO;
> > > +	}
> > >
> > >   	return ret;
> > >   }
>