| Message ID | 20241112163035.2282499-1-michael.j.ruhl@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Support BMG PMT features for Xe | expand |
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. Not sure where the changelog (v1 --> v2) is...
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. - slightly above the above piece the for-loop for (i = 0; i < 6; i++) which probably want to use PCI_STD_RESOURCE_END)
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. > - 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.
On Wed, Nov 13, 2024 at 03:52:01PM +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. > > - 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. Ah, indeed. I always forget that we have two.
> -----Original Message----- > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Sent: Wednesday, November 13, 2024 5:39 AM > To: Ruhl, Michael J <michael.j.ruhl@intel.com> > Cc: intel-xe@lists.freedesktop.org; platform-driver-x86@vger.kernel.org; > david.e.box@linux.intel.com; ilpo.jarvinen@linux.intel.com; > hdegoede@redhat.com; Vivi, Rodrigo <rodrigo.vivi@intel.com>; De Marchi, > Lucas <lucas.demarchi@intel.com> > Subject: Re: [PATCH v2 0/2] Support BMG PMT features for Xe > > 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> Thank you! > If you have wish and time, there are problems with the drivers of different I am working on this patch set in my spare time...If anything loosens up I will keep this in mind. M > 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. > - slightly above the above piece the for-loop > > for (i = 0; i < 6; i++) > > which probably want to use PCI_STD_RESOURCE_END) > > -- > With Best Regards, > Andy Shevchenko >
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
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))) { (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!
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! >