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 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.

I can change this to use resource_contains().

> > - 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.

David