[PATCH 0/7] rtlwifi: PCIe capability access fix + improvements

[PATCH 0/7] rtlwifi: PCIe capability access fix + improvements

[Ilpo Järvinen]

The rtlwifi driver has code fragments that are using old interface or
custom code to access PCIe capabilities.

The use of old interfaces causes an issue with LNKCTL register that
requires locking during RMW operations. Standard PCIe capability
accessors provide the necessary locking so they should be always used.
The first patch of the series addresses that problem.

The rest of the patches cleanup PCIe capability related code.

Additional note: This series provides only a stop-gap solution to the
RMW concurrency issue, the overall plan is to migrate all ASPM related
handling into the ASPM service driver in order for it to accurately
track ASPM state [1].

[1] https://lore.kernel.org/linux-pci/20230918131103.24119-1-ilpo.jarvinen@linux.intel.com/

Ilpo Järvinen (7):
  wifi: rtlwifi: Convert LNKCTL change to PCIe cap RMW accessors
  wifi: rtlwifi: Convert to use PCIe capability accessors
  rtlwifi: rtl8821ae: Remove unnecessary PME_Status bit set
  rtlwifi: rtl8821ae: Reverse PM capability exists check
  rtlwifi: rtl8821ae: Use pci_find_capability()
  rtlwifi: rtl8821ae: Add pdev into _rtl8821ae_clear_pci_pme_status()
  rtlwifi: rtl8821ae: Access full PMCS reg and use pci_regs.h

 drivers/net/wireless/realtek/rtlwifi/pci.c    | 69 ++++++++---------
 drivers/net/wireless/realtek/rtlwifi/pci.h    |  3 -
 .../wireless/realtek/rtlwifi/rtl8821ae/hw.c   | 76 +++++--------------
 3 files changed, 48 insertions(+), 100 deletions(-)

-- 
2.30.2

[PATCH 1/7] wifi: rtlwifi: Convert LNKCTL change to PCIe cap RMW accessors

[Ilpo Järvinen]

The rtlwifi driver comes with custom code to write into PCIe Link
Control register. RMW access for the Link Control register requires
locking that is already provided by the standard PCIe capability
accessors.

Convert the custom RMW code writing into LNKCTL register to standard
RMW capability accessors. The accesses are changed to cover the full
LNKCTL register instead of touching just a single byte of the register.

After custom LNKCTL access code is removed, .num4bytes in the struct
mp_adapter is no longer needed.

Fixes: 0c8173385e54 ("rtl8192ce: Add new driver")
Fixes: 886e14b65a8f ("rtlwifi: Eliminate raw reads and writes from PCIe portion")
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/net/wireless/realtek/rtlwifi/pci.c | 39 +++++++++++-----------
 drivers/net/wireless/realtek/rtlwifi/pci.h |  2 --
 2 files changed, 19 insertions(+), 22 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/pci.c b/drivers/net/wireless/realtek/rtlwifi/pci.c
index 9886e719739b..d67a9617a19c 100644
--- a/drivers/net/wireless/realtek/rtlwifi/pci.c
+++ b/drivers/net/wireless/realtek/rtlwifi/pci.c
@@ -164,21 +164,27 @@ static bool _rtl_pci_platform_switch_device_pci_aspm(
 	struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
 	struct rtl_hal *rtlhal = rtl_hal(rtl_priv(hw));
 
+	value &= PCI_EXP_LNKCTL_ASPMC;
+
 	if (rtlhal->hw_type != HARDWARE_TYPE_RTL8192SE)
 		value |= 0x40;
 
-	pci_write_config_byte(rtlpci->pdev, 0x80, value);
+	pcie_capability_clear_and_set_word(rtlpci->pdev, PCI_EXP_LNKCTL,
+					   PCI_EXP_LNKCTL_ASPMC | value,
+					   value);
 
 	return false;
 }
 
 /*When we set 0x01 to enable clk request. Set 0x0 to disable clk req.*/
-static void _rtl_pci_switch_clk_req(struct ieee80211_hw *hw, u8 value)
+static void _rtl_pci_switch_clk_req(struct ieee80211_hw *hw, u16 value)
 {
 	struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
 	struct rtl_hal *rtlhal = rtl_hal(rtl_priv(hw));
 
-	pci_write_config_byte(rtlpci->pdev, 0x81, value);
+	pcie_capability_clear_and_set_word(rtlpci->pdev, PCI_EXP_LNKCTL,
+					   PCI_EXP_LNKCTL_CLKREQ_EN,
+					   value);
 
 	if (rtlhal->hw_type == HARDWARE_TYPE_RTL8192SE)
 		udelay(100);
@@ -192,11 +198,8 @@ static void rtl_pci_disable_aspm(struct ieee80211_hw *hw)
 	struct rtl_ps_ctl *ppsc = rtl_psc(rtl_priv(hw));
 	struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
 	u8 pcibridge_vendor = pcipriv->ndis_adapter.pcibridge_vendor;
-	u8 num4bytes = pcipriv->ndis_adapter.num4bytes;
 	/*Retrieve original configuration settings. */
 	u8 linkctrl_reg = pcipriv->ndis_adapter.linkctrl_reg;
-	u16 pcibridge_linkctrlreg = pcipriv->ndis_adapter.
-				pcibridge_linkctrlreg;
 	u16 aspmlevel = 0;
 	u8 tmp_u1b = 0;
 
@@ -221,15 +224,12 @@ static void rtl_pci_disable_aspm(struct ieee80211_hw *hw)
 	/*Set corresponding value. */
 	aspmlevel |= BIT(0) | BIT(1);
 	linkctrl_reg &= ~aspmlevel;
-	pcibridge_linkctrlreg &= ~(BIT(0) | BIT(1));
 
 	_rtl_pci_platform_switch_device_pci_aspm(hw, linkctrl_reg);
 	udelay(50);
 
-	/*4 Disable Pci Bridge ASPM */
-	pci_write_config_byte(rtlpci->pdev, (num4bytes << 2),
-			      pcibridge_linkctrlreg);
-
+	pcie_capability_clear_word(rtlpci->pdev, PCI_EXP_LNKCTL,
+					   PCI_EXP_LNKCTL_ASPMC);
 	udelay(50);
 }
 
@@ -245,7 +245,6 @@ static void rtl_pci_enable_aspm(struct ieee80211_hw *hw)
 	struct rtl_ps_ctl *ppsc = rtl_psc(rtl_priv(hw));
 	struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
 	u8 pcibridge_vendor = pcipriv->ndis_adapter.pcibridge_vendor;
-	u8 num4bytes = pcipriv->ndis_adapter.num4bytes;
 	u16 aspmlevel;
 	u8 u_pcibridge_aspmsetting;
 	u8 u_device_aspmsetting;
@@ -268,13 +267,14 @@ static void rtl_pci_enable_aspm(struct ieee80211_hw *hw)
 	if (pcibridge_vendor == PCI_BRIDGE_VENDOR_INTEL)
 		u_pcibridge_aspmsetting &= ~BIT(0);
 
-	pci_write_config_byte(rtlpci->pdev, (num4bytes << 2),
-			      u_pcibridge_aspmsetting);
+	pcie_capability_clear_and_set_word(rtlpci->pdev, PCI_EXP_LNKCTL,
+					   PCI_EXP_LNKCTL_ASPMC,
+					   u_pcibridge_aspmsetting &
+					   PCI_EXP_LNKCTL_ASPMC);
 
 	rtl_dbg(rtlpriv, COMP_INIT, DBG_LOUD,
-		"PlatformEnableASPM(): Write reg[%x] = %x\n",
-		(pcipriv->ndis_adapter.pcibridge_pciehdr_offset + 0x10),
-		u_pcibridge_aspmsetting);
+		"PlatformEnableASPM(): Write ASPM = %x\n",
+		u_pcibridge_aspmsetting & PCI_EXP_LNKCTL_ASPMC);
 
 	udelay(50);
 
@@ -291,7 +291,8 @@ static void rtl_pci_enable_aspm(struct ieee80211_hw *hw)
 
 	if (ppsc->reg_rfps_level & RT_RF_OFF_LEVL_CLK_REQ) {
 		_rtl_pci_switch_clk_req(hw, (ppsc->reg_rfps_level &
-					     RT_RF_OFF_LEVL_CLK_REQ) ? 1 : 0);
+					     RT_RF_OFF_LEVL_CLK_REQ) ?
+					     PCI_EXP_LNKCTL_CLKREQ_EN : 0);
 		RT_SET_PS_LEVEL(ppsc, RT_RF_OFF_LEVL_CLK_REQ);
 	}
 	udelay(100);
@@ -2030,8 +2031,6 @@ static bool _rtl_pci_find_adapter(struct pci_dev *pdev,
 		    PCI_FUNC(bridge_pdev->devfn);
 		pcipriv->ndis_adapter.pcibridge_pciehdr_offset =
 		    pci_pcie_cap(bridge_pdev);
-		pcipriv->ndis_adapter.num4bytes =
-		    (pcipriv->ndis_adapter.pcibridge_pciehdr_offset + 0x10) / 4;
 
 		rtl_pci_get_linkcontrol_field(hw);
 
diff --git a/drivers/net/wireless/realtek/rtlwifi/pci.h b/drivers/net/wireless/realtek/rtlwifi/pci.h
index 866861626a0a..57174b93db83 100644
--- a/drivers/net/wireless/realtek/rtlwifi/pci.h
+++ b/drivers/net/wireless/realtek/rtlwifi/pci.h
@@ -236,8 +236,6 @@ struct mp_adapter {
 	u16 pcibridge_vendorid;
 	u16 pcibridge_deviceid;
 
-	u8 num4bytes;
-
 	u8 pcibridge_pciehdr_offset;
 	u8 pcibridge_linkctrlreg;
 
-- 
2.30.2

Re: [PATCH 1/7] wifi: rtlwifi: Convert LNKCTL change to PCIe cap RMW accessors

[Bjorn Helgaas]

On Fri, Nov 17, 2023 at 11:44:19AM +0200, Ilpo Järvinen wrote:
> The rtlwifi driver comes with custom code to write into PCIe Link
> Control register. RMW access for the Link Control register requires
> locking that is already provided by the standard PCIe capability
> accessors.
> 
> Convert the custom RMW code writing into LNKCTL register to standard
> RMW capability accessors. The accesses are changed to cover the full
> LNKCTL register instead of touching just a single byte of the register.
> 
> After custom LNKCTL access code is removed, .num4bytes in the struct
> mp_adapter is no longer needed.

Looks like some nice fixes here.  I confess they're not all obvious to
me.

> @@ -164,21 +164,27 @@ static bool _rtl_pci_platform_switch_device_pci_aspm(
>  	struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
>  	struct rtl_hal *rtlhal = rtl_hal(rtl_priv(hw));
>  
> +	value &= PCI_EXP_LNKCTL_ASPMC;
> +
>  	if (rtlhal->hw_type != HARDWARE_TYPE_RTL8192SE)
>  		value |= 0x40;

I guess this 0x40 is PCI_EXP_LNKCTL_CCC?

> -	pci_write_config_byte(rtlpci->pdev, 0x80, value);
> +	pcie_capability_clear_and_set_word(rtlpci->pdev, PCI_EXP_LNKCTL,

PCI_EXP_LNKCTL is 0x10, so I guess we know somehow that the PCIe
Capability is at 0x70?

> +					   PCI_EXP_LNKCTL_ASPMC | value,
> +					   value);
>  
>  	return false;
>  }
>  
>  /*When we set 0x01 to enable clk request. Set 0x0 to disable clk req.*/
> -static void _rtl_pci_switch_clk_req(struct ieee80211_hw *hw, u8 value)
> +static void _rtl_pci_switch_clk_req(struct ieee80211_hw *hw, u16 value)
>  {
>  	struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
>  	struct rtl_hal *rtlhal = rtl_hal(rtl_priv(hw));
>  
> -	pci_write_config_byte(rtlpci->pdev, 0x81, value);
> +	pcie_capability_clear_and_set_word(rtlpci->pdev, PCI_EXP_LNKCTL,
> +					   PCI_EXP_LNKCTL_CLKREQ_EN,

Depends on the fact that the caller only passes 0 or 1.  Ugly, but
looks true, and I see you clean this up a little more later.  I like
how you made it explicit in _rtl_pci_platform_switch_device_pci_aspm()
above by masking the value to set.

> +					   value);
>  
>  	if (rtlhal->hw_type == HARDWARE_TYPE_RTL8192SE)
>  		udelay(100);
> @@ -192,11 +198,8 @@ static void rtl_pci_disable_aspm(struct ieee80211_hw *hw)
>  	struct rtl_ps_ctl *ppsc = rtl_psc(rtl_priv(hw));
>  	struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
>  	u8 pcibridge_vendor = pcipriv->ndis_adapter.pcibridge_vendor;
> -	u8 num4bytes = pcipriv->ndis_adapter.num4bytes;
>  	/*Retrieve original configuration settings. */
>  	u8 linkctrl_reg = pcipriv->ndis_adapter.linkctrl_reg;
> -	u16 pcibridge_linkctrlreg = pcipriv->ndis_adapter.
> -				pcibridge_linkctrlreg;
>  	u16 aspmlevel = 0;
>  	u8 tmp_u1b = 0;
>  
> @@ -221,15 +224,12 @@ static void rtl_pci_disable_aspm(struct ieee80211_hw *hw)
>  	/*Set corresponding value. */
>  	aspmlevel |= BIT(0) | BIT(1);
>  	linkctrl_reg &= ~aspmlevel;
> -	pcibridge_linkctrlreg &= ~(BIT(0) | BIT(1));
>  
>  	_rtl_pci_platform_switch_device_pci_aspm(hw, linkctrl_reg);
>  	udelay(50);
>  
> -	/*4 Disable Pci Bridge ASPM */
> -	pci_write_config_byte(rtlpci->pdev, (num4bytes << 2),
> -			      pcibridge_linkctrlreg);
> -
> +	pcie_capability_clear_word(rtlpci->pdev, PCI_EXP_LNKCTL,
> +					   PCI_EXP_LNKCTL_ASPMC);
>  	udelay(50);
>  }
>  
> @@ -245,7 +245,6 @@ static void rtl_pci_enable_aspm(struct ieee80211_hw *hw)
>  	struct rtl_ps_ctl *ppsc = rtl_psc(rtl_priv(hw));
>  	struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
>  	u8 pcibridge_vendor = pcipriv->ndis_adapter.pcibridge_vendor;
> -	u8 num4bytes = pcipriv->ndis_adapter.num4bytes;
>  	u16 aspmlevel;
>  	u8 u_pcibridge_aspmsetting;
>  	u8 u_device_aspmsetting;
> @@ -268,13 +267,14 @@ static void rtl_pci_enable_aspm(struct ieee80211_hw *hw)
>  	if (pcibridge_vendor == PCI_BRIDGE_VENDOR_INTEL)
>  		u_pcibridge_aspmsetting &= ~BIT(0);
>  
> -	pci_write_config_byte(rtlpci->pdev, (num4bytes << 2),
> -			      u_pcibridge_aspmsetting);
> +	pcie_capability_clear_and_set_word(rtlpci->pdev, PCI_EXP_LNKCTL,
> +					   PCI_EXP_LNKCTL_ASPMC,
> +					   u_pcibridge_aspmsetting &
> +					   PCI_EXP_LNKCTL_ASPMC);
>  
>  	rtl_dbg(rtlpriv, COMP_INIT, DBG_LOUD,
> -		"PlatformEnableASPM(): Write reg[%x] = %x\n",
> -		(pcipriv->ndis_adapter.pcibridge_pciehdr_offset + 0x10),
> -		u_pcibridge_aspmsetting);
> +		"PlatformEnableASPM(): Write ASPM = %x\n",
> +		u_pcibridge_aspmsetting & PCI_EXP_LNKCTL_ASPMC);
>  
>  	udelay(50);
>  
> @@ -291,7 +291,8 @@ static void rtl_pci_enable_aspm(struct ieee80211_hw *hw)
>  
>  	if (ppsc->reg_rfps_level & RT_RF_OFF_LEVL_CLK_REQ) {
>  		_rtl_pci_switch_clk_req(hw, (ppsc->reg_rfps_level &
> -					     RT_RF_OFF_LEVL_CLK_REQ) ? 1 : 0);
> +					     RT_RF_OFF_LEVL_CLK_REQ) ?
> +					     PCI_EXP_LNKCTL_CLKREQ_EN : 0);
>  		RT_SET_PS_LEVEL(ppsc, RT_RF_OFF_LEVL_CLK_REQ);
>  	}
>  	udelay(100);
> @@ -2030,8 +2031,6 @@ static bool _rtl_pci_find_adapter(struct pci_dev *pdev,
>  		    PCI_FUNC(bridge_pdev->devfn);
>  		pcipriv->ndis_adapter.pcibridge_pciehdr_offset =
>  		    pci_pcie_cap(bridge_pdev);
> -		pcipriv->ndis_adapter.num4bytes =
> -		    (pcipriv->ndis_adapter.pcibridge_pciehdr_offset + 0x10) / 4;

I don't understand what's going on here.  Are we caching the PCIe
Capability offset of the *upstream bridge* here?  And then computing
the dword offset of the *bridge's* LNKCTL?  And then writing a byte to
the rtlwifi device (not the bridge) at the dword offset << 2, i.e., the
byte offset?  I must be out to lunch, because how could that ever
work?

If we were using the bridge capability location to write to the
rtlwifi device, that would clearly be a bug fix that would merit its
own patch.

Maybe this num4bytes thing could be its own patch, too.  Seems so
cumbersome that it makes me wonder if the device has issues with
larger accesses.

>  		rtl_pci_get_linkcontrol_field(hw);
>  
> diff --git a/drivers/net/wireless/realtek/rtlwifi/pci.h b/drivers/net/wireless/realtek/rtlwifi/pci.h
> index 866861626a0a..57174b93db83 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/pci.h
> +++ b/drivers/net/wireless/realtek/rtlwifi/pci.h
> @@ -236,8 +236,6 @@ struct mp_adapter {
>  	u16 pcibridge_vendorid;
>  	u16 pcibridge_deviceid;
>  
> -	u8 num4bytes;
> -
>  	u8 pcibridge_pciehdr_offset;
>  	u8 pcibridge_linkctrlreg;
>  
> -- 
> 2.30.2
> 

Re: [PATCH 1/7] wifi: rtlwifi: Convert LNKCTL change to PCIe cap RMW accessors

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 5613 bytes --]

On Fri, 17 Nov 2023, Bjorn Helgaas wrote:

> On Fri, Nov 17, 2023 at 11:44:19AM +0200, Ilpo Järvinen wrote:
> > The rtlwifi driver comes with custom code to write into PCIe Link
> > Control register. RMW access for the Link Control register requires
> > locking that is already provided by the standard PCIe capability
> > accessors.
> > 
> > Convert the custom RMW code writing into LNKCTL register to standard
> > RMW capability accessors. The accesses are changed to cover the full
> > LNKCTL register instead of touching just a single byte of the register.
> > 
> > After custom LNKCTL access code is removed, .num4bytes in the struct
> > mp_adapter is no longer needed.
> 
> Looks like some nice fixes here.  I confess they're not all obvious to
> me.

It took a while to figure out what it is doing, yes... and while figuring
it out, I found more and more to cleanup... And seems you also found some
more... ;-) lol.

When going all the way with cleanups, I tend run this kind of things that 
are not all that obvious but those are usually the most valuable cleanups 
(and normally cannot be automated either so we'll have a lot less people 
looking at them).

> > @@ -164,21 +164,27 @@ static bool _rtl_pci_platform_switch_device_pci_aspm(
> >  	struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
> >  	struct rtl_hal *rtlhal = rtl_hal(rtl_priv(hw));
> >  
> > +	value &= PCI_EXP_LNKCTL_ASPMC;
> > +
> >  	if (rtlhal->hw_type != HARDWARE_TYPE_RTL8192SE)
> >  		value |= 0x40;
> 
> I guess this 0x40 is PCI_EXP_LNKCTL_CCC?

Good point, I forgot to change that in 2/7 and it belongs logically into 
this patch anyway so I'll add it to this patch.

> > -	pci_write_config_byte(rtlpci->pdev, 0x80, value);
> > +	pcie_capability_clear_and_set_word(rtlpci->pdev, PCI_EXP_LNKCTL,
> 
> PCI_EXP_LNKCTL is 0x10, so I guess we know somehow that the PCIe
> Capability is at 0x70?

It's what I inferred based on the offsets of LNKCTL & DEVCTL2. And if 
that assumption does not hold with all these devices, the writes would 
just wreck some random havoc. :-/

> > +					   PCI_EXP_LNKCTL_ASPMC | value,
> > +					   value);
> >  
> >  	return false;
> >  }
> >  
> >  /*When we set 0x01 to enable clk request. Set 0x0 to disable clk req.*/
> > -static void _rtl_pci_switch_clk_req(struct ieee80211_hw *hw, u8 value)
> > +static void _rtl_pci_switch_clk_req(struct ieee80211_hw *hw, u16 value)
> >  {
> >  	struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
> >  	struct rtl_hal *rtlhal = rtl_hal(rtl_priv(hw));
> >  
> > -	pci_write_config_byte(rtlpci->pdev, 0x81, value);
> > +	pcie_capability_clear_and_set_word(rtlpci->pdev, PCI_EXP_LNKCTL,
> > +					   PCI_EXP_LNKCTL_CLKREQ_EN,
> 
> Depends on the fact that the caller only passes 0 or 1.  Ugly, but
> looks true, and I see you clean this up a little more later.  I like
> how you made it explicit in _rtl_pci_platform_switch_device_pci_aspm()
> above by masking the value to set.

I thought of adding the masking here too but since it wasn't strictly 
necessary, I didn't. But I can add that now and I'll also update the 
comment to match the code :-).

These code fragments hopefully die anyway once I get the ASPM from 
drivers to core series done.

> > @@ -268,13 +267,14 @@ static void rtl_pci_enable_aspm(struct ieee80211_hw *hw)
> >  	if (pcibridge_vendor == PCI_BRIDGE_VENDOR_INTEL)
> >  		u_pcibridge_aspmsetting &= ~BIT(0);
> >  
> > -	pci_write_config_byte(rtlpci->pdev, (num4bytes << 2),
> > -			      u_pcibridge_aspmsetting);
> > +	pcie_capability_clear_and_set_word(rtlpci->pdev, PCI_EXP_LNKCTL,
> > +					   PCI_EXP_LNKCTL_ASPMC,
> > +					   u_pcibridge_aspmsetting &
> > +					   PCI_EXP_LNKCTL_ASPMC);
> >  
> >  	rtl_dbg(rtlpriv, COMP_INIT, DBG_LOUD,
> > -		"PlatformEnableASPM(): Write reg[%x] = %x\n",
> > -		(pcipriv->ndis_adapter.pcibridge_pciehdr_offset + 0x10),
> > -		u_pcibridge_aspmsetting);
> > +		"PlatformEnableASPM(): Write ASPM = %x\n",
> > +		u_pcibridge_aspmsetting & PCI_EXP_LNKCTL_ASPMC);
> >  
> >  	udelay(50);

> > @@ -2030,8 +2031,6 @@ static bool _rtl_pci_find_adapter(struct pci_dev *pdev,
> >  		    PCI_FUNC(bridge_pdev->devfn);
> >  		pcipriv->ndis_adapter.pcibridge_pciehdr_offset =
> >  		    pci_pcie_cap(bridge_pdev);
> > -		pcipriv->ndis_adapter.num4bytes =
> > -		    (pcipriv->ndis_adapter.pcibridge_pciehdr_offset + 0x10) / 4;
> 
> I don't understand what's going on here.  Are we caching the PCIe
> Capability offset of the *upstream bridge* here?  And then computing
> the dword offset of the *bridge's* LNKCTL? And then writing a byte to
> the rtlwifi device (not the bridge) at the dword offset << 2, i.e., the
> byte offset?  I must be out to lunch, because how could that ever
> work?
>
> If we were using the bridge capability location to write to the
> rtlwifi device, that would clearly be a bug fix that would merit its
> own patch.

Oh no, I entirely missed it because of those nice comments which tell it's 
trying to update bridge's ASPM so I didn't pay attention to which device 
it passes for real...

Now I'm left to wonder what would be the best course of action here...
Since it has never really written to bridge's ASPM at all, perhaps that's 
not needed and those writes could just be dropped rather than point them 
to the correct device.

> Maybe this num4bytes thing could be its own patch, too.  Seems so
> cumbersome that it makes me wonder if the device has issues with
> larger accesses.

I thought of that but since it was just 2 extra context blocks which are 
clearly disjoint from the rest, I didn't separate the removal of the 
member variable to its own patch.


-- 
 i.

Re: [PATCH 1/7] wifi: rtlwifi: Convert LNKCTL change to PCIe cap RMW accessors

[Kalle Valo]

Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> The rtlwifi driver comes with custom code to write into PCIe Link
> Control register. RMW access for the Link Control register requires
> locking that is already provided by the standard PCIe capability
> accessors.
> 
> Convert the custom RMW code writing into LNKCTL register to standard
> RMW capability accessors. The accesses are changed to cover the full
> LNKCTL register instead of touching just a single byte of the register.
> 
> After custom LNKCTL access code is removed, .num4bytes in the struct
> mp_adapter is no longer needed.
> 
> Fixes: 0c8173385e54 ("rtl8192ce: Add new driver")
> Fixes: 886e14b65a8f ("rtlwifi: Eliminate raw reads and writes from PCIe portion")
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

In the next version please add "wifi:" to the title for patches 3-7.

7 patches set to Changes Requested.

13458674 [1/7] wifi: rtlwifi: Convert LNKCTL change to PCIe cap RMW accessors
13458675 [2/7] wifi: rtlwifi: Convert to use PCIe capability accessors
13458676 [3/7] rtlwifi: rtl8821ae: Remove unnecessary PME_Status bit set
13458677 [4/7] rtlwifi: rtl8821ae: Reverse PM capability exists check
13458706 [5/7] rtlwifi: rtl8821ae: Use pci_find_capability()
13458678 [6/7] rtlwifi: rtl8821ae: Add pdev into _rtl8821ae_clear_pci_pme_status()
13458705 [7/7] rtlwifi: rtl8821ae: Access full PMCS reg and use pci_regs.h

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20231117094425.80477-2-ilpo.jarvinen@linux.intel.com/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

[PATCH 2/7] wifi: rtlwifi: Convert to use PCIe capability accessors

[Ilpo Järvinen]

The rtlwifi driver accesses PCIe capabilities through custom config
offsets.

Convert the accesses to use the normal PCIe capability accessors.
pcibridge_pciehdr_offset in the struct mp_adapter becomes unused after
the conversion and can be removed.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/net/wireless/realtek/rtlwifi/pci.c | 30 ++++++++--------------
 drivers/net/wireless/realtek/rtlwifi/pci.h |  1 -
 2 files changed, 11 insertions(+), 20 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/pci.c b/drivers/net/wireless/realtek/rtlwifi/pci.c
index d67a9617a19c..0c6f03d845a9 100644
--- a/drivers/net/wireless/realtek/rtlwifi/pci.c
+++ b/drivers/net/wireless/realtek/rtlwifi/pci.c
@@ -64,7 +64,7 @@ static void _rtl_pci_update_default_setting(struct ieee80211_hw *hw)
 	struct rtl_ps_ctl *ppsc = rtl_psc(rtl_priv(hw));
 	struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
 	u8 pcibridge_vendor = pcipriv->ndis_adapter.pcibridge_vendor;
-	u8 init_aspm;
+	u16 init_aspm;
 
 	ppsc->reg_rfps_level = 0;
 	ppsc->support_aspm = false;
@@ -151,9 +151,10 @@ static void _rtl_pci_update_default_setting(struct ieee80211_hw *hw)
 	/* toshiba aspm issue, toshiba will set aspm selfly
 	 * so we should not set aspm in driver
 	 */
-	pci_read_config_byte(rtlpci->pdev, 0x80, &init_aspm);
+	pcie_capability_read_word(rtlpci->pdev, PCI_EXP_LNKCTL, &init_aspm);
 	if (rtlpriv->rtlhal.hw_type == HARDWARE_TYPE_RTL8192SE &&
-	    init_aspm == 0x43)
+	    ((u8)init_aspm) == (PCI_EXP_LNKCTL_ASPM_L0S |
+				PCI_EXP_LNKCTL_ASPM_L1 | PCI_EXP_LNKCTL_CCC))
 		ppsc->support_aspm = false;
 }
 
@@ -201,7 +202,7 @@ static void rtl_pci_disable_aspm(struct ieee80211_hw *hw)
 	/*Retrieve original configuration settings. */
 	u8 linkctrl_reg = pcipriv->ndis_adapter.linkctrl_reg;
 	u16 aspmlevel = 0;
-	u8 tmp_u1b = 0;
+	u16 tmp_u1b = 0;
 
 	if (!ppsc->support_aspm)
 		return;
@@ -219,7 +220,7 @@ static void rtl_pci_disable_aspm(struct ieee80211_hw *hw)
 	}
 
 	/*for promising device will in L0 state after an I/O. */
-	pci_read_config_byte(rtlpci->pdev, 0x80, &tmp_u1b);
+	pcie_capability_read_word(rtlpci->pdev, PCI_EXP_LNKCTL, &tmp_u1b);
 
 	/*Set corresponding value. */
 	aspmlevel |= BIT(0) | BIT(1);
@@ -363,14 +364,9 @@ static void rtl_pci_get_linkcontrol_field(struct ieee80211_hw *hw)
 {
 	struct rtl_pci_priv *pcipriv = rtl_pcipriv(hw);
 	struct rtl_pci *rtlpci = rtl_pcidev(pcipriv);
-	u8 capabilityoffset = pcipriv->ndis_adapter.pcibridge_pciehdr_offset;
-	u8 linkctrl_reg;
-	u8 num4bbytes;
-
-	num4bbytes = (capabilityoffset + 0x10) / 4;
+	u16 linkctrl_reg;
 
-	/*Read  Link Control Register */
-	pci_read_config_byte(rtlpci->pdev, (num4bbytes << 2), &linkctrl_reg);
+	pcie_capability_read_word(rtlpci->pdev, PCI_EXP_LNKCTL, &linkctrl_reg);
 
 	pcipriv->ndis_adapter.pcibridge_linkctrlreg = linkctrl_reg;
 }
@@ -391,9 +387,8 @@ static void rtl_pci_parse_configuration(struct pci_dev *pdev,
 	rtl_dbg(rtlpriv, COMP_INIT, DBG_TRACE, "Link Control Register =%x\n",
 		pcipriv->ndis_adapter.linkctrl_reg);
 
-	pci_read_config_byte(pdev, 0x98, &tmp);
-	tmp |= BIT(4);
-	pci_write_config_byte(pdev, 0x98, tmp);
+	pcie_capability_set_word(pdev, PCI_EXP_DEVCTL2,
+				 PCI_EXP_DEVCTL2_COMP_TMOUT_DIS);
 
 	tmp = 0x17;
 	pci_write_config_byte(pdev, 0x70f, tmp);
@@ -2029,8 +2024,6 @@ static bool _rtl_pci_find_adapter(struct pci_dev *pdev,
 		    PCI_SLOT(bridge_pdev->devfn);
 		pcipriv->ndis_adapter.pcibridge_funcnum =
 		    PCI_FUNC(bridge_pdev->devfn);
-		pcipriv->ndis_adapter.pcibridge_pciehdr_offset =
-		    pci_pcie_cap(bridge_pdev);
 
 		rtl_pci_get_linkcontrol_field(hw);
 
@@ -2049,12 +2042,11 @@ static bool _rtl_pci_find_adapter(struct pci_dev *pdev,
 		pdev->vendor, pcipriv->ndis_adapter.linkctrl_reg);
 
 	rtl_dbg(rtlpriv, COMP_INIT, DBG_DMESG,
-		"pci_bridge busnumber:devnumber:funcnumber:vendor:pcie_cap:link_ctl_reg:amd %d:%d:%d:%x:%x:%x:%x\n",
+		"pci_bridge busnumber:devnumber:funcnumber:vendor:link_ctl_reg:amd %d:%d:%d:%x:%x:%x\n",
 		pcipriv->ndis_adapter.pcibridge_busnum,
 		pcipriv->ndis_adapter.pcibridge_devnum,
 		pcipriv->ndis_adapter.pcibridge_funcnum,
 		pcibridge_vendors[pcipriv->ndis_adapter.pcibridge_vendor],
-		pcipriv->ndis_adapter.pcibridge_pciehdr_offset,
 		pcipriv->ndis_adapter.pcibridge_linkctrlreg,
 		pcipriv->ndis_adapter.amd_l1_patch);
 
diff --git a/drivers/net/wireless/realtek/rtlwifi/pci.h b/drivers/net/wireless/realtek/rtlwifi/pci.h
index 57174b93db83..281da0b52564 100644
--- a/drivers/net/wireless/realtek/rtlwifi/pci.h
+++ b/drivers/net/wireless/realtek/rtlwifi/pci.h
@@ -236,7 +236,6 @@ struct mp_adapter {
 	u16 pcibridge_vendorid;
 	u16 pcibridge_deviceid;
 
-	u8 pcibridge_pciehdr_offset;
 	u8 pcibridge_linkctrlreg;
 
 	bool amd_l1_patch;
-- 
2.30.2

Re: [PATCH 2/7] wifi: rtlwifi: Convert to use PCIe capability accessors

[Bjorn Helgaas]

On Fri, Nov 17, 2023 at 11:44:20AM +0200, Ilpo Järvinen wrote:
> The rtlwifi driver accesses PCIe capabilities through custom config
> offsets.
> 
> Convert the accesses to use the normal PCIe capability accessors.
> pcibridge_pciehdr_offset in the struct mp_adapter becomes unused after
> the conversion and can be removed.

More good stuff.  I guess patch [1/7] was specifically for the RMW
things, and this one is for the rest?

> @@ -219,7 +220,7 @@ static void rtl_pci_disable_aspm(struct ieee80211_hw *hw)
>  	}
>  
>  	/*for promising device will in L0 state after an I/O. */
> -	pci_read_config_byte(rtlpci->pdev, 0x80, &tmp_u1b);
> +	pcie_capability_read_word(rtlpci->pdev, PCI_EXP_LNKCTL, &tmp_u1b);
>  
>  	/*Set corresponding value. */
>  	aspmlevel |= BIT(0) | BIT(1);

I guess this is PCI_EXP_LNKCTL_ASPM_L0S | PCI_EXP_LNKCTL_ASPM_L1?

There's also a similar u_pcibridge_aspmsetting mask in
rtl_pci_enable_aspm().

And some scary looking stuff in rtl_pci_get_amd_l1_patch().  And
platform_enable_dma64().  No clue what either of those does.

Re: [PATCH 2/7] wifi: rtlwifi: Convert to use PCIe capability accessors

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1623 bytes --]

On Fri, 17 Nov 2023, Bjorn Helgaas wrote:

> On Fri, Nov 17, 2023 at 11:44:20AM +0200, Ilpo Järvinen wrote:
> > The rtlwifi driver accesses PCIe capabilities through custom config
> > offsets.
> > 
> > Convert the accesses to use the normal PCIe capability accessors.
> > pcibridge_pciehdr_offset in the struct mp_adapter becomes unused after
> > the conversion and can be removed.
> 
> More good stuff.  I guess patch [1/7] was specifically for the RMW
> things, and this one is for the rest?

Yes, I wanted to separate them because of the Fixes tag.

> > @@ -219,7 +220,7 @@ static void rtl_pci_disable_aspm(struct ieee80211_hw *hw)
> >  	}
> >  
> >  	/*for promising device will in L0 state after an I/O. */
> > -	pci_read_config_byte(rtlpci->pdev, 0x80, &tmp_u1b);
> > +	pcie_capability_read_word(rtlpci->pdev, PCI_EXP_LNKCTL, &tmp_u1b);
> >  
> >  	/*Set corresponding value. */
> >  	aspmlevel |= BIT(0) | BIT(1);
> 
> I guess this is PCI_EXP_LNKCTL_ASPM_L0S | PCI_EXP_LNKCTL_ASPM_L1?

I'll change it too. There was just so much to cleanup I started to miss 
things even this obvious :-(.

Also, I was not at all sure if that read from LNKCTL is really trying to 
achieve. The comment sounds like it's trying to ensure the dev is in L0
but why it cares? These drivers do so odd things :-).

> There's also a similar u_pcibridge_aspmsetting mask in
> rtl_pci_enable_aspm().

Yes, but I'll put that into 1/7 since it's related to the change made 
there.

> And some scary looking stuff in rtl_pci_get_amd_l1_patch().
> And platform_enable_dma64().  No clue what either of those does.

Those elude me as well.


-- 
 i.

[PATCH 3/7] rtlwifi: rtl8821ae: Remove unnecessary PME_Status bit set

[Ilpo Järvinen]

BIT(7) (PME_Status) is first checked and then set unnecessarily. Remove
the unnecessary setting for the bit that is already on and adjust the
comment related to it.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
index 1633328bc3d1..6ae37d61a2a2 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
@@ -2312,9 +2312,7 @@ static void _rtl8821ae_clear_pci_pme_status(struct ieee80211_hw *hw)
 		pci_read_config_byte(rtlpci->pdev, cap_pointer + 5, &pmcs_reg);
 
 		if (pmcs_reg & BIT(7)) {
-			/* PME event occured, clear the PM_Status by write 1 */
-			pmcs_reg = pmcs_reg | BIT(7);
-
+			/* Clear PME_Status with write */
 			pci_write_config_byte(rtlpci->pdev, cap_pointer + 5,
 					      pmcs_reg);
 			/* Read it back to check */
-- 
2.30.2

[PATCH 4/7] rtlwifi: rtl8821ae: Reverse PM capability exists check

[Ilpo Järvinen]

Check if PM capability does not exists and return early which follows
the usual error handling pattern.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 .../wireless/realtek/rtlwifi/rtl8821ae/hw.c   | 45 ++++++++++---------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
index 6ae37d61a2a2..53cfeed0b030 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
@@ -2305,30 +2305,31 @@ static void _rtl8821ae_clear_pci_pme_status(struct ieee80211_hw *hw)
 		}
 	} while (cnt++ < 200);
 
-	if (cap_id == 0x01) {
-		/* Get the PM CSR (Control/Status Register),
-		 * The PME_Status is located at PM Capatibility offset 5, bit 7
-		 */
-		pci_read_config_byte(rtlpci->pdev, cap_pointer + 5, &pmcs_reg);
-
-		if (pmcs_reg & BIT(7)) {
-			/* Clear PME_Status with write */
-			pci_write_config_byte(rtlpci->pdev, cap_pointer + 5,
-					      pmcs_reg);
-			/* Read it back to check */
-			pci_read_config_byte(rtlpci->pdev, cap_pointer + 5,
-					     &pmcs_reg);
-			rtl_dbg(rtlpriv, COMP_INIT, DBG_DMESG,
-				"Clear PME status 0x%2x to 0x%2x\n",
-				cap_pointer + 5, pmcs_reg);
-		} else {
-			rtl_dbg(rtlpriv, COMP_INIT, DBG_DMESG,
-				"PME status(0x%2x) = 0x%2x\n",
-				cap_pointer + 5, pmcs_reg);
-		}
-	} else {
+	if (cap_id != 0x01) {
 		rtl_dbg(rtlpriv, COMP_INIT, DBG_WARNING,
 			"Cannot find PME Capability\n");
+		return;
+	}
+
+	/* Get the PM CSR (Control/Status Register),
+	 * The PME_Status is located at PM Capatibility offset 5, bit 7
+	 */
+	pci_read_config_byte(rtlpci->pdev, cap_pointer + 5, &pmcs_reg);
+
+	if (pmcs_reg & BIT(7)) {
+		/* Clear PME_Status with write */
+		pci_write_config_byte(rtlpci->pdev, cap_pointer + 5,
+				      pmcs_reg);
+		/* Read it back to check */
+		pci_read_config_byte(rtlpci->pdev, cap_pointer + 5,
+				     &pmcs_reg);
+		rtl_dbg(rtlpriv, COMP_INIT, DBG_DMESG,
+			"Clear PME status 0x%2x to 0x%2x\n",
+			cap_pointer + 5, pmcs_reg);
+	} else {
+		rtl_dbg(rtlpriv, COMP_INIT, DBG_DMESG,
+			"PME status(0x%2x) = 0x%2x\n",
+			cap_pointer + 5, pmcs_reg);
 	}
 }
 
-- 
2.30.2

Re: [PATCH 4/7] rtlwifi: rtl8821ae: Reverse PM capability exists check

[Bjorn Helgaas]

On Fri, Nov 17, 2023 at 11:44:22AM +0200, Ilpo Järvinen wrote:
> Check if PM capability does not exists and return early which follows
> the usual error handling pattern.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  .../wireless/realtek/rtlwifi/rtl8821ae/hw.c   | 45 ++++++++++---------
>  1 file changed, 23 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
> index 6ae37d61a2a2..53cfeed0b030 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
> @@ -2305,30 +2305,31 @@ static void _rtl8821ae_clear_pci_pme_status(struct ieee80211_hw *hw)
>  		}
>  	} while (cnt++ < 200);
>  
> -	if (cap_id == 0x01) {
> -		/* Get the PM CSR (Control/Status Register),
> -		 * The PME_Status is located at PM Capatibility offset 5, bit 7
> -		 */
> -		pci_read_config_byte(rtlpci->pdev, cap_pointer + 5, &pmcs_reg);
> -
> -		if (pmcs_reg & BIT(7)) {
> -			/* Clear PME_Status with write */
> -			pci_write_config_byte(rtlpci->pdev, cap_pointer + 5,
> -					      pmcs_reg);
> -			/* Read it back to check */
> -			pci_read_config_byte(rtlpci->pdev, cap_pointer + 5,
> -					     &pmcs_reg);
> -			rtl_dbg(rtlpriv, COMP_INIT, DBG_DMESG,
> -				"Clear PME status 0x%2x to 0x%2x\n",
> -				cap_pointer + 5, pmcs_reg);
> -		} else {
> -			rtl_dbg(rtlpriv, COMP_INIT, DBG_DMESG,
> -				"PME status(0x%2x) = 0x%2x\n",
> -				cap_pointer + 5, pmcs_reg);
> -		}
> -	} else {
> +	if (cap_id != 0x01) {
>  		rtl_dbg(rtlpriv, COMP_INIT, DBG_WARNING,
>  			"Cannot find PME Capability\n");
> +		return;
> +	}
> +
> +	/* Get the PM CSR (Control/Status Register),
> +	 * The PME_Status is located at PM Capatibility offset 5, bit 7

s/Capatibility/Capability/

But this is PCI_PM_CTRL and PCI_PM_CTRL_PME_STATUS (with a word read),
right?  No need for a comment then.

I don't know why the driver needs to do this, but I'm skeptical.  The
only other drivers that look at PCI_PM_CTRL_PME_STATUS themselves are
bnx2x and ksz884xp (ksz884x.c), so this is highly suspicious.

> +	 */
> +	pci_read_config_byte(rtlpci->pdev, cap_pointer + 5, &pmcs_reg);
> +
> +	if (pmcs_reg & BIT(7)) {
> +		/* Clear PME_Status with write */
> +		pci_write_config_byte(rtlpci->pdev, cap_pointer + 5,
> +				      pmcs_reg);
> +		/* Read it back to check */
> +		pci_read_config_byte(rtlpci->pdev, cap_pointer + 5,
> +				     &pmcs_reg);
> +		rtl_dbg(rtlpriv, COMP_INIT, DBG_DMESG,
> +			"Clear PME status 0x%2x to 0x%2x\n",
> +			cap_pointer + 5, pmcs_reg);
> +	} else {
> +		rtl_dbg(rtlpriv, COMP_INIT, DBG_DMESG,
> +			"PME status(0x%2x) = 0x%2x\n",
> +			cap_pointer + 5, pmcs_reg);
>  	}
>  }
>  
> -- 
> 2.30.2
> 

Re: [PATCH 4/7] rtlwifi: rtl8821ae: Reverse PM capability exists check

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2529 bytes --]

On Fri, 17 Nov 2023, Bjorn Helgaas wrote:

> On Fri, Nov 17, 2023 at 11:44:22AM +0200, Ilpo Järvinen wrote:
> > Check if PM capability does not exists and return early which follows
> > the usual error handling pattern.
> > 
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > ---
> >  .../wireless/realtek/rtlwifi/rtl8821ae/hw.c   | 45 ++++++++++---------
> >  1 file changed, 23 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
> > index 6ae37d61a2a2..53cfeed0b030 100644
> > --- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
> > +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
> > @@ -2305,30 +2305,31 @@ static void _rtl8821ae_clear_pci_pme_status(struct ieee80211_hw *hw)
> >  		}
> >  	} while (cnt++ < 200);
> >  
> > -	if (cap_id == 0x01) {
> > -		/* Get the PM CSR (Control/Status Register),
> > -		 * The PME_Status is located at PM Capatibility offset 5, bit 7
> > -		 */
> > -		pci_read_config_byte(rtlpci->pdev, cap_pointer + 5, &pmcs_reg);
> > -
> > -		if (pmcs_reg & BIT(7)) {
> > -			/* Clear PME_Status with write */
> > -			pci_write_config_byte(rtlpci->pdev, cap_pointer + 5,
> > -					      pmcs_reg);
> > -			/* Read it back to check */
> > -			pci_read_config_byte(rtlpci->pdev, cap_pointer + 5,
> > -					     &pmcs_reg);
> > -			rtl_dbg(rtlpriv, COMP_INIT, DBG_DMESG,
> > -				"Clear PME status 0x%2x to 0x%2x\n",
> > -				cap_pointer + 5, pmcs_reg);
> > -		} else {
> > -			rtl_dbg(rtlpriv, COMP_INIT, DBG_DMESG,
> > -				"PME status(0x%2x) = 0x%2x\n",
> > -				cap_pointer + 5, pmcs_reg);
> > -		}
> > -	} else {
> > +	if (cap_id != 0x01) {
> >  		rtl_dbg(rtlpriv, COMP_INIT, DBG_WARNING,
> >  			"Cannot find PME Capability\n");
> > +		return;
> > +	}
> > +
> > +	/* Get the PM CSR (Control/Status Register),
> > +	 * The PME_Status is located at PM Capatibility offset 5, bit 7

> But this is PCI_PM_CTRL and PCI_PM_CTRL_PME_STATUS (with a word read),
> right?  No need for a comment then.
> 
> I don't know why the driver needs to do this, but I'm skeptical.  The
> only other drivers that look at PCI_PM_CTRL_PME_STATUS themselves are
> bnx2x and ksz884xp (ksz884x.c), so this is highly suspicious.

I hope you are not asking from me because I'm just the poor guy who tries 
to cleanup this mess. ;-) ...So all I can do is shrug and say, I 
unfortunately don't know the answer.

Hopefully somebody more familiar with the devices could chime in.


-- 
 i.

[PATCH 5/7] rtlwifi: rtl8821ae: Use pci_find_capability()

[Ilpo Järvinen]

Instead of open coding the capability structure search, find the PM
Capability using pci_find_capability().

While at it, rename the generic 'cap_pointer' to 'pm_cap' which makes
the intent of the code more obvious.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 .../wireless/realtek/rtlwifi/rtl8821ae/hw.c   | 49 +++----------------
 1 file changed, 8 insertions(+), 41 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
index 53cfeed0b030..7877509c34c7 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
@@ -2270,42 +2270,11 @@ static void _rtl8821ae_clear_pci_pme_status(struct ieee80211_hw *hw)
 {
 	struct rtl_priv *rtlpriv = rtl_priv(hw);
 	struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
-	u16 cap_hdr;
-	u8 cap_pointer;
-	u8 cap_id = 0xff;
 	u8 pmcs_reg;
-	u8 cnt = 0;
+	u8 pm_cap;
 
-	/* Get the Capability pointer first,
-	 * the Capability Pointer is located at
-	 * offset 0x34 from the Function Header */
-
-	pci_read_config_byte(rtlpci->pdev, 0x34, &cap_pointer);
-	rtl_dbg(rtlpriv, COMP_INIT, DBG_LOUD,
-		"PCI configuration 0x34 = 0x%2x\n", cap_pointer);
-
-	do {
-		pci_read_config_word(rtlpci->pdev, cap_pointer, &cap_hdr);
-		cap_id = cap_hdr & 0xFF;
-
-		rtl_dbg(rtlpriv, COMP_INIT, DBG_LOUD,
-			"in pci configuration, cap_pointer%x = %x\n",
-			cap_pointer, cap_id);
-
-		if (cap_id == 0x01) {
-			break;
-		} else {
-			/* point to next Capability */
-			cap_pointer = (cap_hdr >> 8) & 0xFF;
-			/* 0: end of pci capability, 0xff: invalid value */
-			if (cap_pointer == 0x00 || cap_pointer == 0xff) {
-				cap_id = 0xff;
-				break;
-			}
-		}
-	} while (cnt++ < 200);
-
-	if (cap_id != 0x01) {
+	pm_cap = pci_find_capability(rtlpci->pdev, PCI_CAP_ID_PM);
+	if (!pm_cap) {
 		rtl_dbg(rtlpriv, COMP_INIT, DBG_WARNING,
 			"Cannot find PME Capability\n");
 		return;
@@ -2314,22 +2283,20 @@ static void _rtl8821ae_clear_pci_pme_status(struct ieee80211_hw *hw)
 	/* Get the PM CSR (Control/Status Register),
 	 * The PME_Status is located at PM Capatibility offset 5, bit 7
 	 */
-	pci_read_config_byte(rtlpci->pdev, cap_pointer + 5, &pmcs_reg);
+	pci_read_config_byte(rtlpci->pdev, pm_cap + 5, &pmcs_reg);
 
 	if (pmcs_reg & BIT(7)) {
 		/* Clear PME_Status with write */
-		pci_write_config_byte(rtlpci->pdev, cap_pointer + 5,
-				      pmcs_reg);
+		pci_write_config_byte(rtlpci->pdev, pm_cap + 5, pmcs_reg);
 		/* Read it back to check */
-		pci_read_config_byte(rtlpci->pdev, cap_pointer + 5,
-				     &pmcs_reg);
+		pci_read_config_byte(rtlpci->pdev, pm_cap + 5, &pmcs_reg);
 		rtl_dbg(rtlpriv, COMP_INIT, DBG_DMESG,
 			"Clear PME status 0x%2x to 0x%2x\n",
-			cap_pointer + 5, pmcs_reg);
+			pm_cap + 5, pmcs_reg);
 	} else {
 		rtl_dbg(rtlpriv, COMP_INIT, DBG_DMESG,
 			"PME status(0x%2x) = 0x%2x\n",
-			cap_pointer + 5, pmcs_reg);
+			pm_cap + 5, pmcs_reg);
 	}
 }
 
-- 
2.30.2

Re: [PATCH 5/7] rtlwifi: rtl8821ae: Use pci_find_capability()

[Bjorn Helgaas]

On Fri, Nov 17, 2023 at 11:44:23AM +0200, Ilpo Järvinen wrote:
> Instead of open coding the capability structure search, find the PM
> Capability using pci_find_capability().
>
> While at it, rename the generic 'cap_pointer' to 'pm_cap' which makes
> the intent of the code more obvious.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  .../wireless/realtek/rtlwifi/rtl8821ae/hw.c   | 49 +++----------------
>  1 file changed, 8 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
> index 53cfeed0b030..7877509c34c7 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
> @@ -2270,42 +2270,11 @@ static void _rtl8821ae_clear_pci_pme_status(struct ieee80211_hw *hw)
>  {
>  	struct rtl_priv *rtlpriv = rtl_priv(hw);
>  	struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
> -	u16 cap_hdr;
> -	u8 cap_pointer;
> -	u8 cap_id = 0xff;
>  	u8 pmcs_reg;
> -	u8 cnt = 0;
> +	u8 pm_cap;
>  
> -	/* Get the Capability pointer first,
> -	 * the Capability Pointer is located at
> -	 * offset 0x34 from the Function Header */
> -
> -	pci_read_config_byte(rtlpci->pdev, 0x34, &cap_pointer);
> -	rtl_dbg(rtlpriv, COMP_INIT, DBG_LOUD,
> -		"PCI configuration 0x34 = 0x%2x\n", cap_pointer);
> -
> -	do {
> -		pci_read_config_word(rtlpci->pdev, cap_pointer, &cap_hdr);
> -		cap_id = cap_hdr & 0xFF;
> -
> -		rtl_dbg(rtlpriv, COMP_INIT, DBG_LOUD,
> -			"in pci configuration, cap_pointer%x = %x\n",
> -			cap_pointer, cap_id);
> -
> -		if (cap_id == 0x01) {
> -			break;
> -		} else {
> -			/* point to next Capability */
> -			cap_pointer = (cap_hdr >> 8) & 0xFF;
> -			/* 0: end of pci capability, 0xff: invalid value */
> -			if (cap_pointer == 0x00 || cap_pointer == 0xff) {
> -				cap_id = 0xff;
> -				break;
> -			}
> -		}
> -	} while (cnt++ < 200);
> -
> -	if (cap_id != 0x01) {
> +	pm_cap = pci_find_capability(rtlpci->pdev, PCI_CAP_ID_PM);

Hahaha, good work.

> +	if (!pm_cap) {
>  		rtl_dbg(rtlpriv, COMP_INIT, DBG_WARNING,
>  			"Cannot find PME Capability\n");
>  		return;
> @@ -2314,22 +2283,20 @@ static void _rtl8821ae_clear_pci_pme_status(struct ieee80211_hw *hw)
>  	/* Get the PM CSR (Control/Status Register),
>  	 * The PME_Status is located at PM Capatibility offset 5, bit 7

Same comment typo and PCI_PM_CTRL, PCI_PM_CTRL_PME_STATUS comments as
before.

> -	pci_read_config_byte(rtlpci->pdev, cap_pointer + 5, &pmcs_reg);
> +	pci_read_config_byte(rtlpci->pdev, pm_cap + 5, &pmcs_reg);
>  
>  	if (pmcs_reg & BIT(7)) {
>  		/* Clear PME_Status with write */
> -		pci_write_config_byte(rtlpci->pdev, cap_pointer + 5,
> -				      pmcs_reg);
> +		pci_write_config_byte(rtlpci->pdev, pm_cap + 5, pmcs_reg);
>  		/* Read it back to check */
> -		pci_read_config_byte(rtlpci->pdev, cap_pointer + 5,
> -				     &pmcs_reg);
> +		pci_read_config_byte(rtlpci->pdev, pm_cap + 5, &pmcs_reg);
>  		rtl_dbg(rtlpriv, COMP_INIT, DBG_DMESG,
>  			"Clear PME status 0x%2x to 0x%2x\n",
> -			cap_pointer + 5, pmcs_reg);
> +			pm_cap + 5, pmcs_reg);
>  	} else {
>  		rtl_dbg(rtlpriv, COMP_INIT, DBG_DMESG,
>  			"PME status(0x%2x) = 0x%2x\n",
> -			cap_pointer + 5, pmcs_reg);
> +			pm_cap + 5, pmcs_reg);
>  	}
>  }
>  
> -- 
> 2.30.2
> 

[PATCH 6/7] rtlwifi: rtl8821ae: Add pdev into _rtl8821ae_clear_pci_pme_status()

[Ilpo Järvinen]

Add local variable pdev to shorten rtlpci->pdev.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
index 7877509c34c7..7cc648d49f2d 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
@@ -2270,10 +2270,11 @@ static void _rtl8821ae_clear_pci_pme_status(struct ieee80211_hw *hw)
 {
 	struct rtl_priv *rtlpriv = rtl_priv(hw);
 	struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
+	struct pci_dev *pdev = rtlpci->pdev;
 	u8 pmcs_reg;
 	u8 pm_cap;
 
-	pm_cap = pci_find_capability(rtlpci->pdev, PCI_CAP_ID_PM);
+	pm_cap = pci_find_capability(pdev, PCI_CAP_ID_PM);
 	if (!pm_cap) {
 		rtl_dbg(rtlpriv, COMP_INIT, DBG_WARNING,
 			"Cannot find PME Capability\n");
@@ -2283,13 +2284,13 @@ static void _rtl8821ae_clear_pci_pme_status(struct ieee80211_hw *hw)
 	/* Get the PM CSR (Control/Status Register),
 	 * The PME_Status is located at PM Capatibility offset 5, bit 7
 	 */
-	pci_read_config_byte(rtlpci->pdev, pm_cap + 5, &pmcs_reg);
+	pci_read_config_byte(pdev, pm_cap + 5, &pmcs_reg);
 
 	if (pmcs_reg & BIT(7)) {
 		/* Clear PME_Status with write */
-		pci_write_config_byte(rtlpci->pdev, pm_cap + 5, pmcs_reg);
+		pci_write_config_byte(pdev, pm_cap + 5, pmcs_reg);
 		/* Read it back to check */
-		pci_read_config_byte(rtlpci->pdev, pm_cap + 5, &pmcs_reg);
+		pci_read_config_byte(pdev, pm_cap + 5, &pmcs_reg);
 		rtl_dbg(rtlpriv, COMP_INIT, DBG_DMESG,
 			"Clear PME status 0x%2x to 0x%2x\n",
 			pm_cap + 5, pmcs_reg);
-- 
2.30.2

[PATCH 7/7] rtlwifi: rtl8821ae: Access full PMCS reg and use pci_regs.h

[Ilpo Järvinen]

_rtl8821ae_clear_pci_pme_status() accesses the upper byte of the Power
Management Control/Status register (PMCS) with literal 5 offset.

Access the entire PMCS register using defines from pci_regs.h to
improve code readability.

While at it, remove the obvious comment and tweak debug prints
slightly to not sound misleading.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 .../wireless/realtek/rtlwifi/rtl8821ae/hw.c   | 21 +++++++------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
index 7cc648d49f2d..f4b232f038a9 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
@@ -2271,7 +2271,7 @@ static void _rtl8821ae_clear_pci_pme_status(struct ieee80211_hw *hw)
 	struct rtl_priv *rtlpriv = rtl_priv(hw);
 	struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
 	struct pci_dev *pdev = rtlpci->pdev;
-	u8 pmcs_reg;
+	u16 pmcs_reg;
 	u8 pm_cap;
 
 	pm_cap = pci_find_capability(pdev, PCI_CAP_ID_PM);
@@ -2281,23 +2281,16 @@ static void _rtl8821ae_clear_pci_pme_status(struct ieee80211_hw *hw)
 		return;
 	}
 
-	/* Get the PM CSR (Control/Status Register),
-	 * The PME_Status is located at PM Capatibility offset 5, bit 7
-	 */
-	pci_read_config_byte(pdev, pm_cap + 5, &pmcs_reg);
-
-	if (pmcs_reg & BIT(7)) {
+	pci_read_config_word(pdev, pm_cap + PCI_PM_CTRL, &pmcs_reg);
+	if (pmcs_reg & PCI_PM_CTRL_PME_STATUS) {
 		/* Clear PME_Status with write */
-		pci_write_config_byte(pdev, pm_cap + 5, pmcs_reg);
-		/* Read it back to check */
-		pci_read_config_byte(pdev, pm_cap + 5, &pmcs_reg);
+		pci_write_config_word(pdev, pm_cap + PCI_PM_CTRL, pmcs_reg);
+		pci_read_config_word(pdev, pm_cap + PCI_PM_CTRL, &pmcs_reg);
 		rtl_dbg(rtlpriv, COMP_INIT, DBG_DMESG,
-			"Clear PME status 0x%2x to 0x%2x\n",
-			pm_cap + 5, pmcs_reg);
+			"Cleared PME status, PMCS reg = 0x%4x\n", pmcs_reg);
 	} else {
 		rtl_dbg(rtlpriv, COMP_INIT, DBG_DMESG,
-			"PME status(0x%2x) = 0x%2x\n",
-			pm_cap + 5, pmcs_reg);
+			"PMCS reg = 0x%4x\n", pmcs_reg);
 	}
 }
 
-- 
2.30.2

Re: [PATCH 7/7] rtlwifi: rtl8821ae: Access full PMCS reg and use pci_regs.h

[Bjorn Helgaas]

On Fri, Nov 17, 2023 at 11:44:25AM +0200, Ilpo Järvinen wrote:
> _rtl8821ae_clear_pci_pme_status() accesses the upper byte of the Power
> Management Control/Status register (PMCS) with literal 5 offset.
> 
> Access the entire PMCS register using defines from pci_regs.h to
> improve code readability.
> 
> While at it, remove the obvious comment and tweak debug prints
> slightly to not sound misleading.

OK, ignore my previous comments ;)  I should read all the way through
before responding.

Re: [PATCH 7/7] rtlwifi: rtl8821ae: Access full PMCS reg and use pci_regs.h

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 771 bytes --]

On Fri, 17 Nov 2023, Bjorn Helgaas wrote:

> On Fri, Nov 17, 2023 at 11:44:25AM +0200, Ilpo Järvinen wrote:
> > _rtl8821ae_clear_pci_pme_status() accesses the upper byte of the Power
> > Management Control/Status register (PMCS) with literal 5 offset.
> > 
> > Access the entire PMCS register using defines from pci_regs.h to
> > improve code readability.
> > 
> > While at it, remove the obvious comment and tweak debug prints
> > slightly to not sound misleading.
> 
> OK, ignore my previous comments ;)  I should read all the way through
> before responding.

Please don't do that because then you'll just end up forgetting useful 
comments. :-)

I had this all in one patch initially but thought it's better to split it 
a bit.

Thanks a lot for reviewing.

-- 
 i.