* Re: [PATCH RFC v4 15/21] PCI: Allow the failed resources to be reassigned later
From: Bjorn Helgaas @ 2019-03-26 20:58 UTC (permalink / raw)
To: Sergey Miroshnichenko; +Cc: linux-pci, linuxppc-dev, linux
In-Reply-To: <20190311133122.11417-16-s.miroshnichenko@yadro.com>
On Mon, Mar 11, 2019 at 04:31:16PM +0300, Sergey Miroshnichenko wrote:
> Don't lose the size of the requested EP's BAR if it can't be fit
> in a current trial, so this can be retried.
s/EP/device/, since this applies equally to conventional PCI.
> But a failed bridge window must be dropped and recalculated in the
> next trial.
>
> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
> ---
> drivers/pci/setup-bus.c | 3 ++-
> drivers/pci/setup-res.c | 12 ++++++++++++
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index f9d605cd1725..c1559a4a8564 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -309,7 +309,8 @@ static void assign_requested_resources_sorted(struct list_head *head,
> 0 /* don't care */,
> 0 /* don't care */);
> }
> - reset_resource(res);
> + if (!pci_movable_bars_enabled())
> + reset_resource(res);
> }
> }
> }
> diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> index d8ca40a97693..732d18f60f1b 100644
> --- a/drivers/pci/setup-res.c
> +++ b/drivers/pci/setup-res.c
> @@ -298,6 +298,18 @@ static int _pci_assign_resource(struct pci_dev *dev, int resno,
>
> bus = dev->bus;
> while ((ret = __pci_assign_resource(bus, dev, resno, size, min_align))) {
> + if (pci_movable_bars_enabled()) {
> + if (resno >= PCI_BRIDGE_RESOURCES &&
> + resno <= PCI_BRIDGE_RESOURCE_END) {
> + struct resource *res = dev->resource + resno;
> +
> + res->start = 0;
> + res->end = 0;
> + res->flags = 0;
> + }
> + break;
> + }
> +
> if (!bus->parent || !bus->self->transparent)
> break;
> bus = bus->parent;
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH RFC v4 14/21] PCI: Don't reserve memory for hotplug when enabled movable BARs
From: Bjorn Helgaas @ 2019-03-26 20:57 UTC (permalink / raw)
To: Sergey Miroshnichenko; +Cc: linux-pci, linuxppc-dev, linux
In-Reply-To: <20190311133122.11417-15-s.miroshnichenko@yadro.com>
On Mon, Mar 11, 2019 at 04:31:15PM +0300, Sergey Miroshnichenko wrote:
> pbus_size_mem() returns a precise amount of memory required to fit
> all the requested BARs and windows of children bridges.
Please make the commit log complete in itself, without requiring the
subject.
> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
> ---
> drivers/pci/setup-bus.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 9d93f2b32bf1..f9d605cd1725 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -1229,7 +1229,7 @@ void __pci_bus_size_bridges(struct pci_bus *bus, struct list_head *realloc_head)
>
> case PCI_HEADER_TYPE_BRIDGE:
> pci_bridge_check_ranges(bus);
> - if (bus->self->is_hotplug_bridge) {
> + if (bus->self->is_hotplug_bridge && !pci_movable_bars_enabled()) {
> additional_io_size = pci_hotplug_io_size;
> additional_mem_size = pci_hotplug_mem_size;
> }
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH RFC v4 12/21] PCI: Don't allow hotplugged devices to steal resources
From: Bjorn Helgaas @ 2019-03-26 20:55 UTC (permalink / raw)
To: Sergey Miroshnichenko; +Cc: linux-pci, linuxppc-dev, linux
In-Reply-To: <20190311133122.11417-13-s.miroshnichenko@yadro.com>
On Mon, Mar 11, 2019 at 04:31:13PM +0300, Sergey Miroshnichenko wrote:
> When movable BARs are enabled, the PCI subsystem at first releases
> all the bridge windows and then performs an attempt to assign new
> requested resources and re-assign the existing ones.
s/performs an attempt/attempts/
I guess "new requested resources" means "resources to newly hotplugged
devices"?
> If a hotplugged device gets its resources first, there could be no
> space left to re-assign resources of already working devices, which
> is unacceptable. If this happens, this patch marks one of the new
> devices with the new introduced flag PCI_DEV_IGNORE and retries the
> resource assignment.
>
> This patch adds a new res_mask bitmask to the struct pci_dev for
> storing the indices of assigned resources.
>
> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
> ---
> drivers/pci/bus.c | 5 ++
> drivers/pci/pci.h | 11 +++++
> drivers/pci/probe.c | 100 +++++++++++++++++++++++++++++++++++++++-
> drivers/pci/setup-bus.c | 15 ++++++
> include/linux/pci.h | 1 +
> 5 files changed, 130 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 5cb40b2518f9..a9784144d6f2 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -311,6 +311,11 @@ void pci_bus_add_device(struct pci_dev *dev)
> {
> int retval;
>
> + if (pci_dev_is_ignored(dev)) {
> + pci_warn(dev, "%s: don't enable the ignored device\n", __func__);
> + return;
I'm not sure about this. Even if we're unable to assign space for all
the device's BARs, it still should respond to config accesses, and I
think it should show up in sysfs and lspci.
> + }
> +
> /*
> * Can not put in pci_device_add yet because resources
> * are not assigned yet for some devices.
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index e06e8692a7b1..56b905068ac5 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -366,6 +366,7 @@ static inline bool pci_dev_is_disconnected(const struct pci_dev *dev)
>
> /* pci_dev priv_flags */
> #define PCI_DEV_ADDED 0
> +#define PCI_DEV_IGNORE 1
>
> static inline void pci_dev_assign_added(struct pci_dev *dev, bool added)
> {
> @@ -377,6 +378,16 @@ static inline bool pci_dev_is_added(const struct pci_dev *dev)
> return test_bit(PCI_DEV_ADDED, &dev->priv_flags);
> }
>
> +static inline void pci_dev_ignore(struct pci_dev *dev, bool ignore)
> +{
> + assign_bit(PCI_DEV_IGNORE, &dev->priv_flags, ignore);
> +}
> +
> +static inline bool pci_dev_is_ignored(const struct pci_dev *dev)
> +{
> + return test_bit(PCI_DEV_IGNORE, &dev->priv_flags);
> +}
> +
> #ifdef CONFIG_PCIEAER
> #include <linux/aer.h>
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 692752c71f71..62f4058a001f 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -3248,6 +3248,23 @@ unsigned int pci_rescan_bus_bridge_resize(struct pci_dev *bridge)
> return max;
> }
>
> +static unsigned int pci_dev_res_mask(struct pci_dev *dev)
> +{
> + unsigned int res_mask = 0;
> + int i;
> +
> + for (i = 0; i < PCI_BRIDGE_RESOURCES; i++) {
> + struct resource *r = &dev->resource[i];
> +
> + if (!r->flags || (r->flags & IORESOURCE_UNSET) || !r->parent)
> + continue;
> +
> + res_mask |= (1 << i);
> + }
> +
> + return res_mask;
> +}
> +
> static void pci_bus_rescan_prepare(struct pci_bus *bus)
> {
> struct pci_dev *dev;
> @@ -3257,6 +3274,8 @@ static void pci_bus_rescan_prepare(struct pci_bus *bus)
> list_for_each_entry(dev, &bus->devices, bus_list) {
> struct pci_bus *child = dev->subordinate;
>
> + dev->res_mask = pci_dev_res_mask(dev);
> +
> if (child) {
> pci_bus_rescan_prepare(child);
> } else if (dev->driver &&
> @@ -3318,6 +3337,84 @@ static void pci_setup_bridges(struct pci_bus *bus)
> pci_setup_bridge(bus);
> }
>
> +static struct pci_dev *pci_find_next_new_device(struct pci_bus *bus)
> +{
> + struct pci_dev *dev;
> +
> + if (!bus)
> + return NULL;
> +
> + list_for_each_entry(dev, &bus->devices, bus_list) {
> + struct pci_bus *child_bus = dev->subordinate;
> +
> + if (!pci_dev_is_added(dev) && !pci_dev_is_ignored(dev))
> + return dev;
> +
> + if (child_bus) {
> + struct pci_dev *next_new_dev;
> +
> + next_new_dev = pci_find_next_new_device(child_bus);
> + if (next_new_dev)
> + return next_new_dev;
> + }
> + }
> +
> + return NULL;
> +}
> +
> +static bool pci_bus_validate_resources(struct pci_bus *bus)
The name of this function should tell us what the return value means.
Just from the name "pci_bus_validate_resources", I can't tell whether we
call it for side-effects, or whether true or false indicates success.
> +{
> + struct pci_dev *dev;
> + bool ret = true;
> +
> + if (!bus)
> + return false;
> +
> + list_for_each_entry(dev, &bus->devices, bus_list) {
> + struct pci_bus *child = dev->subordinate;
> + unsigned int res_mask = pci_dev_res_mask(dev);
> +
> + if (pci_dev_is_ignored(dev))
> + continue;
> +
> + if (dev->res_mask & ~res_mask) {
> + pci_err(dev, "%s: Non-re-enabled resources found: 0x%x -> 0x%x\n",
> + __func__, dev->res_mask, res_mask);
I don't think __func__ really tells users anything useful, so I would
just omit them. Searching for the text of the message is almost as
good.
> + ret = false;
> + }
> +
> + if (child && !pci_bus_validate_resources(child))
> + ret = false;
> + }
> +
> + return ret;
> +}
> +
> +static void pci_reassign_root_bus_resources(struct pci_bus *root)
> +{
> + do {
> + struct pci_dev *next_new_dev;
> +
> + pci_bus_release_root_bridge_resources(root);
> + pci_assign_unassigned_root_bus_resources(root);
> +
> + if (pci_bus_validate_resources(root))
> + break;
> +
> + next_new_dev = pci_find_next_new_device(root);
> + if (!next_new_dev) {
> + dev_err(&root->dev, "%s: failed to re-assign resources even after ignoring all the hotplugged devices\n",
> + __func__);
> + break;
> + }
> +
> + dev_warn(&root->dev, "%s: failed to re-assign resources, disable the next hotplugged device %s and retry\n",
> + __func__, dev_name(&next_new_dev->dev));
> +
> + pci_dev_ignore(next_new_dev, true);
> + } while (true);
> +}
> +
> /**
> * pci_rescan_bus - Scan a PCI bus for devices
> * @bus: PCI bus to scan
> @@ -3341,8 +3438,7 @@ unsigned int pci_rescan_bus(struct pci_bus *bus)
>
> max = pci_scan_child_bus(root);
>
> - pci_bus_release_root_bridge_resources(root);
> - pci_assign_unassigned_root_bus_resources(root);
> + pci_reassign_root_bus_resources(root);
>
> pci_setup_bridges(root);
> pci_bus_rescan_done(root);
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 36a1907d9509..551108f48df7 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -131,6 +131,9 @@ static void pdev_sort_resources(struct pci_dev *dev, struct list_head *head)
> {
> int i;
>
> + if (pci_dev_is_ignored(dev))
> + return;
> +
> for (i = 0; i < PCI_NUM_RESOURCES; i++) {
> struct resource *r;
> struct pci_dev_resource *dev_res, *tmp;
> @@ -181,6 +184,9 @@ static void __dev_sort_resources(struct pci_dev *dev,
> {
> u16 class = dev->class >> 8;
>
> + if (pci_dev_is_ignored(dev))
> + return;
> +
> /* Don't touch classless devices or host bridges or ioapics. */
> if (class == PCI_CLASS_NOT_DEFINED || class == PCI_CLASS_BRIDGE_HOST)
> return;
> @@ -284,6 +290,9 @@ static void assign_requested_resources_sorted(struct list_head *head,
> int idx;
>
> list_for_each_entry(dev_res, head, list) {
> + if (pci_dev_is_ignored(dev_res->dev))
> + continue;
> +
> res = dev_res->res;
> idx = res - &dev_res->dev->resource[0];
> if (resource_size(res) &&
> @@ -991,6 +1000,9 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
> list_for_each_entry(dev, &bus->devices, bus_list) {
> int i;
>
> + if (pci_dev_is_ignored(dev))
> + continue;
> +
> for (i = 0; i < PCI_NUM_RESOURCES; i++) {
> struct resource *r = &dev->resource[i];
> resource_size_t r_size;
> @@ -1353,6 +1365,9 @@ void __pci_bus_assign_resources(const struct pci_bus *bus,
> pbus_assign_resources_sorted(bus, realloc_head, fail_head);
>
> list_for_each_entry(dev, &bus->devices, bus_list) {
> + if (pci_dev_is_ignored(dev))
> + continue;
> +
> pdev_assign_fixed_resources(dev);
>
> b = dev->subordinate;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 3d52f5538282..26aa59cb6220 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -369,6 +369,7 @@ struct pci_dev {
> */
> unsigned int irq;
> struct resource resource[DEVICE_COUNT_RESOURCE]; /* I/O and memory regions + expansion ROMs */
> + unsigned int res_mask; /* Bitmask of assigned resources */
>
> bool match_driver; /* Skip attaching driver */
>
> --
> 2.20.1
>
^ permalink raw reply
* [PATCH v2 1/2] powerpc: Make some functions static
From: Mathieu Malaterre @ 2019-03-26 20:47 UTC (permalink / raw)
To: Michael Ellerman
Cc: Mathieu Malaterre, Paul Mackerras, linuxppc-dev, linux-kernel
In-Reply-To: <20190312203140.31785-1-malat@debian.org>
In commit cb9e4d10c448 ("[POWERPC] Add support for 750CL Holly board")
new functions were added. Since most of these functions can be made static,
make it so.
Both holly_power_off and holly_halt functions were not changed since they
are unused, making them static would have triggered the following warning
(treated as error):
arch/powerpc/platforms/embedded6xx/holly.c:244:13: error: 'holly_halt' defined but not used [-Werror=unused-function]
Silence the following warnings triggered using W=1:
arch/powerpc/platforms/embedded6xx/holly.c:47:5: error: no previous prototype for 'holly_exclude_device' [-Werror=missing-prototypes]
arch/powerpc/platforms/embedded6xx/holly.c:190:6: error: no previous prototype for 'holly_show_cpuinfo' [-Werror=missing-prototypes]
arch/powerpc/platforms/embedded6xx/holly.c:196:17: error: no previous prototype for 'holly_restart' [-Werror=missing-prototypes]
Signed-off-by: Mathieu Malaterre <malat@debian.org>
---
v2: Split the patch in two operations
arch/powerpc/platforms/embedded6xx/holly.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/platforms/embedded6xx/holly.c b/arch/powerpc/platforms/embedded6xx/holly.c
index 0409714e8070..9d2eefef7b7b 100644
--- a/arch/powerpc/platforms/embedded6xx/holly.c
+++ b/arch/powerpc/platforms/embedded6xx/holly.c
@@ -44,7 +44,8 @@
#define HOLLY_PCI_CFG_PHYS 0x7c000000
-int holly_exclude_device(struct pci_controller *hose, u_char bus, u_char devfn)
+static int holly_exclude_device(struct pci_controller *hose, u_char bus,
+ u_char devfn)
{
if (bus == 0 && PCI_SLOT(devfn) == 0)
return PCIBIOS_DEVICE_NOT_FOUND;
@@ -187,13 +188,13 @@ static void __init holly_init_IRQ(void)
tsi108_write_reg(TSI108_MPIC_OFFSET + 0x30c, 0);
}
-void holly_show_cpuinfo(struct seq_file *m)
+static void holly_show_cpuinfo(struct seq_file *m)
{
seq_printf(m, "vendor\t\t: IBM\n");
seq_printf(m, "machine\t\t: PPC750 GX/CL\n");
}
-void __noreturn holly_restart(char *cmd)
+static void __noreturn holly_restart(char *cmd)
{
__be32 __iomem *ocn_bar1 = NULL;
unsigned long bar;
--
2.20.1
^ permalink raw reply related
* [PATCH v2 2/2] Remove functions holly_power_off and holly_halt since unused
From: Mathieu Malaterre @ 2019-03-26 20:47 UTC (permalink / raw)
To: Michael Ellerman
Cc: Mathieu Malaterre, Paul Mackerras, linuxppc-dev, linux-kernel
In-Reply-To: <20190326204720.18882-1-malat@debian.org>
Silence the following warnings triggered using W=1:
arch/powerpc/platforms/embedded6xx/holly.c:236:6: error: no previous prototype for 'holly_power_off' [-Werror=missing-prototypes]
arch/powerpc/platforms/embedded6xx/holly.c:243:6: error: no previous prototype for 'holly_halt' [-Werror=missing-prototypes]
Signed-off-by: Mathieu Malaterre <malat@debian.org>
---
arch/powerpc/platforms/embedded6xx/holly.c | 12 ------------
1 file changed, 12 deletions(-)
diff --git a/arch/powerpc/platforms/embedded6xx/holly.c b/arch/powerpc/platforms/embedded6xx/holly.c
index 9d2eefef7b7b..829bf3697dc9 100644
--- a/arch/powerpc/platforms/embedded6xx/holly.c
+++ b/arch/powerpc/platforms/embedded6xx/holly.c
@@ -234,18 +234,6 @@ static void __noreturn holly_restart(char *cmd)
for (;;) ;
}
-void holly_power_off(void)
-{
- local_irq_disable();
- /* No way to shut power off with software */
- for (;;) ;
-}
-
-void holly_halt(void)
-{
- holly_power_off();
-}
-
/*
* Called very early, device-tree isn't unflattened
*/
--
2.20.1
^ permalink raw reply related
* Re: [PATCH RFC v4 11/21] PCI: Release and reassign the root bridge resources during rescan
From: Bjorn Helgaas @ 2019-03-26 20:41 UTC (permalink / raw)
To: Sergey Miroshnichenko; +Cc: linux-pci, linuxppc-dev, linux
In-Reply-To: <20190311133122.11417-12-s.miroshnichenko@yadro.com>
On Mon, Mar 11, 2019 at 04:31:12PM +0300, Sergey Miroshnichenko wrote:
> When the movable BARs feature is enabled, don't rely on the memory gaps
> reserved by the BIOS/bootloader/firmware, but instead rearrange the BARs
> and bridge windows starting from the root.
>
> Endpoint device's BARs, after being released, are resorted and written
> back by the pci_assign_unassigned_root_bus_resources().
>
> The last step of writing the recalculated windows to the bridges is done
> by the new pci_setup_bridges() function.
>
> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
> ---
> drivers/pci/pci.h | 1 +
> drivers/pci/probe.c | 22 ++++++++++++++++++++++
> drivers/pci/setup-bus.c | 11 ++++++++++-
> 3 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 224d88634115..e06e8692a7b1 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -248,6 +248,7 @@ void __pci_bus_assign_resources(const struct pci_bus *bus,
> struct list_head *realloc_head,
> struct list_head *fail_head);
> bool pci_bus_clip_resource(struct pci_dev *dev, int idx);
> +void pci_bus_release_root_bridge_resources(struct pci_bus *bus);
>
> void pci_reassigndev_resource_alignment(struct pci_dev *dev);
> void pci_disable_bridge_window(struct pci_dev *dev);
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 1cf6ec960236..692752c71f71 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -3299,6 +3299,25 @@ static void pci_bus_rescan_done(struct pci_bus *bus)
> pm_runtime_put(&bus->dev);
> }
>
> +static void pci_setup_bridges(struct pci_bus *bus)
> +{
> + struct pci_dev *dev;
> +
> + list_for_each_entry(dev, &bus->devices, bus_list) {
> + struct pci_bus *child;
> +
> + if (!pci_dev_is_added(dev) || pci_dev_is_ignored(dev))
> + continue;
> +
> + child = dev->subordinate;
> + if (child)
> + pci_setup_bridges(child);
> + }
> +
> + if (bus->self)
> + pci_setup_bridge(bus);
> +}
> +
> /**
> * pci_rescan_bus - Scan a PCI bus for devices
> * @bus: PCI bus to scan
> @@ -3321,8 +3340,11 @@ unsigned int pci_rescan_bus(struct pci_bus *bus)
> pci_bus_rescan_prepare(root);
>
> max = pci_scan_child_bus(root);
> +
> + pci_bus_release_root_bridge_resources(root);
> pci_assign_unassigned_root_bus_resources(root);
>
> + pci_setup_bridges(root);
> pci_bus_rescan_done(root);
> } else {
> max = pci_scan_child_bus(bus);
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index be7d4e6d7b65..36a1907d9509 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -1584,7 +1584,7 @@ static void pci_bridge_release_resources(struct pci_bus *bus,
> pci_printk(KERN_DEBUG, dev, "resource %d %pR released\n",
> PCI_BRIDGE_RESOURCES + idx, r);
> /* keep the old size */
> - r->end = resource_size(r) - 1;
> + r->end = pci_movable_bars_enabled() ? 0 : (resource_size(r) - 1);
Doesn't this mean we're throwing away the information about the BAR
size, and we'll have to size the BAR again somewhere? I would like to
avoid that. But I don't know yet where you rely on this, so maybe
it's not possible to avoid it.
> r->start = 0;
> r->flags = 0;
>
> @@ -1637,6 +1637,15 @@ static void pci_bus_release_bridge_resources(struct pci_bus *bus,
> pci_bridge_release_resources(bus, type);
> }
>
> +void pci_bus_release_root_bridge_resources(struct pci_bus *root_bus)
> +{
> + pci_bus_release_bridge_resources(root_bus, IORESOURCE_IO, whole_subtree);
> + pci_bus_release_bridge_resources(root_bus, IORESOURCE_MEM, whole_subtree);
> + pci_bus_release_bridge_resources(root_bus,
> + IORESOURCE_MEM_64 | IORESOURCE_PREFETCH,
> + whole_subtree);
> +}
> +
> static void pci_bus_dump_res(struct pci_bus *bus)
> {
> struct resource *res;
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH RFC v4 10/21] PCI: Fix assigning of fixed prefetchable resources
From: Bjorn Helgaas @ 2019-03-26 20:37 UTC (permalink / raw)
To: Sergey Miroshnichenko; +Cc: linux-pci, linuxppc-dev, linux
In-Reply-To: <20190311133122.11417-11-s.miroshnichenko@yadro.com>
On Mon, Mar 11, 2019 at 04:31:11PM +0300, Sergey Miroshnichenko wrote:
> Allow matching them to non-prefetchable windows, as it is done for movable
> resources.
Please make the commit log complete in itself, without requiring the
subject. It's OK if you have to repeat the subject.
IIUC, this is actually a bug fix and is not strictly related to
movable resources. We should be able to have a IORESOURCE_PCI_FIXED
prefetchable BAR in a non-prefetchable window.
I suppose movable windows exposes this case because as currently
implemented, it marks many more BARs as IORESOURCE_PCI_FIXED. I think
we should use something other than IORESOURCE_PCI_FIXED for that case,
so maybe this patch will end up being unnecessary?
> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
> ---
> drivers/pci/setup-bus.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 3644feb13179..be7d4e6d7b65 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -1301,15 +1301,20 @@ static void assign_fixed_resource_on_bus(struct pci_bus *b, struct resource *r)
> {
> int i;
> struct resource *parent_r;
> - unsigned long mask = IORESOURCE_IO | IORESOURCE_MEM |
> - IORESOURCE_PREFETCH;
> + unsigned long mask = IORESOURCE_TYPE_BITS;
>
> pci_bus_for_each_resource(b, parent_r, i) {
> if (!parent_r)
> continue;
>
> - if ((r->flags & mask) == (parent_r->flags & mask) &&
> - resource_contains(parent_r, r))
> + if ((r->flags & mask) != (parent_r->flags & mask))
> + continue;
> +
> + if (parent_r->flags & IORESOURCE_PREFETCH &&
> + !(r->flags & IORESOURCE_PREFETCH))
> + continue;
> +
> + if (resource_contains(parent_r, r))
> request_resource(parent_r, r);
> }
> }
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH RFC v4 09/21] PCI: Mark immovable BARs with PCI_FIXED
From: Bjorn Helgaas @ 2019-03-26 20:28 UTC (permalink / raw)
To: Sergey Miroshnichenko; +Cc: linux-pci, linuxppc-dev, linux
In-Reply-To: <20190311133122.11417-10-s.miroshnichenko@yadro.com>
On Mon, Mar 11, 2019 at 04:31:10PM +0300, Sergey Miroshnichenko wrote:
> If a PCIe device driver doesn't yet have support for movable BARs,
> mark device's BARs with IORESOURCE_PCI_FIXED.
I'm hesitant about using IORESOURCE_PCI_FIXED for this purpose. That
was originally added to describe resources that can not be changed
because they're hardwired in the device, e.g., legacy resources and
Enhanced Allocation resources.
In general, I think the bits in res->flags should tell us things about
the hardware. This particular use would be something about the
*driver*, and I think we should figure that out by looking at
dev->driver.
> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
> ---
> drivers/pci/probe.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index dc935f82a595..1cf6ec960236 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -3262,6 +3262,21 @@ static void pci_bus_rescan_prepare(struct pci_bus *bus)
> } else if (dev->driver &&
> dev->driver->rescan_prepare) {
> dev->driver->rescan_prepare(dev);
> + } else if (dev->driver || ((dev->class >> 8) == PCI_CLASS_DISPLAY_VGA)) {
> + int i;
> +
> + for (i = 0; i < PCI_NUM_RESOURCES; i++) {
> + struct resource *r = &dev->resource[i];
> +
> + if (!r->flags || !r->parent ||
> + (r->flags & IORESOURCE_UNSET) ||
> + (r->flags & IORESOURCE_PCI_FIXED))
> + continue;
> +
> + r->flags |= IORESOURCE_PCI_FIXED;
> + pci_warn(dev, "%s: no support for movable BARs, mark BAR %d (%pR) as fixed\n",
> + __func__, i, r);
> + }
> }
> }
> }
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH RFC v4 08/21] nvme-pci: Handle movable BARs
From: Bjorn Helgaas @ 2019-03-26 20:20 UTC (permalink / raw)
To: Sergey Miroshnichenko
Cc: Jens Axboe, Sagi Grimberg, linux-pci, linux-kernel, linux-nvme,
linux, Keith Busch, linuxppc-dev, Christoph Hellwig
In-Reply-To: <20190311133122.11417-9-s.miroshnichenko@yadro.com>
[+cc Keith, Jens, Christoph, Sagi, linux-nvme, LKML]
On Mon, Mar 11, 2019 at 04:31:09PM +0300, Sergey Miroshnichenko wrote:
> Hotplugged devices can affect the existing ones by moving their BARs.
> PCI subsystem will inform the NVME driver about this by invoking
> reset_prepare()+reset_done(), then iounmap()+ioremap() must be called.
Do you mean the PCI core will invoke ->rescan_prepare() and
->rescan_done() (as opposed to *reset*)?
> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
> ---
> drivers/nvme/host/pci.c | 29 +++++++++++++++++++++++++++--
> 1 file changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 92bad1c810ac..ccea3033a67a 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -106,6 +106,7 @@ struct nvme_dev {
> unsigned int num_vecs;
> int q_depth;
> u32 db_stride;
> + resource_size_t current_phys_bar;
> void __iomem *bar;
> unsigned long bar_mapped_size;
> struct work_struct remove_work;
> @@ -1672,13 +1673,16 @@ static int nvme_remap_bar(struct nvme_dev *dev, unsigned long size)
> {
> struct pci_dev *pdev = to_pci_dev(dev->dev);
>
> - if (size <= dev->bar_mapped_size)
> + if (dev->bar &&
> + dev->current_phys_bar == pci_resource_start(pdev, 0) &&
> + size <= dev->bar_mapped_size)
> return 0;
> if (size > pci_resource_len(pdev, 0))
> return -ENOMEM;
> if (dev->bar)
> iounmap(dev->bar);
> - dev->bar = ioremap(pci_resource_start(pdev, 0), size);
> + dev->current_phys_bar = pci_resource_start(pdev, 0);
> + dev->bar = ioremap(dev->current_phys_bar, size);
dev->current_phys_bar is different from pci_resource_start() in the
case where the PCI core has moved the nvme BAR, but nvme has not yet
remapped it.
I'm not sure it's worth keeping track of current_phys_bar, as opposed
to always unmapping and remapping. Is this a performance path? I
think there are advantages to always exercising the same code path,
regardless of whether the BAR happened to be moved, e.g., if there's a
bug in the "BAR moved" path, it may be a heisenbug because whether we
exercise that path depends on the current configuration.
If you do need to cache current_phys_bar, maybe this, so it's a little
easier to see that you're not changing the ioremap() itself:
dev->bar = ioremap(pci_resource_start(pdev, 0), size);
dev->current_phys_bar = pci_resource_start(pdev, 0);
> if (!dev->bar) {
> dev->bar_mapped_size = 0;
> return -ENOMEM;
> @@ -2504,6 +2508,8 @@ static void nvme_reset_work(struct work_struct *work)
> if (WARN_ON(dev->ctrl.state != NVME_CTRL_RESETTING))
> goto out;
>
> + nvme_remap_bar(dev, db_bar_size(dev, 0));
How is this change connected to rescan? This looks reset-related.
> /*
> * If we're called to reset a live controller first shut it down before
> * moving on.
> @@ -2910,6 +2916,23 @@ static void nvme_error_resume(struct pci_dev *pdev)
> flush_work(&dev->ctrl.reset_work);
> }
>
> +void nvme_rescan_prepare(struct pci_dev *pdev)
> +{
> + struct nvme_dev *dev = pci_get_drvdata(pdev);
> +
> + nvme_dev_disable(dev, false);
> + nvme_dev_unmap(dev);
> + dev->bar = NULL;
> +}
> +
> +void nvme_rescan_done(struct pci_dev *pdev)
> +{
> + struct nvme_dev *dev = pci_get_drvdata(pdev);
> +
> + nvme_dev_map(dev);
> + nvme_reset_ctrl_sync(&dev->ctrl);
> +}
> +
> static const struct pci_error_handlers nvme_err_handler = {
> .error_detected = nvme_error_detected,
> .slot_reset = nvme_slot_reset,
> @@ -2974,6 +2997,8 @@ static struct pci_driver nvme_driver = {
> },
> .sriov_configure = pci_sriov_configure_simple,
> .err_handler = &nvme_err_handler,
> + .rescan_prepare = nvme_rescan_prepare,
> + .rescan_done = nvme_rescan_done,
> };
>
> static int __init nvme_init(void)
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH v2 4/7] dt-bindings: counter: ftm-quaddec
From: Arnout Vandecappelle @ 2019-03-26 15:43 UTC (permalink / raw)
To: Patrick Havelange, Jonathan Cameron, Rob Herring
Cc: Mark Rutland, devicetree, linuxppc-dev, linux-pwm, linux-iio,
linux-kernel, Daniel Lezcano, William Breathitt Gray, Li Yang,
Thierry Reding, linux-arm-kernel, Thomas Gleixner, Shawn Guo,
Esben Haabendal
In-Reply-To: <20190316142138.65860d88@archlinux>
[Full disclosure: I'm a colleague of Patrick.]
On 2019-03-16 14:21:38, Jonathan Cameron wrote:
> On Tue, 12 Mar 2019 14:09:52 -0500
> Rob Herring <robh@kernel.org> wrote:
>
> > On Wed, Mar 06, 2019 at 12:12:05PM +0100, Patrick Havelange wrote:
> > > FlexTimer quadrature decoder driver.
> > >
> > > Signed-off-by: Patrick Havelange <patrick.havelange@essensium.com>
> > > Reviewed-by: Esben Haabendal <esben@haabendal.dk>
> > > ---
> > > Changes v2
> > > - None
> > > ---
> > > .../bindings/counter/ftm-quaddec.txt | 18 ++++++++++++++++++
> > > 1 file changed, 18 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/counter/ftm-quaddec.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/counter/ftm-quaddec.txt \
> > > b/Documentation/devicetree/bindings/counter/ftm-quaddec.txt new file mode 100644
> > > index 000000000000..4d18cd722074
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/counter/ftm-quaddec.txt
> > > @@ -0,0 +1,18 @@
> > > +FlexTimer Quadrature decoder counter
> > > +
> > > +This driver exposes a simple counter for the quadrature decoder mode.
> >
> > Seems like this is more a mode of a h/w block than describing a h/w
> > block. Bindings should do the latter.
As Jonathan writes below, it really is a "hardware mode", since it is tied
very closely to how the device is wired up.
Basically, the same block can be used for pretty diverse functions: a PWM where
the pins are output, a counter where the pins are input, or a timer where the
interrupt or timer value is used purely internally.
This smells a bit like an MFD, but IMO it really isn't, because only one of the
functions can be enabled. So indeed, it's more like a mode.
> The snag is that we need to dig ourselves out of the hole set by:
> fsl,vf610-ftm-pwm etc.
>
> Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt
> Documentation/devicetree/bindings/timer/fsl,ftm-timer.txt
> (I'm assuming these are the same IP block).
>
> Can probably be sorted out though. One core driver binds against the
> ftm and deals with instantiating the others depending on the configuration
> (note that this mode for instance does make sense in DT as it's
> really reflecting the fact there is a quadrature encoder
> connected to the ftm).
>
> Fiddly though :)
The way I see it, there are 3 ways this could be modelled (other than the
current way: several nodes with the same address).
1. The SoC's .dtsi defines a single node, and the compatible string (which would
be defined by the board .dts) both enables the node and sets the compatible
string. The .dtsi would also define the static properties of all the different
functions: some "static" properties, e.g. reg, but also some function
specific-properties, e.g.#pwm-cells (only for PWM), interrupts (only for timer).
The driver (selected through compatible) anyway only uses the properties that it
needs, so it doesn't hurt to have those other-function properties there.
2. Like 1, but instead of defining the compatible string in the .dts, use a
single compatible string for all the different drivers which is set in the
.dtsi, and add a mode property (to be set in the .dts) to select the driver. The
selection can be done either by having a top-level driver that calls out to the
subsystem-specific one based on the mode, or by having each driver bail out of
its probe function if the mode is not as expected.
3. Have a common node that essentially does nothing except occupy the memory
resource, and sub-nodes for each function. This can again be combined with a
common driver that does the common resource allocation, or each function driver
can just look at its parent node to find the resources. A disadvantage of this
one is that it is possible to enable several functions in the DT, while only one
can actually work.
Option 3 is what is used for e.g. stm32-lptimer. It also uses an mfd driver to
model the common part. But possibly it actually allows the different functions
to operate simultaneously.
Option 3 has the additional disadvantage that it requires changes in existing
DTs for ftm-pwm and ftm-timer, because some properties are moved one level down.
Since we need to retain backward compatibility, we'd need to look for those
properties both in the node itself and in the parent node. In particular, the
common driver part would be fairly complicated to implement in a backward
compatible way because it's not enough to do a simple devm_of_platform_populate().
Personally I don't like the common driver part too much. This common driver
does almost nothing (iomap and clock) and it creates dependencies between
different drivers. Combined with the backward compatibility problem, I don't see
much point to it.
I personally like option 2 the most. It's easy to be backward compatible (if
mode is not set, revert to the current behaviour, i.e. assume that the
compatible string has encoded the mode and that you're the only driver). It
doesn't introduce subnodes that have no hardware equivalent. The only messy
thing about it is that properties belonging to the different modes are mixed
together in a single node. And also, I don't think this kind of model is
currently used anywhere else in the kernel.
Regards,
Arnout
^ permalink raw reply
* Re: [PATCH] powerpc: vmlinux.lds: Drop Binutils 2.18 workarounds
From: Christophe Leroy @ 2019-03-26 19:28 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev, Joel Stanley
In-Reply-To: <20190326181914.GS3969@gate.crashing.org>
Le 26/03/2019 à 19:19, Segher Boessenkool a écrit :
> On Tue, Mar 26, 2019 at 07:55:33AM +0000, Christophe Leroy wrote:
>> STACK off 0x00000000 vaddr 0x00000000 paddr 0x00000000 align 2**4
>> filesz 0x00000000 memsz 0x00000000 flags rwx
>
> You need to prevent this one somehow. What object file forces this?
mpc885_ads_defconfig
Christophe
>
> Setting the program headers manually works of course, but that also hides
> other problems.
>
>
> Segher
>
^ permalink raw reply
* Re: [PATCH RFC v4 07/21] PCI: Wake up bridges during rescan when movable BARs enabled
From: Bjorn Helgaas @ 2019-03-26 19:28 UTC (permalink / raw)
To: Sergey Miroshnichenko; +Cc: linux-pci, linuxppc-dev, linux
In-Reply-To: <20190311133122.11417-8-s.miroshnichenko@yadro.com>
On Mon, Mar 11, 2019 at 04:31:08PM +0300, Sergey Miroshnichenko wrote:
> Use the PM runtime methods to wake up the bridges before accessing
> their config space.
>
> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
> ---
> drivers/pci/probe.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 88350dd56344..dc935f82a595 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -3252,6 +3252,8 @@ static void pci_bus_rescan_prepare(struct pci_bus *bus)
> {
> struct pci_dev *dev;
>
> + pm_runtime_get_sync(&bus->dev);
This should be part of the patch that adds the config space access
so we can tell specifically what code requires the wakeup.
> list_for_each_entry(dev, &bus->devices, bus_list) {
> struct pci_bus *child = dev->subordinate;
>
> @@ -3278,6 +3280,8 @@ static void pci_bus_rescan_done(struct pci_bus *bus)
> dev->driver->rescan_done(dev);
> }
> }
> +
> + pm_runtime_put(&bus->dev);
> }
>
> /**
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH RFC v4 05/21] PCI: hotplug: Add a flag for the movable BARs feature
From: Bjorn Helgaas @ 2019-03-26 19:24 UTC (permalink / raw)
To: Sergey Miroshnichenko; +Cc: linux-pci, linuxppc-dev, linux
In-Reply-To: <20190311133122.11417-6-s.miroshnichenko@yadro.com>
On Mon, Mar 11, 2019 at 04:31:06PM +0300, Sergey Miroshnichenko wrote:
> If a new PCIe device has been hot-plugged between the two active ones
> without big enough gap between their BARs,
Just to speak precisely here, a hot-added device is not "between" two
active ones because the new device has zeros in its BARs.
BARs from different devices can be interleaved arbitrarily, subject to
bridge window constraints, so we can really only speak about a *BAR*
(not the entire device) being between two other BARs.
Also, I don't think there's anything here that is PCIe-specific, so we
should talk about "PCI", not "PCIe".
> these BARs should be moved
> if their drivers support this feature. The drivers should be notified
> and paused during the procedure:
>
> 1) dev 8 (new)
> |
> v
> .. | dev 3 | dev 3 | dev 5 | dev 7 |
> .. | BAR 0 | BAR 1 | BAR 0 | BAR 0 |
>
> 2) dev 8
> |
> v
> .. | dev 3 | dev 3 | --> --> | dev 5 | dev 7 |
> .. | BAR 0 | BAR 1 | --> --> | BAR 0 | BAR 0 |
>
> 3)
>
> .. | dev 3 | dev 3 | dev 8 | dev 8 | dev 5 | dev 7 |
> .. | BAR 0 | BAR 1 | BAR 0 | BAR 1 | BAR 0 | BAR 0 |
>
> Thus, prior reservation of memory regions by BIOS/bootloader/firmware
> is not required anymore for the PCIe hotplug.
>
> The PCI_MOVABLE_BARS flag is set by the platform is this feature is
> supported and tested, but can be overridden by the following command
> line option:
> pcie_movable_bars={ off | force }
A chicken switch to turn this functionality off is OK, but I think it
should be enabled by default. There isn't anything about this that's
platform-specific, is there?
> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
> ---
> .../admin-guide/kernel-parameters.txt | 7 ++++++
> drivers/pci/pci.c | 24 +++++++++++++++++++
> include/linux/pci.h | 2 ++
> 3 files changed, 33 insertions(+)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 2b8ee90bb644..d40eaf993f80 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3417,6 +3417,13 @@
> nomsi Do not use MSI for native PCIe PME signaling (this makes
> all PCIe root ports use INTx for all services).
>
> + pcie_movable_bars=[PCIE]
> + Override the movable BARs support detection:
> + off
> + Disable even if supported by the platform
> + force
> + Enable even if not explicitly declared as supported
> +
> pcmv= [HW,PCMCIA] BadgePAD 4
>
> pd_ignore_unused
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 69898fe5255e..4dac49a887ec 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -139,6 +139,30 @@ static int __init pcie_port_pm_setup(char *str)
> }
> __setup("pcie_port_pm=", pcie_port_pm_setup);
>
> +static bool pcie_movable_bars_off;
> +static bool pcie_movable_bars_force;
> +static int __init pcie_movable_bars_setup(char *str)
> +{
> + if (!strcmp(str, "off"))
> + pcie_movable_bars_off = true;
> + else if (!strcmp(str, "force"))
> + pcie_movable_bars_force = true;
> + return 1;
> +}
> +__setup("pcie_movable_bars=", pcie_movable_bars_setup);
> +
> +bool pci_movable_bars_enabled(void)
> +{
> + if (pcie_movable_bars_off)
> + return false;
> +
> + if (pcie_movable_bars_force)
> + return true;
> +
> + return pci_has_flag(PCI_MOVABLE_BARS);
> +}
> +EXPORT_SYMBOL(pci_movable_bars_enabled);
> +
> /* Time to wait after a reset for device to become responsive */
> #define PCIE_RESET_READY_POLL_MS 60000
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index cb2760a31fe2..cbe661aff9f5 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -866,6 +866,7 @@ enum {
> PCI_ENABLE_PROC_DOMAINS = 0x00000010, /* Enable domains in /proc */
> PCI_COMPAT_DOMAIN_0 = 0x00000020, /* ... except domain 0 */
> PCI_SCAN_ALL_PCIE_DEVS = 0x00000040, /* Scan all, not just dev 0 */
> + PCI_MOVABLE_BARS = 0x00000080, /* Runtime BAR reassign after hotplug */
> };
>
> /* These external functions are only available when PCI support is enabled */
> @@ -1345,6 +1346,7 @@ unsigned char pci_bus_max_busnr(struct pci_bus *bus);
> void pci_setup_bridge(struct pci_bus *bus);
> resource_size_t pcibios_window_alignment(struct pci_bus *bus,
> unsigned long type);
> +bool pci_movable_bars_enabled(void);
>
> #define PCI_VGA_STATE_CHANGE_BRIDGE (1 << 0)
> #define PCI_VGA_STATE_CHANGE_DECODES (1 << 1)
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH RFC v4 03/21] PCI: Enable bridge's I/O and MEM access for hotplugged devices
From: Bjorn Helgaas @ 2019-03-26 19:13 UTC (permalink / raw)
To: Sergey Miroshnichenko; +Cc: linux-pci, linuxppc-dev, linux
In-Reply-To: <20190311133122.11417-4-s.miroshnichenko@yadro.com>
On Mon, Mar 11, 2019 at 04:31:04PM +0300, Sergey Miroshnichenko wrote:
> After updating the bridge window resources, the PCI_COMMAND_IO and
> PCI_COMMAND_MEMORY bits of the bridge must be addressed as well.
>
> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
> ---
> drivers/pci/pci.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 895201d4c9e6..69898fe5255e 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1622,6 +1622,14 @@ static void pci_enable_bridge(struct pci_dev *dev)
> pci_enable_bridge(bridge);
>
> if (pci_is_enabled(dev)) {
> + int i, bars = 0;
> +
> + for (i = PCI_BRIDGE_RESOURCES; i < DEVICE_COUNT_RESOURCE; i++) {
> + if (dev->resource[i].flags & (IORESOURCE_MEM | IORESOURCE_IO))
> + bars |= (1 << i);
> + }
> + do_pci_enable_device(dev, bars);
In what situation is this needed, exactly? This code already exists
in pci_enable_device_flags(). Why isn't that enough?
I guess maybe there's some case where we enable the bridge, then
assign bridge windows, then enable a downstream device?
Does this fix a bug with current hotplug?
> if (!dev->is_busmaster)
> pci_set_master(dev);
> mutex_unlock(&dev->enable_mutex);
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH RFC v4 02/21] PCI: Fix race condition in pci_enable/disable_device()
From: Bjorn Helgaas @ 2019-03-26 19:00 UTC (permalink / raw)
To: Sergey Miroshnichenko
Cc: Marta Rybczynska, linux-pci, linux-kernel, linux, Srinath Mannam,
linuxppc-dev
In-Reply-To: <20190311133122.11417-3-s.miroshnichenko@yadro.com>
[+cc Srinath, Marta, LKML]
On Mon, Mar 11, 2019 at 04:31:03PM +0300, Sergey Miroshnichenko wrote:
> CPU0 CPU1
>
> pci_enable_device_mem() pci_enable_device_mem()
> pci_enable_bridge() pci_enable_bridge()
> pci_is_enabled()
> return false;
> atomic_inc_return(enable_cnt)
> Start actual enabling the bridge
> ... pci_is_enabled()
> ... return true;
> ... Start memory requests <-- FAIL
> ...
> Set the PCI_COMMAND_MEMORY bit <-- Must wait for this
>
> This patch protects the pci_enable/disable_device() and pci_enable_bridge()
> with mutexes.
This is a subtle issue that we've tried to fix before, but we've never
had a satisfactory solution, so I hope you've figured out the right
fix.
I'll include some links to previous discussion. This patch is very
similar to [2], which we didn't actually apply. We did apply the
patch from [3] as 40f11adc7cd9 ("PCI: Avoid race while enabling
upstream bridges"), but it caused the regressions reported in [4,5],
so we reverted it with 0f50a49e3008 ("Revert "PCI: Avoid race while
enabling upstream bridges"").
I think the underlying design problem is that we have a driver for
device B calling pci_enable_device(), and it is changing the state of
device A (an upstream bridge). The model generally is that a driver
should only touch the device it is bound to.
It's tricky to get the locking right when several children of device A
all need to operate on A.
That's all to say I'll have to think carefully about this particular
patch, so I'll go on to the others and come back to this one.
Bjorn
[1] https://lore.kernel.org/linux-pci/1494256190-28993-1-git-send-email-srinath.mannam@broadcom.com/T/#u
[RFC PATCH] pci: Concurrency issue in NVMe Init through PCIe switch
[2] https://lore.kernel.org/linux-pci/1496135297-19680-1-git-send-email-srinath.mannam@broadcom.com/T/#u
[RFC PATCH v2] pci: Concurrency issue in NVMe Init through PCIe switch
[3] https://lore.kernel.org/linux-pci/1501858648-22228-1-git-send-email-srinath.mannam@broadcom.com/T/#u
[RFC PATCH v3] pci: Concurrency issue during pci enable bridge
[4] https://lore.kernel.org/linux-pci/150547971091.977464.16294045866179907260.stgit@buzz/T/#u
[PATCH bisected regression in 4.14] PCI: fix race while enabling upstream bridges concurrently
[5] https://lore.kernel.org/linux-wireless/04c9b578-693c-1dc6-9f0f-904580231b21@kernel.dk/T/#u
iwlwifi firmware load broken in current -git
[6] https://lore.kernel.org/linux-pci/744877924.5841545.1521630049567.JavaMail.zimbra@kalray.eu/T/#u
[RFC PATCH] nvme: avoid race-conditions when enabling devices
> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
> ---
> drivers/pci/pci.c | 26 ++++++++++++++++++++++----
> drivers/pci/probe.c | 1 +
> include/linux/pci.h | 1 +
> 3 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index f006068be209..895201d4c9e6 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1615,6 +1615,8 @@ static void pci_enable_bridge(struct pci_dev *dev)
> struct pci_dev *bridge;
> int retval;
>
> + mutex_lock(&dev->enable_mutex);
> +
> bridge = pci_upstream_bridge(dev);
> if (bridge)
> pci_enable_bridge(bridge);
> @@ -1622,6 +1624,7 @@ static void pci_enable_bridge(struct pci_dev *dev)
> if (pci_is_enabled(dev)) {
> if (!dev->is_busmaster)
> pci_set_master(dev);
> + mutex_unlock(&dev->enable_mutex);
> return;
> }
>
> @@ -1630,11 +1633,14 @@ static void pci_enable_bridge(struct pci_dev *dev)
> pci_err(dev, "Error enabling bridge (%d), continuing\n",
> retval);
> pci_set_master(dev);
> + mutex_unlock(&dev->enable_mutex);
> }
>
> static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
> {
> struct pci_dev *bridge;
> + /* Enable-locking of bridges is performed within the pci_enable_bridge() */
> + bool need_lock = !dev->subordinate;
> int err;
> int i, bars = 0;
>
> @@ -1650,8 +1656,13 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
> dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK);
> }
>
> - if (atomic_inc_return(&dev->enable_cnt) > 1)
> + if (need_lock)
> + mutex_lock(&dev->enable_mutex);
> + if (pci_is_enabled(dev)) {
> + if (need_lock)
> + mutex_unlock(&dev->enable_mutex);
> return 0; /* already enabled */
> + }
>
> bridge = pci_upstream_bridge(dev);
> if (bridge)
> @@ -1666,8 +1677,10 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
> bars |= (1 << i);
>
> err = do_pci_enable_device(dev, bars);
> - if (err < 0)
> - atomic_dec(&dev->enable_cnt);
> + if (err >= 0)
> + atomic_inc(&dev->enable_cnt);
> + if (need_lock)
> + mutex_unlock(&dev->enable_mutex);
> return err;
> }
>
> @@ -1910,15 +1923,20 @@ void pci_disable_device(struct pci_dev *dev)
> if (dr)
> dr->enabled = 0;
>
> + mutex_lock(&dev->enable_mutex);
> dev_WARN_ONCE(&dev->dev, atomic_read(&dev->enable_cnt) <= 0,
> "disabling already-disabled device");
>
> - if (atomic_dec_return(&dev->enable_cnt) != 0)
> + if (atomic_dec_return(&dev->enable_cnt) != 0) {
> + mutex_unlock(&dev->enable_mutex);
> return;
> + }
>
> do_pci_disable_device(dev);
>
> dev->is_busmaster = 0;
> +
> + mutex_unlock(&dev->enable_mutex);
> }
> EXPORT_SYMBOL(pci_disable_device);
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 2ec0df04e0dc..977a127ce791 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2267,6 +2267,7 @@ struct pci_dev *pci_alloc_dev(struct pci_bus *bus)
> INIT_LIST_HEAD(&dev->bus_list);
> dev->dev.type = &pci_dev_type;
> dev->bus = pci_bus_get(bus);
> + mutex_init(&dev->enable_mutex);
>
> return dev;
> }
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 77448215ef5b..cb2760a31fe2 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -419,6 +419,7 @@ struct pci_dev {
> unsigned int no_vf_scan:1; /* Don't scan for VFs after IOV enablement */
> pci_dev_flags_t dev_flags;
> atomic_t enable_cnt; /* pci_enable_device has been called */
> + struct mutex enable_mutex;
>
> u32 saved_config_space[16]; /* Config space saved at suspend time */
> struct hlist_head saved_cap_space;
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH v3] powerpc/64: Fix memcmp reading past the end of src/dest
From: Segher Boessenkool @ 2019-03-26 18:39 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, chandan
In-Reply-To: <87va06ngds.fsf@concordia.ellerman.id.au>
On Tue, Mar 26, 2019 at 08:18:07PM +1100, Michael Ellerman wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
> > On Mon, Mar 25, 2019 at 11:33:56PM +1100, Michael Ellerman wrote:
> >> Segher Boessenkool <segher@kernel.crashing.org> writes:
> >> > On Fri, Mar 22, 2019 at 11:37:24PM +1100, Michael Ellerman wrote:
> >> >> + clrldi r6,r4,(64-12) // r6 = r4 & 0xfff
> >> >
> >> > You can just write
> >> > rlwinm r6,r4,0,0x0fff
> >>
> >> > if that is clearer? Or do you still want a comment with that :-)
> >>
> >> I don't think it's clearer doing a rotate of zero bits :)
> >>
> >> And yeah I'd probably still leave the comment, so I'm inclined to stick
> >> with the clrldi?
> >
> > I always have to think what the clrldi etc. do exactly, while with rlwinm
> > it is obvious. But yeah this may be different for other people who are
> > used to different idiom.
>
> Interesting, I'm the opposite. You know ppc assembler better than me so
> I guess I just need to spend more time on it and embrace the zen of the
> rotate instructions.
There is only one rlwinm instruction, but there are 9 extended mnemonics
for it. (And similarly for the 64-bit ops, where there are 3 basic
mnemonics but 9 extended mnemonics). Of course I use {s,rot}{l,r}{w,d}i,
those are actually *simpler* than rlwinm / rldic{,l,r}, but not the other
stuff.
It may also be because the disassembler doesn't show these things. Dunno.
> I'm not sure it's vastly more hostile though than `andi. 6,4,0xffff`
> being valid but `andi. 6,4,0x1ffff` being not valid.
But that is the same for *all* UIMM insns.
> Yeah, I guess a new `andi` instruction is the only real answer :)
Too bad there is no space in the opcode map for it, already not in the
original POWER architecture (where this is "andil.") :-/
Segher
^ permalink raw reply
* Re: [PATCH] powerpc: vmlinux.lds: Drop Binutils 2.18 workarounds
From: Segher Boessenkool @ 2019-03-26 18:19 UTC (permalink / raw)
To: Christophe Leroy; +Cc: linuxppc-dev, Joel Stanley
In-Reply-To: <d340ae4b-a009-0ef3-9621-da3e0a7684f5@c-s.fr>
On Tue, Mar 26, 2019 at 07:55:33AM +0000, Christophe Leroy wrote:
> STACK off 0x00000000 vaddr 0x00000000 paddr 0x00000000 align 2**4
> filesz 0x00000000 memsz 0x00000000 flags rwx
You need to prevent this one somehow. What object file forces this?
Setting the program headers manually works of course, but that also hides
other problems.
Segher
^ permalink raw reply
* [PATCH v2] kbuild: strip whitespace in cmd_record_mcount findstring
From: Joe Lawrence @ 2019-03-26 17:33 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Michal Marek, Linux Kbuild mailing list,
Linux Kernel Mailing List, Steven Rostedt, Nicholas Piggin,
linuxppc-dev
In-Reply-To: <CAK7LNARyD=tG0JhwN_mjdyDRSPAF8UoZKvqAT3Ah_v7GfymCPg@mail.gmail.com>
On Tue, Mar 26, 2019 at 02:29:47PM +0900, Masahiro Yamada wrote:
> On Tue, Mar 26, 2019 at 1:05 AM Joe Lawrence <joe.lawrence@redhat.com> wrote:
> >
> > CC_FLAGS_FTRACE may contain trailing whitespace that interferes with
> > findstring.
> >
> > For example, commit 6977f95e63b9 ("powerpc: avoid -mno-sched-epilog on
> > GCC 4.9 and newer") introduced a change such that on my ppc64le box,
> > CC_FLAGS_FTRACE="-pg -mprofile-kernel ". (Note the trailing space.)
> > When cmd_record_mcount is now invoked, findstring fails as the ftrace
> > flags were found at very end of _c_flags, without the trailing space.
> >
> > _c_flags=" ... -pg -mprofile-kernel"
> > CC_FLAGS_FTRACE="-pg -mprofile-kernel "
> > ^
> > findstring is looking for this extra space
> >
> > Remove the redundant whitespaces from CC_FLAGS_FTRACE in
> > cmd_record_mcount to avoid this problem.
> >
> > Fixes: 6977f95e63b9 ("powerpc: avoid -mno-sched-epilog on GCC 4.9 and newer").
> > Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> > ---
> >
> > Standard disclaimer: I'm not a kbuild expert, but this works around the
> > problem I reported where ftrace and livepatch self-tests were failing as
> > specified object files were not run through the recordmcount.pl script:
> >
> > ppc64le: ftrace self-tests and $(CC_FLAGS_FTRACE) broken?
> > https://lists.ozlabs.org/pipermail/linuxppc-dev/2019-March/187298.html
> >
> > scripts/Makefile.build | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> > index 2554a15ecf2b..74d402b5aa3c 100644
> > --- a/scripts/Makefile.build
> > +++ b/scripts/Makefile.build
> > @@ -199,10 +199,10 @@ sub_cmd_record_mcount = perl $(srctree)/scripts/recordmcount.pl "$(ARCH)" \
> > "$(if $(part-of-module),1,0)" "$(@)";
> > recordmcount_source := $(srctree)/scripts/recordmcount.pl
> > endif # BUILD_C_RECORDMCOUNT
> > -cmd_record_mcount = \
> > - if [ "$(findstring $(CC_FLAGS_FTRACE),$(_c_flags))" = \
> > - "$(CC_FLAGS_FTRACE)" ]; then \
> > - $(sub_cmd_record_mcount) \
> > +cmd_record_mcount = \
> > + if [ "$(findstring $(strip $(CC_FLAGS_FTRACE)),$(_c_flags))" = \
> > + "$(strip $(CC_FLAGS_FTRACE))" ]; then \
> > + $(sub_cmd_record_mcount) \
> > fi
> > endif # CC_USING_RECORD_MCOUNT
> > endif # CONFIG_FTRACE_MCOUNT_RECORD
> > --
> > 2.20.1
> >
>
>
>
> I do not see a point in using the shell command here
> in the first place.
>
> Instead of adding crappy workarounds,
> I guess the following simple code should work:
>
>
> index 2554a15..5f13021 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -199,11 +199,8 @@ sub_cmd_record_mcount = perl
> $(srctree)/scripts/recordmcount.pl "$(ARCH)" \
> "$(if $(part-of-module),1,0)" "$(@)";
> recordmcount_source := $(srctree)/scripts/recordmcount.pl
> endif # BUILD_C_RECORDMCOUNT
> -cmd_record_mcount = \
> - if [ "$(findstring $(CC_FLAGS_FTRACE),$(_c_flags))" = \
> - "$(CC_FLAGS_FTRACE)" ]; then \
> - $(sub_cmd_record_mcount) \
> - fi
> +cmd_record_mcount = $(if $(findstring $(CC_FLAGS_FTRACE),$(_c_flags)),\
> + $(sub_cmd_record_mcount))
> endif # CC_USING_RECORD_MCOUNT
> endif # CONFIG_FTRACE_MCOUNT_RECORD
>
Hi Masahiro,
Agreed on the shell command ugliness, however I still think we need to
strip the search pattern here. With your suggestion:
% rm -f kernel/trace/trace_selftest_dynamic.o
% make kernel/trace/trace_selftest_dynamic.o
CALL scripts/checksyscalls.sh
CALL scripts/atomic/check-atomics.sh
CC kernel/trace/trace_selftest_dynamic.o
% eu-readelf --sections kernel/trace/trace_selftest_dynamic.o | grep mcount
(nothing)
Adding it back as, as below, restores those sections and the self tests
work again.
Regards,
-- Joe
-->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
From 6a85e8ecf4179b3e80601a327ec43d8d49f0e3cd Mon Sep 17 00:00:00 2001
From: Joe Lawrence <joe.lawrence@redhat.com>
Date: Tue, 26 Mar 2019 10:50:28 -0400
Subject: [PATCH v2] kbuild: strip whitespace in cmd_record_mcount findstring
CC_FLAGS_FTRACE may contain trailing whitespace that interferes with
findstring.
For example, commit 6977f95e63b9 ("powerpc: avoid -mno-sched-epilog on
GCC 4.9 and newer") introduced a change such that on my ppc64le box,
CC_FLAGS_FTRACE="-pg -mprofile-kernel ". (Note the trailing space.)
When cmd_record_mcount is now invoked, findstring fails as the ftrace
flags were found at very end of _c_flags, without the trailing space.
_c_flags=" ... -pg -mprofile-kernel"
CC_FLAGS_FTRACE="-pg -mprofile-kernel "
^
findstring is looking for this extra space
Remove the redundant whitespaces from CC_FLAGS_FTRACE in
cmd_record_mcount to avoid this problem.
Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com> (refactoring)
Fixes: 6977f95e63b9 ("powerpc: avoid -mno-sched-epilog on GCC 4.9 and newer").
Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
---
scripts/Makefile.build | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 2554a15ecf2b..76ca30cc4791 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -199,11 +199,8 @@ sub_cmd_record_mcount = perl $(srctree)/scripts/recordmcount.pl "$(ARCH)" \
"$(if $(part-of-module),1,0)" "$(@)";
recordmcount_source := $(srctree)/scripts/recordmcount.pl
endif # BUILD_C_RECORDMCOUNT
-cmd_record_mcount = \
- if [ "$(findstring $(CC_FLAGS_FTRACE),$(_c_flags))" = \
- "$(CC_FLAGS_FTRACE)" ]; then \
- $(sub_cmd_record_mcount) \
- fi
+cmd_record_mcount = $(if $(findstring $(strip $(CC_FLAGS_FTRACE)),$(_c_flags)), \
+ $(sub_cmd_record_mcount))
endif # CC_USING_RECORD_MCOUNT
endif # CONFIG_FTRACE_MCOUNT_RECORD
--
2.20.1
^ permalink raw reply related
* [PATCH v2 1/2] PCI/AER: Do not use APEI/HEST to disable AER services globally
From: Alexandru Gagniuc @ 2019-03-26 17:23 UTC (permalink / raw)
To: bhelgaas
Cc: alex_gagniuc, Sam Bobroff, Rafael J. Wysocki, Shyam_Iyer, okaya,
linux-kernel, linux-pci, keith.busch, linux-acpi, lukas,
leo.duran, Alexandru Gagniuc, scott.faasse, Oliver O'Halloran,
austin_bolen, linuxppc-dev, Len Brown
In-Reply-To: <20190326172343.28946-1-mr.nuke.me@gmail.com>
As part of the ACPI Platform Error Interfaces (APEI), the HEST table
describes the meaning of errors sources. Although HEST is related to
ownership of AER, the gatekeeper for AER ownership is the _OSC method.
HEST can identify error sources as firmware-first with granularity
ranging from device-level to global. It's not uncommon for HEST to
say "all AER errors sent via of APEI are firmware-first" by setting
the firmware-first and global bits. It's still allowable to do this
and grant the OS AER control over part of the PCIe topology.
Because there is quite some flexibility in how HEST and _OSC can
interact, it is wrong to assume that global firmware-first implies
_OSC will never grant AER control. If we don't ask for control, we are
not going to get it, and we may have entire parts of the PCIe tree
that do not report errors. Thus, ask for AER ownership, regardless of
HEST.
Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
drivers/acpi/pci_root.c | 9 ++-------
drivers/pci/pcie/aer.c | 25 +------------------------
include/linux/pci-acpi.h | 6 ------
3 files changed, 3 insertions(+), 37 deletions(-)
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 707aafc7c2aa..32b2053bb0fa 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -491,13 +491,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
control |= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
- if (pci_aer_available()) {
- if (aer_acpi_firmware_first())
- dev_info(&device->dev,
- "PCIe AER handled by firmware\n");
- else
- control |= OSC_PCI_EXPRESS_AER_CONTROL;
- }
+ if (pci_aer_available())
+ control |= OSC_PCI_EXPRESS_AER_CONTROL;
requested = control;
status = acpi_pci_osc_control_set(handle, &control,
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index f8fc2114ad39..d029979e61f6 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -313,29 +313,6 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev)
return dev->__aer_firmware_first;
}
-static bool aer_firmware_first;
-
-/**
- * aer_acpi_firmware_first - Check if APEI should control AER.
- */
-bool aer_acpi_firmware_first(void)
-{
- static bool parsed = false;
- struct aer_hest_parse_info info = {
- .pci_dev = NULL, /* Check all PCIe devices */
- .firmware_first = 0,
- };
-
- if (pcie_ports_native)
- return false;
-
- if (!parsed) {
- apei_hest_parse(aer_hest_parse, &info);
- aer_firmware_first = info.firmware_first;
- parsed = true;
- }
- return aer_firmware_first;
-}
#endif
#define PCI_EXP_AER_FLAGS (PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \
@@ -1450,7 +1427,7 @@ static struct pcie_port_service_driver aerdriver = {
*/
int __init pcie_aer_init(void)
{
- if (!pci_aer_available() || aer_acpi_firmware_first())
+ if (!pci_aer_available())
return -ENXIO;
return pcie_port_service_register(&aerdriver);
}
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 8082b612f561..2e9c0b973eba 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -116,10 +116,4 @@ static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
#endif /* CONFIG_ACPI */
-#ifdef CONFIG_ACPI_APEI
-extern bool aer_acpi_firmware_first(void);
-#else
-static inline bool aer_acpi_firmware_first(void) { return false; }
-#endif
-
#endif /* _PCI_ACPI_H_ */
--
2.19.2
^ permalink raw reply related
* [PATCH v2 0/2] PCI/AER: Consistently use _OSC to determine who owns AER
From: Alexandru Gagniuc @ 2019-03-26 17:23 UTC (permalink / raw)
To: bhelgaas
Cc: alex_gagniuc, Sam Bobroff, Rafael J. Wysocki, Shyam_Iyer, okaya,
linux-kernel, linux-pci, keith.busch, linux-acpi, lukas,
leo.duran, Alexandru Gagniuc, scott.faasse, Oliver O'Halloran,
austin_bolen, linuxppc-dev, Len Brown
This started as a nudge from Keith, who pointed out that it doesn't make sense
to disable AER services when only one device has a FIRMWARE_FIRST HEST.
I won't re-phrase the points in the original patch [1]. The patch started a
long discussion in the ACPI Software Working Group (ASWG). The nearly unanimous
conclusion is that my original interpretation is correct.
I'd like to quote one of the tables that was produced as part of that
conversation:
(_OSC AER Control, HEST AER Structure FFS) = (0, 0)
* OSPM is prevented from writing to the PCI Express AER registers.
* OSPM has no guidance on how AER errors are being handled – but it
does know that it is not in control of AER registers. PCI-e errors
that make it to the OS (via NMI, etc) would be treated as spurious
since access to the AER registers isn’t allowed for proper sourcing.
(_OSC AER Control, HEST AER Structure FFS) = (0, 1)
* OSPM is prevented from writing to the PCI Express AER registers.
* OSPM is being given guidance that Firmware is handling AER errors and
those interrupts are routed to the platform. Firmware may pass along
error information via GHES
(_OSC AER Control, HEST AER Structure FFS) = (0, Does not exist)
* OSPM is prevented from writing to the PCI Express AER registers.
* OSPM has no guidance on how AER errors are being handled – but it
does know that it is not in control of AER registers. PCI-e errors
that make it to the OS (via NMI, etc) would be treated as spurious
since access to the AER registers isn’t allowed for proper sourcing.
(_OSC AER Control, HEST AER Structure FFS) = (1, 0)
* OSPM is in control of writing to the PCI Express AER registers.
* OSPM is being given guidance that AER errors will interrupt the OS
directly and that the OS is expected to handle all AER capability
structure read/clears for the devices with this attribute (or all if
the Global Bit is set.)
(_OSC AER Control, HEST AER Structure FFS) = (1, 1)
* OSPM is in control of writing to the PCI Express AER registers.
* OSPM is being given guidance that although OS is in control of AER
read/writes – the actual interrupt is being routed to the platform
first.
* Subsequent fields with masks/enables should be performed by the OS
during initialization on behalf of firmware. These are to be honoured
in this mode because with FF, the firmware needs to be able to handle
the errors it expects and not be given errors it was not expecting to
handle.
* Firmware may pass along error information via GHES, or generate an OS
interrupt and allow the OS to interrogate AER status directly via the
AER capability structures.
(_OSC AER Control, HEST AER Structure FFS) = (0, Does not exist)
* OSPM is in control of writing to the PCI Express AER registers.
* OSPM has no guidance from the platform and is in complete control of
AER error handling.
There may be one caveat. Someone mentioned in the original discussions that
there may exist machines which make the assumption that HEST is authoritative,
but did not identify any such machine. We should keep in mind that they may
require a quirk.
Alex
[1] https://lkml.org/lkml/2018/11/16/202
Changes since v1:
* Started 6-month conversation in ASWG
* Re-phrased commit message to reflect some of the points in ASWG discussion
Alexandru Gagniuc (2):
PCI/AER: Do not use APEI/HEST to disable AER services globally
PCI/AER: Determine AER ownership based on _OSC instead of HEST
drivers/acpi/pci_root.c | 9 +----
drivers/pci/pcie/aer.c | 82 ++--------------------------------------
include/linux/pci-acpi.h | 6 ---
3 files changed, 5 insertions(+), 92 deletions(-)
--
2.19.2
^ permalink raw reply
* [PATCH v2 2/2] PCI/AER: Determine AER ownership based on _OSC instead of HEST
From: Alexandru Gagniuc @ 2019-03-26 17:23 UTC (permalink / raw)
To: bhelgaas
Cc: alex_gagniuc, Sam Bobroff, Rafael J. Wysocki, Shyam_Iyer, okaya,
linux-kernel, linux-pci, keith.busch, linux-acpi, lukas,
leo.duran, Alexandru Gagniuc, scott.faasse, Oliver O'Halloran,
austin_bolen, linuxppc-dev, Len Brown
In-Reply-To: <20190326172343.28946-1-mr.nuke.me@gmail.com>
HEST is used to describe the meaning of errors received as part of ACPI
Platform Error Interfaces (APEI), however the correct way to determine
AER ownership is the _OSC method.
The ACPI spec allows _OSC and HEST to say things that might not make
sense on a specific hardware implementation.
For example _OSC can say "OS has control" on the root bus, while HEST
says "This PCIe device is firmware-first". This is fine when the
platform can differentiate error sources at the root complex. On x86
platforms, AER errors can be routed to either to an MSI vector
(native AER), or to SMM (firmware-first). As this is done at the root
complex level, the example above would not make sense.
Notice that in neither case is it correct to go to HEST to determine
AER ownership. This is the conclusion of a six-months discussion in
the ACPI Software Working Group.
pci_dev->__aer_firmware_first is used to prevent modification of AER
registers when firmware owns AER. This is synonymous with the AER
ownership negotiated during _OSC. Thus _OSC is the correct way to
use to set this flag, not HEST.
Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
drivers/pci/pcie/aer.c | 57 ++----------------------------------------
1 file changed, 2 insertions(+), 55 deletions(-)
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index d029979e61f6..08cee30b3ef3 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -237,66 +237,13 @@ static inline bool hest_match_type(struct acpi_hest_header *hest_hdr,
return false;
}
-struct aer_hest_parse_info {
- struct pci_dev *pci_dev;
- int firmware_first;
-};
-
-static int hest_source_is_pcie_aer(struct acpi_hest_header *hest_hdr)
-{
- if (hest_hdr->type == ACPI_HEST_TYPE_AER_ROOT_PORT ||
- hest_hdr->type == ACPI_HEST_TYPE_AER_ENDPOINT ||
- hest_hdr->type == ACPI_HEST_TYPE_AER_BRIDGE)
- return 1;
- return 0;
-}
-
-static int aer_hest_parse(struct acpi_hest_header *hest_hdr, void *data)
-{
- struct aer_hest_parse_info *info = data;
- struct acpi_hest_aer_common *p;
- int ff;
-
- if (!hest_source_is_pcie_aer(hest_hdr))
- return 0;
-
- p = (struct acpi_hest_aer_common *)(hest_hdr + 1);
- ff = !!(p->flags & ACPI_HEST_FIRMWARE_FIRST);
-
- /*
- * If no specific device is supplied, determine whether
- * FIRMWARE_FIRST is set for *any* PCIe device.
- */
- if (!info->pci_dev) {
- info->firmware_first |= ff;
- return 0;
- }
- /* Otherwise, check the specific device */
- if (p->flags & ACPI_HEST_GLOBAL) {
- if (hest_match_type(hest_hdr, info->pci_dev))
- info->firmware_first = ff;
- } else
- if (hest_match_pci(p, info->pci_dev))
- info->firmware_first = ff;
-
- return 0;
-}
static void aer_set_firmware_first(struct pci_dev *pci_dev)
{
- int rc;
- struct aer_hest_parse_info info = {
- .pci_dev = pci_dev,
- .firmware_first = 0,
- };
+ struct pci_host_bridge *host = pci_find_host_bridge(pci_dev->bus);
- rc = apei_hest_parse(aer_hest_parse, &info);
-
- if (rc)
- pci_dev->__aer_firmware_first = 0;
- else
- pci_dev->__aer_firmware_first = info.firmware_first;
+ pci_dev->__aer_firmware_first = !host->native_aer;
pci_dev->__aer_firmware_first_valid = 1;
}
--
2.19.2
^ permalink raw reply related
* Re: [PATCH v6 04/19] powerpc: mm: Add p?d_large() definitions
From: Christophe Leroy @ 2019-03-26 16:58 UTC (permalink / raw)
To: Steven Price, linux-mm
Cc: Mark Rutland, x86, James Morse, Arnd Bergmann, Ard Biesheuvel,
Peter Zijlstra, Catalin Marinas, Dave Hansen, Will Deacon,
linux-kernel, kvm-ppc, Jérôme Glisse, Ingo Molnar,
Paul Mackerras, Andy Lutomirski, H. Peter Anvin, Borislav Petkov,
Thomas Gleixner, linuxppc-dev, linux-arm-kernel, Liang, Kan
In-Reply-To: <20190326162624.20736-5-steven.price@arm.com>
Le 26/03/2019 à 17:26, Steven Price a écrit :
> walk_page_range() is going to be allowed to walk page tables other than
> those of user space. For this it needs to know when it has reached a
> 'leaf' entry in the page tables. This information is provided by the
> p?d_large() functions/macros.
>
> For powerpc pmd_large() was already implemented, so hoist it out of the
> CONFIG_TRANSPARENT_HUGEPAGE condition and implement the other levels.
>
> Also since we now have a pmd_large always implemented we can drop the
> pmd_is_leaf() function.
Wouldn't it be better to drop the pmd_is_leaf() in a second patch ?
Christophe
>
> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> CC: Paul Mackerras <paulus@samba.org>
> CC: Michael Ellerman <mpe@ellerman.id.au>
> CC: linuxppc-dev@lists.ozlabs.org
> CC: kvm-ppc@vger.kernel.org
> Signed-off-by: Steven Price <steven.price@arm.com>
> ---
> arch/powerpc/include/asm/book3s/64/pgtable.h | 30 ++++++++++++++------
> arch/powerpc/kvm/book3s_64_mmu_radix.c | 12 ++------
> 2 files changed, 24 insertions(+), 18 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
> index 581f91be9dd4..f6d1ac8b832e 100644
> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
> @@ -897,6 +897,12 @@ static inline int pud_present(pud_t pud)
> return !!(pud_raw(pud) & cpu_to_be64(_PAGE_PRESENT));
> }
>
> +#define pud_large pud_large
> +static inline int pud_large(pud_t pud)
> +{
> + return !!(pud_raw(pud) & cpu_to_be64(_PAGE_PTE));
> +}
> +
> extern struct page *pud_page(pud_t pud);
> extern struct page *pmd_page(pmd_t pmd);
> static inline pte_t pud_pte(pud_t pud)
> @@ -940,6 +946,12 @@ static inline int pgd_present(pgd_t pgd)
> return !!(pgd_raw(pgd) & cpu_to_be64(_PAGE_PRESENT));
> }
>
> +#define pgd_large pgd_large
> +static inline int pgd_large(pgd_t pgd)
> +{
> + return !!(pgd_raw(pgd) & cpu_to_be64(_PAGE_PTE));
> +}
> +
> static inline pte_t pgd_pte(pgd_t pgd)
> {
> return __pte_raw(pgd_raw(pgd));
> @@ -1093,6 +1105,15 @@ static inline bool pmd_access_permitted(pmd_t pmd, bool write)
> return pte_access_permitted(pmd_pte(pmd), write);
> }
>
> +#define pmd_large pmd_large
> +/*
> + * returns true for pmd migration entries, THP, devmap, hugetlb
> + */
> +static inline int pmd_large(pmd_t pmd)
> +{
> + return !!(pmd_raw(pmd) & cpu_to_be64(_PAGE_PTE));
> +}
> +
> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> extern pmd_t pfn_pmd(unsigned long pfn, pgprot_t pgprot);
> extern pmd_t mk_pmd(struct page *page, pgprot_t pgprot);
> @@ -1119,15 +1140,6 @@ pmd_hugepage_update(struct mm_struct *mm, unsigned long addr, pmd_t *pmdp,
> return hash__pmd_hugepage_update(mm, addr, pmdp, clr, set);
> }
>
> -/*
> - * returns true for pmd migration entries, THP, devmap, hugetlb
> - * But compile time dependent on THP config
> - */
> -static inline int pmd_large(pmd_t pmd)
> -{
> - return !!(pmd_raw(pmd) & cpu_to_be64(_PAGE_PTE));
> -}
> -
> static inline pmd_t pmd_mknotpresent(pmd_t pmd)
> {
> return __pmd(pmd_val(pmd) & ~_PAGE_PRESENT);
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> index f55ef071883f..1b57b4e3f819 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_radix.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> @@ -363,12 +363,6 @@ static void kvmppc_pte_free(pte_t *ptep)
> kmem_cache_free(kvm_pte_cache, ptep);
> }
>
> -/* Like pmd_huge() and pmd_large(), but works regardless of config options */
> -static inline int pmd_is_leaf(pmd_t pmd)
> -{
> - return !!(pmd_val(pmd) & _PAGE_PTE);
> -}
> -
> static pmd_t *kvmppc_pmd_alloc(void)
> {
> return kmem_cache_alloc(kvm_pmd_cache, GFP_KERNEL);
> @@ -460,7 +454,7 @@ static void kvmppc_unmap_free_pmd(struct kvm *kvm, pmd_t *pmd, bool full,
> for (im = 0; im < PTRS_PER_PMD; ++im, ++p) {
> if (!pmd_present(*p))
> continue;
> - if (pmd_is_leaf(*p)) {
> + if (pmd_large(*p)) {
> if (full) {
> pmd_clear(p);
> } else {
> @@ -593,7 +587,7 @@ int kvmppc_create_pte(struct kvm *kvm, pgd_t *pgtable, pte_t pte,
> else if (level <= 1)
> new_pmd = kvmppc_pmd_alloc();
>
> - if (level == 0 && !(pmd && pmd_present(*pmd) && !pmd_is_leaf(*pmd)))
> + if (level == 0 && !(pmd && pmd_present(*pmd) && !pmd_large(*pmd)))
> new_ptep = kvmppc_pte_alloc();
>
> /* Check if we might have been invalidated; let the guest retry if so */
> @@ -662,7 +656,7 @@ int kvmppc_create_pte(struct kvm *kvm, pgd_t *pgtable, pte_t pte,
> new_pmd = NULL;
> }
> pmd = pmd_offset(pud, gpa);
> - if (pmd_is_leaf(*pmd)) {
> + if (pmd_large(*pmd)) {
> unsigned long lgpa = gpa & PMD_MASK;
>
> /* Check if we raced and someone else has set the same thing */
>
^ permalink raw reply
* Re: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted
From: Michael S. Tsirkin @ 2019-03-26 16:53 UTC (permalink / raw)
To: Christoph Hellwig
Cc: kevin.tian, Lorenzo.Pieralisi, tnowicki, Jean-Philippe Brucker,
Jason Wang, Alexey Kardashevskiy, Ram Pai, linux-kernel,
Will.Deacon, virtualization, eric.auger, iommu, Marc.Zyngier,
Robin.Murphy, bharat.bhushan, linuxppc-dev, joro,
Thiago Jung Bauermann, David Gibson
In-Reply-To: <20190130074427.GA29516@lst.de>
On Wed, Jan 30, 2019 at 08:44:27AM +0100, Christoph Hellwig wrote:
> On Tue, Jan 29, 2019 at 09:36:08PM -0500, Michael S. Tsirkin wrote:
> > This has been discussed ad nauseum. virtio is all about compatibility.
> > Losing a couple of lines of code isn't worth breaking working setups.
> > People that want "just use DMA API no tricks" now have the option.
> > Setting a flag in a feature bit map is literally a single line
> > of code in the hypervisor. So stop pushing for breaking working
> > legacy setups and just fix it in the right place.
>
> I agree with the legacy aspect. What I am missing is an extremely
> strong wording that says you SHOULD always set this flag for new
> hosts, including an explanation why.
So as far as power is concerned, IIUC the issue they are struggling with is
that some platforms do not support pass-through mode in the emulated IOMMU.
Disabling PLATFORM_ACCESS is so far a way around that, unfortunately just for
virtio devices. I would like virtio-iommu to be able to address that need as
well.
--
MST
^ permalink raw reply
* [PATCH v6 04/19] powerpc: mm: Add p?d_large() definitions
From: Steven Price @ 2019-03-26 16:26 UTC (permalink / raw)
To: linux-mm
Cc: Mark Rutland, Peter Zijlstra, Dave Hansen, Will Deacon,
Paul Mackerras, H. Peter Anvin, Liang, Kan, x86, Steven Price,
Ingo Molnar, Catalin Marinas, Arnd Bergmann, kvm-ppc,
Jérôme Glisse, Borislav Petkov, Andy Lutomirski,
Thomas Gleixner, linux-arm-kernel, Ard Biesheuvel, linux-kernel,
James Morse, linuxppc-dev
In-Reply-To: <20190326162624.20736-1-steven.price@arm.com>
walk_page_range() is going to be allowed to walk page tables other than
those of user space. For this it needs to know when it has reached a
'leaf' entry in the page tables. This information is provided by the
p?d_large() functions/macros.
For powerpc pmd_large() was already implemented, so hoist it out of the
CONFIG_TRANSPARENT_HUGEPAGE condition and implement the other levels.
Also since we now have a pmd_large always implemented we can drop the
pmd_is_leaf() function.
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: Paul Mackerras <paulus@samba.org>
CC: Michael Ellerman <mpe@ellerman.id.au>
CC: linuxppc-dev@lists.ozlabs.org
CC: kvm-ppc@vger.kernel.org
Signed-off-by: Steven Price <steven.price@arm.com>
---
arch/powerpc/include/asm/book3s/64/pgtable.h | 30 ++++++++++++++------
arch/powerpc/kvm/book3s_64_mmu_radix.c | 12 ++------
2 files changed, 24 insertions(+), 18 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
index 581f91be9dd4..f6d1ac8b832e 100644
--- a/arch/powerpc/include/asm/book3s/64/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
@@ -897,6 +897,12 @@ static inline int pud_present(pud_t pud)
return !!(pud_raw(pud) & cpu_to_be64(_PAGE_PRESENT));
}
+#define pud_large pud_large
+static inline int pud_large(pud_t pud)
+{
+ return !!(pud_raw(pud) & cpu_to_be64(_PAGE_PTE));
+}
+
extern struct page *pud_page(pud_t pud);
extern struct page *pmd_page(pmd_t pmd);
static inline pte_t pud_pte(pud_t pud)
@@ -940,6 +946,12 @@ static inline int pgd_present(pgd_t pgd)
return !!(pgd_raw(pgd) & cpu_to_be64(_PAGE_PRESENT));
}
+#define pgd_large pgd_large
+static inline int pgd_large(pgd_t pgd)
+{
+ return !!(pgd_raw(pgd) & cpu_to_be64(_PAGE_PTE));
+}
+
static inline pte_t pgd_pte(pgd_t pgd)
{
return __pte_raw(pgd_raw(pgd));
@@ -1093,6 +1105,15 @@ static inline bool pmd_access_permitted(pmd_t pmd, bool write)
return pte_access_permitted(pmd_pte(pmd), write);
}
+#define pmd_large pmd_large
+/*
+ * returns true for pmd migration entries, THP, devmap, hugetlb
+ */
+static inline int pmd_large(pmd_t pmd)
+{
+ return !!(pmd_raw(pmd) & cpu_to_be64(_PAGE_PTE));
+}
+
#ifdef CONFIG_TRANSPARENT_HUGEPAGE
extern pmd_t pfn_pmd(unsigned long pfn, pgprot_t pgprot);
extern pmd_t mk_pmd(struct page *page, pgprot_t pgprot);
@@ -1119,15 +1140,6 @@ pmd_hugepage_update(struct mm_struct *mm, unsigned long addr, pmd_t *pmdp,
return hash__pmd_hugepage_update(mm, addr, pmdp, clr, set);
}
-/*
- * returns true for pmd migration entries, THP, devmap, hugetlb
- * But compile time dependent on THP config
- */
-static inline int pmd_large(pmd_t pmd)
-{
- return !!(pmd_raw(pmd) & cpu_to_be64(_PAGE_PTE));
-}
-
static inline pmd_t pmd_mknotpresent(pmd_t pmd)
{
return __pmd(pmd_val(pmd) & ~_PAGE_PRESENT);
diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c
index f55ef071883f..1b57b4e3f819 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_radix.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c
@@ -363,12 +363,6 @@ static void kvmppc_pte_free(pte_t *ptep)
kmem_cache_free(kvm_pte_cache, ptep);
}
-/* Like pmd_huge() and pmd_large(), but works regardless of config options */
-static inline int pmd_is_leaf(pmd_t pmd)
-{
- return !!(pmd_val(pmd) & _PAGE_PTE);
-}
-
static pmd_t *kvmppc_pmd_alloc(void)
{
return kmem_cache_alloc(kvm_pmd_cache, GFP_KERNEL);
@@ -460,7 +454,7 @@ static void kvmppc_unmap_free_pmd(struct kvm *kvm, pmd_t *pmd, bool full,
for (im = 0; im < PTRS_PER_PMD; ++im, ++p) {
if (!pmd_present(*p))
continue;
- if (pmd_is_leaf(*p)) {
+ if (pmd_large(*p)) {
if (full) {
pmd_clear(p);
} else {
@@ -593,7 +587,7 @@ int kvmppc_create_pte(struct kvm *kvm, pgd_t *pgtable, pte_t pte,
else if (level <= 1)
new_pmd = kvmppc_pmd_alloc();
- if (level == 0 && !(pmd && pmd_present(*pmd) && !pmd_is_leaf(*pmd)))
+ if (level == 0 && !(pmd && pmd_present(*pmd) && !pmd_large(*pmd)))
new_ptep = kvmppc_pte_alloc();
/* Check if we might have been invalidated; let the guest retry if so */
@@ -662,7 +656,7 @@ int kvmppc_create_pte(struct kvm *kvm, pgd_t *pgtable, pte_t pte,
new_pmd = NULL;
}
pmd = pmd_offset(pud, gpa);
- if (pmd_is_leaf(*pmd)) {
+ if (pmd_large(*pmd)) {
unsigned long lgpa = gpa & PMD_MASK;
/* Check if we raced and someone else has set the same thing */
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v2 4/7] dt-bindings: counter: ftm-quaddec
From: Rob Herring @ 2019-03-26 16:06 UTC (permalink / raw)
To: Arnout Vandecappelle
Cc: Mark Rutland, devicetree, linuxppc-dev,
linux-kernel@vger.kernel.org, Linux PWM List,
open list:IIO SUBSYSTEM AND DRIVERS, Patrick Havelange,
Daniel Lezcano, William Breathitt Gray, Li Yang, Thierry Reding,
Esben Haabendal, Thomas Gleixner, Shawn Guo, Jonathan Cameron,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <55b4aaba-a94f-2a9d-c96d-e591877fe075@mind.be>
On Tue, Mar 26, 2019 at 10:43 AM Arnout Vandecappelle <arnout@mind.be> wrote:
>
> [Full disclosure: I'm a colleague of Patrick.]
>
> On 2019-03-16 14:21:38, Jonathan Cameron wrote:
>
> > On Tue, 12 Mar 2019 14:09:52 -0500
> > Rob Herring <robh@kernel.org> wrote:
> >
> > > On Wed, Mar 06, 2019 at 12:12:05PM +0100, Patrick Havelange wrote:
> > > > FlexTimer quadrature decoder driver.
> > > >
> > > > Signed-off-by: Patrick Havelange <patrick.havelange@essensium.com>
> > > > Reviewed-by: Esben Haabendal <esben@haabendal.dk>
> > > > ---
> > > > Changes v2
> > > > - None
> > > > ---
> > > > .../bindings/counter/ftm-quaddec.txt | 18 ++++++++++++++++++
> > > > 1 file changed, 18 insertions(+)
> > > > create mode 100644 Documentation/devicetree/bindings/counter/ftm-quaddec.txt
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/counter/ftm-quaddec.txt \
> > > > b/Documentation/devicetree/bindings/counter/ftm-quaddec.txt new file mode 100644
> > > > index 000000000000..4d18cd722074
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/counter/ftm-quaddec.txt
> > > > @@ -0,0 +1,18 @@
> > > > +FlexTimer Quadrature decoder counter
> > > > +
> > > > +This driver exposes a simple counter for the quadrature decoder mode.
> > >
> > > Seems like this is more a mode of a h/w block than describing a h/w
> > > block. Bindings should do the latter.
>
>
> As Jonathan writes below, it really is a "hardware mode", since it is tied
> very closely to how the device is wired up.
Okay, as it is describing what is attached, I agree.
The thing to consider is whether you will need to describe more than
just the mode. We often start adding properties of an attached device
in the controller node only to realize later that we should have a
node for the device itself. Even for something as simple as an LED
we've ended up there.
> Basically, the same block can be used for pretty diverse functions: a PWM where
> the pins are output, a counter where the pins are input, or a timer where the
> interrupt or timer value is used purely internally.
>
> This smells a bit like an MFD, but IMO it really isn't, because only one of the
> functions can be enabled. So indeed, it's more like a mode.
Agreed.
> > The snag is that we need to dig ourselves out of the hole set by:
> > fsl,vf610-ftm-pwm etc.
> >
> > Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt
> > Documentation/devicetree/bindings/timer/fsl,ftm-timer.txt
> > (I'm assuming these are the same IP block).
> >
> > Can probably be sorted out though. One core driver binds against the
> > ftm and deals with instantiating the others depending on the configuration
> > (note that this mode for instance does make sense in DT as it's
> > really reflecting the fact there is a quadrature encoder
> > connected to the ftm).
> >
> > Fiddly though :)
>
>
> The way I see it, there are 3 ways this could be modelled (other than the
> current way: several nodes with the same address).
>
>
> 1. The SoC's .dtsi defines a single node, and the compatible string (which would
> be defined by the board .dts) both enables the node and sets the compatible
> string. The .dtsi would also define the static properties of all the different
> functions: some "static" properties, e.g. reg, but also some function
> specific-properties, e.g.#pwm-cells (only for PWM), interrupts (only for timer).
> The driver (selected through compatible) anyway only uses the properties that it
> needs, so it doesn't hurt to have those other-function properties there.
>
>
> 2. Like 1, but instead of defining the compatible string in the .dts, use a
> single compatible string for all the different drivers which is set in the
> .dtsi, and add a mode property (to be set in the .dts) to select the driver. The
> selection can be done either by having a top-level driver that calls out to the
> subsystem-specific one based on the mode, or by having each driver bail out of
> its probe function if the mode is not as expected.
>
>
> 3. Have a common node that essentially does nothing except occupy the memory
> resource, and sub-nodes for each function. This can again be combined with a
> common driver that does the common resource allocation, or each function driver
> can just look at its parent node to find the resources. A disadvantage of this
> one is that it is possible to enable several functions in the DT, while only one
> can actually work.
>
>
> Option 3 is what is used for e.g. stm32-lptimer. It also uses an mfd driver to
> model the common part. But possibly it actually allows the different functions
> to operate simultaneously.
I'm not all that thrilled with how that one ended up.
> Option 3 has the additional disadvantage that it requires changes in existing
> DTs for ftm-pwm and ftm-timer, because some properties are moved one level down.
> Since we need to retain backward compatibility, we'd need to look for those
> properties both in the node itself and in the parent node. In particular, the
> common driver part would be fairly complicated to implement in a backward
> compatible way because it's not enough to do a simple devm_of_platform_populate().
>
>
> Personally I don't like the common driver part too much. This common driver
> does almost nothing (iomap and clock) and it creates dependencies between
> different drivers. Combined with the backward compatibility problem, I don't see
> much point to it.
>
>
> I personally like option 2 the most. It's easy to be backward compatible (if
> mode is not set, revert to the current behaviour, i.e. assume that the
> compatible string has encoded the mode and that you're the only driver). It
> doesn't introduce subnodes that have no hardware equivalent. The only messy
> thing about it is that properties belonging to the different modes are mixed
> together in a single node. And also, I don't think this kind of model is
> currently used anywhere else in the kernel.
Option 2 seems best to me.
Rob
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox