Re: [PATCH V2] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[okaya]

On 2017-03-03 04:43, Patel, Mayurkumar wrote:
> Hi Kaya
>
> > Hi Mayurkumar
> >
> > On 3/2/2017 11:05 AM, Patel, Mayurkumar wrote:
> > > > Hi Bjorn,
> > > >
> > > > On 2/28/2017 1:57 PM, Patel, Mayurkumar wrote:
> > > > > > I was trying to figure out when to use saved values vs. the values 
> > > > > > in
> > > > > > registers by looking at the enable_cnt.
> > > > > > enable_cnt is 0 during boot on my system.
> > > > > enable_cnt for the root port on my system is set to 1 for "root 
> > > > > port" already without saving
> > > > > any ASPM related settings.
> > > >
> > > > What would be the best way to figure out when to save power-on 
> > > > values from
> > > > the registers?
> > >
> > > I can upload the diffs) because enable_cnt in pci_enable_device() can 
> > > be triggered
> > > from multiple places at boot time so it might not be safe to use it?
> >
> > Go ahead and share your diff. It doesn't hurt to look at other 
> > alternatives.
>
> So basically, I introduced a new atomic variable to save the
> aspm_policy for the first time.
> Below is my diff.

Thanks.

Ok, i will incorporate your change and add your signoff, then repost the 
next version after testing next week. I am OoO until next week.


> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 3dd8bcb..c8e1e3a 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -338,8 +338,9 @@ static void pcie_aspm_check_latency(struct pci_dev
> *endpoint)
>         }
>  }
>
> -static void pcie_aspm_cap_init(struct pcie_link_state *link, int 
> blacklist)
> +static void pcie_aspm_cap_init(struct pci_dev *pdev, int blacklist)
>  {
> +       struct pcie_link_state *link = pdev->link_state;
>         struct pci_dev *child, *parent = link->pdev;
>         struct pci_bus *linkbus = parent->subordinate;
>         struct aspm_register_info upreg, dwreg;
> @@ -397,8 +398,21 @@ static void pcie_aspm_cap_init(struct
> pcie_link_state *link, int blacklist)
>         link->latency_up.l1 = 
> calc_l1_latency(upreg.latency_encoding_l1);
>         link->latency_dw.l1 = 
> calc_l1_latency(dwreg.latency_encoding_l1);
>
> -       /* Save default state */
> -       link->aspm_default = link->aspm_enabled;
> +       /*
> +        * Save default state from FW when enabling ASPM for the first 
> time
> +        * during boot by looking at the calculated link->aspm_enabled 
> bits
> +        * above and aspm_enable_cnt will be zero.
> +        *
> +        * If this path is getting called for the second/third time
> +        * (aspm_enable_cnt will be non-zero). Assume that the current 
> state
> +        * of the ASPM registers may not necessarily match what FW 
> asked us to
> +        * do as in the case of hotplug insertion/removal.
> +        */
> +       if (atomic_inc_return(&pdev->aspm_enable_cnt) == 1)
> +               pdev->aspm_default = link->aspm_default = 
> link->aspm_enabled;
> +       else
> +               link->aspm_default = pdev->aspm_default;
> +
>
>         /* Setup initial capable state. Will be updated later */
>         link->aspm_capable = link->aspm_support;
> @@ -606,7 +620,7 @@ void pcie_aspm_init_link_state(struct pci_dev 
> *pdev)
>          * upstream links also because capable state of them can be
>          * update through pcie_aspm_cap_init().
>          */
> -       pcie_aspm_cap_init(link, blacklist);
> +      pcie_aspm_cap_init(pdev, blacklist);
>
>         /* Setup initial Clock PM state */
>         pcie_clkpm_cap_init(link, blacklist);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index e2d1a12..aa7bd7e 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -321,6 +321,8 @@ struct pci_dev {
>
>  #ifdef CONFIG_PCIEASPM
>          struct pcie_link_state  *link_state;    /* ASPM link state */
> +       unsigned int    aspm_default;           /* ASPM policy set by 
> BIOS */
> +       atomic_t        aspm_enable_cnt;        /* ASPM policy 
> initialization */