RE: [PATCH v2] EDAC/versalnet: Report PFN and page offset for DDR errors

[Zhuo, Qiuxu]

> From: Shubhrajyoti Datta <shubhrajyoti.datta@xxxxxxxxx>
> [...]
> Subject: Re: [PATCH v2] EDAC/versalnet: Report PFN and page offset for DDR
> [...]
> > > -     if (stat->error_type == MC5_ERR_TYPE_UE) {
> > > -             pinf = stat->ueinfo[stat->channel];
> > > -             snprintf(priv->message, sizeof(priv->message),
> > > -                      "Error type:%s controller %d Addr at %lx\n",
> > > -                      "UE", ctl_num, convert_to_physical(priv, pinf,
> > > ctl_num, error_data));
> > > +     pa = convert_to_physical(priv, pinf, ctl_num, error_data);
> >
> > I noticed that the return comment of convert_to_physical() says
> > "physical address of the DDR memory" [1]. I'm just curious if this refers to:
> >
> >   - the DDR address space (DRAM-only, without MMIO holes), or
> >   - the system physical address space (including MMIO holes)
> 
> It is the second one that it returns.
> Let me know if the below comment should be updated
> 
> - * Returns physical address of the DDR memory
> + * Returns the system physical address corresponding to the reported DDR
> error.
> 

Yes, please update the return comment accordingly. 
Other than that, LGTM. Thanks!

   Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@xxxxxxxxx>

> >
> > If it's the former, it may need to convert it to the latter one by
> > properly adding back the MMIO hole(s), since the OS uses system physical
> addresses.
> >