* [PATCH] pcie_aspm: Make link_state_store consistent with link_state_show
@ 2015-11-19 16:05 Andy Lutomirski
2015-12-03 17:21 ` Bjorn Helgaas
0 siblings, 1 reply; 3+ messages in thread
From: Andy Lutomirski @ 2015-11-19 16:05 UTC (permalink / raw)
To: Matthew Garrett, Bjorn Helgaas; +Cc: linux-pci, Andy Lutomirski
If CONFIG_PCIEASPM_DEBUG is set, then pci devices have a link_state
attribute. Reading that attribute shows the state as a bit mask: 1
means L0S upstream, 2 means L0S downstream, and 4 means L1.
Oddly, writing to link_state is inconsistent and gets translated,
leading to mysterious results in which the value you store isn't
comparable the value you load back out.
Fix it by making link_state_store match link_state_show.
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
drivers/pci/pcie/aspm.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 317e3558a35e..e543dac8e3f2 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -834,22 +834,16 @@ static ssize_t link_state_store(struct device *dev,
{
struct pci_dev *pdev = to_pci_dev(dev);
struct pcie_link_state *link, *root = pdev->link_state->root;
- u32 val, state = 0;
+ u32 state;
- if (kstrtouint(buf, 10, &val))
+ if (kstrtouint(buf, 10, &state))
return -EINVAL;
if (aspm_disabled)
return -EPERM;
- if (n < 1 || val > 3)
+ if ((state & ~ASPM_STATE_ALL) != 0)
return -EINVAL;
- /* Convert requested state to ASPM state */
- if (val & PCIE_LINK_STATE_L0S)
- state |= ASPM_STATE_L0S;
- if (val & PCIE_LINK_STATE_L1)
- state |= ASPM_STATE_L1;
-
down_read(&pci_bus_sem);
mutex_lock(&aspm_lock);
list_for_each_entry(link, &link_list, sibling) {
--
2.5.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] pcie_aspm: Make link_state_store consistent with link_state_show
2015-11-19 16:05 [PATCH] pcie_aspm: Make link_state_store consistent with link_state_show Andy Lutomirski
@ 2015-12-03 17:21 ` Bjorn Helgaas
2015-12-03 20:35 ` Andy Lutomirski
0 siblings, 1 reply; 3+ messages in thread
From: Bjorn Helgaas @ 2015-12-03 17:21 UTC (permalink / raw)
To: Andy Lutomirski; +Cc: Matthew Garrett, Bjorn Helgaas, linux-pci
On Thu, Nov 19, 2015 at 08:05:35AM -0800, Andy Lutomirski wrote:
> If CONFIG_PCIEASPM_DEBUG is set, then pci devices have a link_state
> attribute. Reading that attribute shows the state as a bit mask: 1
> means L0S upstream, 2 means L0S downstream, and 4 means L1.
>
> Oddly, writing to link_state is inconsistent and gets translated,
> leading to mysterious results in which the value you store isn't
> comparable the value you load back out.
>
> Fix it by making link_state_store match link_state_show.
>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
Applied to pci/aspm for v4.5, thanks! You didn't touch the
"aspm_disabled" check, but I moved it above the input validation code to
make the patch look like the attached. I think it's better to not
even look at the input if aspm_disabled, and this also puts the
validation code all together.
Let me know if you object.
Bjorn
commit 57d86a0485a30145382ad03b9504cc03ba4641c7
Author: Andy Lutomirski <luto@kernel.org>
Date: Thu Nov 19 08:05:35 2015 -0800
PCI/ASPM: Make sysfs link_state_store() consistent with link_state_show()
If CONFIG_PCIEASPM_DEBUG is set, then PCI devices have a link_state
attribute. Reading that attribute shows the state as a bit mask: 1
means L0S upstream, 2 means L0S downstream, and 4 means L1.
Oddly, writing to link_state is inconsistent and gets translated, leading
to mysterious results in which the value you store isn't comparable the
value you load back out.
Fix it by making link_state_store() match link_state_show().
[bhelgaas: Check "aspm_disabled" *before* validating input. When
"aspm_disabled" is set, this changes the error for invalid input from
-EINVAL to -EPERM.]
Signed-off-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 317e355..2dfe7fd 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -834,21 +834,15 @@ static ssize_t link_state_store(struct device *dev,
{
struct pci_dev *pdev = to_pci_dev(dev);
struct pcie_link_state *link, *root = pdev->link_state->root;
- u32 val, state = 0;
-
- if (kstrtouint(buf, 10, &val))
- return -EINVAL;
+ u32 state;
if (aspm_disabled)
return -EPERM;
- if (n < 1 || val > 3)
- return -EINVAL;
- /* Convert requested state to ASPM state */
- if (val & PCIE_LINK_STATE_L0S)
- state |= ASPM_STATE_L0S;
- if (val & PCIE_LINK_STATE_L1)
- state |= ASPM_STATE_L1;
+ if (kstrtouint(buf, 10, &state))
+ return -EINVAL;
+ if ((state & ~ASPM_STATE_ALL) != 0)
+ return -EINVAL;
down_read(&pci_bus_sem);
mutex_lock(&aspm_lock);
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] pcie_aspm: Make link_state_store consistent with link_state_show
2015-12-03 17:21 ` Bjorn Helgaas
@ 2015-12-03 20:35 ` Andy Lutomirski
0 siblings, 0 replies; 3+ messages in thread
From: Andy Lutomirski @ 2015-12-03 20:35 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Bjorn Helgaas, linux-pci, Matthew Garrett
On Dec 3, 2015 9:21 AM, "Bjorn Helgaas" <helgaas@kernel.org> wrote:
>
> On Thu, Nov 19, 2015 at 08:05:35AM -0800, Andy Lutomirski wrote:
> > If CONFIG_PCIEASPM_DEBUG is set, then pci devices have a link_state
> > attribute. Reading that attribute shows the state as a bit mask: 1
> > means L0S upstream, 2 means L0S downstream, and 4 means L1.
> >
> > Oddly, writing to link_state is inconsistent and gets translated,
> > leading to mysterious results in which the value you store isn't
> > comparable the value you load back out.
> >
> > Fix it by making link_state_store match link_state_show.
> >
> > Signed-off-by: Andy Lutomirski <luto@kernel.org>
>
> Applied to pci/aspm for v4.5, thanks! You didn't touch the
> "aspm_disabled" check, but I moved it above the input validation code to
> make the patch look like the attached. I think it's better to not
> even look at the input if aspm_disabled, and this also puts the
> validation code all together.
Looks good to me.
--Andy
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-12-03 20:35 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-19 16:05 [PATCH] pcie_aspm: Make link_state_store consistent with link_state_show Andy Lutomirski
2015-12-03 17:21 ` Bjorn Helgaas
2015-12-03 20:35 ` Andy Lutomirski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.