diff mbox series

[v5,06/11] drm/radeon: Use RMW accessors for changing LNKCTL

Message ID 20230717120503.15276-7-ilpo.jarvinen@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series None | expand

Commit Message

Ilpo Järvinen July 17, 2023, 12:04 p.m. UTC 
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(-)

Comments

Alex Deucher Aug. 18, 2023, 4:12 p.m. UTC | #1 
[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
Ilpo Järvinen Aug. 21, 2023, 9:57 a.m. UTC | #2 
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.
Bjorn Helgaas Aug. 21, 2023, 7:12 p.m. UTC | #3 
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
diff mbox series

Patch

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,