* [PATCH v5 05/11] drm/amdgpu: Use RMW accessors for changing LNKCTL [not found] <20230717120503.15276-1-ilpo.jarvinen@linux.intel.com> @ 2023-07-17 12:04 ` Ilpo Järvinen 2023-07-20 21:55 ` Bjorn Helgaas 2023-07-17 12:04 ` [PATCH v5 06/11] drm/radeon: " Ilpo Järvinen 1 sibling, 1 reply; 9+ messages in thread From: Ilpo Järvinen @ 2023-07-17 12:04 UTC (permalink / raw) To: linux-pci, Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczyński, Emmanuel Grumbach, Rafael J . Wysocki, Heiner Kallweit, Lukas Wunner, Andy Shevchenko, Alex Deucher, Christian König, Pan, Xinhui, David Airlie, Daniel Vetter, Jammy Zhou, Ken Wang, amd-gfx, dri-devel, linux-kernel Cc: Dean Luick, Ilpo Järvinen, stable, Jonas Dreßler Don't assume that only the driver would be accessing LNKCTL. ASPM policy changes can trigger write to LNKCTL outside of driver's control. And in the case of upstream bridge, the driver does not even own the device it's changing the registers for. Use RMW capability accessors which do proper locking to avoid losing concurrent updates to the register value. Fixes: a2e73f56fa62 ("drm/amdgpu: Add support for CIK parts") Fixes: 62a37553414a ("drm/amdgpu: add si implementation v10") Suggested-by: Lukas Wunner <lukas@wunner.de> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Cc: stable@vger.kernel.org --- drivers/gpu/drm/amd/amdgpu/cik.c | 36 +++++++++----------------------- drivers/gpu/drm/amd/amdgpu/si.c | 36 +++++++++----------------------- 2 files changed, 20 insertions(+), 52 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c b/drivers/gpu/drm/amd/amdgpu/cik.c index 5641cf05d856..e63abdf52b6c 100644 --- a/drivers/gpu/drm/amd/amdgpu/cik.c +++ b/drivers/gpu/drm/amd/amdgpu/cik.c @@ -1574,17 +1574,8 @@ static void cik_pcie_gen3_enable(struct amdgpu_device *adev) u16 bridge_cfg2, gpu_cfg2; u32 max_lw, current_lw, tmp; - pcie_capability_read_word(root, PCI_EXP_LNKCTL, - &bridge_cfg); - pcie_capability_read_word(adev->pdev, PCI_EXP_LNKCTL, - &gpu_cfg); - - tmp16 = bridge_cfg | PCI_EXP_LNKCTL_HAWD; - pcie_capability_write_word(root, PCI_EXP_LNKCTL, tmp16); - - tmp16 = gpu_cfg | PCI_EXP_LNKCTL_HAWD; - pcie_capability_write_word(adev->pdev, PCI_EXP_LNKCTL, - tmp16); + pcie_capability_set_word(root, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_HAWD); + pcie_capability_set_word(adev->pdev, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_HAWD); tmp = RREG32_PCIE(ixPCIE_LC_STATUS1); max_lw = (tmp & PCIE_LC_STATUS1__LC_DETECTED_LINK_WIDTH_MASK) >> @@ -1637,21 +1628,14 @@ static void cik_pcie_gen3_enable(struct amdgpu_device *adev) msleep(100); /* linkctl */ - pcie_capability_read_word(root, PCI_EXP_LNKCTL, - &tmp16); - tmp16 &= ~PCI_EXP_LNKCTL_HAWD; - tmp16 |= (bridge_cfg & PCI_EXP_LNKCTL_HAWD); - pcie_capability_write_word(root, PCI_EXP_LNKCTL, - tmp16); - - pcie_capability_read_word(adev->pdev, - PCI_EXP_LNKCTL, - &tmp16); - tmp16 &= ~PCI_EXP_LNKCTL_HAWD; - tmp16 |= (gpu_cfg & PCI_EXP_LNKCTL_HAWD); - pcie_capability_write_word(adev->pdev, - PCI_EXP_LNKCTL, - tmp16); + pcie_capability_clear_and_set_word(root, PCI_EXP_LNKCTL, + PCI_EXP_LNKCTL_HAWD, + bridge_cfg & + PCI_EXP_LNKCTL_HAWD); + pcie_capability_clear_and_set_word(adev->pdev, PCI_EXP_LNKCTL, + PCI_EXP_LNKCTL_HAWD, + gpu_cfg & + PCI_EXP_LNKCTL_HAWD); /* linkctl2 */ pcie_capability_read_word(root, PCI_EXP_LNKCTL2, diff --git a/drivers/gpu/drm/amd/amdgpu/si.c b/drivers/gpu/drm/amd/amdgpu/si.c index f64b87b11b1b..4b81f29e5fd5 100644 --- a/drivers/gpu/drm/amd/amdgpu/si.c +++ b/drivers/gpu/drm/amd/amdgpu/si.c @@ -2276,17 +2276,8 @@ static void si_pcie_gen3_enable(struct amdgpu_device *adev) u16 bridge_cfg2, gpu_cfg2; u32 max_lw, current_lw, tmp; - pcie_capability_read_word(root, PCI_EXP_LNKCTL, - &bridge_cfg); - pcie_capability_read_word(adev->pdev, PCI_EXP_LNKCTL, - &gpu_cfg); - - tmp16 = bridge_cfg | PCI_EXP_LNKCTL_HAWD; - pcie_capability_write_word(root, PCI_EXP_LNKCTL, tmp16); - - tmp16 = gpu_cfg | PCI_EXP_LNKCTL_HAWD; - pcie_capability_write_word(adev->pdev, PCI_EXP_LNKCTL, - tmp16); + pcie_capability_set_word(root, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_HAWD); + pcie_capability_set_word(adev->pdev, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_HAWD); tmp = RREG32_PCIE(PCIE_LC_STATUS1); max_lw = (tmp & LC_DETECTED_LINK_WIDTH_MASK) >> LC_DETECTED_LINK_WIDTH_SHIFT; @@ -2331,21 +2322,14 @@ static void si_pcie_gen3_enable(struct amdgpu_device *adev) mdelay(100); - pcie_capability_read_word(root, PCI_EXP_LNKCTL, - &tmp16); - tmp16 &= ~PCI_EXP_LNKCTL_HAWD; - tmp16 |= (bridge_cfg & PCI_EXP_LNKCTL_HAWD); - pcie_capability_write_word(root, PCI_EXP_LNKCTL, - tmp16); - - pcie_capability_read_word(adev->pdev, - PCI_EXP_LNKCTL, - &tmp16); - tmp16 &= ~PCI_EXP_LNKCTL_HAWD; - tmp16 |= (gpu_cfg & PCI_EXP_LNKCTL_HAWD); - pcie_capability_write_word(adev->pdev, - PCI_EXP_LNKCTL, - tmp16); + pcie_capability_clear_and_set_word(root, PCI_EXP_LNKCTL, + PCI_EXP_LNKCTL_HAWD, + bridge_cfg & + PCI_EXP_LNKCTL_HAWD); + pcie_capability_clear_and_set_word(adev->pdev, PCI_EXP_LNKCTL, + PCI_EXP_LNKCTL_HAWD, + gpu_cfg & + PCI_EXP_LNKCTL_HAWD); pcie_capability_read_word(root, PCI_EXP_LNKCTL2, &tmp16); -- 2.30.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v5 05/11] drm/amdgpu: Use RMW accessors for changing LNKCTL 2023-07-17 12:04 ` [PATCH v5 05/11] drm/amdgpu: Use RMW accessors for changing LNKCTL Ilpo Järvinen @ 2023-07-20 21:55 ` Bjorn Helgaas 2023-07-21 8:07 ` Ilpo Järvinen 0 siblings, 1 reply; 9+ messages in thread From: Bjorn Helgaas @ 2023-07-20 21:55 UTC (permalink / raw) To: Ilpo Järvinen Cc: Krzysztof Wilczyński, Emmanuel Grumbach, Jammy Zhou, linux-pci, dri-devel, David Airlie, Rob Herring, Lorenzo Pieralisi, Jonas Dreßler, Rafael J . Wysocki, amd-gfx, Dean Luick, Bjorn Helgaas, Andy Shevchenko, Pan, Xinhui, linux-kernel, stable, Lukas Wunner, Daniel Vetter, Alex Deucher, Ken Wang, Christian König, Heiner Kallweit On Mon, Jul 17, 2023 at 03:04:57PM +0300, Ilpo Järvinen wrote: > Don't assume that only the driver would be accessing LNKCTL. ASPM > policy changes can trigger write to LNKCTL outside of driver's control. > And in the case of upstream bridge, the driver does not even own the > device it's changing the registers for. > > Use RMW capability accessors which do proper locking to avoid losing > concurrent updates to the register value. > > Fixes: a2e73f56fa62 ("drm/amdgpu: Add support for CIK parts") > Fixes: 62a37553414a ("drm/amdgpu: add si implementation v10") > Suggested-by: Lukas Wunner <lukas@wunner.de> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > Cc: stable@vger.kernel.org Do we have any reports of problems that are fixed by this patch (or by others in the series)? If not, I'm not sure it really fits the usual stable kernel criteria: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?id=v6.4 > --- > drivers/gpu/drm/amd/amdgpu/cik.c | 36 +++++++++----------------------- > drivers/gpu/drm/amd/amdgpu/si.c | 36 +++++++++----------------------- > 2 files changed, 20 insertions(+), 52 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c b/drivers/gpu/drm/amd/amdgpu/cik.c > index 5641cf05d856..e63abdf52b6c 100644 > --- a/drivers/gpu/drm/amd/amdgpu/cik.c > +++ b/drivers/gpu/drm/amd/amdgpu/cik.c > @@ -1574,17 +1574,8 @@ static void cik_pcie_gen3_enable(struct amdgpu_device *adev) > u16 bridge_cfg2, gpu_cfg2; > u32 max_lw, current_lw, tmp; > > - pcie_capability_read_word(root, PCI_EXP_LNKCTL, > - &bridge_cfg); > - pcie_capability_read_word(adev->pdev, PCI_EXP_LNKCTL, > - &gpu_cfg); > - > - tmp16 = bridge_cfg | PCI_EXP_LNKCTL_HAWD; > - pcie_capability_write_word(root, PCI_EXP_LNKCTL, tmp16); > - > - tmp16 = gpu_cfg | PCI_EXP_LNKCTL_HAWD; > - pcie_capability_write_word(adev->pdev, PCI_EXP_LNKCTL, > - tmp16); > + pcie_capability_set_word(root, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_HAWD); > + pcie_capability_set_word(adev->pdev, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_HAWD); > > tmp = RREG32_PCIE(ixPCIE_LC_STATUS1); > max_lw = (tmp & PCIE_LC_STATUS1__LC_DETECTED_LINK_WIDTH_MASK) >> > @@ -1637,21 +1628,14 @@ static void cik_pcie_gen3_enable(struct amdgpu_device *adev) > msleep(100); > > /* linkctl */ > - pcie_capability_read_word(root, PCI_EXP_LNKCTL, > - &tmp16); > - tmp16 &= ~PCI_EXP_LNKCTL_HAWD; > - tmp16 |= (bridge_cfg & PCI_EXP_LNKCTL_HAWD); > - pcie_capability_write_word(root, PCI_EXP_LNKCTL, > - tmp16); > - > - pcie_capability_read_word(adev->pdev, > - PCI_EXP_LNKCTL, > - &tmp16); > - tmp16 &= ~PCI_EXP_LNKCTL_HAWD; > - tmp16 |= (gpu_cfg & PCI_EXP_LNKCTL_HAWD); > - pcie_capability_write_word(adev->pdev, > - PCI_EXP_LNKCTL, > - tmp16); > + pcie_capability_clear_and_set_word(root, PCI_EXP_LNKCTL, > + PCI_EXP_LNKCTL_HAWD, > + bridge_cfg & > + PCI_EXP_LNKCTL_HAWD); > + pcie_capability_clear_and_set_word(adev->pdev, PCI_EXP_LNKCTL, > + PCI_EXP_LNKCTL_HAWD, > + gpu_cfg & > + PCI_EXP_LNKCTL_HAWD); Wow, there's a lot of pointless-looking work going on here: set root PCI_EXP_LNKCTL_HAWD set GPU PCI_EXP_LNKCTL_HAWD for (i = 0; i < 10; i++) { read root PCI_EXP_LNKCTL read GPU PCI_EXP_LNKCTL clear root PCI_EXP_LNKCTL_HAWD if (root PCI_EXP_LNKCTL_HAWD was set) set root PCI_EXP_LNKCTL_HAWD clear GPU PCI_EXP_LNKCTL_HAWD if (GPU PCI_EXP_LNKCTL_HAWD was set) set GPU PCI_EXP_LNKCTL_HAWD } If it really *is* pointless, it would be nice to clean it up, but that wouldn't be material for this patch, so what you have looks good. > /* linkctl2 */ > pcie_capability_read_word(root, PCI_EXP_LNKCTL2, The PCI_EXP_LNKCTL2 stuff also includes RMW updates. I don't see any uses of PCI_EXP_LNKCTL2 outside this driver that look relevant, so I guess we don't care about making the PCI_EXP_LNKCTL2 updates atomic? Bjorn ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 05/11] drm/amdgpu: Use RMW accessors for changing LNKCTL 2023-07-20 21:55 ` Bjorn Helgaas @ 2023-07-21 8:07 ` Ilpo Järvinen 2023-07-21 14:52 ` Alex Deucher 0 siblings, 1 reply; 9+ messages in thread From: Ilpo Järvinen @ 2023-07-21 8:07 UTC (permalink / raw) To: Bjorn Helgaas Cc: Krzysztof Wilczyński, Emmanuel Grumbach, Jammy Zhou, linux-pci, dri-devel, David Airlie, Rob Herring, Lorenzo Pieralisi, Jonas Dreßler, Rafael J . Wysocki, amd-gfx, Dean Luick, Bjorn Helgaas, Andy Shevchenko, Pan, Xinhui, LKML, stable, Lukas Wunner, Daniel Vetter, Alex Deucher, Ken Wang, Christian König, Heiner Kallweit [-- Attachment #1: Type: text/plain, Size: 5914 bytes --] On Thu, 20 Jul 2023, Bjorn Helgaas wrote: > On Mon, Jul 17, 2023 at 03:04:57PM +0300, Ilpo Järvinen wrote: > > Don't assume that only the driver would be accessing LNKCTL. ASPM > > policy changes can trigger write to LNKCTL outside of driver's control. > > And in the case of upstream bridge, the driver does not even own the > > device it's changing the registers for. > > > > Use RMW capability accessors which do proper locking to avoid losing > > concurrent updates to the register value. > > > > Fixes: a2e73f56fa62 ("drm/amdgpu: Add support for CIK parts") > > Fixes: 62a37553414a ("drm/amdgpu: add si implementation v10") > > Suggested-by: Lukas Wunner <lukas@wunner.de> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > Cc: stable@vger.kernel.org > > Do we have any reports of problems that are fixed by this patch (or by > others in the series)? If not, I'm not sure it really fits the usual > stable kernel criteria: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?id=v6.4 I was on the edge with this. The answer to your direct question is no, there are no such reports so it would be okay to leave stable out I think. This applies to all patches in this series. Basically, this series came to be after Lukas noted the potential concurrency issues with how LNKCTL is unprotected when reviewing (internally) my bandwidth controller series. Then I went to look around all LNKCTL usage and realized existing things might alreary have similar issues. Do you want me to send another version w/o cc stable or you'll take care of that? > > --- > > drivers/gpu/drm/amd/amdgpu/cik.c | 36 +++++++++----------------------- > > drivers/gpu/drm/amd/amdgpu/si.c | 36 +++++++++----------------------- > > 2 files changed, 20 insertions(+), 52 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c b/drivers/gpu/drm/amd/amdgpu/cik.c > > index 5641cf05d856..e63abdf52b6c 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/cik.c > > +++ b/drivers/gpu/drm/amd/amdgpu/cik.c > > @@ -1574,17 +1574,8 @@ static void cik_pcie_gen3_enable(struct amdgpu_device *adev) > > u16 bridge_cfg2, gpu_cfg2; > > u32 max_lw, current_lw, tmp; > > > > - pcie_capability_read_word(root, PCI_EXP_LNKCTL, > > - &bridge_cfg); > > - pcie_capability_read_word(adev->pdev, PCI_EXP_LNKCTL, > > - &gpu_cfg); > > - > > - tmp16 = bridge_cfg | PCI_EXP_LNKCTL_HAWD; > > - pcie_capability_write_word(root, PCI_EXP_LNKCTL, tmp16); > > - > > - tmp16 = gpu_cfg | PCI_EXP_LNKCTL_HAWD; > > - pcie_capability_write_word(adev->pdev, PCI_EXP_LNKCTL, > > - tmp16); > > + pcie_capability_set_word(root, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_HAWD); > > + pcie_capability_set_word(adev->pdev, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_HAWD); > > > > tmp = RREG32_PCIE(ixPCIE_LC_STATUS1); > > max_lw = (tmp & PCIE_LC_STATUS1__LC_DETECTED_LINK_WIDTH_MASK) >> > > @@ -1637,21 +1628,14 @@ static void cik_pcie_gen3_enable(struct amdgpu_device *adev) > > msleep(100); > > > > /* linkctl */ > > - pcie_capability_read_word(root, PCI_EXP_LNKCTL, > > - &tmp16); > > - tmp16 &= ~PCI_EXP_LNKCTL_HAWD; > > - tmp16 |= (bridge_cfg & PCI_EXP_LNKCTL_HAWD); > > - pcie_capability_write_word(root, PCI_EXP_LNKCTL, > > - tmp16); > > - > > - pcie_capability_read_word(adev->pdev, > > - PCI_EXP_LNKCTL, > > - &tmp16); > > - tmp16 &= ~PCI_EXP_LNKCTL_HAWD; > > - tmp16 |= (gpu_cfg & PCI_EXP_LNKCTL_HAWD); > > - pcie_capability_write_word(adev->pdev, > > - PCI_EXP_LNKCTL, > > - tmp16); > > + pcie_capability_clear_and_set_word(root, PCI_EXP_LNKCTL, > > + PCI_EXP_LNKCTL_HAWD, > > + bridge_cfg & > > + PCI_EXP_LNKCTL_HAWD); > > + pcie_capability_clear_and_set_word(adev->pdev, PCI_EXP_LNKCTL, > > + PCI_EXP_LNKCTL_HAWD, > > + gpu_cfg & > > + PCI_EXP_LNKCTL_HAWD); > > Wow, there's a lot of pointless-looking work going on here: > > set root PCI_EXP_LNKCTL_HAWD > set GPU PCI_EXP_LNKCTL_HAWD > > for (i = 0; i < 10; i++) { > read root PCI_EXP_LNKCTL > read GPU PCI_EXP_LNKCTL > > clear root PCI_EXP_LNKCTL_HAWD > if (root PCI_EXP_LNKCTL_HAWD was set) > set root PCI_EXP_LNKCTL_HAWD > > clear GPU PCI_EXP_LNKCTL_HAWD > if (GPU PCI_EXP_LNKCTL_HAWD was set) > set GPU PCI_EXP_LNKCTL_HAWD > } > > If it really *is* pointless, it would be nice to clean it up, but that > wouldn't be material for this patch, so what you have looks good. I really don't know if it's needed or not. There's stuff which looks hw specific going on besides those things you point out and I've not really understood what all that does. One annoying thing is that this code has been copy-pasted to appear in almost identical form in 4 files. I agree it certainly looks there might be room for cleaning things up here but such cleanups look a bit too scary to me w/o hw to test them. > > /* linkctl2 */ > > pcie_capability_read_word(root, PCI_EXP_LNKCTL2, > > The PCI_EXP_LNKCTL2 stuff also includes RMW updates. I don't see any > uses of PCI_EXP_LNKCTL2 outside this driver that look relevant, so I > guess we don't care about making the PCI_EXP_LNKCTL2 updates atomic? Currently no, which is why I left it out from this patchset. It is going to change soon though as I intend to submit bandwidth controller series after this series which will add RMW ops for LNKCTL2. The LNKCTL2 RMW parts are now in that series rather than in this one. After adding the bandwidth controller, this driver might be able to use it instead of tweaking LNKCTL2 directly to alter PCIe link speed (but I don't expect myself to be able to test these drivers and it feels too risky to make such a change without testing it, unfortunately). -- i. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 05/11] drm/amdgpu: Use RMW accessors for changing LNKCTL 2023-07-21 8:07 ` Ilpo Järvinen @ 2023-07-21 14:52 ` Alex Deucher 2023-08-03 14:12 ` Ilpo Järvinen 0 siblings, 1 reply; 9+ messages in thread From: Alex Deucher @ 2023-07-21 14:52 UTC (permalink / raw) To: Ilpo Järvinen Cc: Dean Luick, Krzysztof Wilczyński, Alex Deucher, Emmanuel Grumbach, Jammy Zhou, linux-pci, Jonas Dreßler, Pan, Xinhui, dri-devel, LKML, Bjorn Helgaas, Lorenzo Pieralisi, Bjorn Helgaas, amd-gfx, Ken Wang, Rafael J . Wysocki, stable, Andy Shevchenko, Christian König, Heiner Kallweit On Fri, Jul 21, 2023 at 4:18 AM Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote: > > On Thu, 20 Jul 2023, Bjorn Helgaas wrote: > > > On Mon, Jul 17, 2023 at 03:04:57PM +0300, Ilpo Järvinen wrote: > > > Don't assume that only the driver would be accessing LNKCTL. ASPM > > > policy changes can trigger write to LNKCTL outside of driver's control. > > > And in the case of upstream bridge, the driver does not even own the > > > device it's changing the registers for. > > > > > > Use RMW capability accessors which do proper locking to avoid losing > > > concurrent updates to the register value. > > > > > > Fixes: a2e73f56fa62 ("drm/amdgpu: Add support for CIK parts") > > > Fixes: 62a37553414a ("drm/amdgpu: add si implementation v10") > > > Suggested-by: Lukas Wunner <lukas@wunner.de> > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > > Cc: stable@vger.kernel.org > > > > Do we have any reports of problems that are fixed by this patch (or by > > others in the series)? If not, I'm not sure it really fits the usual > > stable kernel criteria: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?id=v6.4 > > I was on the edge with this. The answer to your direct question is no, > there are no such reports so it would be okay to leave stable out I think. > This applies to all patches in this series. > > Basically, this series came to be after Lukas noted the potential > concurrency issues with how LNKCTL is unprotected when reviewing > (internally) my bandwidth controller series. Then I went to look around > all LNKCTL usage and realized existing things might alreary have similar > issues. > > Do you want me to send another version w/o cc stable or you'll take care > of that? > > > > --- > > > drivers/gpu/drm/amd/amdgpu/cik.c | 36 +++++++++----------------------- > > > drivers/gpu/drm/amd/amdgpu/si.c | 36 +++++++++----------------------- > > > 2 files changed, 20 insertions(+), 52 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c b/drivers/gpu/drm/amd/amdgpu/cik.c > > > index 5641cf05d856..e63abdf52b6c 100644 > > > --- a/drivers/gpu/drm/amd/amdgpu/cik.c > > > +++ b/drivers/gpu/drm/amd/amdgpu/cik.c > > > @@ -1574,17 +1574,8 @@ static void cik_pcie_gen3_enable(struct amdgpu_device *adev) > > > u16 bridge_cfg2, gpu_cfg2; > > > u32 max_lw, current_lw, tmp; > > > > > > - pcie_capability_read_word(root, PCI_EXP_LNKCTL, > > > - &bridge_cfg); > > > - pcie_capability_read_word(adev->pdev, PCI_EXP_LNKCTL, > > > - &gpu_cfg); > > > - > > > - tmp16 = bridge_cfg | PCI_EXP_LNKCTL_HAWD; > > > - pcie_capability_write_word(root, PCI_EXP_LNKCTL, tmp16); > > > - > > > - tmp16 = gpu_cfg | PCI_EXP_LNKCTL_HAWD; > > > - pcie_capability_write_word(adev->pdev, PCI_EXP_LNKCTL, > > > - tmp16); > > > + pcie_capability_set_word(root, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_HAWD); > > > + pcie_capability_set_word(adev->pdev, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_HAWD); > > > > > > tmp = RREG32_PCIE(ixPCIE_LC_STATUS1); > > > max_lw = (tmp & PCIE_LC_STATUS1__LC_DETECTED_LINK_WIDTH_MASK) >> > > > @@ -1637,21 +1628,14 @@ static void cik_pcie_gen3_enable(struct amdgpu_device *adev) > > > msleep(100); > > > > > > /* linkctl */ > > > - pcie_capability_read_word(root, PCI_EXP_LNKCTL, > > > - &tmp16); > > > - tmp16 &= ~PCI_EXP_LNKCTL_HAWD; > > > - tmp16 |= (bridge_cfg & PCI_EXP_LNKCTL_HAWD); > > > - pcie_capability_write_word(root, PCI_EXP_LNKCTL, > > > - tmp16); > > > - > > > - pcie_capability_read_word(adev->pdev, > > > - PCI_EXP_LNKCTL, > > > - &tmp16); > > > - tmp16 &= ~PCI_EXP_LNKCTL_HAWD; > > > - tmp16 |= (gpu_cfg & PCI_EXP_LNKCTL_HAWD); > > > - pcie_capability_write_word(adev->pdev, > > > - PCI_EXP_LNKCTL, > > > - tmp16); > > > + pcie_capability_clear_and_set_word(root, PCI_EXP_LNKCTL, > > > + PCI_EXP_LNKCTL_HAWD, > > > + bridge_cfg & > > > + PCI_EXP_LNKCTL_HAWD); > > > + pcie_capability_clear_and_set_word(adev->pdev, PCI_EXP_LNKCTL, > > > + PCI_EXP_LNKCTL_HAWD, > > > + gpu_cfg & > > > + PCI_EXP_LNKCTL_HAWD); > > > > Wow, there's a lot of pointless-looking work going on here: > > > > set root PCI_EXP_LNKCTL_HAWD > > set GPU PCI_EXP_LNKCTL_HAWD > > > > for (i = 0; i < 10; i++) { > > read root PCI_EXP_LNKCTL > > read GPU PCI_EXP_LNKCTL > > > > clear root PCI_EXP_LNKCTL_HAWD > > if (root PCI_EXP_LNKCTL_HAWD was set) > > set root PCI_EXP_LNKCTL_HAWD > > > > clear GPU PCI_EXP_LNKCTL_HAWD > > if (GPU PCI_EXP_LNKCTL_HAWD was set) > > set GPU PCI_EXP_LNKCTL_HAWD > > } > > > > If it really *is* pointless, it would be nice to clean it up, but that > > wouldn't be material for this patch, so what you have looks good. > > I really don't know if it's needed or not. There's stuff which looks hw > specific going on besides those things you point out and I've not really > understood what all that does. > > One annoying thing is that this code has been copy-pasted to appear in > almost identical form in 4 files. > > I agree it certainly looks there might be room for cleaning things up here > but such cleanups look a bit too scary to me w/o hw to test them. > > > > /* linkctl2 */ > > > pcie_capability_read_word(root, PCI_EXP_LNKCTL2, > > > > The PCI_EXP_LNKCTL2 stuff also includes RMW updates. I don't see any > > uses of PCI_EXP_LNKCTL2 outside this driver that look relevant, so I > > guess we don't care about making the PCI_EXP_LNKCTL2 updates atomic? > > Currently no, which is why I left it out from this patchset. > > It is going to change soon though as I intend to submit bandwidth > controller series after this series which will add RMW ops for LNKCTL2. > The LNKCTL2 RMW parts are now in that series rather than in this one. > > After adding the bandwidth controller, this driver might be able to use > it instead of tweaking LNKCTL2 directly to alter PCIe link speed (but I > don't expect myself to be able to test these drivers and it feels too > risky to make such a change without testing it, unfortunately). Thanks for the background. It was not clear what the point of this patch set was. This code and the similar code in radeon is just to change the link speed of the GPU. Some older platforms used default to slower link on boot so we added this code to renegotiate the link to a faster speed when the driver loaded. If you are adding core infrastructure to do that, we can switch to that. This was just the programming sequence I got from the hardware team back when this code was written. Most platforms I've seen these days come up at the max supported speed of the platform and endpoint so I don't think the code actually gets used much anymore. Taking a step back, what is the end goal of the bandwidth controller changes? The reason I ask is that today, we look at the currently negotiated speed of the link and use that for the baseline in the driver. The driver then enables PCIe dynamic power management where the system management unit on the GPU dynamically adjusts the link speed, width, and clock on demand based on the PCIe bandwidth requirements of the currently executing GPU jobs to save power. This might conflict with software if the goal is for some software component to do something similar. Alex ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 05/11] drm/amdgpu: Use RMW accessors for changing LNKCTL 2023-07-21 14:52 ` Alex Deucher @ 2023-08-03 14:12 ` Ilpo Järvinen 0 siblings, 0 replies; 9+ messages in thread From: Ilpo Järvinen @ 2023-08-03 14:12 UTC (permalink / raw) To: Alex Deucher Cc: Dean Luick, Krzysztof Wilczyński, Alex Deucher, Emmanuel Grumbach, Jammy Zhou, linux-pci, Jonas Dreßler, Pan, Xinhui, dri-devel, LKML, Bjorn Helgaas, Lorenzo Pieralisi, Bjorn Helgaas, amd-gfx, Ken Wang, Rafael J . Wysocki, stable, Andy Shevchenko, Christian König, Heiner Kallweit [-- Attachment #1: Type: text/plain, Size: 10069 bytes --] On Fri, 21 Jul 2023, Alex Deucher wrote: > On Fri, Jul 21, 2023 at 4:18 AM Ilpo Järvinen > <ilpo.jarvinen@linux.intel.com> wrote: > > > > On Thu, 20 Jul 2023, Bjorn Helgaas wrote: > > > > > On Mon, Jul 17, 2023 at 03:04:57PM +0300, Ilpo Järvinen wrote: > > > > Don't assume that only the driver would be accessing LNKCTL. ASPM > > > > policy changes can trigger write to LNKCTL outside of driver's control. > > > > And in the case of upstream bridge, the driver does not even own the > > > > device it's changing the registers for. > > > > > > > > Use RMW capability accessors which do proper locking to avoid losing > > > > concurrent updates to the register value. > > > > > > > > Fixes: a2e73f56fa62 ("drm/amdgpu: Add support for CIK parts") > > > > Fixes: 62a37553414a ("drm/amdgpu: add si implementation v10") > > > > Suggested-by: Lukas Wunner <lukas@wunner.de> > > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > > > Cc: stable@vger.kernel.org > > > > > > Do we have any reports of problems that are fixed by this patch (or by > > > others in the series)? If not, I'm not sure it really fits the usual > > > stable kernel criteria: > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?id=v6.4 > > > > I was on the edge with this. The answer to your direct question is no, > > there are no such reports so it would be okay to leave stable out I think. > > This applies to all patches in this series. > > > > Basically, this series came to be after Lukas noted the potential > > concurrency issues with how LNKCTL is unprotected when reviewing > > (internally) my bandwidth controller series. Then I went to look around > > all LNKCTL usage and realized existing things might alreary have similar > > issues. > > > > Do you want me to send another version w/o cc stable or you'll take care > > of that? > > > > > > --- > > > > drivers/gpu/drm/amd/amdgpu/cik.c | 36 +++++++++----------------------- > > > > drivers/gpu/drm/amd/amdgpu/si.c | 36 +++++++++----------------------- > > > > 2 files changed, 20 insertions(+), 52 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c b/drivers/gpu/drm/amd/amdgpu/cik.c > > > > index 5641cf05d856..e63abdf52b6c 100644 > > > > --- a/drivers/gpu/drm/amd/amdgpu/cik.c > > > > +++ b/drivers/gpu/drm/amd/amdgpu/cik.c > > > > @@ -1574,17 +1574,8 @@ static void cik_pcie_gen3_enable(struct amdgpu_device *adev) > > > > u16 bridge_cfg2, gpu_cfg2; > > > > u32 max_lw, current_lw, tmp; > > > > > > > > - pcie_capability_read_word(root, PCI_EXP_LNKCTL, > > > > - &bridge_cfg); > > > > - pcie_capability_read_word(adev->pdev, PCI_EXP_LNKCTL, > > > > - &gpu_cfg); > > > > - > > > > - tmp16 = bridge_cfg | PCI_EXP_LNKCTL_HAWD; > > > > - pcie_capability_write_word(root, PCI_EXP_LNKCTL, tmp16); > > > > - > > > > - tmp16 = gpu_cfg | PCI_EXP_LNKCTL_HAWD; > > > > - pcie_capability_write_word(adev->pdev, PCI_EXP_LNKCTL, > > > > - tmp16); > > > > + pcie_capability_set_word(root, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_HAWD); > > > > + pcie_capability_set_word(adev->pdev, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_HAWD); > > > > > > > > tmp = RREG32_PCIE(ixPCIE_LC_STATUS1); > > > > max_lw = (tmp & PCIE_LC_STATUS1__LC_DETECTED_LINK_WIDTH_MASK) >> > > > > @@ -1637,21 +1628,14 @@ static void cik_pcie_gen3_enable(struct amdgpu_device *adev) > > > > msleep(100); > > > > > > > > /* linkctl */ > > > > - pcie_capability_read_word(root, PCI_EXP_LNKCTL, > > > > - &tmp16); > > > > - tmp16 &= ~PCI_EXP_LNKCTL_HAWD; > > > > - tmp16 |= (bridge_cfg & PCI_EXP_LNKCTL_HAWD); > > > > - pcie_capability_write_word(root, PCI_EXP_LNKCTL, > > > > - tmp16); > > > > - > > > > - pcie_capability_read_word(adev->pdev, > > > > - PCI_EXP_LNKCTL, > > > > - &tmp16); > > > > - tmp16 &= ~PCI_EXP_LNKCTL_HAWD; > > > > - tmp16 |= (gpu_cfg & PCI_EXP_LNKCTL_HAWD); > > > > - pcie_capability_write_word(adev->pdev, > > > > - PCI_EXP_LNKCTL, > > > > - tmp16); > > > > + pcie_capability_clear_and_set_word(root, PCI_EXP_LNKCTL, > > > > + PCI_EXP_LNKCTL_HAWD, > > > > + bridge_cfg & > > > > + PCI_EXP_LNKCTL_HAWD); > > > > + pcie_capability_clear_and_set_word(adev->pdev, PCI_EXP_LNKCTL, > > > > + PCI_EXP_LNKCTL_HAWD, > > > > + gpu_cfg & > > > > + PCI_EXP_LNKCTL_HAWD); > > > > > > Wow, there's a lot of pointless-looking work going on here: > > > > > > set root PCI_EXP_LNKCTL_HAWD > > > set GPU PCI_EXP_LNKCTL_HAWD > > > > > > for (i = 0; i < 10; i++) { > > > read root PCI_EXP_LNKCTL > > > read GPU PCI_EXP_LNKCTL > > > > > > clear root PCI_EXP_LNKCTL_HAWD > > > if (root PCI_EXP_LNKCTL_HAWD was set) > > > set root PCI_EXP_LNKCTL_HAWD > > > > > > clear GPU PCI_EXP_LNKCTL_HAWD > > > if (GPU PCI_EXP_LNKCTL_HAWD was set) > > > set GPU PCI_EXP_LNKCTL_HAWD > > > } > > > > > > If it really *is* pointless, it would be nice to clean it up, but that > > > wouldn't be material for this patch, so what you have looks good. > > > > I really don't know if it's needed or not. There's stuff which looks hw > > specific going on besides those things you point out and I've not really > > understood what all that does. > > > > One annoying thing is that this code has been copy-pasted to appear in > > almost identical form in 4 files. > > > > I agree it certainly looks there might be room for cleaning things up here > > but such cleanups look a bit too scary to me w/o hw to test them. > > > > > > /* linkctl2 */ > > > > pcie_capability_read_word(root, PCI_EXP_LNKCTL2, > > > > > > The PCI_EXP_LNKCTL2 stuff also includes RMW updates. I don't see any > > > uses of PCI_EXP_LNKCTL2 outside this driver that look relevant, so I > > > guess we don't care about making the PCI_EXP_LNKCTL2 updates atomic? > > > > Currently no, which is why I left it out from this patchset. > > > > It is going to change soon though as I intend to submit bandwidth > > controller series after this series which will add RMW ops for LNKCTL2. > > The LNKCTL2 RMW parts are now in that series rather than in this one. > > > > After adding the bandwidth controller, this driver might be able to use > > it instead of tweaking LNKCTL2 directly to alter PCIe link speed (but I > > don't expect myself to be able to test these drivers and it feels too > > risky to make such a change without testing it, unfortunately). > > Thanks for the background. It was not clear what the point of this > patch set was. Thanks for chimming in! There are two reasons, one is to fix the existing RMW races and the other is the addition of BW controller. The RMW race issue was discovered while Lukas was reviewing BW controller's code and it was disjoint/generic enough from the BW controller to go into its own patchset. > This code and the similar code in radeon is just to > change the link speed of the GPU. Some older platforms used default > to slower link on boot so we added this code to renegotiate the link > to a faster speed when the driver loaded. If you are adding core > infrastructure to do that, we can switch to that. This was just the > programming sequence I got from the hardware team back when this code > was written. Most platforms I've seen these days come up at the max > supported speed of the platform and endpoint so I don't think the code > actually gets used much anymore. If I understood the code correctly, it's the device side with these GPUs which triggers the actual Link Speed change based on those HW specific writes? With the BW controller, the speed change would be initiated by setting the Root Port to do Link retraining. > Taking a step back, what is the end goal of the bandwidth controller > changes? The reason I ask is that today, we look at the currently > negotiated speed of the link and use that for the baseline in the > driver. The driver then enables PCIe dynamic power management where > the system management unit on the GPU dynamically adjusts the link > speed, width, and clock on demand based on the PCIe bandwidth > requirements of the currently executing GPU jobs to save power. This > might conflict with software if the goal is for some software > component to do something similar. BW controller is mainly done for thermal reasons (a thermal side cooling device is provided per root port for thermald/userspace to tap into) but it is not to say there couldn't be other usecases. If there's another actor besides BW controller altering Link Speeds, I think what would happen is that BW controller setting Root Port's Target Speed would just upper-bound the Link Speed requests from GPU (which makes sense at least from thermal point-of-view). -- i. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v5 06/11] drm/radeon: Use RMW accessors for changing LNKCTL [not found] <20230717120503.15276-1-ilpo.jarvinen@linux.intel.com> 2023-07-17 12:04 ` [PATCH v5 05/11] drm/amdgpu: Use RMW accessors for changing LNKCTL Ilpo Järvinen @ 2023-07-17 12:04 ` Ilpo Järvinen 2023-08-18 16:12 ` Deucher, Alexander 1 sibling, 1 reply; 9+ messages in thread From: Ilpo Järvinen @ 2023-07-17 12:04 UTC (permalink / raw) To: linux-pci, Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczyński, Emmanuel Grumbach, Rafael J . Wysocki, Heiner Kallweit, Lukas Wunner, Andy Shevchenko, Alex Deucher, Christian König, Pan, Xinhui, David Airlie, Daniel Vetter, amd-gfx, dri-devel, linux-kernel Cc: Dean Luick, Ilpo Järvinen, stable, Jonas Dreßler Don't assume that only the driver would be accessing LNKCTL. ASPM policy changes can trigger write to LNKCTL outside of driver's control. And in the case of upstream bridge, the driver does not even own the device it's changing the registers for. Use RMW capability accessors which do proper locking to avoid losing concurrent updates to the register value. Fixes: 8a7cd27679d0 ("drm/radeon/cik: add support for pcie gen1/2/3 switching") Fixes: b9d305dfb66c ("drm/radeon: implement pcie gen2/3 support for SI") Suggested-by: Lukas Wunner <lukas@wunner.de> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Cc: stable@vger.kernel.org --- drivers/gpu/drm/radeon/cik.c | 36 ++++++++++------------------------- drivers/gpu/drm/radeon/si.c | 37 ++++++++++-------------------------- 2 files changed, 20 insertions(+), 53 deletions(-) diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c index 5819737c21c6..a6f3c811ceb8 100644 --- a/drivers/gpu/drm/radeon/cik.c +++ b/drivers/gpu/drm/radeon/cik.c @@ -9534,17 +9534,8 @@ static void cik_pcie_gen3_enable(struct radeon_device *rdev) u16 bridge_cfg2, gpu_cfg2; u32 max_lw, current_lw, tmp; - pcie_capability_read_word(root, PCI_EXP_LNKCTL, - &bridge_cfg); - pcie_capability_read_word(rdev->pdev, PCI_EXP_LNKCTL, - &gpu_cfg); - - tmp16 = bridge_cfg | PCI_EXP_LNKCTL_HAWD; - pcie_capability_write_word(root, PCI_EXP_LNKCTL, tmp16); - - tmp16 = gpu_cfg | PCI_EXP_LNKCTL_HAWD; - pcie_capability_write_word(rdev->pdev, PCI_EXP_LNKCTL, - tmp16); + pcie_capability_set_word(root, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_HAWD); + pcie_capability_set_word(rdev->pdev, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_HAWD); tmp = RREG32_PCIE_PORT(PCIE_LC_STATUS1); max_lw = (tmp & LC_DETECTED_LINK_WIDTH_MASK) >> LC_DETECTED_LINK_WIDTH_SHIFT; @@ -9591,21 +9582,14 @@ static void cik_pcie_gen3_enable(struct radeon_device *rdev) msleep(100); /* linkctl */ - pcie_capability_read_word(root, PCI_EXP_LNKCTL, - &tmp16); - tmp16 &= ~PCI_EXP_LNKCTL_HAWD; - tmp16 |= (bridge_cfg & PCI_EXP_LNKCTL_HAWD); - pcie_capability_write_word(root, PCI_EXP_LNKCTL, - tmp16); - - pcie_capability_read_word(rdev->pdev, - PCI_EXP_LNKCTL, - &tmp16); - tmp16 &= ~PCI_EXP_LNKCTL_HAWD; - tmp16 |= (gpu_cfg & PCI_EXP_LNKCTL_HAWD); - pcie_capability_write_word(rdev->pdev, - PCI_EXP_LNKCTL, - tmp16); + pcie_capability_clear_and_set_word(root, PCI_EXP_LNKCTL, + PCI_EXP_LNKCTL_HAWD, + bridge_cfg & + PCI_EXP_LNKCTL_HAWD); + pcie_capability_clear_and_set_word(rdev->pdev, PCI_EXP_LNKCTL, + PCI_EXP_LNKCTL_HAWD, + gpu_cfg & + PCI_EXP_LNKCTL_HAWD); /* linkctl2 */ pcie_capability_read_word(root, PCI_EXP_LNKCTL2, diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c index 8d5e4b25609d..a91012447b56 100644 --- a/drivers/gpu/drm/radeon/si.c +++ b/drivers/gpu/drm/radeon/si.c @@ -7131,17 +7131,8 @@ static void si_pcie_gen3_enable(struct radeon_device *rdev) u16 bridge_cfg2, gpu_cfg2; u32 max_lw, current_lw, tmp; - pcie_capability_read_word(root, PCI_EXP_LNKCTL, - &bridge_cfg); - pcie_capability_read_word(rdev->pdev, PCI_EXP_LNKCTL, - &gpu_cfg); - - tmp16 = bridge_cfg | PCI_EXP_LNKCTL_HAWD; - pcie_capability_write_word(root, PCI_EXP_LNKCTL, tmp16); - - tmp16 = gpu_cfg | PCI_EXP_LNKCTL_HAWD; - pcie_capability_write_word(rdev->pdev, PCI_EXP_LNKCTL, - tmp16); + pcie_capability_set_word(root, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_HAWD); + pcie_capability_set_word(rdev->pdev, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_HAWD); tmp = RREG32_PCIE(PCIE_LC_STATUS1); max_lw = (tmp & LC_DETECTED_LINK_WIDTH_MASK) >> LC_DETECTED_LINK_WIDTH_SHIFT; @@ -7188,22 +7179,14 @@ static void si_pcie_gen3_enable(struct radeon_device *rdev) msleep(100); /* linkctl */ - pcie_capability_read_word(root, PCI_EXP_LNKCTL, - &tmp16); - tmp16 &= ~PCI_EXP_LNKCTL_HAWD; - tmp16 |= (bridge_cfg & PCI_EXP_LNKCTL_HAWD); - pcie_capability_write_word(root, - PCI_EXP_LNKCTL, - tmp16); - - pcie_capability_read_word(rdev->pdev, - PCI_EXP_LNKCTL, - &tmp16); - tmp16 &= ~PCI_EXP_LNKCTL_HAWD; - tmp16 |= (gpu_cfg & PCI_EXP_LNKCTL_HAWD); - pcie_capability_write_word(rdev->pdev, - PCI_EXP_LNKCTL, - tmp16); + pcie_capability_clear_and_set_word(root, PCI_EXP_LNKCTL, + PCI_EXP_LNKCTL_HAWD, + bridge_cfg & + PCI_EXP_LNKCTL_HAWD); + pcie_capability_clear_and_set_word(rdev->pdev, PCI_EXP_LNKCTL, + PCI_EXP_LNKCTL_HAWD, + gpu_cfg & + PCI_EXP_LNKCTL_HAWD); /* linkctl2 */ pcie_capability_read_word(root, PCI_EXP_LNKCTL2, -- 2.30.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* RE: [PATCH v5 06/11] drm/radeon: Use RMW accessors for changing LNKCTL 2023-07-17 12:04 ` [PATCH v5 06/11] drm/radeon: " Ilpo Järvinen @ 2023-08-18 16:12 ` Deucher, Alexander 2023-08-21 9:57 ` Ilpo Järvinen 2023-08-21 19:12 ` Bjorn Helgaas 0 siblings, 2 replies; 9+ messages in thread From: Deucher, Alexander @ 2023-08-18 16:12 UTC (permalink / raw) To: Ilpo Järvinen, linux-pci@vger.kernel.org, Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczyński, Emmanuel Grumbach, Rafael J . Wysocki, Heiner Kallweit, Lukas Wunner, Andy Shevchenko, Koenig, Christian, Pan, Xinhui, David Airlie, Daniel Vetter, amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org Cc: Dean Luick, stable@vger.kernel.org, Jonas Dreßler [Public] > -----Original Message----- > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > Sent: Monday, July 17, 2023 8:05 AM > To: linux-pci@vger.kernel.org; Bjorn Helgaas <bhelgaas@google.com>; Lorenzo > Pieralisi <lorenzo.pieralisi@arm.com>; Rob Herring <robh@kernel.org>; > Krzysztof Wilczyński <kw@linux.com>; Emmanuel Grumbach > <emmanuel.grumbach@intel.com>; Rafael J . Wysocki <rafael@kernel.org>; > Heiner Kallweit <hkallweit1@gmail.com>; Lukas Wunner <lukas@wunner.de>; > Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Deucher, Alexander > <Alexander.Deucher@amd.com>; Koenig, Christian > <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; David > Airlie <airlied@gmail.com>; Daniel Vetter <daniel@ffwll.ch>; amd- > gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux- > kernel@vger.kernel.org > Cc: Dean Luick <dean.luick@cornelisnetworks.com>; Jonas Dreßler > <verdre@v0yd.nl>; Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>; > stable@vger.kernel.org > Subject: [PATCH v5 06/11] drm/radeon: Use RMW accessors for changing > LNKCTL > > Don't assume that only the driver would be accessing LNKCTL. ASPM policy > changes can trigger write to LNKCTL outside of driver's control. > And in the case of upstream bridge, the driver does not even own the device > it's changing the registers for. > > Use RMW capability accessors which do proper locking to avoid losing > concurrent updates to the register value. > > Fixes: 8a7cd27679d0 ("drm/radeon/cik: add support for pcie gen1/2/3 > switching") > Fixes: b9d305dfb66c ("drm/radeon: implement pcie gen2/3 support for SI") > Suggested-by: Lukas Wunner <lukas@wunner.de> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > Cc: stable@vger.kernel.org For this and the amdgpu patch: Acked-by: Alex Deucher <alexander.deucher@amd.com> I'm not sure if this is stable material however. Is there some issue today? > --- > drivers/gpu/drm/radeon/cik.c | 36 ++++++++++------------------------- > drivers/gpu/drm/radeon/si.c | 37 ++++++++++-------------------------- > 2 files changed, 20 insertions(+), 53 deletions(-) > > diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c > index 5819737c21c6..a6f3c811ceb8 100644 > --- a/drivers/gpu/drm/radeon/cik.c > +++ b/drivers/gpu/drm/radeon/cik.c > @@ -9534,17 +9534,8 @@ static void cik_pcie_gen3_enable(struct > radeon_device *rdev) > u16 bridge_cfg2, gpu_cfg2; > u32 max_lw, current_lw, tmp; > > - pcie_capability_read_word(root, PCI_EXP_LNKCTL, > - &bridge_cfg); > - pcie_capability_read_word(rdev->pdev, > PCI_EXP_LNKCTL, > - &gpu_cfg); > - > - tmp16 = bridge_cfg | PCI_EXP_LNKCTL_HAWD; > - pcie_capability_write_word(root, PCI_EXP_LNKCTL, > tmp16); > - > - tmp16 = gpu_cfg | PCI_EXP_LNKCTL_HAWD; > - pcie_capability_write_word(rdev->pdev, > PCI_EXP_LNKCTL, > - tmp16); > + pcie_capability_set_word(root, PCI_EXP_LNKCTL, > PCI_EXP_LNKCTL_HAWD); > + pcie_capability_set_word(rdev->pdev, > PCI_EXP_LNKCTL, > +PCI_EXP_LNKCTL_HAWD); > > tmp = RREG32_PCIE_PORT(PCIE_LC_STATUS1); > max_lw = (tmp & LC_DETECTED_LINK_WIDTH_MASK) > >> LC_DETECTED_LINK_WIDTH_SHIFT; @@ -9591,21 +9582,14 @@ static > void cik_pcie_gen3_enable(struct radeon_device *rdev) > msleep(100); > > /* linkctl */ > - pcie_capability_read_word(root, > PCI_EXP_LNKCTL, > - &tmp16); > - tmp16 &= ~PCI_EXP_LNKCTL_HAWD; > - tmp16 |= (bridge_cfg & > PCI_EXP_LNKCTL_HAWD); > - pcie_capability_write_word(root, > PCI_EXP_LNKCTL, > - tmp16); > - > - pcie_capability_read_word(rdev->pdev, > - PCI_EXP_LNKCTL, > - &tmp16); > - tmp16 &= ~PCI_EXP_LNKCTL_HAWD; > - tmp16 |= (gpu_cfg & > PCI_EXP_LNKCTL_HAWD); > - pcie_capability_write_word(rdev->pdev, > - PCI_EXP_LNKCTL, > - tmp16); > + pcie_capability_clear_and_set_word(root, > PCI_EXP_LNKCTL, > + > PCI_EXP_LNKCTL_HAWD, > + bridge_cfg & > + > PCI_EXP_LNKCTL_HAWD); > + pcie_capability_clear_and_set_word(rdev- > >pdev, PCI_EXP_LNKCTL, > + > PCI_EXP_LNKCTL_HAWD, > + gpu_cfg & > + > PCI_EXP_LNKCTL_HAWD); > > /* linkctl2 */ > pcie_capability_read_word(root, > PCI_EXP_LNKCTL2, diff --git a/drivers/gpu/drm/radeon/si.c > b/drivers/gpu/drm/radeon/si.c index 8d5e4b25609d..a91012447b56 > 100644 > --- a/drivers/gpu/drm/radeon/si.c > +++ b/drivers/gpu/drm/radeon/si.c > @@ -7131,17 +7131,8 @@ static void si_pcie_gen3_enable(struct > radeon_device *rdev) > u16 bridge_cfg2, gpu_cfg2; > u32 max_lw, current_lw, tmp; > > - pcie_capability_read_word(root, PCI_EXP_LNKCTL, > - &bridge_cfg); > - pcie_capability_read_word(rdev->pdev, > PCI_EXP_LNKCTL, > - &gpu_cfg); > - > - tmp16 = bridge_cfg | PCI_EXP_LNKCTL_HAWD; > - pcie_capability_write_word(root, PCI_EXP_LNKCTL, > tmp16); > - > - tmp16 = gpu_cfg | PCI_EXP_LNKCTL_HAWD; > - pcie_capability_write_word(rdev->pdev, > PCI_EXP_LNKCTL, > - tmp16); > + pcie_capability_set_word(root, PCI_EXP_LNKCTL, > PCI_EXP_LNKCTL_HAWD); > + pcie_capability_set_word(rdev->pdev, > PCI_EXP_LNKCTL, > +PCI_EXP_LNKCTL_HAWD); > > tmp = RREG32_PCIE(PCIE_LC_STATUS1); > max_lw = (tmp & LC_DETECTED_LINK_WIDTH_MASK) > >> LC_DETECTED_LINK_WIDTH_SHIFT; @@ -7188,22 +7179,14 @@ static > void si_pcie_gen3_enable(struct radeon_device *rdev) > msleep(100); > > /* linkctl */ > - pcie_capability_read_word(root, > PCI_EXP_LNKCTL, > - &tmp16); > - tmp16 &= ~PCI_EXP_LNKCTL_HAWD; > - tmp16 |= (bridge_cfg & > PCI_EXP_LNKCTL_HAWD); > - pcie_capability_write_word(root, > - PCI_EXP_LNKCTL, > - tmp16); > - > - pcie_capability_read_word(rdev->pdev, > - PCI_EXP_LNKCTL, > - &tmp16); > - tmp16 &= ~PCI_EXP_LNKCTL_HAWD; > - tmp16 |= (gpu_cfg & > PCI_EXP_LNKCTL_HAWD); > - pcie_capability_write_word(rdev->pdev, > - PCI_EXP_LNKCTL, > - tmp16); > + pcie_capability_clear_and_set_word(root, > PCI_EXP_LNKCTL, > + > PCI_EXP_LNKCTL_HAWD, > + bridge_cfg & > + > PCI_EXP_LNKCTL_HAWD); > + pcie_capability_clear_and_set_word(rdev- > >pdev, PCI_EXP_LNKCTL, > + > PCI_EXP_LNKCTL_HAWD, > + gpu_cfg & > + > PCI_EXP_LNKCTL_HAWD); > > /* linkctl2 */ > pcie_capability_read_word(root, > PCI_EXP_LNKCTL2, > -- > 2.30.2 ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH v5 06/11] drm/radeon: Use RMW accessors for changing LNKCTL 2023-08-18 16:12 ` Deucher, Alexander @ 2023-08-21 9:57 ` Ilpo Järvinen 2023-08-21 19:12 ` Bjorn Helgaas 1 sibling, 0 replies; 9+ messages in thread From: Ilpo Järvinen @ 2023-08-21 9:57 UTC (permalink / raw) To: Deucher, Alexander Cc: Dean Luick, Krzysztof Wilczyński, Emmanuel Grumbach, Rob Herring, Jonas Dreßler, linux-pci@vger.kernel.org, Rafael J . Wysocki, Pan, Xinhui, amd-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org, Lorenzo Pieralisi, dri-devel@lists.freedesktop.org, Daniel Vetter, Bjorn Helgaas, Lukas Wunner, stable@vger.kernel.org, Andy Shevchenko, David Airlie, Koenig, Christian, Heiner Kallweit [-- Attachment #1: Type: text/plain, Size: 2113 bytes --] On Fri, 18 Aug 2023, Deucher, Alexander wrote: > [Public] > > > -----Original Message----- > > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > Sent: Monday, July 17, 2023 8:05 AM > > To: linux-pci@vger.kernel.org; Bjorn Helgaas <bhelgaas@google.com>; Lorenzo > > Pieralisi <lorenzo.pieralisi@arm.com>; Rob Herring <robh@kernel.org>; > > Krzysztof Wilczyński <kw@linux.com>; Emmanuel Grumbach > > <emmanuel.grumbach@intel.com>; Rafael J . Wysocki <rafael@kernel.org>; > > Heiner Kallweit <hkallweit1@gmail.com>; Lukas Wunner <lukas@wunner.de>; > > Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Deucher, Alexander > > <Alexander.Deucher@amd.com>; Koenig, Christian > > <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; David > > Airlie <airlied@gmail.com>; Daniel Vetter <daniel@ffwll.ch>; amd- > > gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux- > > kernel@vger.kernel.org > > Cc: Dean Luick <dean.luick@cornelisnetworks.com>; Jonas Dreßler > > <verdre@v0yd.nl>; Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>; > > stable@vger.kernel.org > > Subject: [PATCH v5 06/11] drm/radeon: Use RMW accessors for changing > > LNKCTL > > > > Don't assume that only the driver would be accessing LNKCTL. ASPM policy > > changes can trigger write to LNKCTL outside of driver's control. > > And in the case of upstream bridge, the driver does not even own the device > > it's changing the registers for. > > > > Use RMW capability accessors which do proper locking to avoid losing > > concurrent updates to the register value. > > > > Fixes: 8a7cd27679d0 ("drm/radeon/cik: add support for pcie gen1/2/3 > > switching") > > Fixes: b9d305dfb66c ("drm/radeon: implement pcie gen2/3 support for SI") > > Suggested-by: Lukas Wunner <lukas@wunner.de> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > Cc: stable@vger.kernel.org > > For this and the amdgpu patch: > Acked-by: Alex Deucher <alexander.deucher@amd.com> > I'm not sure if this is stable material however. Is there some issue today? These were added without Cc stable into pci.git/pcie-rmw. -- i. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 06/11] drm/radeon: Use RMW accessors for changing LNKCTL 2023-08-18 16:12 ` Deucher, Alexander 2023-08-21 9:57 ` Ilpo Järvinen @ 2023-08-21 19:12 ` Bjorn Helgaas 1 sibling, 0 replies; 9+ messages in thread From: Bjorn Helgaas @ 2023-08-21 19:12 UTC (permalink / raw) To: Deucher, Alexander Cc: Dean Luick, Krzysztof Wilczyński, Lorenzo Pieralisi, Rob Herring, Jonas Dreßler, linux-pci@vger.kernel.org, Rafael J . Wysocki, Lukas Wunner, Pan, Xinhui, amd-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org, Emmanuel Grumbach, dri-devel@lists.freedesktop.org, Daniel Vetter, Bjorn Helgaas, Ilpo Järvinen, stable@vger.kernel.org, Andy Shevchenko, David Airlie, Koenig, Christian, Heiner Kallweit On Fri, Aug 18, 2023 at 04:12:57PM +0000, Deucher, Alexander wrote: > > -----Original Message----- > > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > Sent: Monday, July 17, 2023 8:05 AM > > To: linux-pci@vger.kernel.org; Bjorn Helgaas <bhelgaas@google.com>; Lorenzo > > Pieralisi <lorenzo.pieralisi@arm.com>; Rob Herring <robh@kernel.org>; > > Krzysztof Wilczyński <kw@linux.com>; Emmanuel Grumbach > > <emmanuel.grumbach@intel.com>; Rafael J . Wysocki <rafael@kernel.org>; > > Heiner Kallweit <hkallweit1@gmail.com>; Lukas Wunner <lukas@wunner.de>; > > Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Deucher, Alexander > > <Alexander.Deucher@amd.com>; Koenig, Christian > > <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; David > > Airlie <airlied@gmail.com>; Daniel Vetter <daniel@ffwll.ch>; amd- > > gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux- > > kernel@vger.kernel.org > > Cc: Dean Luick <dean.luick@cornelisnetworks.com>; Jonas Dreßler > > <verdre@v0yd.nl>; Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>; > > stable@vger.kernel.org > > Subject: [PATCH v5 06/11] drm/radeon: Use RMW accessors for changing > > LNKCTL > > > > Don't assume that only the driver would be accessing LNKCTL. ASPM policy > > changes can trigger write to LNKCTL outside of driver's control. > > And in the case of upstream bridge, the driver does not even own the device > > it's changing the registers for. > > > > Use RMW capability accessors which do proper locking to avoid losing > > concurrent updates to the register value. > > > > Fixes: 8a7cd27679d0 ("drm/radeon/cik: add support for pcie gen1/2/3 > > switching") > > Fixes: b9d305dfb66c ("drm/radeon: implement pcie gen2/3 support for SI") > > Suggested-by: Lukas Wunner <lukas@wunner.de> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > Cc: stable@vger.kernel.org > > For this and the amdgpu patch: > Acked-by: Alex Deucher <alexander.deucher@amd.com> > I'm not sure if this is stable material however. Is there some issue today? Added your ack, thanks! I dropped the stable tag on the whole series. Bjorn ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-08-21 19:13 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20230717120503.15276-1-ilpo.jarvinen@linux.intel.com>
2023-07-17 12:04 ` [PATCH v5 05/11] drm/amdgpu: Use RMW accessors for changing LNKCTL Ilpo Järvinen
2023-07-20 21:55 ` Bjorn Helgaas
2023-07-21 8:07 ` Ilpo Järvinen
2023-07-21 14:52 ` Alex Deucher
2023-08-03 14:12 ` Ilpo Järvinen
2023-07-17 12:04 ` [PATCH v5 06/11] drm/radeon: " Ilpo Järvinen
2023-08-18 16:12 ` Deucher, Alexander
2023-08-21 9:57 ` Ilpo Järvinen
2023-08-21 19:12 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox