On 03.10.2019 00:23, Heiner Kallweit wrote:
> On 03.10.2019 00:10, Bjorn Helgaas wrote:
>> On Wed, Oct 02, 2019 at 11:10:55PM +0200, Heiner Kallweit wrote:
>>> On 02.10.2019 21:55, Bjorn Helgaas wrote:
>>>> On Sun, Sep 29, 2019 at 07:15:05PM +0200, Heiner Kallweit wrote:
>>>>> On 07.09.2019 22:32, Bjorn Helgaas wrote:
>>>>>> On Sat, Aug 31, 2019 at 10:20:47PM +0200, Heiner Kallweit wrote:
>>
>>>>>>> +static struct pcie_link_state *aspm_get_parent_link(struct pci_dev *pdev)
>>>>>>
>>>>>> I know the ASPM code is pretty confused, but I don't think "parent
>>>>>> link" really makes sense. "Parent" implies a parent/child
>>>>>> relationship, but a link doesn't have a parent or a child; it only has
>>>>>> an upstream end and a downstream end.
>>>>>>
>>>>> I basically copied this "parent" stuff from __pci_disable_link_state.
>>>>> Fine with me to change the naming.
>>>>> What confuses me a little is that we have different versions of getting
>>>>> the pcie_link_state for a pci_dev in:
>>>>>
>>>>> - this new function of mine
>>>>> - __pci_disable_link_state
>>>>> - pcie_aspm_enabled
>>>>>
>>>>> The latter uses pci_upstream_bridge instead of accessing pdev->bus->self
>>>>> directly and doesn't include the call to pcie_downstream_port.
>>>>> I wonder whether the functionality could be factored out to a generic
>>>>> helper that works in all these places.
>>>>
>>>> Definitely. I think your pcie_aspm_get_link() (from the v6 patch)
>>>> could be used directly in those places. You could add a new patch
>>>> that just adds pcie_aspm_get_link() and uses it.
>>>>
>>>
>>> OK
>>>
>>>>>>> +{
>>>>>>> + struct pci_dev *parent = pdev->bus->self;
>>>>>>> +
>>>>>>> + if (pcie_downstream_port(pdev))
>>>>>>> + parent = pdev;
>>>>>>> +
>>>>>>> + return parent ? parent->link_state : NULL;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static bool pcie_check_valid_aspm_endpoint(struct pci_dev *pdev)
>>>>>>> +{
>>>>>>> + struct pcie_link_state *link;
>>>>>>> +
>>>>>>> + if (!pci_is_pcie(pdev) || pci_pcie_type(pdev) != PCI_EXP_TYPE_ENDPOINT)
>>>>>>
>>>>>> Do you intend to exclude other Upstream Ports like Legacy Endpoints,
>>>>>> Upstream Switch Ports, and PCIe-to-PCI/PCI-X Bridges? They also have
>>>>>> a link leading to them, so we might want them to have knobs as well.
>>>>>> Or if we don't want the knobs, a comment about why not would be
>>>>>> useful.
>>>>>>
>>>>> My use case is about endpoints only and I'm not really a PCI expert.
>>>>> Based on your list in addition to PCI_EXP_TYPE_ENDPOINT we'd enable
>>>>> the ASPM sysfs fils for:
>>>>> - PCI_EXP_TYPE_LEG_END
>>>>> - PCI_EXP_TYPE_UPSTREAM
>>>>> - PCI_EXP_TYPE_PCI_BRIDGE
>>>>> - PCI_EXP_TYPE_PCIE_BRIDGE
>>>>> If you can confirm the list I'd extend my patch accordingly.
>>>>
>>>> Yes, I think the list would be right, but looking at this again, I
>>>> don't think you need this function at all -- you can just use
>>>> pcie_aspm_get_link(). Then aspm_ctrl_attrs_are_visible() uses exactly
>>>> the same test as the show/store functions. Actually, I think then you
>>>> could omit the "if (!link)" tests from the show/store functions
>>>> because those functions can never be called unless
>>>> aspm_ctrl_attrs_are_visible() found a link.
>>>>
>>> Right, the !link checks can be removed from the show/store functions.
>>> In pcie_is_aspm_dev() I think we need to check at least whether
>>> device is PCIe and whether link is ASPM-capable. Making the sysfs
>>> attributes visible for a non-PCIe device doesn't make sense,
>>> the same applies to PCIe devices with a link that is not ASPM-capable.
>>
>> I agree we don't want these attributes visible for non-PCIe or
>> non-ASPM-capable situations, but I think you can do this:
>>
>> static struct pcie_link_state *pcie_aspm_get_link(struct pci_dev *pdev)
>> {
>> struct pci_dev *bridge = pci_upstream_bridge(pdev);
>>
>> if (bridge)
>> return bridge->link_state;
>>
>> return NULL;
>> }
>>
>> static umode_t aspm_ctrl_attrs_are_visible(...)
>> {
>> ...
>> if (pcie_aspm_get_link(pdev))
>> return a->mode;
>>
>> return 0;
>> }
>>
>> We can rely on pcie_aspm_init_link_state() to only set
>> bridge->link_state if the devices on both ends of the link are PCIe
>> and support ASPM.
>>
> With the first one I agree. However there may be links where e.g. the
> bridge doesn't support ASPM. One example is my small Zotac test system:
>
> Intel Corporation Celeron N3350/Pentium N4200/Atom E3900 Series PCI Express Port
> LnkCap: Port #3, Speed 5GT/s, Width x1, ASPM not supported
>
After thinking once more about it:
pcie_aspm_get_link() looks like this in my series and w/o a prior call
to pci_is_pcie() we may call pcie_downstream_port() for a non-PCIe
device what results in a fake PCI_EXP_TYPE_ENDPOINT result.
I don't want to rely on side effects and therefore would like to
keep the call to pci_is_pcie(). I'll submit a v7 and we can continue
to discuss based on that.
static struct pcie_link_state *pcie_aspm_get_link(struct pci_dev *pdev)
{
struct pci_dev *upstream;
if (pcie_downstream_port(pdev))
upstream = pdev;
else
upstream = pci_upstream_bridge(pdev);
return upstream ? upstream->link_state : NULL;
}
