This patch series solves two general issues with fw_devlink=on
Patch 1/2 addresses the issue of firmware nodes that look like they'll
have struct devices created for them, but will never actually have
struct devices added for them. For example, DT nodes with a compatible
property that don't have devices added for them.
Patch 2/2 address (for static kernels) the issue of optional suppliers
that'll never have a driver registered for them. So, if the device could
have probed with fw_devlink=permissive with a static kernel, this patch
should allow those devices to probe with a fw_devlink=on. This doesn't
solve it for the case where modules are enabled because there's no way
to tell if a driver will never be registered or it's just about to be
registered. I have some other ideas for that, but it'll have to come
later thinking about it a bit.
These two patches might remove the need for several other patches that
went in as fixes for commit e590474768f1 ("driver core: Set
fw_devlink=on by default"), but I think all those fixes are good
changes. So I think we should leave those in.
Marek, Geert,
Can you try this series on a static kernel with your OF_POPULATED
changes reverted? I just want to make sure these patches can identify
and fix those cases.
Tudor,
You should still make the clock driver fix (because it's a bug), but I
think this series will fix your issue too (even without the clock driver
fix). Can you please give this a shot?
Marc,
Can you try this series with the gpiolib fix reverted please? I'm pretty
sure this will fix that case.
Linus,
This series very likely removes the need for the gpiolib patch (we can
wait for Marc to confirm). I'm split on whether we should leave it in or
not. Thoughts?
Thanks,
Saravana
Saravana Kannan (2):
driver core: fw_devlink: Detect supplier devices that will never be
added
driver core: fw_devlink: Handle missing drivers for optional suppliers
drivers/base/base.h | 2 +
drivers/base/core.c | 134 +++++++++++++++++++++++++++++++++++++-------
drivers/base/dd.c | 5 ++
3 files changed, 121 insertions(+), 20 deletions(-)
--
2.30.0.365.g02bc693789-goog
After a deferred probe attempt has exhaused all the devices that can be
bound, any device that remains unbound has one/both of these conditions
true:
(1) It is waiting on its supplier to bind
(2) It does not have a matching driver
So, to make fw_devlink=on more forgiving of missing drivers for optional
suppliers, after we've done a full deferred probe attempt, this patch
deletes all device links created by fw_devlink where the supplier hasn't
probed yet and the supplier itself is not waiting on any of its
suppliers. This allows consumers to probe during another deferred probe
attempt if they were waiting on optional suppliers.
When modules are enabled, we can't differentiate between a driver
that'll never be registered vs a driver that'll be registered soon by
loading a module. So, this patch doesn't do anything for the case where
modules are enabled.
Fixes: e590474768f1 ("driver core: Set fw_devlink=on by default")
Signed-off-by: Saravana Kannan <[email protected]>
---
drivers/base/base.h | 2 +
drivers/base/core.c | 104 ++++++++++++++++++++++++++++++++++++--------
drivers/base/dd.c | 5 +++
3 files changed, 94 insertions(+), 17 deletions(-)
diff --git a/drivers/base/base.h b/drivers/base/base.h
index f5600a83124f..34befe9475cb 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -186,6 +186,8 @@ extern void device_links_no_driver(struct device *dev);
extern bool device_links_busy(struct device *dev);
extern void device_links_unbind_consumers(struct device *dev);
+bool fw_devlink_deferred_probe_retry(void);
+
/* device pm support */
void device_pm_move_to_tail(struct device *dev);
diff --git a/drivers/base/core.c b/drivers/base/core.c
index f380133df63b..512771dd71df 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -50,6 +50,7 @@ static LIST_HEAD(deferred_sync);
static unsigned int defer_sync_state_count = 1;
static DEFINE_MUTEX(fwnode_link_lock);
static bool fw_devlink_is_permissive(void);
+static bool fw_devlink_def_probe_retry;
/**
* fwnode_link_add - Create a link between two fwnode_handles.
@@ -880,6 +881,13 @@ static void device_link_put_kref(struct device_link *link)
WARN(1, "Unable to drop a managed device link reference\n");
}
+static void device_link_drop_managed(struct device_link *link)
+{
+ link->flags &= ~DL_FLAG_MANAGED;
+ WRITE_ONCE(link->status, DL_STATE_NONE);
+ kref_put(&link->kref, __device_link_del);
+}
+
/**
* device_link_del - Delete a stateless link between two devices.
* @link: Device link to delete.
@@ -942,6 +950,29 @@ static void device_links_missing_supplier(struct device *dev)
}
}
+/**
+ * device_links_probe_blocked_by - Return first supplier blocking probe
+ * @dev: Consumer device.
+ *
+ * Checks if the probe of @dev is blocked by a supplier without a driver. If
+ * yes, return that supplier dev. Otherwise, return NULL.
+ */
+static struct device *device_links_probe_blocked_by(struct device *dev)
+{
+ struct device_link *link;
+
+ list_for_each_entry(link, &dev->links.suppliers, c_node) {
+ if (!(link->flags & DL_FLAG_MANAGED) ||
+ link->flags & DL_FLAG_SYNC_STATE_ONLY)
+ continue;
+
+ if (link->status != DL_STATE_AVAILABLE)
+ return link->supplier;
+ }
+
+ return NULL;
+}
+
/**
* device_links_check_suppliers - Check presence of supplier drivers.
* @dev: Consumer device.
@@ -960,7 +991,7 @@ static void device_links_missing_supplier(struct device *dev)
*/
int device_links_check_suppliers(struct device *dev)
{
- struct device_link *link;
+ struct device_link *link, *tmp;
int ret = 0;
/*
@@ -981,19 +1012,47 @@ int device_links_check_suppliers(struct device *dev)
device_links_write_lock();
- list_for_each_entry(link, &dev->links.suppliers, c_node) {
+ list_for_each_entry_safe(link, tmp, &dev->links.suppliers, c_node) {
if (!(link->flags & DL_FLAG_MANAGED))
continue;
- if (link->status != DL_STATE_AVAILABLE &&
- !(link->flags & DL_FLAG_SYNC_STATE_ONLY)) {
- device_links_missing_supplier(dev);
- dev_dbg(dev, "probe deferral - supplier %s not ready\n",
- dev_name(link->supplier));
- ret = -EPROBE_DEFER;
- break;
+
+ if (link->status == DL_STATE_AVAILABLE ||
+ link->flags & DL_FLAG_SYNC_STATE_ONLY) {
+ WRITE_ONCE(link->status, DL_STATE_CONSUMER_PROBE);
+ continue;
+ }
+
+ /*
+ * After a deferred probe attempt has exhaused all the devices
+ * that can be bound, any device that remains unbound has
+ * one/both of these conditions true:
+ *
+ * (1) It is waiting on its supplier to bind
+ * (2) It does not have a matching driver
+ *
+ * If this device is waiting on a supplier to bind to a driver,
+ * we make sure condition (1) above is not true for the
+ * supplier. In which case, condition (2) has to be true for
+ * the supplier. That is, the supplier doesn't have a matching
+ * driver.
+ *
+ * When we find such a supplier, we delete the device link if
+ * it was created by fw_devlink. This it to allow the consumer
+ * to probe in case the supplier is an optional.
+ */
+ if (fw_devlink_def_probe_retry &&
+ link->flags & DL_FLAG_INFERRED &&
+ !device_links_probe_blocked_by(link->supplier)) {
+ device_link_drop_managed(link);
+ continue;
}
- WRITE_ONCE(link->status, DL_STATE_CONSUMER_PROBE);
+
+ device_links_missing_supplier(dev);
+ dev_dbg(dev, "probe deferral - supplier %s not ready\n",
+ dev_name(link->supplier));
+ ret = -EPROBE_DEFER;
+ break;
}
dev->links.status = DL_DEV_PROBING;
@@ -1131,13 +1190,6 @@ static void __device_links_supplier_defer_sync(struct device *sup)
list_add_tail(&sup->links.defer_sync, &deferred_sync);
}
-static void device_link_drop_managed(struct device_link *link)
-{
- link->flags &= ~DL_FLAG_MANAGED;
- WRITE_ONCE(link->status, DL_STATE_NONE);
- kref_put(&link->kref, __device_link_del);
-}
-
static ssize_t waiting_for_supplier_show(struct device *dev,
struct device_attribute *attr,
char *buf)
@@ -1596,6 +1648,24 @@ static int fw_devlink_relax_cycle(struct device *con, void *sup)
return ret;
}
+/** fw_devlink_deferred_probe_retry - Set up fw_devlink for probe retries
+ *
+ * This function requests fw_devlink to set itself up for a deferred probe
+ * retry. This allows fw_devlink to ignore device links it created to
+ * suppliers that'll never probe. This is necessary in case some of the
+ * suppliers are optional and their consumers can probe without them.
+ *
+ * Returns true if deferred probe retry is likely to make any difference.
+ */
+bool fw_devlink_deferred_probe_retry(void)
+{
+ if (IS_ENABLED(CONFIG_MODULES))
+ return false;
+
+ fw_devlink_def_probe_retry = true;
+ return fw_devlink_get_flags() && !fw_devlink_is_permissive();
+}
+
/**
* fw_devlink_create_devlink - Create a device link from a consumer to fwnode
* @con - Consumer device for the device link
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 9179825ff646..11325df2327f 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -317,6 +317,11 @@ static int deferred_probe_initcall(void)
driver_deferred_probe_trigger();
/* Sort as many dependencies as possible before exiting initcalls */
flush_work(&deferred_probe_work);
+
+ if (fw_devlink_deferred_probe_retry()) {
+ driver_deferred_probe_trigger();
+ flush_work(&deferred_probe_work);
+ }
initcalls_done = true;
/*
--
2.30.0.365.g02bc693789-goog
On Fri, Jan 29, 2021 at 8:03 PM Saravana Kannan <[email protected]> wrote:
>
> This patch series solves two general issues with fw_devlink=on
>
> Patch 1/2 addresses the issue of firmware nodes that look like they'll
> have struct devices created for them, but will never actually have
> struct devices added for them. For example, DT nodes with a compatible
> property that don't have devices added for them.
>
> Patch 2/2 address (for static kernels) the issue of optional suppliers
> that'll never have a driver registered for them. So, if the device could
> have probed with fw_devlink=permissive with a static kernel, this patch
> should allow those devices to probe with a fw_devlink=on. This doesn't
> solve it for the case where modules are enabled because there's no way
> to tell if a driver will never be registered or it's just about to be
> registered. I have some other ideas for that, but it'll have to come
> later thinking about it a bit.
>
> These two patches might remove the need for several other patches that
> went in as fixes for commit e590474768f1 ("driver core: Set
> fw_devlink=on by default"), but I think all those fixes are good
> changes. So I think we should leave those in.
>
> Marek, Geert,
>
> Can you try this series on a static kernel with your OF_POPULATED
> changes reverted? I just want to make sure these patches can identify
> and fix those cases.
>
> Tudor,
>
> You should still make the clock driver fix (because it's a bug), but I
> think this series will fix your issue too (even without the clock driver
> fix). Can you please give this a shot?
>
> Marc,
>
> Can you try this series with the gpiolib fix reverted please? I'm pretty
> sure this will fix that case.
>
> Linus,
>
> This series very likely removes the need for the gpiolib patch (we can
> wait for Marc to confirm). I'm split on whether we should leave it in or
> not. Thoughts?
Actually, thinking more about this, we should keep the gpiolib patch.
It'll ensure the suspend/resume order is always correct.
This series basically gives up on creating device links to firmware
nodes that don't have a corresponding device added. The gpiolib patch
makes sure the nodes have a device that corresponds to them. So device
links will get created to the gpio_device and will make sure the
parent of the gpio_device doesn't suspend before the consumers of the
gpio.
-Saravana
Hi Saravana,
On Sat, Jan 30, 2021 at 5:03 AM Saravana Kannan <[email protected]> wrote:
> After a deferred probe attempt has exhaused all the devices that can be
> bound, any device that remains unbound has one/both of these conditions
> true:
>
> (1) It is waiting on its supplier to bind
> (2) It does not have a matching driver
>
> So, to make fw_devlink=on more forgiving of missing drivers for optional
> suppliers, after we've done a full deferred probe attempt, this patch
> deletes all device links created by fw_devlink where the supplier hasn't
> probed yet and the supplier itself is not waiting on any of its
> suppliers. This allows consumers to probe during another deferred probe
> attempt if they were waiting on optional suppliers.
>
> When modules are enabled, we can't differentiate between a driver
> that'll never be registered vs a driver that'll be registered soon by
> loading a module. So, this patch doesn't do anything for the case where
> modules are enabled.
For the modular case, can't you do a probe regardless? Or limit it
to devices where the missing provider is a DMAC or IOMMU driver?
Many drivers can handle missing DMAC controller drivers, and are even
supposed to work that way. They may even retry obtaining DMA releases
later.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Saravana,
Thanks for this.
On 2021-01-30 04:03, Saravana Kannan wrote:
> This patch series solves two general issues with fw_devlink=on
>
> Patch 1/2 addresses the issue of firmware nodes that look like they'll
> have struct devices created for them, but will never actually have
> struct devices added for them. For example, DT nodes with a compatible
> property that don't have devices added for them.
>
> Patch 2/2 address (for static kernels) the issue of optional suppliers
> that'll never have a driver registered for them. So, if the device
> could
> have probed with fw_devlink=permissive with a static kernel, this patch
> should allow those devices to probe with a fw_devlink=on. This doesn't
> solve it for the case where modules are enabled because there's no way
> to tell if a driver will never be registered or it's just about to be
> registered. I have some other ideas for that, but it'll have to come
> later thinking about it a bit.
>
> These two patches might remove the need for several other patches that
> went in as fixes for commit e590474768f1 ("driver core: Set
> fw_devlink=on by default"), but I think all those fixes are good
> changes. So I think we should leave those in.
>
> Marek, Geert,
>
> Can you try this series on a static kernel with your OF_POPULATED
> changes reverted? I just want to make sure these patches can identify
> and fix those cases.
>
> Tudor,
>
> You should still make the clock driver fix (because it's a bug), but I
> think this series will fix your issue too (even without the clock
> driver
> fix). Can you please give this a shot?
>
> Marc,
>
> Can you try this series with the gpiolib fix reverted please? I'm
> pretty
> sure this will fix that case.
Almost. The board boots and behaves as expected, except that a few
devices
such as the SD card are unusable (probably because the corresponding
suppliers are still not identified as being available:
# find /sys -name waiting_for_supplier| xargs grep .| grep -v :0
/sys/devices/platform/vcc3v0-sd/waiting_for_supplier:1
/sys/devices/platform/vbus-typec/waiting_for_supplier:1
/sys/devices/platform/sdio-pwrseq/waiting_for_supplier:1
/sys/devices/platform/ir-receiver/waiting_for_supplier:1
With the GPIO patch that I reverted, no device is waiting for
a supplier.
Let me know if I can further help.
M.
--
Jazz is not dead. It just smells funny...
On Mon, Feb 1, 2021 at 2:32 AM Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Saravana,
>
> On Sat, Jan 30, 2021 at 5:03 AM Saravana Kannan <[email protected]> wrote:
> > After a deferred probe attempt has exhaused all the devices that can be
> > bound, any device that remains unbound has one/both of these conditions
> > true:
> >
> > (1) It is waiting on its supplier to bind
> > (2) It does not have a matching driver
> >
> > So, to make fw_devlink=on more forgiving of missing drivers for optional
> > suppliers, after we've done a full deferred probe attempt, this patch
> > deletes all device links created by fw_devlink where the supplier hasn't
> > probed yet and the supplier itself is not waiting on any of its
> > suppliers. This allows consumers to probe during another deferred probe
> > attempt if they were waiting on optional suppliers.
> >
> > When modules are enabled, we can't differentiate between a driver
> > that'll never be registered vs a driver that'll be registered soon by
> > loading a module. So, this patch doesn't do anything for the case where
> > modules are enabled.
>
> For the modular case, can't you do a probe regardless? Or limit it
> to devices where the missing provider is a DMAC or IOMMU driver?
> Many drivers can handle missing DMAC controller drivers, and are even
> supposed to work that way. They may even retry obtaining DMA releases
> later.
I don't want to handle this at a property/provider-type level. It'll
be a whack-a-mole that'll never end -- there'll be some driver that
would work without some resource. Letting it probe is not difficult (I
just need to drop these device links), but the problem is that a lot
of drivers are not written properly to be able to handle getting
deferred and then getting reattempted before the supplier. Either
because:
1. They were never built and tested as a module
2. The supplier gets deferred and the consumer doesn't have proper
deferred probe implementation and when we drop the device links, the
consumer might be attempted before the supplier and things go bad.
One hack I'm thinking of is that with CONFIG_MODULES, I can drop these
unmet device links after a N-second timeout, but having the timeout
extended everytime a new driver is registered. So as long as no two
modules are loaded further than N seconds apart during boot up, it
would all just work out fine. But it doesn't solve the problem fully
either. But maybe it'll be good enough? I haven't analyzed this fully
yet -- so apologies in advance if it's stupid.
-Saravana
Hi Saravana,
On Mon, Feb 1, 2021 at 9:49 PM Saravana Kannan <[email protected]> wrote:
> On Mon, Feb 1, 2021 at 2:32 AM Geert Uytterhoeven <[email protected]> wrote:
> > On Sat, Jan 30, 2021 at 5:03 AM Saravana Kannan <[email protected]> wrote:
> > > After a deferred probe attempt has exhaused all the devices that can be
> > > bound, any device that remains unbound has one/both of these conditions
> > > true:
> > >
> > > (1) It is waiting on its supplier to bind
> > > (2) It does not have a matching driver
> > >
> > > So, to make fw_devlink=on more forgiving of missing drivers for optional
> > > suppliers, after we've done a full deferred probe attempt, this patch
> > > deletes all device links created by fw_devlink where the supplier hasn't
> > > probed yet and the supplier itself is not waiting on any of its
> > > suppliers. This allows consumers to probe during another deferred probe
> > > attempt if they were waiting on optional suppliers.
> > >
> > > When modules are enabled, we can't differentiate between a driver
> > > that'll never be registered vs a driver that'll be registered soon by
> > > loading a module. So, this patch doesn't do anything for the case where
> > > modules are enabled.
> >
> > For the modular case, can't you do a probe regardless? Or limit it
> > to devices where the missing provider is a DMAC or IOMMU driver?
> > Many drivers can handle missing DMAC controller drivers, and are even
> > supposed to work that way. They may even retry obtaining DMA releases
> > later.
>
> I don't want to handle this at a property/provider-type level. It'll
> be a whack-a-mole that'll never end -- there'll be some driver that
> would work without some resource. Letting it probe is not difficult (I
> just need to drop these device links), but the problem is that a lot
> of drivers are not written properly to be able to handle getting
> deferred and then getting reattempted before the supplier. Either
> because:
>
> 1. They were never built and tested as a module
> 2. The supplier gets deferred and the consumer doesn't have proper
> deferred probe implementation and when we drop the device links, the
> consumer might be attempted before the supplier and things go bad.
You may be a bit too pessimistic here: we had deferred probing for
years. With devices with complex dependencies, it's not uncommon for a
driver to be probed 3 or 4 times, before it succeeds (and FTR, would be
happy to see this fixed). So most drivers should handle this already.
And if they don't, they're already broken.
> One hack I'm thinking of is that with CONFIG_MODULES, I can drop these
> unmet device links after a N-second timeout, but having the timeout
> extended everytime a new driver is registered. So as long as no two
> modules are loaded further than N seconds apart during boot up, it
> would all just work out fine. But it doesn't solve the problem fully
> either. But maybe it'll be good enough? I haven't analyzed this fully
> yet -- so apologies in advance if it's stupid.
So you would introduce an additional delay when e.g. mounting the root
file system, as the SDHI driver probe may be postponed due to the DMAC
driver not being available?
Let's consider the possible combinations here:
1. If both provider and consumer are built-in, there's no issue.
2. If the provider is built-in, and the consumer is modular, it should
just work, too.
3. If the consumer is built-in, but the provider is modular (or not
available), what to do?
Wouldn't it be safe to assume the user did intend the consumer to
probe without the provider, and thus continue, if possible (e.g.
for an optional DMA channel)? Else the user would have configured
the provider driver built-in, too.
4. If both provider and consumer are modular, perhaps userspace should
try to load the modules in the right order, i.e. provider drivers
first?
For DMACs, it's the consumer that knows if it can work without the DMAC
driver present, and fall back to PIO, or not. So we could add a flag
for that to struct driver.
For IOMMUs, it's different: the consumer drivers are not aware of it,
only the driver/IOMMU subsystem is. And whether to work in the absence
of an IOMMU driver is more like a system (security) policy decision.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Tue, Feb 2, 2021 at 12:49 AM Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Saravana,
>
> On Mon, Feb 1, 2021 at 9:49 PM Saravana Kannan <[email protected]> wrote:
> > On Mon, Feb 1, 2021 at 2:32 AM Geert Uytterhoeven <[email protected]> wrote:
> > > On Sat, Jan 30, 2021 at 5:03 AM Saravana Kannan <[email protected]> wrote:
> > > > After a deferred probe attempt has exhaused all the devices that can be
> > > > bound, any device that remains unbound has one/both of these conditions
> > > > true:
> > > >
> > > > (1) It is waiting on its supplier to bind
> > > > (2) It does not have a matching driver
> > > >
> > > > So, to make fw_devlink=on more forgiving of missing drivers for optional
> > > > suppliers, after we've done a full deferred probe attempt, this patch
> > > > deletes all device links created by fw_devlink where the supplier hasn't
> > > > probed yet and the supplier itself is not waiting on any of its
> > > > suppliers. This allows consumers to probe during another deferred probe
> > > > attempt if they were waiting on optional suppliers.
> > > >
> > > > When modules are enabled, we can't differentiate between a driver
> > > > that'll never be registered vs a driver that'll be registered soon by
> > > > loading a module. So, this patch doesn't do anything for the case where
> > > > modules are enabled.
> > >
> > > For the modular case, can't you do a probe regardless? Or limit it
> > > to devices where the missing provider is a DMAC or IOMMU driver?
> > > Many drivers can handle missing DMAC controller drivers, and are even
> > > supposed to work that way. They may even retry obtaining DMA releases
> > > later.
> >
> > I don't want to handle this at a property/provider-type level. It'll
> > be a whack-a-mole that'll never end -- there'll be some driver that
> > would work without some resource. Letting it probe is not difficult (I
> > just need to drop these device links), but the problem is that a lot
> > of drivers are not written properly to be able to handle getting
> > deferred and then getting reattempted before the supplier. Either
> > because:
> >
> > 1. They were never built and tested as a module
> > 2. The supplier gets deferred and the consumer doesn't have proper
> > deferred probe implementation and when we drop the device links, the
> > consumer might be attempted before the supplier and things go bad.
>
> You may be a bit too pessimistic here: we had deferred probing for
> years. With devices with complex dependencies, it's not uncommon for a
> driver to be probed 3 or 4 times, before it succeeds (and FTR, would be
> happy to see this fixed). So most drivers should handle this already.
> And if they don't, they're already broken.
I fully agree the drivers are broken and they should be fixed (like
we've been doing so far). I'd happily say "fix your driver" and just
mark properties (iommu, dmas) as "optional" and be done with it.
Actually, handling optional properties is pretty simple -- we just
need to stop parsing them (delete code/make it a flag).
But my understanding is, you can't set fw_devlink=on by default and
break devices that used to boot fine before. But we can't find the
broken drivers without setting fw_devlink=on by default. So it's a
catch-22. I'm happy to continue helping debug the issues if we are
okay with leaving fw_devlink=on in 5.12, but I'm not sure everyone
will agree to that.
Also, there's also no way to selectively enable fw_devlink on a
DT/board level (Rob will have to agree to a property in the chosen {}
node). So, without that, I'm forced to go for the lowest common
denominator.
>
> > One hack I'm thinking of is that with CONFIG_MODULES, I can drop these
> > unmet device links after a N-second timeout, but having the timeout
> > extended everytime a new driver is registered. So as long as no two
> > modules are loaded further than N seconds apart during boot up, it
> > would all just work out fine. But it doesn't solve the problem fully
> > either. But maybe it'll be good enough? I haven't analyzed this fully
> > yet -- so apologies in advance if it's stupid.
>
> So you would introduce an additional delay when e.g. mounting the root
> file system, as the SDHI driver probe may be postponed due to the DMAC
> driver not being available?
Honestly I'm thinking of deleting/adding a conditional for iommu/dma
parsing. So boards that know all iommus/dmas are needed can set some
flag if they want.
In your example, if the DMAC driver is needed for SDHI to work (mount
root), then I'd expect it to be loaded(ramdisk)/builtin before mount
root. And it'd just work. If the DMA is optional and SDHI should have
mounted root without it, then the solution is what I said above.
> Let's consider the possible combinations here:
> 1. If both provider and consumer are built-in, there's no issue.
> 2. If the provider is built-in, and the consumer is modular, it should
> just work, too.
> 3. If the consumer is built-in, but the provider is modular (or not
> available), what to do?
> Wouldn't it be safe to assume the user did intend the consumer to
> probe without the provider, and thus continue, if possible (e.g.
> for an optional DMA channel)? Else the user would have configured
> the provider driver built-in, too.
> 4. If both provider and consumer are modular, perhaps userspace should
> try to load the modules in the right order, i.e. provider drivers
> first?
One of the main benefits/goal of fw_devlink is that it makes module
load ordering irrelevant. There are other correctness issues with
depending on module load ordering too. So I don't want to go down the
(fix your module load order) route.
>
> For DMACs, it's the consumer that knows if it can work without the DMAC
> driver present, and fall back to PIO, or not. So we could add a flag
> for that to struct driver.
The whole point of fw_devlink is to figure stuff out without the
drivers being available (sync_state() is a whole another layer of
complexity). If the drivers come into picture, we don't really need
fw_devlink to parse dmas. The dma framework code can add device links
once a consumer starts using the dma (similar to how iommus do it).
> For IOMMUs, it's different: the consumer drivers are not aware of it,
> only the driver/IOMMU subsystem is. And whether to work in the absence
> of an IOMMU driver is more like a system (security) policy decision.
I think some consumers can make this decision too. For example, not
supporting DRM content if the IOMMU isn't available, etc.
Long story short, I don't have a problem with saying fix the driver.
But I don't think I can have fw_devlink=on by default with that
argument. And making it fw_devlink=on by default is my goal.
-Saravana
Greeting,
FYI, we noticed the following commit (built with gcc-9):
commit: 4731210c09f5977300f439b6c56ba220c65b2348 ("[PATCH v1 2/2] driver core: fw_devlink: Handle missing drivers for optional suppliers")
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master
in testcase: kernel-selftests
version: kernel-selftests-x86_64-b553cffa-1_20210122
with following parameters:
group: group-01
ucode: 0xe2
test-description: The kernel contains a set of "self tests" under the tools/testing/selftests/ directory. These are intended to be small unit tests to exercise individual code paths in the kernel.
test-url: https://www.kernel.org/doc/Documentation/kselftest.txt
on test machine: 4 threads Intel(R) Core(TM) i5-6500 CPU @ 3.20GHz with 32G memory
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>
[ 388.912130] BUG: kernel NULL pointer dereference, address: 0000000000000590
[ 388.919103] #PF: supervisor read access in kernel mode
[ 388.924245] #PF: error_code(0x0000) - not-present page
[ 388.929389] PGD 0 P4D 0
[ 388.931925] Oops: 0000 [#1] PREEMPT SMP PTI
[ 388.936114] CPU: 2 PID: 20288 Comm: modprobe Not tainted 5.11.0-rc5-00017-g4731210c09f5 #1
[ 388.944388] Hardware name: Dell Inc. OptiPlex 7040/0Y7WYT, BIOS 1.8.1 12/05/2017
[ 388.951789] RIP: 0010:__list_del_entry_valid (kbuild/src/consumer/lib/list_debug.c:43)
[ 388.956850] Code: 0f 85 43 00 73 00 48 39 d7 0f 84 23 00 73 00 4c 39 cf 0f 84 1a 00 73 00 b8 01 00 00 00 c3 66 66 2e 0f 1f 84 00 00 00 00 00 90 <48> 8b 17 4c 8b 47 08 48 b8 00 01 00 00 00 00 ad de 48 39 c2 0f 84
All code
========
0: 0f 85 43 00 73 00 jne 0x730049
6: 48 39 d7 cmp %rdx,%rdi
9: 0f 84 23 00 73 00 je 0x730032
f: 4c 39 cf cmp %r9,%rdi
12: 0f 84 1a 00 73 00 je 0x730032
18: b8 01 00 00 00 mov $0x1,%eax
1d: c3 retq
1e: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
25: 00 00 00 00
29: 90 nop
2a:* 48 8b 17 mov (%rdi),%rdx <-- trapping instruction
2d: 4c 8b 47 08 mov 0x8(%rdi),%r8
31: 48 b8 00 01 00 00 00 movabs $0xdead000000000100,%rax
38: 00 ad de
3b: 48 39 c2 cmp %rax,%rdx
3e: 0f .byte 0xf
3f: 84 .byte 0x84
Code starting with the faulting instruction
===========================================
0: 48 8b 17 mov (%rdi),%rdx
3: 4c 8b 47 08 mov 0x8(%rdi),%r8
7: 48 b8 00 01 00 00 00 movabs $0xdead000000000100,%rax
e: 00 ad de
11: 48 39 c2 cmp %rax,%rdx
14: 0f .byte 0xf
15: 84 .byte 0x84
[ 388.975647] RSP: 0018:ffffc90000b3fdc0 EFLAGS: 00010282
[ 388.980878] RAX: ffffffff81717b80 RBX: ffff8888191c8040 RCX: 0000000000000000
[ 388.988021] RDX: 0000000000000001 RSI: ffffffff827a46fa RDI: 0000000000000590
[ 388.995180] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000001
[ 389.002323] R10: ffff888100bc3200 R11: 0000000000000000 R12: ffff8888137ab400
[ 389.009468] R13: ffff8888191c8008 R14: 0000000000000000 R15: ffff8888191cacb8
[ 389.016611] FS: 00007f73471e5480(0000) GS:ffff88881dd00000(0000) knlGS:0000000000000000
[ 389.024722] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 389.030484] CR2: 0000000000000590 CR3: 00000008160c4001 CR4: 00000000003706e0
[ 389.037637] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 389.044791] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 389.051935] Call Trace:
[ 389.054382] gpiodevice_release (kbuild/src/consumer/include/linux/list.h:132 kbuild/src/consumer/include/linux/list.h:146 kbuild/src/consumer/drivers/gpio/gpiolib.c:477)
[ 389.058400] device_release (kbuild/src/consumer/drivers/base/core.c:2059)
[ 389.062066] kobject_release (kbuild/src/consumer/lib/kobject.c:709 kbuild/src/consumer/lib/kobject.c:736)
[ 389.065906] release_nodes (kbuild/src/consumer/drivers/base/devres.c:524 (discriminator 12))
[ 389.069662] device_release_driver_internal (kbuild/src/consumer/drivers/base/dd.c:1164 kbuild/src/consumer/drivers/base/dd.c:1187)
[ 389.074894] driver_detach (kbuild/src/consumer/drivers/base/dd.c:1251)
[ 389.078473] bus_remove_driver (kbuild/src/consumer/drivers/base/bus.c:680)
[ 389.082401] gpio_mockup_exit (gpio-mockup.c:?) gpio_mockup
[ 389.087548] __x64_sys_delete_module (kbuild/src/consumer/kernel/module.c:1064 kbuild/src/consumer/kernel/module.c:1006 kbuild/src/consumer/kernel/module.c:1006)
[ 389.092187] ? syscall_enter_from_user_mode (kbuild/src/consumer/arch/x86/include/asm/irqflags.h:54 kbuild/src/consumer/arch/x86/include/asm/irqflags.h:94 kbuild/src/consumer/kernel/entry/common.c:106)
[ 389.097259] ? lockdep_hardirqs_on (kbuild/src/consumer/kernel/locking/lockdep.c:4162)
[ 389.101620] do_syscall_64 (kbuild/src/consumer/arch/x86/entry/common.c:46)
[ 389.105211] entry_SYSCALL_64_after_hwframe (kbuild/src/consumer/arch/x86/entry/entry_64.S:127)
[ 389.110282] RIP: 0033:0x7f7347305dd7
[ 389.113860] Code: 73 01 c3 48 8b 0d b9 10 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 b8 b0 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 89 10 0c 00 f7 d8 64 89 01 48
All code
========
0: 73 01 jae 0x3
2: c3 retq
3: 48 8b 0d b9 10 0c 00 mov 0xc10b9(%rip),%rcx # 0xc10c3
a: f7 d8 neg %eax
c: 64 89 01 mov %eax,%fs:(%rcx)
f: 48 83 c8 ff or $0xffffffffffffffff,%rax
13: c3 retq
14: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1)
1b: 00 00 00
1e: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1)
23: b8 b0 00 00 00 mov $0xb0,%eax
28: 0f 05 syscall
2a:* 48 3d 01 f0 ff ff cmp $0xfffffffffffff001,%rax <-- trapping instruction
30: 73 01 jae 0x33
32: c3 retq
33: 48 8b 0d 89 10 0c 00 mov 0xc1089(%rip),%rcx # 0xc10c3
3a: f7 d8 neg %eax
3c: 64 89 01 mov %eax,%fs:(%rcx)
3f: 48 rex.W
Code starting with the faulting instruction
===========================================
0: 48 3d 01 f0 ff ff cmp $0xfffffffffffff001,%rax
6: 73 01 jae 0x9
8: c3 retq
9: 48 8b 0d 89 10 0c 00 mov 0xc1089(%rip),%rcx # 0xc1099
10: f7 d8 neg %eax
12: 64 89 01 mov %eax,%fs:(%rcx)
15: 48 rex.W
[ 389.132653] RSP: 002b:00007ffcd5f52a78 EFLAGS: 00000206 ORIG_RAX: 00000000000000b0
[ 389.140232] RAX: ffffffffffffffda RBX: 0000564dfdabc100 RCX: 00007f7347305dd7
[ 389.147374] RDX: 0000000000000000 RSI: 0000000000000800 RDI: 0000564dfdabc168
[ 389.154515] RBP: 0000564dfdabc168 R08: 00007ffcd5f51a21 R09: 0000000000000000
[ 389.161657] R10: 00007f7347377ae0 R11: 0000000000000206 R12: 0000000000000000
[ 389.168798] R13: 0000000000000000 R14: 0000564dfdabc168 R15: 0000564dfdabc210
[ 389.175941] Modules linked in: gpio_mockup(-) btrfs blake2b_generic xor zstd_compress raid6_pq libcrc32c sd_mod t10_pi sg intel_rapl_msr intel_rapl_common dell_wmi x86_pkg_temp_thermal intel_powerclamp coretemp dell_smbios crct10dif_pclmul ipmi_devintf crc32_pclmul mei_wdt crc32c_intel ipmi_msghandler dell_wmi_descriptor sparse_keymap wmi_bmof dcdbas ahci ghash_clmulni_intel i915 libahci i2c_i801 mei_me rapl i2c_smbus intel_cstate libata intel_uncore mei intel_pch_thermal wmi intel_gtt video acpi_pad intel_pmc_core ip_tables [last unloaded: preemptirq_delay_test]
[ 389.225888] CR2: 0000000000000590
[ 389.229218] ---[ end trace 0b0c7ec922ff47d5 ]---
[ 389.233853] RIP: 0010:__list_del_entry_valid (kbuild/src/consumer/lib/list_debug.c:43)
[ 389.238910] Code: 0f 85 43 00 73 00 48 39 d7 0f 84 23 00 73 00 4c 39 cf 0f 84 1a 00 73 00 b8 01 00 00 00 c3 66 66 2e 0f 1f 84 00 00 00 00 00 90 <48> 8b 17 4c 8b 47 08 48 b8 00 01 00 00 00 00 ad de 48 39 c2 0f 84
All code
========
0: 0f 85 43 00 73 00 jne 0x730049
6: 48 39 d7 cmp %rdx,%rdi
9: 0f 84 23 00 73 00 je 0x730032
f: 4c 39 cf cmp %r9,%rdi
12: 0f 84 1a 00 73 00 je 0x730032
18: b8 01 00 00 00 mov $0x1,%eax
1d: c3 retq
1e: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
25: 00 00 00 00
29: 90 nop
2a:* 48 8b 17 mov (%rdi),%rdx <-- trapping instruction
2d: 4c 8b 47 08 mov 0x8(%rdi),%r8
31: 48 b8 00 01 00 00 00 movabs $0xdead000000000100,%rax
38: 00 ad de
3b: 48 39 c2 cmp %rax,%rdx
3e: 0f .byte 0xf
3f: 84 .byte 0x84
Code starting with the faulting instruction
===========================================
0: 48 8b 17 mov (%rdi),%rdx
3: 4c 8b 47 08 mov 0x8(%rdi),%r8
7: 48 b8 00 01 00 00 00 movabs $0xdead000000000100,%rax
e: 00 ad de
11: 48 39 c2 cmp %rax,%rdx
14: 0f .byte 0xf
15: 84 .byte 0x84
To reproduce:
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp install job.yaml # job file is attached in this email
bin/lkp split-job --compatible job.yaml
bin/lkp run compatible-job.yaml
---
0DAY/LKP+ Test Infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/[email protected] Intel Corporation
Thanks,
Oliver Sang