With more and more SOCs having pci device integrated into chip (e.g. Intel
Atom series), it's useful to add an interface to cleanly hide pci devices from
pci device scanning, which is because:
1. phone or tablet OEMs may choose disabling some pci device in the SOC,
such as camera ISP in Intel Atom Z2580 chip, and etc.
2. if such disabled devices are not cleanly removed from pci device tree,
then pci-core will still try to operate on the relative device control
registers while S3 suspend and resume.
3. so hiding such devices from early begining will not only reduce the kernel
boot time, but also optimize the latency of system suspend and resume.
To hide pci devices, just pass such parameters to kernel at boot stage:
pci=hide=[<domain>:]<bus>:<slot>.<func>[; ...]
Cc: Bjorn Helgaas <[email protected]>
Cc: [email protected]
Signed-off-by: Haicheng Li <[email protected]>
---
drivers/pci/pci.c | 2 ++
drivers/pci/pci.h | 2 ++
drivers/pci/probe.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 98 insertions(+)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a899d8b..e228d00 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3915,6 +3915,8 @@ static int __init pci_setup(char *str)
pcie_bus_config = PCIE_BUS_PEER2PEER;
} else if (!strncmp(str, "pcie_scan_all", 13)) {
pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
+ } else if (!strncmp(str, "hide=", 5)) {
+ pci_hide_devices(str + 5, strlen(str + 5));
} else {
printk(KERN_ERR "PCI: Unknown option `%s'\n",
str);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index d1182c4..783703a 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -317,4 +317,6 @@ static inline int pci_dev_specific_reset(struct pci_dev *dev, int probe)
}
#endif
+void pci_hide_devices(const char *, size_t);
+
#endif /* DRIVERS_PCI_H */
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 70f10fa..4223ef3 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -11,6 +11,7 @@
#include <linux/cpumask.h>
#include <linux/pci-aspm.h>
#include <asm-generic/pci-bridge.h>
+#include <asm/setup.h>
#include "pci.h"
#define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */
@@ -1352,10 +1353,103 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
pci_proc_attach_device(dev);
}
+static LIST_HEAD(pci_hidden_dev_list);
+struct pci_hidden_dev {
+ struct list_head list;
+ int domain_nr;
+ int devfn;
+ unsigned char bus_nr;
+};
+
+static void do_pci_hide_device(int domain_nr, unsigned char bus_nr, int devfn)
+{
+ struct pci_hidden_dev *d;
+
+ list_for_each_entry(d, &pci_hidden_dev_list, list)
+ if (devfn == d->devfn && bus_nr == d->bus_nr &&
+ domain_nr == d->domain_nr)
+ return;
+
+ d = kzalloc(sizeof(*d), GFP_KERNEL);
+ if (!d)
+ return;
+
+ d->domain_nr = domain_nr;
+ d->bus_nr = bus_nr;
+ d->devfn = devfn;
+
+ list_add_tail(&d->list, &pci_hidden_dev_list);
+}
+
+static bool init_hidden = true;
+
+#define PCI_HIDE_PARAM_SIZE COMMAND_LINE_SIZE
+static char pci_hide_param[PCI_HIDE_PARAM_SIZE] = {'\0'};
+
+static void parse_hidden_devices(void)
+{
+ char *p = pci_hide_param;
+ int seg, bus, slot, func, devfn, count;
+
+ /* parse pci.hide= command-line */
+ while (*p != '\0') {
+ count = 0;
+ if (sscanf(p, "%x:%x:%x.%x%n",
+ &seg, &bus, &slot, &func, &count) != 4) {
+ seg = 0;
+ if (sscanf(p, "%x:%x.%x%n",
+ &bus, &slot, &func, &count) != 3) {
+ /* Invalid format */
+ printk(KERN_ERR "PCI: Can't parse "
+ "hide parameter: %s\n", p);
+ break;
+ }
+ }
+ p += count;
+ devfn = (slot << 3) | func;
+
+ do_pci_hide_device(seg, bus, devfn);
+ if (*p != ';') {
+ /* End of param or invalid format */
+ break;
+ }
+ p++;
+ }
+ init_hidden = false;
+}
+
+static bool is_device_hidden(struct pci_bus *bus, int devfn)
+{
+ struct pci_hidden_dev *d;
+
+ if (unlikely(init_hidden))
+ parse_hidden_devices();
+
+ list_for_each_entry(d, &pci_hidden_dev_list, list)
+ if (d->devfn == devfn && d->bus_nr == bus->number &&
+ d->domain_nr == pci_domain_nr(bus))
+ return true;
+
+ return false;
+}
+
+void pci_hide_devices(const char *str, size_t count)
+{
+ if (!str)
+ return;
+ if (count > PCI_HIDE_PARAM_SIZE - 1)
+ count = PCI_HIDE_PARAM_SIZE - 1;
+ strncpy(pci_hide_param, str, count);
+ pci_hide_param[count] = '\0';
+}
+
struct pci_dev *__ref pci_scan_single_device(struct pci_bus *bus, int devfn)
{
struct pci_dev *dev;
+ if (unlikely(is_device_hidden(bus, devfn)))
+ return NULL;
+
dev = pci_get_slot(bus, devfn);
if (dev) {
pci_dev_put(dev);
--
1.7.9.5
Cc: Rob Landley <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Haicheng Li <[email protected]>
---
Documentation/kernel-parameters.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 2fe6e76..4da0834 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -2359,6 +2359,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
pcie_scan_all Scan all possible PCIe devices. Otherwise we
only look for one device below a PCIe downstream
port.
+ hide= Format: [<domain>:]<bus>:<slot>.<func>[; ...]
+ Specifies devices to hide from pci subsystem.
pcie_aspm= [PCIE] Forcibly enable or disable PCIe Active State Power
Management.
--
1.7.9.5
Cc: Len Brown <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: [email protected]
Signed-off-by: Haicheng Li <[email protected]>
---
drivers/acpi/scan.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 27da630..fcc0cdc 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -29,7 +29,6 @@ extern struct acpi_device *acpi_root;
static const char *dummy_hid = "device";
-static LIST_HEAD(acpi_device_list);
static LIST_HEAD(acpi_bus_id_list);
static DEFINE_MUTEX(acpi_scan_lock);
static LIST_HEAD(acpi_scan_handlers_list);
--
1.7.9.5
On Wed, Jul 3, 2013 at 9:16 AM, Haicheng Li <[email protected]> wrote:
> Cc: Rob Landley <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Haicheng Li <[email protected]>
> ---
> Documentation/kernel-parameters.txt | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 2fe6e76..4da0834 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -2359,6 +2359,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> pcie_scan_all Scan all possible PCIe devices. Otherwise we
> only look for one device below a PCIe downstream
> port.
> + hide= Format: [<domain>:]<bus>:<slot>.<func>[; ...]
> + Specifies devices to hide from pci subsystem.
>
> pcie_aspm= [PCIE] Forcibly enable or disable PCIe Active State Power
> Management.
The documentation update should be in the same patch as the
functionality it adds.
On Wed, Jul 3, 2013 at 9:16 AM, Haicheng Li <[email protected]> wrote:
> With more and more SOCs having pci device integrated into chip (e.g. Intel
> Atom series), it's useful to add an interface to cleanly hide pci devices from
> pci device scanning, which is because:
>
> 1. phone or tablet OEMs may choose disabling some pci device in the SOC,
> such as camera ISP in Intel Atom Z2580 chip, and etc.
> 2. if such disabled devices are not cleanly removed from pci device tree,
> then pci-core will still try to operate on the relative device control
> registers while S3 suspend and resume.
> 3. so hiding such devices from early begining will not only reduce the kernel
> boot time, but also optimize the latency of system suspend and resume.
Normally the chip provides a way to disable devices by writing a
configuration register. Then the device doesn't respond when Linux
enumerates devices, so nothing special is required in the kernel.
What's different about the Z2580? I'd be surprised if Intel forgot to
include such a register. Maybe the firmware just isn't smart enough
to disable the device? If so, it would be better to fix the firmware
than to add kludges in the kernel.
> To hide pci devices, just pass such parameters to kernel at boot stage:
> pci=hide=[<domain>:]<bus>:<slot>.<func>[; ...]
>
> Cc: Bjorn Helgaas <[email protected]>
> Cc: [email protected]
> Signed-off-by: Haicheng Li <[email protected]>
> ---
> drivers/pci/pci.c | 2 ++
> drivers/pci/pci.h | 2 ++
> drivers/pci/probe.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 98 insertions(+)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index a899d8b..e228d00 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3915,6 +3915,8 @@ static int __init pci_setup(char *str)
> pcie_bus_config = PCIE_BUS_PEER2PEER;
> } else if (!strncmp(str, "pcie_scan_all", 13)) {
> pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
> + } else if (!strncmp(str, "hide=", 5)) {
> + pci_hide_devices(str + 5, strlen(str + 5));
> } else {
> printk(KERN_ERR "PCI: Unknown option `%s'\n",
> str);
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index d1182c4..783703a 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -317,4 +317,6 @@ static inline int pci_dev_specific_reset(struct pci_dev *dev, int probe)
> }
> #endif
>
> +void pci_hide_devices(const char *, size_t);
> +
> #endif /* DRIVERS_PCI_H */
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 70f10fa..4223ef3 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -11,6 +11,7 @@
> #include <linux/cpumask.h>
> #include <linux/pci-aspm.h>
> #include <asm-generic/pci-bridge.h>
> +#include <asm/setup.h>
> #include "pci.h"
>
> #define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */
> @@ -1352,10 +1353,103 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
> pci_proc_attach_device(dev);
> }
>
> +static LIST_HEAD(pci_hidden_dev_list);
> +struct pci_hidden_dev {
> + struct list_head list;
> + int domain_nr;
> + int devfn;
> + unsigned char bus_nr;
> +};
> +
> +static void do_pci_hide_device(int domain_nr, unsigned char bus_nr, int devfn)
> +{
> + struct pci_hidden_dev *d;
> +
> + list_for_each_entry(d, &pci_hidden_dev_list, list)
> + if (devfn == d->devfn && bus_nr == d->bus_nr &&
> + domain_nr == d->domain_nr)
> + return;
> +
> + d = kzalloc(sizeof(*d), GFP_KERNEL);
> + if (!d)
> + return;
> +
> + d->domain_nr = domain_nr;
> + d->bus_nr = bus_nr;
> + d->devfn = devfn;
> +
> + list_add_tail(&d->list, &pci_hidden_dev_list);
> +}
> +
> +static bool init_hidden = true;
> +
> +#define PCI_HIDE_PARAM_SIZE COMMAND_LINE_SIZE
> +static char pci_hide_param[PCI_HIDE_PARAM_SIZE] = {'\0'};
> +
> +static void parse_hidden_devices(void)
> +{
> + char *p = pci_hide_param;
> + int seg, bus, slot, func, devfn, count;
> +
> + /* parse pci.hide= command-line */
> + while (*p != '\0') {
> + count = 0;
> + if (sscanf(p, "%x:%x:%x.%x%n",
> + &seg, &bus, &slot, &func, &count) != 4) {
> + seg = 0;
> + if (sscanf(p, "%x:%x.%x%n",
> + &bus, &slot, &func, &count) != 3) {
> + /* Invalid format */
> + printk(KERN_ERR "PCI: Can't parse "
> + "hide parameter: %s\n", p);
> + break;
> + }
> + }
> + p += count;
> + devfn = (slot << 3) | func;
> +
> + do_pci_hide_device(seg, bus, devfn);
> + if (*p != ';') {
> + /* End of param or invalid format */
> + break;
> + }
> + p++;
> + }
> + init_hidden = false;
> +}
> +
> +static bool is_device_hidden(struct pci_bus *bus, int devfn)
> +{
> + struct pci_hidden_dev *d;
> +
> + if (unlikely(init_hidden))
> + parse_hidden_devices();
> +
> + list_for_each_entry(d, &pci_hidden_dev_list, list)
> + if (d->devfn == devfn && d->bus_nr == bus->number &&
> + d->domain_nr == pci_domain_nr(bus))
> + return true;
> +
> + return false;
> +}
> +
> +void pci_hide_devices(const char *str, size_t count)
> +{
> + if (!str)
> + return;
> + if (count > PCI_HIDE_PARAM_SIZE - 1)
> + count = PCI_HIDE_PARAM_SIZE - 1;
> + strncpy(pci_hide_param, str, count);
> + pci_hide_param[count] = '\0';
> +}
> +
> struct pci_dev *__ref pci_scan_single_device(struct pci_bus *bus, int devfn)
> {
> struct pci_dev *dev;
>
> + if (unlikely(is_device_hidden(bus, devfn)))
> + return NULL;
> +
> dev = pci_get_slot(bus, devfn);
> if (dev) {
> pci_dev_put(dev);
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/04/2013 12:09 AM, Bjorn Helgaas wrote:
> On Wed, Jul 3, 2013 at 9:16 AM, Haicheng Li <[email protected]> wrote:
>> With more and more SOCs having pci device integrated into chip (e.g. Intel
>> Atom series), it's useful to add an interface to cleanly hide pci devices from
>> pci device scanning, which is because:
>>
>> 1. phone or tablet OEMs may choose disabling some pci device in the SOC,
>> such as camera ISP in Intel Atom Z2580 chip, and etc.
>> 2. if such disabled devices are not cleanly removed from pci device tree,
>> then pci-core will still try to operate on the relative device control
>> registers while S3 suspend and resume.
>> 3. so hiding such devices from early begining will not only reduce the kernel
>> boot time, but also optimize the latency of system suspend and resume.
>
> Normally the chip provides a way to disable devices by writing a
> configuration register. Then the device doesn't respond when Linux
> enumerates devices, so nothing special is required in the kernel.
> What's different about the Z2580? I'd be surprised if Intel forgot to
> include such a register. Maybe the firmware just isn't smart enough
> to disable the device? If so, it would be better to fix the firmware
> than to add kludges in the kernel.
Agree, it would be great if chipset and firmware could cooperate to
handle this issue. Otherwise the interface may be abused and causes
trouble to PCI hotplug operations because the notation seg:bus:dev.func
isn't reliable. The PCI bus number may reassigned by OS.
>
>> To hide pci devices, just pass such parameters to kernel at boot stage:
>> pci=hide=[<domain>:]<bus>:<slot>.<func>[; ...]
>>
>> Cc: Bjorn Helgaas <[email protected]>
>> Cc: [email protected]
>> Signed-off-by: Haicheng Li <[email protected]>
>> ---
>> drivers/pci/pci.c | 2 ++
>> drivers/pci/pci.h | 2 ++
>> drivers/pci/probe.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 98 insertions(+)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index a899d8b..e228d00 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -3915,6 +3915,8 @@ static int __init pci_setup(char *str)
>> pcie_bus_config = PCIE_BUS_PEER2PEER;
>> } else if (!strncmp(str, "pcie_scan_all", 13)) {
>> pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
>> + } else if (!strncmp(str, "hide=", 5)) {
>> + pci_hide_devices(str + 5, strlen(str + 5));
>> } else {
>> printk(KERN_ERR "PCI: Unknown option `%s'\n",
>> str);
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index d1182c4..783703a 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -317,4 +317,6 @@ static inline int pci_dev_specific_reset(struct pci_dev *dev, int probe)
>> }
>> #endif
>>
>> +void pci_hide_devices(const char *, size_t);
>> +
>> #endif /* DRIVERS_PCI_H */
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index 70f10fa..4223ef3 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -11,6 +11,7 @@
>> #include <linux/cpumask.h>
>> #include <linux/pci-aspm.h>
>> #include <asm-generic/pci-bridge.h>
>> +#include <asm/setup.h>
>> #include "pci.h"
>>
>> #define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */
>> @@ -1352,10 +1353,103 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
>> pci_proc_attach_device(dev);
>> }
>>
>> +static LIST_HEAD(pci_hidden_dev_list);
>> +struct pci_hidden_dev {
>> + struct list_head list;
>> + int domain_nr;
>> + int devfn;
>> + unsigned char bus_nr;
>> +};
>> +
>> +static void do_pci_hide_device(int domain_nr, unsigned char bus_nr, int devfn)
>> +{
>> + struct pci_hidden_dev *d;
>> +
>> + list_for_each_entry(d, &pci_hidden_dev_list, list)
>> + if (devfn == d->devfn && bus_nr == d->bus_nr &&
>> + domain_nr == d->domain_nr)
>> + return;
>> +
>> + d = kzalloc(sizeof(*d), GFP_KERNEL);
>> + if (!d)
>> + return;
>> +
>> + d->domain_nr = domain_nr;
>> + d->bus_nr = bus_nr;
>> + d->devfn = devfn;
>> +
>> + list_add_tail(&d->list, &pci_hidden_dev_list);
>> +}
>> +
>> +static bool init_hidden = true;
>> +
>> +#define PCI_HIDE_PARAM_SIZE COMMAND_LINE_SIZE
>> +static char pci_hide_param[PCI_HIDE_PARAM_SIZE] = {'\0'};
>> +
>> +static void parse_hidden_devices(void)
>> +{
>> + char *p = pci_hide_param;
>> + int seg, bus, slot, func, devfn, count;
>> +
>> + /* parse pci.hide= command-line */
>> + while (*p != '\0') {
>> + count = 0;
>> + if (sscanf(p, "%x:%x:%x.%x%n",
>> + &seg, &bus, &slot, &func, &count) != 4) {
>> + seg = 0;
>> + if (sscanf(p, "%x:%x.%x%n",
>> + &bus, &slot, &func, &count) != 3) {
>> + /* Invalid format */
>> + printk(KERN_ERR "PCI: Can't parse "
>> + "hide parameter: %s\n", p);
>> + break;
>> + }
>> + }
>> + p += count;
>> + devfn = (slot << 3) | func;
>> +
>> + do_pci_hide_device(seg, bus, devfn);
>> + if (*p != ';') {
>> + /* End of param or invalid format */
>> + break;
>> + }
>> + p++;
>> + }
>> + init_hidden = false;
>> +}
>> +
>> +static bool is_device_hidden(struct pci_bus *bus, int devfn)
>> +{
>> + struct pci_hidden_dev *d;
>> +
>> + if (unlikely(init_hidden))
>> + parse_hidden_devices();
>> +
>> + list_for_each_entry(d, &pci_hidden_dev_list, list)
>> + if (d->devfn == devfn && d->bus_nr == bus->number &&
>> + d->domain_nr == pci_domain_nr(bus))
>> + return true;
>> +
>> + return false;
>> +}
>> +
>> +void pci_hide_devices(const char *str, size_t count)
>> +{
>> + if (!str)
>> + return;
>> + if (count > PCI_HIDE_PARAM_SIZE - 1)
>> + count = PCI_HIDE_PARAM_SIZE - 1;
>> + strncpy(pci_hide_param, str, count);
>> + pci_hide_param[count] = '\0';
>> +}
>> +
>> struct pci_dev *__ref pci_scan_single_device(struct pci_bus *bus, int devfn)
>> {
>> struct pci_dev *dev;
>>
>> + if (unlikely(is_device_hidden(bus, devfn)))
>> + return NULL;
>> +
>> dev = pci_get_slot(bus, devfn);
>> if (dev) {
>> pci_dev_put(dev);
>> --
>> 1.7.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On Wednesday, July 03, 2013 11:16:18 PM Haicheng Li wrote:
> Cc: Len Brown <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: [email protected]
> Signed-off-by: Haicheng Li <[email protected]>
-ENOCHANGELOG
Apart from this, please send the *whole* patchset to linux-acpi.
Thanks,
Rafael
> ---
> drivers/acpi/scan.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 27da630..fcc0cdc 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -29,7 +29,6 @@ extern struct acpi_device *acpi_root;
>
> static const char *dummy_hid = "device";
>
> -static LIST_HEAD(acpi_device_list);
> static LIST_HEAD(acpi_bus_id_list);
> static DEFINE_MUTEX(acpi_scan_lock);
> static LIST_HEAD(acpi_scan_handlers_list);
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Wed, Jul 03, 2013 at 10:00:41AM -0600, Bjorn Helgaas wrote:
> On Wed, Jul 3, 2013 at 9:16 AM, Haicheng Li <[email protected]> wrote:
> > Cc: Rob Landley <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Signed-off-by: Haicheng Li <[email protected]>
> > ---
> > Documentation/kernel-parameters.txt | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> > index 2fe6e76..4da0834 100644
> > --- a/Documentation/kernel-parameters.txt
> > +++ b/Documentation/kernel-parameters.txt
> > @@ -2359,6 +2359,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> > pcie_scan_all Scan all possible PCIe devices. Otherwise we
> > only look for one device below a PCIe downstream
> > port.
> > + hide= Format: [<domain>:]<bus>:<slot>.<func>[; ...]
> > + Specifies devices to hide from pci subsystem.
> >
> > pcie_aspm= [PCIE] Forcibly enable or disable PCIe Active State Power
> > Management.
>
> The documentation update should be in the same patch as the
> functionality it adds.
Okay, got it. thanks.
On Wed, Jul 03, 2013 at 11:24:03PM +0200, Rafael J. Wysocki wrote:
> On Wednesday, July 03, 2013 11:16:18 PM Haicheng Li wrote:
> > Cc: Len Brown <[email protected]>
> > Cc: "Rafael J. Wysocki" <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Haicheng Li <[email protected]>
>
> -ENOCHANGELOG
Ah, just thought the patch is so trivial a cleanup, should be self-explainable:).
> Apart from this, please send the *whole* patchset to linux-acpi.
Sorry for the confusion. This is actually a standalone patch. I just noticed this
piece of code is not used by acpi anymore and can be removed cleanly.
If needed, I can resend it as a standalone patch.
> Thanks,
> Rafael
>
>
> > ---
> > drivers/acpi/scan.c | 1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > index 27da630..fcc0cdc 100644
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -29,7 +29,6 @@ extern struct acpi_device *acpi_root;
> >
> > static const char *dummy_hid = "device";
> >
> > -static LIST_HEAD(acpi_device_list);
> > static LIST_HEAD(acpi_bus_id_list);
> > static DEFINE_MUTEX(acpi_scan_lock);
> > static LIST_HEAD(acpi_scan_handlers_list);
> >
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
acpi_device_list is not used by acpi code anymore, could be removed.
Signed-off-by: Haicheng Li <[email protected]>
---
drivers/acpi/scan.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 27da630..fcc0cdc 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -29,7 +29,6 @@ extern struct acpi_device *acpi_root;
static const char *dummy_hid = "device";
-static LIST_HEAD(acpi_device_list);
static LIST_HEAD(acpi_bus_id_list);
static DEFINE_MUTEX(acpi_scan_lock);
static LIST_HEAD(acpi_scan_handlers_list);
--
1.7.9.5
On Wed, Jul 03, 2013 at 10:09:32AM -0600, Bjorn Helgaas wrote:
> On Wed, Jul 3, 2013 at 9:16 AM, Haicheng Li <[email protected]> wrote:
> > With more and more SOCs having pci device integrated into chip (e.g. Intel
> > Atom series), it's useful to add an interface to cleanly hide pci devices from
> > pci device scanning, which is because:
> >
> > 1. phone or tablet OEMs may choose disabling some pci device in the SOC,
> > such as camera ISP in Intel Atom Z2580 chip, and etc.
> > 2. if such disabled devices are not cleanly removed from pci device tree,
> > then pci-core will still try to operate on the relative device control
> > registers while S3 suspend and resume.
> > 3. so hiding such devices from early begining will not only reduce the kernel
> > boot time, but also optimize the latency of system suspend and resume.
>
> Normally the chip provides a way to disable devices by writing a
> configuration register. Then the device doesn't respond when Linux
> enumerates devices, so nothing special is required in the kernel.
Agreed, this is true.
> What's different about the Z2580? I'd be surprised if Intel forgot to
> include such a register.
A pci shim faked by firmware was introduced to help easily port Linux onto Z2XXX
SOC chips, which enumerates both real and fake PCI devices inside the SOC (The camera ISP
I mentioned above is a real PCI device in this case)
A detailed tech talk about this technology by Jacob Pan in elc2010 can be found online
here: http://elinux.org/images/e/ee/Jacob-Pan-x86MID-elc2010.pdf
> Maybe the firmware just isn't smart enough
> to disable the device? If so, it would be better to fix the firmware
> than to add kludges in the kernel.
On PC or server, end-user/OEM can disable/hide a pci device easily thru BIOS setting or
by hacking BIOS code directly when they find some device is broken or useless.
However on phone or tablet equipment, there is no BIOS-setting alike UI exposed to end-user/developer
to disable broken device easily (and physically removing the device is not doable on Phone or tablet)
OTOH, this i/f is really *helpful* for kernel developer to power-on a new platform, debug system
problem, or to do performance tuning of suspend/resume. At least it makes my daily job easier:).
So if people are strongly against this, I would still suggest accept this i/f as a debug i/f
at least:).
> > To hide pci devices, just pass such parameters to kernel at boot stage:
> > pci=hide=[<domain>:]<bus>:<slot>.<func>[; ...]
On Thursday, July 04, 2013 12:07:11 PM Haicheng Li wrote:
> acpi_device_list is not used by acpi code anymore, could be removed.
>
> Signed-off-by: Haicheng Li <[email protected]>
OK, applied.
Thanks,
Rafael
> ---
> drivers/acpi/scan.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 27da630..fcc0cdc 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -29,7 +29,6 @@ extern struct acpi_device *acpi_root;
>
> static const char *dummy_hid = "device";
>
> -static LIST_HEAD(acpi_device_list);
> static LIST_HEAD(acpi_bus_id_list);
> static DEFINE_MUTEX(acpi_scan_lock);
> static LIST_HEAD(acpi_scan_handlers_list);
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Wed, Jul 3, 2013 at 10:53 PM, Haicheng Li
<[email protected]> wrote:
> On Wed, Jul 03, 2013 at 10:09:32AM -0600, Bjorn Helgaas wrote:
>> On Wed, Jul 3, 2013 at 9:16 AM, Haicheng Li <[email protected]> wrote:
>> > With more and more SOCs having pci device integrated into chip (e.g. Intel
>> > Atom series), it's useful to add an interface to cleanly hide pci devices from
>> > pci device scanning, which is because:
>> >
>> > 1. phone or tablet OEMs may choose disabling some pci device in the SOC,
>> > such as camera ISP in Intel Atom Z2580 chip, and etc.
>> > 2. if such disabled devices are not cleanly removed from pci device tree,
>> > then pci-core will still try to operate on the relative device control
>> > registers while S3 suspend and resume.
>> > 3. so hiding such devices from early begining will not only reduce the kernel
>> > boot time, but also optimize the latency of system suspend and resume.
>>
>> Normally the chip provides a way to disable devices by writing a
>> configuration register. Then the device doesn't respond when Linux
>> enumerates devices, so nothing special is required in the kernel.
> Agreed, this is true.
>
>> What's different about the Z2580? I'd be surprised if Intel forgot to
>> include such a register.
> A pci shim faked by firmware was introduced to help easily port Linux onto Z2XXX
> SOC chips, which enumerates both real and fake PCI devices inside the SOC (The camera ISP
> I mentioned above is a real PCI device in this case)
>
> A detailed tech talk about this technology by Jacob Pan in elc2010 can be found online
> here: http://elinux.org/images/e/ee/Jacob-Pan-x86MID-elc2010.pdf
>
>> Maybe the firmware just isn't smart enough
>> to disable the device? If so, it would be better to fix the firmware
>> than to add kludges in the kernel.
> On PC or server, end-user/OEM can disable/hide a pci device easily thru BIOS setting or
> by hacking BIOS code directly when they find some device is broken or useless.
>
> However on phone or tablet equipment, there is no BIOS-setting alike UI exposed to end-user/developer
> to disable broken device easily (and physically removing the device is not doable on Phone or tablet)
>
> OTOH, this i/f is really *helpful* for kernel developer to power-on a new platform, debug system
> problem, or to do performance tuning of suspend/resume. At least it makes my daily job easier:).
>
> So if people are strongly against this, I would still suggest accept this i/f as a debug i/f
> at least:).
>
>> > To hide pci devices, just pass such parameters to kernel at boot stage:
>> > pci=hide=[<domain>:]<bus>:<slot>.<func>[; ...]
This doesn't seem generally useful to me. It might be useful as a
development aid, and you can easily maintain it as an out-of-tree
patch for that.
Bjorn
On 07/04/2013 12:09 AM, Bjorn Helgaas wrote:
> On Wed, Jul 3, 2013 at 9:16 AM, Haicheng Li <[email protected]> wrote:
>> With more and more SOCs having pci device integrated into chip (e.g. Intel
>> Atom series), it's useful to add an interface to cleanly hide pci devices from
>> pci device scanning, which is because:
>>
>> 1. phone or tablet OEMs may choose disabling some pci device in the SOC,
>> such as camera ISP in Intel Atom Z2580 chip, and etc.
>> 2. if such disabled devices are not cleanly removed from pci device tree,
>> then pci-core will still try to operate on the relative device control
>> registers while S3 suspend and resume.
>> 3. so hiding such devices from early begining will not only reduce the kernel
>> boot time, but also optimize the latency of system suspend and resume.
>
> Normally the chip provides a way to disable devices by writing a
> configuration register. Then the device doesn't respond when Linux
> enumerates devices, so nothing special is required in the kernel.
> What's different about the Z2580? I'd be surprised if Intel forgot to
> include such a register. Maybe the firmware just isn't smart enough
> to disable the device? If so, it would be better to fix the firmware
> than to add kludges in the kernel.
Agree, it would be great if chipset and firmware could cooperate to
handle this issue. Otherwise the interface may be abused and causes
trouble to PCI hotplug operations because the notation seg:bus:dev.func
isn't reliable. The PCI bus number may reassigned by OS.
>
>> To hide pci devices, just pass such parameters to kernel at boot stage:
>> pci=hide=[<domain>:]<bus>:<slot>.<func>[; ...]
>>
>> Cc: Bjorn Helgaas <[email protected]>
>> Cc: [email protected]
>> Signed-off-by: Haicheng Li <[email protected]>
>> ---
>> drivers/pci/pci.c | 2 ++
>> drivers/pci/pci.h | 2 ++
>> drivers/pci/probe.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 98 insertions(+)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index a899d8b..e228d00 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -3915,6 +3915,8 @@ static int __init pci_setup(char *str)
>> pcie_bus_config = PCIE_BUS_PEER2PEER;
>> } else if (!strncmp(str, "pcie_scan_all", 13)) {
>> pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
>> + } else if (!strncmp(str, "hide=", 5)) {
>> + pci_hide_devices(str + 5, strlen(str + 5));
>> } else {
>> printk(KERN_ERR "PCI: Unknown option `%s'\n",
>> str);
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index d1182c4..783703a 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -317,4 +317,6 @@ static inline int pci_dev_specific_reset(struct pci_dev *dev, int probe)
>> }
>> #endif
>>
>> +void pci_hide_devices(const char *, size_t);
>> +
>> #endif /* DRIVERS_PCI_H */
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index 70f10fa..4223ef3 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -11,6 +11,7 @@
>> #include <linux/cpumask.h>
>> #include <linux/pci-aspm.h>
>> #include <asm-generic/pci-bridge.h>
>> +#include <asm/setup.h>
>> #include "pci.h"
>>
>> #define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */
>> @@ -1352,10 +1353,103 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
>> pci_proc_attach_device(dev);
>> }
>>
>> +static LIST_HEAD(pci_hidden_dev_list);
>> +struct pci_hidden_dev {
>> + struct list_head list;
>> + int domain_nr;
>> + int devfn;
>> + unsigned char bus_nr;
>> +};
>> +
>> +static void do_pci_hide_device(int domain_nr, unsigned char bus_nr, int devfn)
>> +{
>> + struct pci_hidden_dev *d;
>> +
>> + list_for_each_entry(d, &pci_hidden_dev_list, list)
>> + if (devfn == d->devfn && bus_nr == d->bus_nr &&
>> + domain_nr == d->domain_nr)
>> + return;
>> +
>> + d = kzalloc(sizeof(*d), GFP_KERNEL);
>> + if (!d)
>> + return;
>> +
>> + d->domain_nr = domain_nr;
>> + d->bus_nr = bus_nr;
>> + d->devfn = devfn;
>> +
>> + list_add_tail(&d->list, &pci_hidden_dev_list);
>> +}
>> +
>> +static bool init_hidden = true;
>> +
>> +#define PCI_HIDE_PARAM_SIZE COMMAND_LINE_SIZE
>> +static char pci_hide_param[PCI_HIDE_PARAM_SIZE] = {'\0'};
>> +
>> +static void parse_hidden_devices(void)
>> +{
>> + char *p = pci_hide_param;
>> + int seg, bus, slot, func, devfn, count;
>> +
>> + /* parse pci.hide= command-line */
>> + while (*p != '\0') {
>> + count = 0;
>> + if (sscanf(p, "%x:%x:%x.%x%n",
>> + &seg, &bus, &slot, &func, &count) != 4) {
>> + seg = 0;
>> + if (sscanf(p, "%x:%x.%x%n",
>> + &bus, &slot, &func, &count) != 3) {
>> + /* Invalid format */
>> + printk(KERN_ERR "PCI: Can't parse "
>> + "hide parameter: %s\n", p);
>> + break;
>> + }
>> + }
>> + p += count;
>> + devfn = (slot << 3) | func;
>> +
>> + do_pci_hide_device(seg, bus, devfn);
>> + if (*p != ';') {
>> + /* End of param or invalid format */
>> + break;
>> + }
>> + p++;
>> + }
>> + init_hidden = false;
>> +}
>> +
>> +static bool is_device_hidden(struct pci_bus *bus, int devfn)
>> +{
>> + struct pci_hidden_dev *d;
>> +
>> + if (unlikely(init_hidden))
>> + parse_hidden_devices();
>> +
>> + list_for_each_entry(d, &pci_hidden_dev_list, list)
>> + if (d->devfn == devfn && d->bus_nr == bus->number &&
>> + d->domain_nr == pci_domain_nr(bus))
>> + return true;
>> +
>> + return false;
>> +}
>> +
>> +void pci_hide_devices(const char *str, size_t count)
>> +{
>> + if (!str)
>> + return;
>> + if (count > PCI_HIDE_PARAM_SIZE - 1)
>> + count = PCI_HIDE_PARAM_SIZE - 1;
>> + strncpy(pci_hide_param, str, count);
>> + pci_hide_param[count] = '\0';
>> +}
>> +
>> struct pci_dev *__ref pci_scan_single_device(struct pci_bus *bus, int devfn)
>> {
>> struct pci_dev *dev;
>>
>> + if (unlikely(is_device_hidden(bus, devfn)))
>> + return NULL;
>> +
>> dev = pci_get_slot(bus, devfn);
>> if (dev) {
>> pci_dev_put(dev);
>> --
>> 1.7.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On Sun, Jul 07, 2013 at 10:43:35AM +0800, Jiang Liu wrote:
> On 07/04/2013 12:09 AM, Bjorn Helgaas wrote:
> > On Wed, Jul 3, 2013 at 9:16 AM, Haicheng Li <[email protected]> wrote:
> >> With more and more SOCs having pci device integrated into chip (e.g. Intel
> >> Atom series), it's useful to add an interface to cleanly hide pci devices from
> >> pci device scanning, which is because:
> >>
> >> 1. phone or tablet OEMs may choose disabling some pci device in the SOC,
> >> such as camera ISP in Intel Atom Z2580 chip, and etc.
> >> 2. if such disabled devices are not cleanly removed from pci device tree,
> >> then pci-core will still try to operate on the relative device control
> >> registers while S3 suspend and resume.
> >> 3. so hiding such devices from early begining will not only reduce the kernel
> >> boot time, but also optimize the latency of system suspend and resume.
> >
> > Normally the chip provides a way to disable devices by writing a
> > configuration register. Then the device doesn't respond when Linux
> > enumerates devices, so nothing special is required in the kernel.
> > What's different about the Z2580? I'd be surprised if Intel forgot to
> > include such a register. Maybe the firmware just isn't smart enough
> > to disable the device? If so, it would be better to fix the firmware
> > than to add kludges in the kernel.
Sorry for slow response due to a travel recently.
> Agree, it would be great if chipset and firmware could cooperate to
> handle this issue. Otherwise the interface may be abused and causes
> trouble to PCI hotplug operations because the notation seg:bus:dev.func
> isn't reliable. The PCI bus number may reassigned by OS.
I quickly went thru the code and thought the bus # should not be changed in runtime
by current code. do you mean after a reboot, the bus# maybe changed with new slot hotplugged?
Can you please elabrate this case? hopefully I can enhance the patch to cover it.
> >
> >> To hide pci devices, just pass such parameters to kernel at boot stage:
> >> pci=hide=[<domain>:]<bus>:<slot>.<func>[; ...]
On Fri, Jul 05, 2013 at 11:28:40AM -0600, Bjorn Helgaas wrote:
> >> > To hide pci devices, just pass such parameters to kernel at boot stage:
> >> > pci=hide=[<domain>:]<bus>:<slot>.<func>[; ...]
>
> This doesn't seem generally useful to me. It might be useful as a
> development aid, and you can easily maintain it as an out-of-tree
> patch for that.
That's fine, I can maintain it as out-of-tree patch:).
Thank you for the review.
> Bjorn