Re: [PATCH v2 0/2] Support BMG PMT features for Xe

["David E. Box" <david.e.box@linux.intel.com>]

On Wed, 2024-11-13 at 20:59 +0200, Andy Shevchenko wrote:
> On Wed, Nov 13, 2024 at 10:40:42AM -0800, David E. Box wrote:
> > On Wed, 2024-11-13 at 15:52 +0200, Ilpo Järvinen wrote:
> > > On Wed, 13 Nov 2024, Andy Shevchenko wrote:
> > > > On Tue, Nov 12, 2024 at 11:30:33AM -0500, Michael J. Ruhl wrote:
> > > > > Updates for PMT to support user offsets from the sysfs API.
> > > > > 
> > > > > Addressed review comments for the Xe driver udpates.
> > > > 
> > > > FWIW,
> > > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > 
> > > > If you have wish and time, there are problems with the drivers of
> > > > different
> > > > severities (from "fine as is" to "good to be fixed, but okay as is") I
> > > > have
> > > > noticed so far:
> > > > - it uses s*printf() instead of sysfs_emit*()
> > > > - it most likely never tested the corner cases. e.g.,
> > > > 
> > > > 	if (disc_res->start >= pci_resource_start(pci_dev, i) &&
> > > > 	    (disc_res->start <= pci_resource_end(pci_dev, i))) {
> > > > 
> > > >   what is this supposed to mean? Probably someone wanted
> > > > resource_contains()
> > > > or
> > > >   alike to be called here.
> > 
> > This is a corner case that occurs for devices that are non-compliant, in
> > this
> > case meaning devices that don't follow our PMT spec convention of specifying
> > which BAR an address belongs to. Without this information, we have to deduce
> > the
> > BAR manually to access other needed registers that are offset from the base
> > of
> > that BAR.
> 
> What I am pointing out is that we compare start address (and only start!) to
> both, start _and_ end of the given resource. So currently the second check is
> redundant and that looks suspicious. I believe one wanted to have
> 
> 	if (disc_res->start >= pci_resource_start(pci_dev, i) &&
> 	    (disc_res->end <= pci_resource_end(pci_dev, i))) {

I see. The assumption was that the device didn't provide us an address range
that crosses BARs, so testing that the start address is within a BAR would be
enough. But the above is a better check in case that's not true which would be a
hardware bug.

David

> 
> (note end!) and if using helpers, this would never happened :-)
> 
> > I can change this to use resource_contains().
> 
> Please, will clarify the above confusion..
> 
> > > > - slightly above the above piece the for-loop
> > > > 
> > > > 	for (i = 0; i < 6; i++)
> > > > 
> > > >   which probably want to use PCI_STD_RESOURCE_END)
> > > 
> > > While both work, in practice PCI_STD_NUM_BARS is way more common than 
> > > PCI_STD_RESOURCE_END.
> > 
> > Will change this too. Thanks.
> 
> You are welcome!
>