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

  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(-)
  

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
>
  

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.
  

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