ACK: [vivid][PATCH] UBUNTU: [Config] CONFIG_PCIEASPM_DEBUG=y

[Colin Ian King]

On 08/12/14 08:52, Andy Whitcroft wrote:
> On Sun, Dec 07, 2014 at 11:18:28PM +0000, Colin Ian King wrote:
>> On 05/12/14 19:51, Chris J Arges wrote:
>>> Here are the device attributes that get added and their permissions:
>>>
>>> static DEVICE_ATTR(link_state, 0644, link_state_show, link_state_store);
>>> static DEVICE_ATTR(clk_ctl, 0644, clk_ctl_show, clk_ctl_store);
>>>
>>> These are only added if link_state->{aspm_support,clkpm_capable} are
>>> true respectively.
>>>
>>> So this does allow for storing those variables. And looking through the
>>> kernel I don't see many examples of this pattern; in addition the
>>> parameter checking seems a bit weak:
>>>
>>> static ssize_t link_state_store(struct device *dev,
>>>                 struct device_attribute *attr,
>>>                 const char *buf,
>>>                 size_t n)
>>> {
>>>         struct pci_dev *pdev = to_pci_dev(dev);
>>>         struct pcie_link_state *link, *root = pdev->link_state->root;
>>>         u32 val = buf[0] - '0', state = 0;
> 
> Ok, I've checked that buf cannot be NULL, and indeed count cannot be 0
> ...
> 
>>>         if (aspm_disabled)
>>>                 return -EPERM;
>>>         if (n < 1 || val > 3)
>>>                 return -EINVAL;
> 
> This here actually checks count (n), and the converted value is between
> 0 and 3, which are the valid values.
> 
>>> static ssize_t clk_ctl_store(struct device *dev,
>>>                 struct device_attribute *attr,
>>>                 const char *buf,
>>>                 size_t n)
>>> {
>>>         struct pci_dev *pdev = to_pci_dev(dev);
>>>         int state;
>>>
>>>         if (n < 1)
>>>                 return -EINVAL;
>>>         state = buf[0]-'0';
>>> ...
>>>
> 
> Here the state is actually used as below:
> 
>         pcie_set_clkpm_nocheck(pdev->link_state, !!state);
> 
> ie, it is either true (!0) or false (0), and is "validated" in some sense.
> 
> I have also confirmed those are the only two new interfaces.  So I think
> we should be safe here.
> 
> -apw
> 
Thanks apw for checking that.

I which case I think this is useful for those who really want to tweak
their settings to justify this config change.

Acked-by: Colin Ian King <colin.king at canonical.com>