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.
