On Mon, Jun 29, 2009 at 04:49:10PM +0800, Kenji Kaneshige wrote: > Shaohua Li wrote: > > On Fri, Jun 26, 2009 at 10:59:20AM +0800, Kenji Kaneshige wrote: > >> Shaohua Li wrote: > >>> On Fri, Jun 19, 2009 at 01:28:44PM +0800, Kenji Kaneshige wrote: > >>>> Shaohua Li wrote: > >>>>> On Fri, Jun 19, 2009 at 11:19:49AM +0800, Kenji Kaneshige wrote: > >>>>>> Shaohua Li wrote: > >>>>>>> On Fri, Jun 19, 2009 at 05:28:41AM +0800, Jesse Barnes wrote: > >>>>>>>> On Thu, 21 May 2009 11:10:06 +0900 > >>>>>>>> Kenji Kaneshige <kaneshige.kenji@xxxxxxxxxxxxxx> wrote: > >>>>>>>> > >>>>>>>>> Hi, > >>>>>>>>> > >>>>>>>>> This patch changes ASPM driver to enable more detailed ASPM > >>>>>>>>> configuration. With this patch, > >>>>>>>>> > >>>>>>>>> - ASPM can be enabled partially in the hierarchy. > >>>>>>>>> - L0s can be managed for each direction (upstream direction and > >>>>>>>>> downstream direction) of the link. > >>>>>>> Well, these are nice features, but what kind of machine does you test? > >>>>>>> Does it have several level hierarchy? The most powerful system I saw > >>>>>>> just has two levels, which sounds not worth adding partial aspm. > >>>>>> Thank you very much for comments. > >>>>>> > >>>>>> I think it is useful also for the system that doesn't have so > >>>>>> many levels of hierarchy, such as mobile systems. Here is "lspci -t" > >>>>>> output on my environment with some comments. My system also has > >>>>>> just two levels, but with the patch, upstream L0s is enabled on the > >>>>>> link 0000:15.00.0 (switch downstream port) to 0000:16:00.0 (endpoint) > >>>>>> in powersave mode. > >>>>> Can you send me the output of lspci -vvvxxx please? > >>>>> > >>>> Here it is. Please note that I'm using pcie_aspm=force option. > >>> Hi, > >>> can you please split the patch into two patches, then I will have more detail > >>> review/test. Also please don't do cleanup in the patches for new feature. > >>> I had a simple look at the patch, and found at least one issue: > >>> A link might have multiple child links, we need check all child links's latency to > >>> enable the parent's ASPM. In pcie_config_aspm_path(), the patch seems not check > >>> peer link. > >>> > >> My understanding is that we can enable parent ASPM even if one > >> or more child links don't meet the latency requirement and ASPM > >> is disabled on them. In this case, parent link will never enter > >> the upstream direction L0s nor L1 state because parent link's > >> downstream component (switch upstream port) never becomes idle. > > I thought we need to check other links, considering below case: > > Say we have three links, parent/child1/child2. When you do partial aspm > > enabling, endpoints in child1 have latency requirement and the result is child1 > > is enabled and parent is disabled. The we check child2, and latency requirement > > meets and then you enable child2 and parent. This will makes endpoints in link1 > > hasn't required latency and breaks them. > > > > In this case, my patch doesn't enable ASPM on parent the link. > > My patch is doing as follows: > > 1) Detect parent link and initialize 'aspm_support'(*1) and > 'aspm_capable'(*1). At this point 'aspm_capable' is equal to > 'aspm_support'(*2) because no endpoints are detected at this > point. And enable ASPM based on 'aspm_capable' on parent. > > The states are as follows at this point (note that all ASPM > states are mixed up in this example). > > parent: Support+, Capable+, Enabled+ > child1: N/A , N/A , N/A > child2: N/A , N/A , N/A > > (*1): Each bits in 'aspm_capable' is associated to upstream and > downstream direction L0s and L1. The bit is set if the link > supports associated ASPM state and meets latency requirement. > > (*2): Each bits in 'aspm_support' is associated to upstream and > downstream direction L0s and L1. The bit is set if the link > supports associated ASPM state. > > > 2) Detect child1 link and initialize 'aspm_support' and 'aspm_capable'. > First, 'aspm_capable' is initialized by 'aspm_support' and then > updated based on endpoint's acceptable latency. The parent's > 'aspm_capable' is also updated here. In this update, bits can be > unset, but never be set. In your example, 'aspm_capable' > is unset because it doesn't meets latency requirement. And then > configure ASPM based on 'aspm_capable' on child1 and parent. > > The states are as follows at this point. > > parent: Support+, Capable-, Enabled- > child1: Support+, Capable+, Enabled+ > child2: N/A , N/A , N/A > > 3) Detect child2 link and initialize 'aspm_support' and 'aspm_capable'. > First, 'aspm_capable' is initialized by 'aspm_support' and then > updated based on endpoint's acceptable latency. The parent's > 'aspm_capable' is also updated here, but bits are never set in this > update. In your example, parent meets latency requirement for > endpoint under child2 link, but 'aspm_capable' of parent is never > changed. And then configure ASPM based on 'aspm_capable' on child2 > and parent. > > The states are as follows at the end. > > parent: Support+, Capable-, Enabled- > child1: Support+, Capable+, Enabled+ > child2: Support+, Capable+, Enabled+ Ok, you are right, I missed you set capable -. Thanks, Shaohua -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
