Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On Thu, Feb 11, 2021 at 9:48 AM Rafael J. Wysocki wrote:
>
> On Thu, Feb 11, 2021 at 6:15 PM Saravana Kannan wrote:
> >
> > On Thu, Feb 11, 2021 at 7:03 AM Rafael J. Wysocki wrote:
> > >
> > > On Thu, Feb 11, 2021 at 1:02 AM Saravana Kannan
> > > wrote:
> > > >
> > > > On Thu, Jan 28, 2021 at 7:03 AM Jon Hunter wrote:
> > > > >
> > > > >
> > > > > On 14/01/2021 16:56, Jon Hunter wrote:
> > > > > >
> > > > > > On 14/01/2021 16:47, Saravana Kannan wrote:
> > > > > >
> > > > > > ...
> > > > > >
> > > > > >>> Yes this is the warning shown here [0] and this is coming from
> > > > > >>> the 'Generic PHY stmmac-0:00' device.
> > > > > >>
> > > > > >> Can you print the supplier and consumer device when this warning is
> > > > > >> happening and let me know? That'd help too. I'm guessing the phy is
> > > > > >> the consumer.
> > > > > >
> > > > > >
> > > > > > Sorry I should have included that. I added a print to dump this on
> > > > > > another build but failed to include here.
> > > > > >
> > > > > > WARNING KERN Generic PHY stmmac-0:00: supplier 220.gpio (status
> > > > > > 1)
> > > > > >
> > > > > > The status is the link->status and looks like the supplier is the
> > > > > > gpio controller. I have verified that the gpio controller is probed
> > > > > > before this successfully.
> > > > > >
> > > > > >> So the warning itself isn't a problem -- it's not breaking
> > > > > >> anything or
> > > > > >> leaking memory or anything like that. But the device link is
> > > > > >> jumping
> > > > > >> states in an incorrect manner. With enough context of this code
> > > > > >> (why
> > > > > >> the device_bind_driver() is being called directly instead of going
> > > > > >> through the normal probe path), it should be easy to fix (I'll just
> > > > > >> need to fix up the device link state).
> > > > > >
> > > > > > Correct, the board seems to boot fine, we just get this warning.
> > > > >
> > > > >
> > > > > Have you had chance to look at this further?
> > > >
> > > > Hi Jon,
> > > >
> > > > I finally got around to looking into this. Here's the email[1] that
> > > > describes why it's done this way.
> > > >
> > > > [1] - https://lore.kernel.org/lkml/ycrjmpkjk0pxk...@lunn.ch/
> > > >
> > > > >
> > > > > The following does appear to avoid the warning, but I am not sure if
> > > > > this is the correct thing to do ...
> > > > >
> > > > > index 9179825ff646..095aba84f7c2 100644
> > > > > --- a/drivers/base/dd.c
> > > > > +++ b/drivers/base/dd.c
> > > > > @@ -456,6 +456,10 @@ int device_bind_driver(struct device *dev)
> > > > > {
> > > > > int ret;
> > > > >
> > > > > + ret = device_links_check_suppliers(dev);
> > > > > + if (ret)
> > > > > + return ret;
> > > > > +
> > > > > ret = driver_sysfs_add(dev);
> > > > > if (!ret)
> > > > > driver_bound(dev);
> > > >
> > > > So digging deeper into the usage of device_bind_driver and looking at
> > > > [1], it doesn't look like returning an error here is a good option.
> > > > When device_bind_driver() is called, the driver's probe function isn't
> > > > even called. So, there's no way for the driver to even defer probing
> > > > based on any of the suppliers. So, we have a couple of options:
> > > >
> > > > 1. Delete all the links to suppliers that haven't bound.
> > >
> > > Or maybe convert them to stateless links? Would that be doable at all?
> >
> > Yeah, I think it should be doable.
> >
> > >
> > > > We'll still leave the links to active suppliers alone in case it helps
> > > > with
> > > > suspend/resume correctness.
> > > > 2. Fix the warning to not warn on suppliers that haven't probed if the
> > > > device's driver has no probe function. But this will also need fixing
> > > > up the cleanup part when device_release_driver() is called. Also, I'm
> > > > not sure if device_bind_driver() is ever called when the driver
> > > > actually has a probe() function.
> > > >
> > > > Rafael,
> > > >
> > > > Option 1 above is pretty straightforward.
> > >
> > > I would prefer this ->
> >
> > Ok
> >
> > >
> > > > Option 2 would look something like what's at the end of this email +
> > > > caveat about whether the probe check is sufficient.
> > >
> > > -> because "fix the warning" really means that we haven't got the
> > > device link state machine right and getting it right may imply a major
> > > redesign.
> > >
> > > Overall, I'd prefer to take a step back and allow things to stabilize
> > > for a while to let people catch up with this.
> >
> > Are you referring to if/when we implement Option 2? Or do you want to
> > step back for a while even before implementing Option 1?
>
> I would do option 1 and if then see what happens and maybe go back
> from there if need be until getting a reasonably stable situation
> (that is all of the systems that used to work before still work at
> least).
Ok, I'll implement Option 1 soon. Also, thinking more about it, I
don't like converting it into STATELESS links. It's
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On Thu, Feb 11, 2021 at 6:15 PM Saravana Kannan wrote:
>
> On Thu, Feb 11, 2021 at 7:03 AM Rafael J. Wysocki wrote:
> >
> > On Thu, Feb 11, 2021 at 1:02 AM Saravana Kannan
> > wrote:
> > >
> > > On Thu, Jan 28, 2021 at 7:03 AM Jon Hunter wrote:
> > > >
> > > >
> > > > On 14/01/2021 16:56, Jon Hunter wrote:
> > > > >
> > > > > On 14/01/2021 16:47, Saravana Kannan wrote:
> > > > >
> > > > > ...
> > > > >
> > > > >>> Yes this is the warning shown here [0] and this is coming from
> > > > >>> the 'Generic PHY stmmac-0:00' device.
> > > > >>
> > > > >> Can you print the supplier and consumer device when this warning is
> > > > >> happening and let me know? That'd help too. I'm guessing the phy is
> > > > >> the consumer.
> > > > >
> > > > >
> > > > > Sorry I should have included that. I added a print to dump this on
> > > > > another build but failed to include here.
> > > > >
> > > > > WARNING KERN Generic PHY stmmac-0:00: supplier 220.gpio (status 1)
> > > > >
> > > > > The status is the link->status and looks like the supplier is the
> > > > > gpio controller. I have verified that the gpio controller is probed
> > > > > before this successfully.
> > > > >
> > > > >> So the warning itself isn't a problem -- it's not breaking anything
> > > > >> or
> > > > >> leaking memory or anything like that. But the device link is jumping
> > > > >> states in an incorrect manner. With enough context of this code (why
> > > > >> the device_bind_driver() is being called directly instead of going
> > > > >> through the normal probe path), it should be easy to fix (I'll just
> > > > >> need to fix up the device link state).
> > > > >
> > > > > Correct, the board seems to boot fine, we just get this warning.
> > > >
> > > >
> > > > Have you had chance to look at this further?
> > >
> > > Hi Jon,
> > >
> > > I finally got around to looking into this. Here's the email[1] that
> > > describes why it's done this way.
> > >
> > > [1] - https://lore.kernel.org/lkml/ycrjmpkjk0pxk...@lunn.ch/
> > >
> > > >
> > > > The following does appear to avoid the warning, but I am not sure if
> > > > this is the correct thing to do ...
> > > >
> > > > index 9179825ff646..095aba84f7c2 100644
> > > > --- a/drivers/base/dd.c
> > > > +++ b/drivers/base/dd.c
> > > > @@ -456,6 +456,10 @@ int device_bind_driver(struct device *dev)
> > > > {
> > > > int ret;
> > > >
> > > > + ret = device_links_check_suppliers(dev);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > ret = driver_sysfs_add(dev);
> > > > if (!ret)
> > > > driver_bound(dev);
> > >
> > > So digging deeper into the usage of device_bind_driver and looking at
> > > [1], it doesn't look like returning an error here is a good option.
> > > When device_bind_driver() is called, the driver's probe function isn't
> > > even called. So, there's no way for the driver to even defer probing
> > > based on any of the suppliers. So, we have a couple of options:
> > >
> > > 1. Delete all the links to suppliers that haven't bound.
> >
> > Or maybe convert them to stateless links? Would that be doable at all?
>
> Yeah, I think it should be doable.
>
> >
> > > We'll still leave the links to active suppliers alone in case it helps
> > > with
> > > suspend/resume correctness.
> > > 2. Fix the warning to not warn on suppliers that haven't probed if the
> > > device's driver has no probe function. But this will also need fixing
> > > up the cleanup part when device_release_driver() is called. Also, I'm
> > > not sure if device_bind_driver() is ever called when the driver
> > > actually has a probe() function.
> > >
> > > Rafael,
> > >
> > > Option 1 above is pretty straightforward.
> >
> > I would prefer this ->
>
> Ok
>
> >
> > > Option 2 would look something like what's at the end of this email +
> > > caveat about whether the probe check is sufficient.
> >
> > -> because "fix the warning" really means that we haven't got the
> > device link state machine right and getting it right may imply a major
> > redesign.
> >
> > Overall, I'd prefer to take a step back and allow things to stabilize
> > for a while to let people catch up with this.
>
> Are you referring to if/when we implement Option 2? Or do you want to
> step back for a while even before implementing Option 1?
I would do option 1 and if then see what happens and maybe go back
from there if need be until getting a reasonably stable situation
(that is all of the systems that used to work before still work at
least).
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On Thu, Feb 11, 2021 at 7:03 AM Rafael J. Wysocki wrote:
>
> On Thu, Feb 11, 2021 at 1:02 AM Saravana Kannan wrote:
> >
> > On Thu, Jan 28, 2021 at 7:03 AM Jon Hunter wrote:
> > >
> > >
> > > On 14/01/2021 16:56, Jon Hunter wrote:
> > > >
> > > > On 14/01/2021 16:47, Saravana Kannan wrote:
> > > >
> > > > ...
> > > >
> > > >>> Yes this is the warning shown here [0] and this is coming from
> > > >>> the 'Generic PHY stmmac-0:00' device.
> > > >>
> > > >> Can you print the supplier and consumer device when this warning is
> > > >> happening and let me know? That'd help too. I'm guessing the phy is
> > > >> the consumer.
> > > >
> > > >
> > > > Sorry I should have included that. I added a print to dump this on
> > > > another build but failed to include here.
> > > >
> > > > WARNING KERN Generic PHY stmmac-0:00: supplier 220.gpio (status 1)
> > > >
> > > > The status is the link->status and looks like the supplier is the
> > > > gpio controller. I have verified that the gpio controller is probed
> > > > before this successfully.
> > > >
> > > >> So the warning itself isn't a problem -- it's not breaking anything or
> > > >> leaking memory or anything like that. But the device link is jumping
> > > >> states in an incorrect manner. With enough context of this code (why
> > > >> the device_bind_driver() is being called directly instead of going
> > > >> through the normal probe path), it should be easy to fix (I'll just
> > > >> need to fix up the device link state).
> > > >
> > > > Correct, the board seems to boot fine, we just get this warning.
> > >
> > >
> > > Have you had chance to look at this further?
> >
> > Hi Jon,
> >
> > I finally got around to looking into this. Here's the email[1] that
> > describes why it's done this way.
> >
> > [1] - https://lore.kernel.org/lkml/ycrjmpkjk0pxk...@lunn.ch/
> >
> > >
> > > The following does appear to avoid the warning, but I am not sure if
> > > this is the correct thing to do ...
> > >
> > > index 9179825ff646..095aba84f7c2 100644
> > > --- a/drivers/base/dd.c
> > > +++ b/drivers/base/dd.c
> > > @@ -456,6 +456,10 @@ int device_bind_driver(struct device *dev)
> > > {
> > > int ret;
> > >
> > > + ret = device_links_check_suppliers(dev);
> > > + if (ret)
> > > + return ret;
> > > +
> > > ret = driver_sysfs_add(dev);
> > > if (!ret)
> > > driver_bound(dev);
> >
> > So digging deeper into the usage of device_bind_driver and looking at
> > [1], it doesn't look like returning an error here is a good option.
> > When device_bind_driver() is called, the driver's probe function isn't
> > even called. So, there's no way for the driver to even defer probing
> > based on any of the suppliers. So, we have a couple of options:
> >
> > 1. Delete all the links to suppliers that haven't bound.
>
> Or maybe convert them to stateless links? Would that be doable at all?
Yeah, I think it should be doable.
>
> > We'll still leave the links to active suppliers alone in case it helps with
> > suspend/resume correctness.
> > 2. Fix the warning to not warn on suppliers that haven't probed if the
> > device's driver has no probe function. But this will also need fixing
> > up the cleanup part when device_release_driver() is called. Also, I'm
> > not sure if device_bind_driver() is ever called when the driver
> > actually has a probe() function.
> >
> > Rafael,
> >
> > Option 1 above is pretty straightforward.
>
> I would prefer this ->
Ok
>
> > Option 2 would look something like what's at the end of this email +
> > caveat about whether the probe check is sufficient.
>
> -> because "fix the warning" really means that we haven't got the
> device link state machine right and getting it right may imply a major
> redesign.
>
> Overall, I'd prefer to take a step back and allow things to stabilize
> for a while to let people catch up with this.
Are you referring to if/when we implement Option 2? Or do you want to
step back for a while even before implementing Option 1?
-Saravana
>
> > Do you have a preference between Option 1 vs 2? Or do you have some
> > other option in mind?
> >
> > Thanks,
> > Saravana
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 5481b6940a02..8102b3c48bbc 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -1247,7 +1247,8 @@ void device_links_driver_bound(struct device *dev)
> > */
> > device_link_drop_managed(link);
> > } else {
> > - WARN_ON(link->status != DL_STATE_CONSUMER_PROBE);
> > + WARN_ON(link->status != DL_STATE_CONSUMER_PROBE &&
> > + dev->driver->probe);
> > WRITE_ONCE(link->status, DL_STATE_ACTIVE);
> > }
> >
> > @@ -1302,7 +1303,8 @@ static void __device_links_no_driver(struct device
> > *dev)
> > if (link->supplier->links.status ==
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On Thu, Feb 11, 2021 at 1:02 AM Saravana Kannan wrote:
>
> On Thu, Jan 28, 2021 at 7:03 AM Jon Hunter wrote:
> >
> >
> > On 14/01/2021 16:56, Jon Hunter wrote:
> > >
> > > On 14/01/2021 16:47, Saravana Kannan wrote:
> > >
> > > ...
> > >
> > >>> Yes this is the warning shown here [0] and this is coming from
> > >>> the 'Generic PHY stmmac-0:00' device.
> > >>
> > >> Can you print the supplier and consumer device when this warning is
> > >> happening and let me know? That'd help too. I'm guessing the phy is
> > >> the consumer.
> > >
> > >
> > > Sorry I should have included that. I added a print to dump this on
> > > another build but failed to include here.
> > >
> > > WARNING KERN Generic PHY stmmac-0:00: supplier 220.gpio (status 1)
> > >
> > > The status is the link->status and looks like the supplier is the
> > > gpio controller. I have verified that the gpio controller is probed
> > > before this successfully.
> > >
> > >> So the warning itself isn't a problem -- it's not breaking anything or
> > >> leaking memory or anything like that. But the device link is jumping
> > >> states in an incorrect manner. With enough context of this code (why
> > >> the device_bind_driver() is being called directly instead of going
> > >> through the normal probe path), it should be easy to fix (I'll just
> > >> need to fix up the device link state).
> > >
> > > Correct, the board seems to boot fine, we just get this warning.
> >
> >
> > Have you had chance to look at this further?
>
> Hi Jon,
>
> I finally got around to looking into this. Here's the email[1] that
> describes why it's done this way.
>
> [1] - https://lore.kernel.org/lkml/ycrjmpkjk0pxk...@lunn.ch/
>
> >
> > The following does appear to avoid the warning, but I am not sure if
> > this is the correct thing to do ...
> >
> > index 9179825ff646..095aba84f7c2 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -456,6 +456,10 @@ int device_bind_driver(struct device *dev)
> > {
> > int ret;
> >
> > + ret = device_links_check_suppliers(dev);
> > + if (ret)
> > + return ret;
> > +
> > ret = driver_sysfs_add(dev);
> > if (!ret)
> > driver_bound(dev);
>
> So digging deeper into the usage of device_bind_driver and looking at
> [1], it doesn't look like returning an error here is a good option.
> When device_bind_driver() is called, the driver's probe function isn't
> even called. So, there's no way for the driver to even defer probing
> based on any of the suppliers. So, we have a couple of options:
>
> 1. Delete all the links to suppliers that haven't bound.
Or maybe convert them to stateless links? Would that be doable at all?
> We'll still leave the links to active suppliers alone in case it helps with
> suspend/resume correctness.
> 2. Fix the warning to not warn on suppliers that haven't probed if the
> device's driver has no probe function. But this will also need fixing
> up the cleanup part when device_release_driver() is called. Also, I'm
> not sure if device_bind_driver() is ever called when the driver
> actually has a probe() function.
>
> Rafael,
>
> Option 1 above is pretty straightforward.
I would prefer this ->
> Option 2 would look something like what's at the end of this email +
> caveat about whether the probe check is sufficient.
-> because "fix the warning" really means that we haven't got the
device link state machine right and getting it right may imply a major
redesign.
Overall, I'd prefer to take a step back and allow things to stabilize
for a while to let people catch up with this.
> Do you have a preference between Option 1 vs 2? Or do you have some
> other option in mind?
>
> Thanks,
> Saravana
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 5481b6940a02..8102b3c48bbc 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1247,7 +1247,8 @@ void device_links_driver_bound(struct device *dev)
> */
> device_link_drop_managed(link);
> } else {
> - WARN_ON(link->status != DL_STATE_CONSUMER_PROBE);
> + WARN_ON(link->status != DL_STATE_CONSUMER_PROBE &&
> + dev->driver->probe);
> WRITE_ONCE(link->status, DL_STATE_ACTIVE);
> }
>
> @@ -1302,7 +1303,8 @@ static void __device_links_no_driver(struct device *dev)
> if (link->supplier->links.status == DL_DEV_DRIVER_BOUND) {
> WRITE_ONCE(link->status, DL_STATE_AVAILABLE);
> } else {
> - WARN_ON(!(link->flags & DL_FLAG_SYNC_STATE_ONLY));
> + WARN_ON(!(link->flags & DL_FLAG_SYNC_STATE_ONLY) &&
> + dev->driver->probe);
> WRITE_ONCE(link->status, DL_STATE_DORMANT);
> }
> }
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On Thu, Jan 28, 2021 at 7:03 AM Jon Hunter wrote:
>
>
> On 14/01/2021 16:56, Jon Hunter wrote:
> >
> > On 14/01/2021 16:47, Saravana Kannan wrote:
> >
> > ...
> >
> >>> Yes this is the warning shown here [0] and this is coming from
> >>> the 'Generic PHY stmmac-0:00' device.
> >>
> >> Can you print the supplier and consumer device when this warning is
> >> happening and let me know? That'd help too. I'm guessing the phy is
> >> the consumer.
> >
> >
> > Sorry I should have included that. I added a print to dump this on
> > another build but failed to include here.
> >
> > WARNING KERN Generic PHY stmmac-0:00: supplier 220.gpio (status 1)
> >
> > The status is the link->status and looks like the supplier is the
> > gpio controller. I have verified that the gpio controller is probed
> > before this successfully.
> >
> >> So the warning itself isn't a problem -- it's not breaking anything or
> >> leaking memory or anything like that. But the device link is jumping
> >> states in an incorrect manner. With enough context of this code (why
> >> the device_bind_driver() is being called directly instead of going
> >> through the normal probe path), it should be easy to fix (I'll just
> >> need to fix up the device link state).
> >
> > Correct, the board seems to boot fine, we just get this warning.
>
>
> Have you had chance to look at this further?
Hi Jon,
I finally got around to looking into this. Here's the email[1] that
describes why it's done this way.
[1] - https://lore.kernel.org/lkml/ycrjmpkjk0pxk...@lunn.ch/
>
> The following does appear to avoid the warning, but I am not sure if
> this is the correct thing to do ...
>
> index 9179825ff646..095aba84f7c2 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -456,6 +456,10 @@ int device_bind_driver(struct device *dev)
> {
> int ret;
>
> + ret = device_links_check_suppliers(dev);
> + if (ret)
> + return ret;
> +
> ret = driver_sysfs_add(dev);
> if (!ret)
> driver_bound(dev);
So digging deeper into the usage of device_bind_driver and looking at
[1], it doesn't look like returning an error here is a good option.
When device_bind_driver() is called, the driver's probe function isn't
even called. So, there's no way for the driver to even defer probing
based on any of the suppliers. So, we have a couple of options:
1. Delete all the links to suppliers that haven't bound. We'll still
leave the links to active suppliers alone in case it helps with
suspend/resume correctness.
2. Fix the warning to not warn on suppliers that haven't probed if the
device's driver has no probe function. But this will also need fixing
up the cleanup part when device_release_driver() is called. Also, I'm
not sure if device_bind_driver() is ever called when the driver
actually has a probe() function.
Rafael,
Option 1 above is pretty straightforward.
Option 2 would look something like what's at the end of this email +
caveat about whether the probe check is sufficient.
Do you have a preference between Option 1 vs 2? Or do you have some
other option in mind?
Thanks,
Saravana
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 5481b6940a02..8102b3c48bbc 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1247,7 +1247,8 @@ void device_links_driver_bound(struct device *dev)
*/
device_link_drop_managed(link);
} else {
- WARN_ON(link->status != DL_STATE_CONSUMER_PROBE);
+ WARN_ON(link->status != DL_STATE_CONSUMER_PROBE &&
+ dev->driver->probe);
WRITE_ONCE(link->status, DL_STATE_ACTIVE);
}
@@ -1302,7 +1303,8 @@ static void __device_links_no_driver(struct device *dev)
if (link->supplier->links.status == DL_DEV_DRIVER_BOUND) {
WRITE_ONCE(link->status, DL_STATE_AVAILABLE);
} else {
- WARN_ON(!(link->flags & DL_FLAG_SYNC_STATE_ONLY));
+ WARN_ON(!(link->flags & DL_FLAG_SYNC_STATE_ONLY) &&
+ dev->driver->probe);
WRITE_ONCE(link->status, DL_STATE_DORMANT);
}
}
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On Thu, Jan 28, 2021 at 7:03 AM Jon Hunter wrote:
>
>
> On 14/01/2021 16:56, Jon Hunter wrote:
> >
> > On 14/01/2021 16:47, Saravana Kannan wrote:
> >
> > ...
> >
> >>> Yes this is the warning shown here [0] and this is coming from
> >>> the 'Generic PHY stmmac-0:00' device.
> >>
> >> Can you print the supplier and consumer device when this warning is
> >> happening and let me know? That'd help too. I'm guessing the phy is
> >> the consumer.
> >
> >
> > Sorry I should have included that. I added a print to dump this on
> > another build but failed to include here.
> >
> > WARNING KERN Generic PHY stmmac-0:00: supplier 220.gpio (status 1)
> >
> > The status is the link->status and looks like the supplier is the
> > gpio controller. I have verified that the gpio controller is probed
> > before this successfully.
> >
> >> So the warning itself isn't a problem -- it's not breaking anything or
> >> leaking memory or anything like that. But the device link is jumping
> >> states in an incorrect manner. With enough context of this code (why
> >> the device_bind_driver() is being called directly instead of going
> >> through the normal probe path), it should be easy to fix (I'll just
> >> need to fix up the device link state).
> >
> > Correct, the board seems to boot fine, we just get this warning.
>
Hi Jon,
>
> Have you had chance to look at this further?
No, I feel like I'm just spending all my "upstream time" just
replying to email :)
>
> The following does appear to avoid the warning, but I am not sure if
> this is the correct thing to do ...
>
> index 9179825ff646..095aba84f7c2 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -456,6 +456,10 @@ int device_bind_driver(struct device *dev)
> {
> int ret;
>
> + ret = device_links_check_suppliers(dev);
> + if (ret)
> + return ret;
> +
Yeah I knew calling this function (where device_bind_driver() was
called) would take away the warning, but I first want to understand
why the caller wasn't going through the typical device/driver probe
path before I started adding more of the typical device/driver probe
path code in. I don't want to add in code they might have been
explicitly trying to avoid.
Also, once you do this, you'll need the reverse of this (deleting
links/unsetting state change) somewhere.
Also, device_bind_driver() is used in a bunch of places. Need to check
if it's right to call device_links_check_suppliers() in those
instances.
Feel free to look at those items above. I'll try to get to this once I
take care of the "my device is not working!" issues.
Thanks,
Saravana
> ret = driver_sysfs_add(dev);
> if (!ret)
> driver_bound(dev);
>
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On 14/01/2021 16:56, Jon Hunter wrote:
>
> On 14/01/2021 16:47, Saravana Kannan wrote:
>
> ...
>
>>> Yes this is the warning shown here [0] and this is coming from
>>> the 'Generic PHY stmmac-0:00' device.
>>
>> Can you print the supplier and consumer device when this warning is
>> happening and let me know? That'd help too. I'm guessing the phy is
>> the consumer.
>
>
> Sorry I should have included that. I added a print to dump this on
> another build but failed to include here.
>
> WARNING KERN Generic PHY stmmac-0:00: supplier 220.gpio (status 1)
>
> The status is the link->status and looks like the supplier is the
> gpio controller. I have verified that the gpio controller is probed
> before this successfully.
>
>> So the warning itself isn't a problem -- it's not breaking anything or
>> leaking memory or anything like that. But the device link is jumping
>> states in an incorrect manner. With enough context of this code (why
>> the device_bind_driver() is being called directly instead of going
>> through the normal probe path), it should be easy to fix (I'll just
>> need to fix up the device link state).
>
> Correct, the board seems to boot fine, we just get this warning.
Have you had chance to look at this further?
The following does appear to avoid the warning, but I am not sure if
this is the correct thing to do ...
index 9179825ff646..095aba84f7c2 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -456,6 +456,10 @@ int device_bind_driver(struct device *dev)
{
int ret;
+ ret = device_links_check_suppliers(dev);
+ if (ret)
+ return ret;
+
ret = driver_sysfs_add(dev);
if (!ret)
driver_bound(dev);
Cheers
Jon
--
nvpublic
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On Fri, Jan 15, 2021 at 8:13 AM Jon Hunter wrote:
>
>
> On 14/01/2021 21:50, Saravana Kannan wrote:
> > On Thu, Jan 14, 2021 at 10:55 AM Jon Hunter wrote:
> >>
> >>
> >> On 14/01/2021 16:52, Saravana Kannan wrote:
> >>
> >> ...
> >>
> >>> Thanks! I think you forgot to enable those logs though. Also, while
> >>> you are at it, maybe enable the logs in device_link_add() too please?
> >>
> >>
> >> Sorry try this one.
> >>
> >> Cheers
> >> Jon
> >
> > Phew! That took almost 4 hours to debug on the side! I think I figured
> > it out. Can you try this patch? If it works or improves things, I'll
> > explain why it helps.
> >
> > -Saravana
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index 5f9eed79a8aa..1c8c65c4a887 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -1258,6 +1258,8 @@ DEFINE_SIMPLE_PROP(pinctrl5, "pinctrl-5", NULL)
> > DEFINE_SIMPLE_PROP(pinctrl6, "pinctrl-6", NULL)
> > DEFINE_SIMPLE_PROP(pinctrl7, "pinctrl-7", NULL)
> > DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL)
> > +DEFINE_SIMPLE_PROP(gpio_compat, "gpio", "#gpio-cells")
> > +DEFINE_SIMPLE_PROP(gpios_compat, "gpios", "#gpio-cells")
> > DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
> > DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
> > DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells")
> > @@ -1296,6 +1298,8 @@ static const struct supplier_bindings
> > of_supplier_bindings[] = {
> > { .parse_prop = parse_pinctrl6, },
> > { .parse_prop = parse_pinctrl7, },
> > { .parse_prop = parse_pinctrl8, },
> > + { .parse_prop = parse_gpio_compat, },
> > + { .parse_prop = parse_gpios_compat, },
> > { .parse_prop = parse_regulators, },
> > { .parse_prop = parse_gpio, },
> > { .parse_prop = parse_gpios, },
> >
>
> Thanks, that worked!
>
> Tested-by: Jon Hunter
>
> Thanks for digging into that one. Would have taken me more than 4 hours!
Thanks for testing. What was happening was that there was a cycle of
2-3 devices. A -(depends on)-> B -> C -> A.
And fw_devlink only understood A -> B since the rest were the gpio
bindings I added above. Without fw_devlink seeing the cycle, it can't
do cycle workarounds. So C's driver was deferring probe waiting on A
and none of them probed.
Once I added these and made the cycle visible to fw_devlink, it
handled it fine (basically between A, B and C, the device links don't
affect probe order anymore).
-Saravana
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On 14/01/2021 21:50, Saravana Kannan wrote:
> On Thu, Jan 14, 2021 at 10:55 AM Jon Hunter wrote:
>>
>>
>> On 14/01/2021 16:52, Saravana Kannan wrote:
>>
>> ...
>>
>>> Thanks! I think you forgot to enable those logs though. Also, while
>>> you are at it, maybe enable the logs in device_link_add() too please?
>>
>>
>> Sorry try this one.
>>
>> Cheers
>> Jon
>
> Phew! That took almost 4 hours to debug on the side! I think I figured
> it out. Can you try this patch? If it works or improves things, I'll
> explain why it helps.
>
> -Saravana
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 5f9eed79a8aa..1c8c65c4a887 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1258,6 +1258,8 @@ DEFINE_SIMPLE_PROP(pinctrl5, "pinctrl-5", NULL)
> DEFINE_SIMPLE_PROP(pinctrl6, "pinctrl-6", NULL)
> DEFINE_SIMPLE_PROP(pinctrl7, "pinctrl-7", NULL)
> DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL)
> +DEFINE_SIMPLE_PROP(gpio_compat, "gpio", "#gpio-cells")
> +DEFINE_SIMPLE_PROP(gpios_compat, "gpios", "#gpio-cells")
> DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
> DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
> DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells")
> @@ -1296,6 +1298,8 @@ static const struct supplier_bindings
> of_supplier_bindings[] = {
> { .parse_prop = parse_pinctrl6, },
> { .parse_prop = parse_pinctrl7, },
> { .parse_prop = parse_pinctrl8, },
> + { .parse_prop = parse_gpio_compat, },
> + { .parse_prop = parse_gpios_compat, },
> { .parse_prop = parse_regulators, },
> { .parse_prop = parse_gpio, },
> { .parse_prop = parse_gpios, },
>
Thanks, that worked!
Tested-by: Jon Hunter
Thanks for digging into that one. Would have taken me more than 4 hours!
Jon
--
nvpublic
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On Thu, Jan 14, 2021 at 10:55 AM Jon Hunter wrote:
>
>
> On 14/01/2021 16:52, Saravana Kannan wrote:
>
> ...
>
> > Thanks! I think you forgot to enable those logs though. Also, while
> > you are at it, maybe enable the logs in device_link_add() too please?
>
>
> Sorry try this one.
>
> Cheers
> Jon
Phew! That took almost 4 hours to debug on the side! I think I figured
it out. Can you try this patch? If it works or improves things, I'll
explain why it helps.
-Saravana
diff --git a/drivers/of/property.c b/drivers/of/property.c
index 5f9eed79a8aa..1c8c65c4a887 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1258,6 +1258,8 @@ DEFINE_SIMPLE_PROP(pinctrl5, "pinctrl-5", NULL)
DEFINE_SIMPLE_PROP(pinctrl6, "pinctrl-6", NULL)
DEFINE_SIMPLE_PROP(pinctrl7, "pinctrl-7", NULL)
DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL)
+DEFINE_SIMPLE_PROP(gpio_compat, "gpio", "#gpio-cells")
+DEFINE_SIMPLE_PROP(gpios_compat, "gpios", "#gpio-cells")
DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells")
@@ -1296,6 +1298,8 @@ static const struct supplier_bindings
of_supplier_bindings[] = {
{ .parse_prop = parse_pinctrl6, },
{ .parse_prop = parse_pinctrl7, },
{ .parse_prop = parse_pinctrl8, },
+ { .parse_prop = parse_gpio_compat, },
+ { .parse_prop = parse_gpios_compat, },
{ .parse_prop = parse_regulators, },
{ .parse_prop = parse_gpio, },
{ .parse_prop = parse_gpios, },
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On 14/01/2021 16:47, Saravana Kannan wrote: ... >> Yes this is the warning shown here [0] and this is coming from >> the 'Generic PHY stmmac-0:00' device. > > Can you print the supplier and consumer device when this warning is > happening and let me know? That'd help too. I'm guessing the phy is > the consumer. Sorry I should have included that. I added a print to dump this on another build but failed to include here. WARNING KERN Generic PHY stmmac-0:00: supplier 220.gpio (status 1) The status is the link->status and looks like the supplier is the gpio controller. I have verified that the gpio controller is probed before this successfully. > So the warning itself isn't a problem -- it's not breaking anything or > leaking memory or anything like that. But the device link is jumping > states in an incorrect manner. With enough context of this code (why > the device_bind_driver() is being called directly instead of going > through the normal probe path), it should be easy to fix (I'll just > need to fix up the device link state). Correct, the board seems to boot fine, we just get this warning. Cheers Jon -- nvpublic
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On Thu, Jan 14, 2021 at 8:48 AM Jon Hunter wrote: > > > On 14/01/2021 16:40, Saravana Kannan wrote: > > On Thu, Jan 14, 2021 at 3:35 AM Jon Hunter wrote: > >> > >> > >> On 13/01/2021 21:29, Saravana Kannan wrote: > >> > >> ... > >> > I am seeing the same problem on Tegra30 Cardhu A04 where several > regulators > are continuously deferred and prevents the board from booting ... > > [2.518334] platform panel: probe deferral - supplier regulator@11 > not ready > > [2.525503] platform regulator@1: probe deferral - supplier 4-002d > not ready > > [2.533141] platform regulator@3: probe deferral - supplier > regulator@101 not ready > > [2.540856] platform regulator@5: probe deferral - supplier > regulator@101 not ready > > [2.548589] platform regulator@6: probe deferral - supplier > regulator@101 not ready > > [2.556316] platform regulator@7: probe deferral - supplier > regulator@101 not ready > > [2.564041] platform regulator@8: probe deferral - supplier > regulator@101 not ready > > [2.571743] platform regulator@9: probe deferral - supplier > regulator@101 not ready > > [2.579463] platform regulator@10: probe deferral - supplier > regulator@101 not ready > > [2.587273] platform regulator@11: probe deferral - supplier > regulator@101 not ready > > [2.595088] platform regulator@12: probe deferral - supplier > regulator@104 not ready > > [2.603837] platform regulator@102: probe deferral - supplier > regulator@104 not ready > > [2.611726] platform regulator@103: probe deferral - supplier > regulator@104 not ready > > [2.620137] platform 3000.pcie: probe deferral - supplier regulator@5 > not ready > >>> > >>> Looks like this is not the whole log? Do you see any "wait for > >>> supplier" logs? That's what all these boot issues should boil down to. > >>> And as usual, pointer to DT for this board please. > >> > >> Ah yes I see ... > >> > >> platform regulator@1: probe deferral - wait for supplier tps65911@2d > > > > Do you mind sharing the full log please? It's hard to tell you > > anything useful with bits and pieces of logs. > > > >> Yes the device-tree for this board can be found here [0]. Looks like > >> there is a circular dependency between the vddctrl_reg and vddcore_reg. > >> This is part of coupled regulators which have a two-way linkage [1]. So > >> this change appears to conflict with this. > > > > fw_devlink doesn't track "regulator-coupled-with". So that's probably > > not it. Also, this patch series was made to handle simple cycles > > properly. It'll functionally disable the device links it created when > > it comes to probe ordering. Only two overlapping cycles might cause > > issues -- and even that, not all the time. So yeah, full log please. > > > No problem. Please find attached. Thanks! I think you forgot to enable those logs though. Also, while you are at it, maybe enable the logs in device_link_add() too please? -Saravana > > Cheers > Jon > > > -- > nvpublic
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On Wed, Jan 13, 2021 at 11:48 PM Andy Shevchenko wrote: > > > > On Monday, December 21, 2020, Jisheng Zhang > wrote: >> >> On Thu, 17 Dec 2020 19:16:58 -0800 Saravana Kannan wrote: >> >> >> > >> > >> > As discussed in LPC 2020, cyclic dependencies in firmware that couldn't >> > be broken using logic was one of the last remaining reasons >> > fw_devlink=on couldn't be set by default. >> > >> > This series changes fw_devlink so that when a cyclic dependency is found >> > in firmware, the links between those devices fallback to permissive mode >> > behavior. This way, the rest of the system still benefits from >> > fw_devlink, but the ambiguous cases fallback to permissive mode. >> > >> > Setting fw_devlink=on by default brings a bunch of benefits (currently, >> > only for systems with device tree firmware): >> > * Significantly cuts down deferred probes. >> > * Device probe is effectively attempted in graph order. >> > * Makes it much easier to load drivers as modules without having to >> > worry about functional dependencies between modules (depmod is still >> > needed for symbol dependencies). >> > >> > Greg/Rafael, >> > >> > Can we get this pulled into 5.11-rc1 or -rc2 soon please? I expect to >> > see some issues due to device drivers that aren't following best >> > practices (they don't expose the device to driver core). Want to >> > identify those early on and try to have them fixed before 5.11 release. >> > See [1] for an example of such a case. >> > >> > If we do end up have to revert anything, it'll just be Patch 5/5 (a one >> > liner). >> > >> > Marc, >> > >> > You had hit issues with fw_devlink=on before on some of your systems. >> > Want to give this a shot? >> > >> > Jisheng, >> > >> > Want to fix up one of those gpio drivers you were having problems with? >> > >> >> Hi Saravana, >> >> I didn't send fix for the gpio-dwapb.c in last development window, so can >> send patch once 5.11-rc1 is released. > > > If you are going to do anything with that GPIO driver, it should be removal > of compatible strings from the device child nodes. The driver IIRC never used > them anyhow anyway. We already discussed this in a different thread. Just deleting DT is not okay. That breaks a new kernel + old DT combo. Upgrading the kernel shouldn't break a board. -Saravana
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On Thu, Jan 14, 2021 at 8:11 AM Jon Hunter wrote: > > > On 13/01/2021 21:26, Saravana Kannan wrote: > > On Wed, Jan 13, 2021 at 3:30 AM Jon Hunter wrote: > >> > >> > >> On 18/12/2020 03:16, Saravana Kannan wrote: > >>> As discussed in LPC 2020, cyclic dependencies in firmware that couldn't > >>> be broken using logic was one of the last remaining reasons > >>> fw_devlink=on couldn't be set by default. > >>> > >>> This series changes fw_devlink so that when a cyclic dependency is found > >>> in firmware, the links between those devices fallback to permissive mode > >>> behavior. This way, the rest of the system still benefits from > >>> fw_devlink, but the ambiguous cases fallback to permissive mode. > >>> > >>> Setting fw_devlink=on by default brings a bunch of benefits (currently, > >>> only for systems with device tree firmware): > >>> * Significantly cuts down deferred probes. > >>> * Device probe is effectively attempted in graph order. > >>> * Makes it much easier to load drivers as modules without having to > >>> worry about functional dependencies between modules (depmod is still > >>> needed for symbol dependencies). > >> > >> > >> One issue we have come across with this is the of_mdio.c driver. On > >> Tegra194 Jetson Xavier I am seeing the following ... > >> > >> boot: logs: [ 4.194791] WARNING KERN WARNING: CPU: 0 PID: 1 at > >> /dvs/git/dirty/git-master_l4t-upstream/kernel/drivers/base/core.c:1189 > >> device_links_driver_bound+0x240/0x260 > >> boot: logs: [ 4.207683] WARNING KERN Modules linked in: > >> boot: logs: [ 4.210691] WARNING KERN CPU: 0 PID: 1 Comm: swapper/0 > >> Not tainted 5.11.0-rc3-next-20210112-gdf869cab4b35 #1 > >> boot: logs: [ 4.219221] WARNING KERN Hardware name: NVIDIA Jetson > >> AGX Xavier Developer Kit (DT) > >> boot: logs: [ 4.225628] WARNING KERN pstate: 8049 (Nzcv daif > >> +PAN -UAO -TCO BTYPE=--) > >> boot: logs: [ 4.231542] WARNING KERN pc : > >> device_links_driver_bound+0x240/0x260 > >> boot: logs: [ 4.236587] WARNING KERN lr : > >> device_links_driver_bound+0xf8/0x260 > >> boot: logs: [ 4.241560] WARNING KERN sp : 800011f4b980 > >> boot: logs: [ 4.244819] WARNING KERN x29: 800011f4b980 x28: > >> 8208a0a0 > >> boot: logs: [ 4.250051] WARNING KERN x27: 8208a080 x26: > >> > >> boot: logs: [ 4.255271] WARNING KERN x25: 0003 x24: > >> 800011b99000 > >> boot: logs: [ 4.260489] WARNING KERN x23: 0001 x22: > >> 800011df14f0 > >> boot: logs: [ 4.265706] WARNING KERN x21: 800011f4b9f8 x20: > >> 800011df1000 > >> boot: logs: [ 4.270934] WARNING KERN x19: 8208a000 x18: > >> 0005 > >> boot: logs: [ 4.276166] WARNING KERN x17: 0007 x16: > >> 0001 > >> boot: logs: [ 4.281382] WARNING KERN x15: 80030c90 x14: > >> 805c9df8 > >> boot: logs: [ 4.286618] WARNING KERN x13: x12: > >> 80030c90 > >> boot: logs: [ 4.291847] WARNING KERN x11: 805c9da8 x10: > >> 0040 > >> boot: logs: [ 4.297061] WARNING KERN x9 : 80030c98 x8 : > >> > >> boot: logs: [ 4.302291] WARNING KERN x7 : 0009 x6 : > >> > >> boot: logs: [ 4.307509] WARNING KERN x5 : 8010 x4 : > >> > >> boot: logs: [ 4.312739] WARNING KERN x3 : 800011df1e38 x2 : > >> 80908c10 > >> boot: logs: [ 4.317956] WARNING KERN x1 : 0001 x0 : > >> 809ca400 > >> boot: logs: [ 4.323183] WARNING KERN Call trace: > >> boot: logs: [ 4.325593] WARNING KERN > >> device_links_driver_bound+0x240/0x260 > >> boot: logs: [ 4.330301] WARNING KERN driver_bound+0x70/0xd0 > >> boot: logs: [ 4.333740] WARNING KERN device_bind_driver+0x50/0x60 > >> boot: logs: [ 4.337671] WARNING KERN phy_attach_direct+0x258/0x2e0 > >> boot: logs: [ 4.341718] WARNING KERN > >> phylink_of_phy_connect+0x7c/0x140 > >> boot: logs: [ 4.346081] WARNING KERN stmmac_open+0xb04/0xc70 > >> boot: logs: [ 4.349612] WARNING KERN __dev_open+0xe0/0x190 > >> boot: logs: [ 4.352972] WARNING KERN __dev_change_flags+0x16c/0x1b8 > >> boot: logs: [ 4.357081] WARNING KERN dev_change_flags+0x20/0x60 > >> boot: logs: [ 4.360856] WARNING KERN ip_auto_config+0x2a0/0xfe8 > >> boot: logs: [ 4.364633] WARNING KERN do_one_initcall+0x58/0x1b8 > >> boot: logs: [ 4.368405] WARNING KERN > >> kernel_init_freeable+0x1ec/0x240 > >> boot: logs: [ 4.372698] WARNING KERN kernel_init+0x10/0x110 > >> boot: logs: [ 4.376130] WARNING KERN ret_from_fork+0x10/0x18 > >> > >> > >> So looking at this change does this mean that the of_mdio needs to be > >> converted to a proper driver? > > > > Sorry, there's not enough context in this log for me to tell how
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On 14/01/2021 16:40, Saravana Kannan wrote: > On Thu, Jan 14, 2021 at 3:35 AM Jon Hunter wrote: >> >> >> On 13/01/2021 21:29, Saravana Kannan wrote: >> >> ... >> I am seeing the same problem on Tegra30 Cardhu A04 where several regulators are continuously deferred and prevents the board from booting ... [2.518334] platform panel: probe deferral - supplier regulator@11 not ready [2.525503] platform regulator@1: probe deferral - supplier 4-002d not ready [2.533141] platform regulator@3: probe deferral - supplier regulator@101 not ready [2.540856] platform regulator@5: probe deferral - supplier regulator@101 not ready [2.548589] platform regulator@6: probe deferral - supplier regulator@101 not ready [2.556316] platform regulator@7: probe deferral - supplier regulator@101 not ready [2.564041] platform regulator@8: probe deferral - supplier regulator@101 not ready [2.571743] platform regulator@9: probe deferral - supplier regulator@101 not ready [2.579463] platform regulator@10: probe deferral - supplier regulator@101 not ready [2.587273] platform regulator@11: probe deferral - supplier regulator@101 not ready [2.595088] platform regulator@12: probe deferral - supplier regulator@104 not ready [2.603837] platform regulator@102: probe deferral - supplier regulator@104 not ready [2.611726] platform regulator@103: probe deferral - supplier regulator@104 not ready [2.620137] platform 3000.pcie: probe deferral - supplier regulator@5 not ready >>> >>> Looks like this is not the whole log? Do you see any "wait for >>> supplier" logs? That's what all these boot issues should boil down to. >>> And as usual, pointer to DT for this board please. >> >> Ah yes I see ... >> >> platform regulator@1: probe deferral - wait for supplier tps65911@2d > > Do you mind sharing the full log please? It's hard to tell you > anything useful with bits and pieces of logs. > >> Yes the device-tree for this board can be found here [0]. Looks like >> there is a circular dependency between the vddctrl_reg and vddcore_reg. >> This is part of coupled regulators which have a two-way linkage [1]. So >> this change appears to conflict with this. > > fw_devlink doesn't track "regulator-coupled-with". So that's probably > not it. Also, this patch series was made to handle simple cycles > properly. It'll functionally disable the device links it created when > it comes to probe ordering. Only two overlapping cycles might cause > issues -- and even that, not all the time. So yeah, full log please. No problem. Please find attached. Cheers Jon -- nvpublic [ 111.269288] VFS: Unable to mount root fs via NFS. [ 111.274136] devtmpfs: mounted [ 111.278710] Freeing unused kernel memory: 1024K [ 111.297234] Run /sbin/init as init process [ 111.301355] with arguments: [ 111.304328] /sbin/init [ 111.307076] netdevwait [ 111.309794] with environment: [ 111.312940] HOME=/ [ 111.315303] TERM=linux [ 111.318333] Run /etc/init as init process [ 111.322358] with arguments: [ 111.325330] /etc/init [ 111.327984] netdevwait [ 111.330702] with environment: [ 111.333847] HOME=/ [ 111.336265] TERM=linux [ 111.339174] Run /bin/init as init process [ 111.343196] with arguments: [ 111.346206] /bin/init [ 111.348837] netdevwait [ 111.351548] with environment: [ 111.354692] HOME=/ [ 111.357110] TERM=linux [ 111.360010] Run /bin/sh as init process [ 111.363858] with arguments: [ 111.366860] /bin/sh [ 111.369316] netdevwait [ 111.372027] with environment: [ 111.375170] HOME=/ [ 111.377608] TERM=linux [ 111.380510] Kernel panic - not syncing: No working init found. Try passing init= option to kernel. See Linux Documentation/admin-guide/init.rst for guidance. [ 111.394699] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.11.0-rc3-next-20210112-gdf869cab4b35 #1 [ 111.403415] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree) [ 111.409697] [] (unwind_backtrace) from [] (show_stack+0x10/0x14) [ 111.417489] [] (show_stack) from [] (dump_stack+0xc8/0xdc) [ 111.424750] [] (dump_stack) from [] (panic+0x114/0x32c) [ 111.431736] [] (panic) from [] (kernel_init+0x108/0x118) [ 111.438807] [] (kernel_init) from [] (ret_from_fork+0x14/0x24) [ 111.446399] Exception stack(0xc1507fb0 to 0xc1507ff8) [ 111.451461] 7fa0: [ 111.459652] 7fc0: [ 111.467841] 7fe0: 0013 [ 111.474475] CPU3: stopping [ 111.477198] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 5.11.0-rc3-next-20210112-gdf869cab4b35
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On Thu, Jan 14, 2021 at 3:35 AM Jon Hunter wrote: > > > On 13/01/2021 21:29, Saravana Kannan wrote: > > ... > > >> I am seeing the same problem on Tegra30 Cardhu A04 where several regulators > >> are continuously deferred and prevents the board from booting ... > >> > >> [2.518334] platform panel: probe deferral - supplier regulator@11 not > >> ready > >> > >> [2.525503] platform regulator@1: probe deferral - supplier 4-002d not > >> ready > >> > >> [2.533141] platform regulator@3: probe deferral - supplier > >> regulator@101 not ready > >> > >> [2.540856] platform regulator@5: probe deferral - supplier > >> regulator@101 not ready > >> > >> [2.548589] platform regulator@6: probe deferral - supplier > >> regulator@101 not ready > >> > >> [2.556316] platform regulator@7: probe deferral - supplier > >> regulator@101 not ready > >> > >> [2.564041] platform regulator@8: probe deferral - supplier > >> regulator@101 not ready > >> > >> [2.571743] platform regulator@9: probe deferral - supplier > >> regulator@101 not ready > >> > >> [2.579463] platform regulator@10: probe deferral - supplier > >> regulator@101 not ready > >> > >> [2.587273] platform regulator@11: probe deferral - supplier > >> regulator@101 not ready > >> > >> [2.595088] platform regulator@12: probe deferral - supplier > >> regulator@104 not ready > >> > >> [2.603837] platform regulator@102: probe deferral - supplier > >> regulator@104 not ready > >> > >> [2.611726] platform regulator@103: probe deferral - supplier > >> regulator@104 not ready > >> > >> [2.620137] platform 3000.pcie: probe deferral - supplier regulator@5 > >> not ready > > > > Looks like this is not the whole log? Do you see any "wait for > > supplier" logs? That's what all these boot issues should boil down to. > > And as usual, pointer to DT for this board please. > > Ah yes I see ... > > platform regulator@1: probe deferral - wait for supplier tps65911@2d Do you mind sharing the full log please? It's hard to tell you anything useful with bits and pieces of logs. > Yes the device-tree for this board can be found here [0]. Looks like > there is a circular dependency between the vddctrl_reg and vddcore_reg. > This is part of coupled regulators which have a two-way linkage [1]. So > this change appears to conflict with this. fw_devlink doesn't track "regulator-coupled-with". So that's probably not it. Also, this patch series was made to handle simple cycles properly. It'll functionally disable the device links it created when it comes to probe ordering. Only two overlapping cycles might cause issues -- and even that, not all the time. So yeah, full log please. -Saravana > Jon > > [0] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/tegra30-cardhu-a04.dts > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/regulator/regulator.yaml#n129 > > -- > nvpublic
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On 13/01/2021 21:26, Saravana Kannan wrote: > On Wed, Jan 13, 2021 at 3:30 AM Jon Hunter wrote: >> >> >> On 18/12/2020 03:16, Saravana Kannan wrote: >>> As discussed in LPC 2020, cyclic dependencies in firmware that couldn't >>> be broken using logic was one of the last remaining reasons >>> fw_devlink=on couldn't be set by default. >>> >>> This series changes fw_devlink so that when a cyclic dependency is found >>> in firmware, the links between those devices fallback to permissive mode >>> behavior. This way, the rest of the system still benefits from >>> fw_devlink, but the ambiguous cases fallback to permissive mode. >>> >>> Setting fw_devlink=on by default brings a bunch of benefits (currently, >>> only for systems with device tree firmware): >>> * Significantly cuts down deferred probes. >>> * Device probe is effectively attempted in graph order. >>> * Makes it much easier to load drivers as modules without having to >>> worry about functional dependencies between modules (depmod is still >>> needed for symbol dependencies). >> >> >> One issue we have come across with this is the of_mdio.c driver. On >> Tegra194 Jetson Xavier I am seeing the following ... >> >> boot: logs: [ 4.194791] WARNING KERN WARNING: CPU: 0 PID: 1 at >> /dvs/git/dirty/git-master_l4t-upstream/kernel/drivers/base/core.c:1189 >> device_links_driver_bound+0x240/0x260 >> boot: logs: [ 4.207683] WARNING KERN Modules linked in: >> boot: logs: [ 4.210691] WARNING KERN CPU: 0 PID: 1 Comm: swapper/0 Not >> tainted 5.11.0-rc3-next-20210112-gdf869cab4b35 #1 >> boot: logs: [ 4.219221] WARNING KERN Hardware name: NVIDIA Jetson AGX >> Xavier Developer Kit (DT) >> boot: logs: [ 4.225628] WARNING KERN pstate: 8049 (Nzcv daif +PAN >> -UAO -TCO BTYPE=--) >> boot: logs: [ 4.231542] WARNING KERN pc : >> device_links_driver_bound+0x240/0x260 >> boot: logs: [ 4.236587] WARNING KERN lr : >> device_links_driver_bound+0xf8/0x260 >> boot: logs: [ 4.241560] WARNING KERN sp : 800011f4b980 >> boot: logs: [ 4.244819] WARNING KERN x29: 800011f4b980 x28: >> 8208a0a0 >> boot: logs: [ 4.250051] WARNING KERN x27: 8208a080 x26: >> >> boot: logs: [ 4.255271] WARNING KERN x25: 0003 x24: >> 800011b99000 >> boot: logs: [ 4.260489] WARNING KERN x23: 0001 x22: >> 800011df14f0 >> boot: logs: [ 4.265706] WARNING KERN x21: 800011f4b9f8 x20: >> 800011df1000 >> boot: logs: [ 4.270934] WARNING KERN x19: 8208a000 x18: >> 0005 >> boot: logs: [ 4.276166] WARNING KERN x17: 0007 x16: >> 0001 >> boot: logs: [ 4.281382] WARNING KERN x15: 80030c90 x14: >> 805c9df8 >> boot: logs: [ 4.286618] WARNING KERN x13: x12: >> 80030c90 >> boot: logs: [ 4.291847] WARNING KERN x11: 805c9da8 x10: >> 0040 >> boot: logs: [ 4.297061] WARNING KERN x9 : 80030c98 x8 : >> >> boot: logs: [ 4.302291] WARNING KERN x7 : 0009 x6 : >> >> boot: logs: [ 4.307509] WARNING KERN x5 : 8010 x4 : >> >> boot: logs: [ 4.312739] WARNING KERN x3 : 800011df1e38 x2 : >> 80908c10 >> boot: logs: [ 4.317956] WARNING KERN x1 : 0001 x0 : >> 809ca400 >> boot: logs: [ 4.323183] WARNING KERN Call trace: >> boot: logs: [ 4.325593] WARNING KERN >> device_links_driver_bound+0x240/0x260 >> boot: logs: [ 4.330301] WARNING KERN driver_bound+0x70/0xd0 >> boot: logs: [ 4.333740] WARNING KERN device_bind_driver+0x50/0x60 >> boot: logs: [ 4.337671] WARNING KERN phy_attach_direct+0x258/0x2e0 >> boot: logs: [ 4.341718] WARNING KERN phylink_of_phy_connect+0x7c/0x140 >> boot: logs: [ 4.346081] WARNING KERN stmmac_open+0xb04/0xc70 >> boot: logs: [ 4.349612] WARNING KERN __dev_open+0xe0/0x190 >> boot: logs: [ 4.352972] WARNING KERN __dev_change_flags+0x16c/0x1b8 >> boot: logs: [ 4.357081] WARNING KERN dev_change_flags+0x20/0x60 >> boot: logs: [ 4.360856] WARNING KERN ip_auto_config+0x2a0/0xfe8 >> boot: logs: [ 4.364633] WARNING KERN do_one_initcall+0x58/0x1b8 >> boot: logs: [ 4.368405] WARNING KERN kernel_init_freeable+0x1ec/0x240 >> boot: logs: [ 4.372698] WARNING KERN kernel_init+0x10/0x110 >> boot: logs: [ 4.376130] WARNING KERN ret_from_fork+0x10/0x18 >> >> >> So looking at this change does this mean that the of_mdio needs to be >> converted to a proper driver? > > Sorry, there's not enough context in this log for me to tell how this > is even related to of_mdio.c. My guess is this is related to network > stack directly calling device_bind_driver() and not updating device > link state correctly. See what device_links_check_suppliers() does in > the normal path. I think I know
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On 13/01/2021 21:29, Saravana Kannan wrote: ... >> I am seeing the same problem on Tegra30 Cardhu A04 where several regulators >> are continuously deferred and prevents the board from booting ... >> >> [2.518334] platform panel: probe deferral - supplier regulator@11 not >> ready >> >> [2.525503] platform regulator@1: probe deferral - supplier 4-002d not >> ready >> >> [2.533141] platform regulator@3: probe deferral - supplier regulator@101 >> not ready >> >> [2.540856] platform regulator@5: probe deferral - supplier regulator@101 >> not ready >> >> [2.548589] platform regulator@6: probe deferral - supplier regulator@101 >> not ready >> >> [2.556316] platform regulator@7: probe deferral - supplier regulator@101 >> not ready >> >> [2.564041] platform regulator@8: probe deferral - supplier regulator@101 >> not ready >> >> [2.571743] platform regulator@9: probe deferral - supplier regulator@101 >> not ready >> >> [2.579463] platform regulator@10: probe deferral - supplier >> regulator@101 not ready >> >> [2.587273] platform regulator@11: probe deferral - supplier >> regulator@101 not ready >> >> [2.595088] platform regulator@12: probe deferral - supplier >> regulator@104 not ready >> >> [2.603837] platform regulator@102: probe deferral - supplier >> regulator@104 not ready >> >> [2.611726] platform regulator@103: probe deferral - supplier >> regulator@104 not ready >> >> [2.620137] platform 3000.pcie: probe deferral - supplier regulator@5 not >> ready > > Looks like this is not the whole log? Do you see any "wait for > supplier" logs? That's what all these boot issues should boil down to. > And as usual, pointer to DT for this board please. Ah yes I see ... platform regulator@1: probe deferral - wait for supplier tps65911@2d Yes the device-tree for this board can be found here [0]. Looks like there is a circular dependency between the vddctrl_reg and vddcore_reg. This is part of coupled regulators which have a two-way linkage [1]. So this change appears to conflict with this. Jon [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/tegra30-cardhu-a04.dts [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/regulator/regulator.yaml#n129 -- nvpublic
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On Wed, Jan 13, 2021 at 3:48 AM Marc Zyngier wrote: > > On 2021-01-13 11:44, Nicolas Saenz Julienne wrote: > > On Thu, 2020-12-17 at 19:16 -0800, Saravana Kannan wrote: > >> As discussed in LPC 2020, cyclic dependencies in firmware that > >> couldn't > >> be broken using logic was one of the last remaining reasons > >> fw_devlink=on couldn't be set by default. > >> > >> This series changes fw_devlink so that when a cyclic dependency is > >> found > >> in firmware, the links between those devices fallback to permissive > >> mode > >> behavior. This way, the rest of the system still benefits from > >> fw_devlink, but the ambiguous cases fallback to permissive mode. > >> > >> Setting fw_devlink=on by default brings a bunch of benefits > >> (currently, > >> only for systems with device tree firmware): > >> * Significantly cuts down deferred probes. > >> * Device probe is effectively attempted in graph order. > >> * Makes it much easier to load drivers as modules without having to > >> worry about functional dependencies between modules (depmod is still > >> needed for symbol dependencies). > > > > FWIW I don't see any issues with this on Raspberry Pi 4 :). > > Keep bragging! ;-) > Yay! Thanks for confirming Nicolas. -Saravana
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On Wed, Jan 13, 2021 at 3:30 AM Jon Hunter wrote: > > > On 18/12/2020 03:16, Saravana Kannan wrote: > > As discussed in LPC 2020, cyclic dependencies in firmware that couldn't > > be broken using logic was one of the last remaining reasons > > fw_devlink=on couldn't be set by default. > > > > This series changes fw_devlink so that when a cyclic dependency is found > > in firmware, the links between those devices fallback to permissive mode > > behavior. This way, the rest of the system still benefits from > > fw_devlink, but the ambiguous cases fallback to permissive mode. > > > > Setting fw_devlink=on by default brings a bunch of benefits (currently, > > only for systems with device tree firmware): > > * Significantly cuts down deferred probes. > > * Device probe is effectively attempted in graph order. > > * Makes it much easier to load drivers as modules without having to > > worry about functional dependencies between modules (depmod is still > > needed for symbol dependencies). > > > One issue we have come across with this is the of_mdio.c driver. On > Tegra194 Jetson Xavier I am seeing the following ... > > boot: logs: [ 4.194791] WARNING KERN WARNING: CPU: 0 PID: 1 at > /dvs/git/dirty/git-master_l4t-upstream/kernel/drivers/base/core.c:1189 > device_links_driver_bound+0x240/0x260 > boot: logs: [ 4.207683] WARNING KERN Modules linked in: > boot: logs: [ 4.210691] WARNING KERN CPU: 0 PID: 1 Comm: swapper/0 Not > tainted 5.11.0-rc3-next-20210112-gdf869cab4b35 #1 > boot: logs: [ 4.219221] WARNING KERN Hardware name: NVIDIA Jetson AGX > Xavier Developer Kit (DT) > boot: logs: [ 4.225628] WARNING KERN pstate: 8049 (Nzcv daif +PAN > -UAO -TCO BTYPE=--) > boot: logs: [ 4.231542] WARNING KERN pc : > device_links_driver_bound+0x240/0x260 > boot: logs: [ 4.236587] WARNING KERN lr : > device_links_driver_bound+0xf8/0x260 > boot: logs: [ 4.241560] WARNING KERN sp : 800011f4b980 > boot: logs: [ 4.244819] WARNING KERN x29: 800011f4b980 x28: > 8208a0a0 > boot: logs: [ 4.250051] WARNING KERN x27: 8208a080 x26: > > boot: logs: [ 4.255271] WARNING KERN x25: 0003 x24: > 800011b99000 > boot: logs: [ 4.260489] WARNING KERN x23: 0001 x22: > 800011df14f0 > boot: logs: [ 4.265706] WARNING KERN x21: 800011f4b9f8 x20: > 800011df1000 > boot: logs: [ 4.270934] WARNING KERN x19: 8208a000 x18: > 0005 > boot: logs: [ 4.276166] WARNING KERN x17: 0007 x16: > 0001 > boot: logs: [ 4.281382] WARNING KERN x15: 80030c90 x14: > 805c9df8 > boot: logs: [ 4.286618] WARNING KERN x13: x12: > 80030c90 > boot: logs: [ 4.291847] WARNING KERN x11: 805c9da8 x10: > 0040 > boot: logs: [ 4.297061] WARNING KERN x9 : 80030c98 x8 : > > boot: logs: [ 4.302291] WARNING KERN x7 : 0009 x6 : > > boot: logs: [ 4.307509] WARNING KERN x5 : 8010 x4 : > > boot: logs: [ 4.312739] WARNING KERN x3 : 800011df1e38 x2 : > 80908c10 > boot: logs: [ 4.317956] WARNING KERN x1 : 0001 x0 : > 809ca400 > boot: logs: [ 4.323183] WARNING KERN Call trace: > boot: logs: [ 4.325593] WARNING KERN > device_links_driver_bound+0x240/0x260 > boot: logs: [ 4.330301] WARNING KERN driver_bound+0x70/0xd0 > boot: logs: [ 4.333740] WARNING KERN device_bind_driver+0x50/0x60 > boot: logs: [ 4.337671] WARNING KERN phy_attach_direct+0x258/0x2e0 > boot: logs: [ 4.341718] WARNING KERN phylink_of_phy_connect+0x7c/0x140 > boot: logs: [ 4.346081] WARNING KERN stmmac_open+0xb04/0xc70 > boot: logs: [ 4.349612] WARNING KERN __dev_open+0xe0/0x190 > boot: logs: [ 4.352972] WARNING KERN __dev_change_flags+0x16c/0x1b8 > boot: logs: [ 4.357081] WARNING KERN dev_change_flags+0x20/0x60 > boot: logs: [ 4.360856] WARNING KERN ip_auto_config+0x2a0/0xfe8 > boot: logs: [ 4.364633] WARNING KERN do_one_initcall+0x58/0x1b8 > boot: logs: [ 4.368405] WARNING KERN kernel_init_freeable+0x1ec/0x240 > boot: logs: [ 4.372698] WARNING KERN kernel_init+0x10/0x110 > boot: logs: [ 4.376130] WARNING KERN ret_from_fork+0x10/0x18 > > > So looking at this change does this mean that the of_mdio needs to be > converted to a proper driver? Sorry, there's not enough context in this log for me to tell how this is even related to of_mdio.c. My guess is this is related to network stack directly calling device_bind_driver() and not updating device link state correctly. See what device_links_check_suppliers() does in the normal path. I think I know which warning this is, but can you check your tree and tell me the code you see in drivers/base/core.c:1189 ? Also, can you give me
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On Wed, Jan 13, 2021 at 7:27 AM Jon Hunter wrote: > > > On 13/01/2021 11:11, Marc Zyngier wrote: > > On 2021-01-07 20:05, Greg Kroah-Hartman wrote: > >> On Thu, Dec 17, 2020 at 07:16:58PM -0800, Saravana Kannan wrote: > >>> As discussed in LPC 2020, cyclic dependencies in firmware that couldn't > >>> be broken using logic was one of the last remaining reasons > >>> fw_devlink=on couldn't be set by default. > >>> > >>> This series changes fw_devlink so that when a cyclic dependency is found > >>> in firmware, the links between those devices fallback to permissive mode > >>> behavior. This way, the rest of the system still benefits from > >>> fw_devlink, but the ambiguous cases fallback to permissive mode. > >>> > >>> Setting fw_devlink=on by default brings a bunch of benefits (currently, > >>> only for systems with device tree firmware): > >>> * Significantly cuts down deferred probes. > >>> * Device probe is effectively attempted in graph order. > >>> * Makes it much easier to load drivers as modules without having to > >>> worry about functional dependencies between modules (depmod is still > >>> needed for symbol dependencies). > >>> > >>> Greg/Rafael, > >>> > >>> Can we get this pulled into 5.11-rc1 or -rc2 soon please? I expect to > >>> see some issues due to device drivers that aren't following best > >>> practices (they don't expose the device to driver core). Want to > >>> identify those early on and try to have them fixed before 5.11 release. > >>> See [1] for an example of such a case. > >> > >> Now queued up in my tree, will show up in linux-next in a few days, > >> let's see what breaks! :) > >> > >> And it is scheduled for 5.12-rc1, not 5.11, sorry. > > > > For the record, this breaks my rk3399 board, (NanoPC-T4) as no mass > > storage can be discovered (it lives on PCIe): > > > > (initramfs) find /sys -name 'waiting_for_supplier'| xargs grep .| egrep > > -v ':0$' > > /sys/devices/platform/ff3d.i2c/i2c-4/4-0022/waiting_for_supplier:1 > > /sys/devices/platform/f800.pcie/waiting_for_supplier:1 > > /sys/devices/platform/fe32.mmc/waiting_for_supplier:1 > > /sys/devices/platform/sdio-pwrseq/waiting_for_supplier:1 > > /sys/devices/platform/ff3c.i2c/i2c-0/0-001b/waiting_for_supplier:1 > > > > Enabling the debug prints in device_links_check_suppliers(), I end up with > > the dump below (apologies for the size). > > > I am seeing the same problem on Tegra30 Cardhu A04 where several regulators > are continuously deferred and prevents the board from booting ... > > [2.518334] platform panel: probe deferral - supplier regulator@11 not > ready > > [2.525503] platform regulator@1: probe deferral - supplier 4-002d not > ready > > [2.533141] platform regulator@3: probe deferral - supplier regulator@101 > not ready > > [2.540856] platform regulator@5: probe deferral - supplier regulator@101 > not ready > > [2.548589] platform regulator@6: probe deferral - supplier regulator@101 > not ready > > [2.556316] platform regulator@7: probe deferral - supplier regulator@101 > not ready > > [2.564041] platform regulator@8: probe deferral - supplier regulator@101 > not ready > > [2.571743] platform regulator@9: probe deferral - supplier regulator@101 > not ready > > [2.579463] platform regulator@10: probe deferral - supplier regulator@101 > not ready > > [2.587273] platform regulator@11: probe deferral - supplier regulator@101 > not ready > > [2.595088] platform regulator@12: probe deferral - supplier regulator@104 > not ready > > [2.603837] platform regulator@102: probe deferral - supplier > regulator@104 not ready > > [2.611726] platform regulator@103: probe deferral - supplier > regulator@104 not ready > > [2.620137] platform 3000.pcie: probe deferral - supplier regulator@5 not > ready Looks like this is not the whole log? Do you see any "wait for supplier" logs? That's what all these boot issues should boil down to. And as usual, pointer to DT for this board please. -Saravana
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On Wed, Jan 13, 2021 at 3:11 AM Marc Zyngier wrote: > > On 2021-01-07 20:05, Greg Kroah-Hartman wrote: > > On Thu, Dec 17, 2020 at 07:16:58PM -0800, Saravana Kannan wrote: > >> As discussed in LPC 2020, cyclic dependencies in firmware that > >> couldn't > >> be broken using logic was one of the last remaining reasons > >> fw_devlink=on couldn't be set by default. > >> > >> This series changes fw_devlink so that when a cyclic dependency is > >> found > >> in firmware, the links between those devices fallback to permissive > >> mode > >> behavior. This way, the rest of the system still benefits from > >> fw_devlink, but the ambiguous cases fallback to permissive mode. > >> > >> Setting fw_devlink=on by default brings a bunch of benefits > >> (currently, > >> only for systems with device tree firmware): > >> * Significantly cuts down deferred probes. > >> * Device probe is effectively attempted in graph order. > >> * Makes it much easier to load drivers as modules without having to > >> worry about functional dependencies between modules (depmod is still > >> needed for symbol dependencies). > >> > >> Greg/Rafael, > >> > >> Can we get this pulled into 5.11-rc1 or -rc2 soon please? I expect to > >> see some issues due to device drivers that aren't following best > >> practices (they don't expose the device to driver core). Want to > >> identify those early on and try to have them fixed before 5.11 > >> release. > >> See [1] for an example of such a case. > > > > Now queued up in my tree, will show up in linux-next in a few days, > > let's see what breaks! :) > > > > And it is scheduled for 5.12-rc1, not 5.11, sorry. > > For the record, this breaks my rk3399 board, (NanoPC-T4) as no mass > storage can be discovered (it lives on PCIe): > > (initramfs) find /sys -name 'waiting_for_supplier'| xargs grep .| egrep > -v ':0$' > /sys/devices/platform/ff3d.i2c/i2c-4/4-0022/waiting_for_supplier:1 > /sys/devices/platform/f800.pcie/waiting_for_supplier:1 > /sys/devices/platform/fe32.mmc/waiting_for_supplier:1 > /sys/devices/platform/sdio-pwrseq/waiting_for_supplier:1 > /sys/devices/platform/ff3c.i2c/i2c-0/0-001b/waiting_for_supplier:1 > > Enabling the debug prints in device_links_check_suppliers(), I end up > with > the dump below (apologies for the size). > > This seems to all hang on the GPIO banks, but it is pretty unclear what > is wrong with them. > > Happy to test things further. Thanks for the logs Marc. Looks like a majority/all of the issue is due to gpio device nodes [1] being "probed" without creating a proper struct device for it. You can see here [2] how the driver for the parent device just loops through the child DT nodes and initializes them. This would be okay if the DT nodes didn't have a "compatible" property for the gpio device [1]. So to fix this, the driver[2] needs to be updated to properly populate the child devices and then probe them using a proper driver for "rockchip,gpio-bank". And most of the gpio init code into this new driver. The DT implementation of fw_devlink has the expectation that device tree nodes with "compatible" properties will have struct devices created for them. Without this expectation, it has no way to know how far up the ancestor chain fw_devlink needs to walk up before it can expect a supplier device to create a device link to. Heiko, Could you please refactor drivers/pinctrl/pinctrl-rockchip.c to create and probe struct devices for "rockchip,gpio-bank" nodes? This allows fw_devlink to work for these devices and makes sure devices probe in the right order, suspend/resume in the right order, etc. -Saravana [1] - https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1956 [2] - https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pinctrl/pinctrl-rockchip.c#n3566 > > M. > > platform vcc3v3-sys: probe deferral - supplier vcc12v0-sys not ready > platform vcc5v0-sys: probe deferral - supplier vcc12v0-sys not ready > platform vcc1v8-s3: probe deferral - wait for supplier pmic@1b > platform vcc3v0-sd: probe deferral - supplier vcc3v3-sys not ready > platform vcca0v9-s3: probe deferral - supplier vcc1v8-s3 not ready > platform vcca1v8-s3: probe deferral - supplier vcc1v8-s3 not ready > platform vbus-typec: probe deferral - supplier vcc5v0-sys not ready > platform vcc5v0-host0: probe deferral - supplier vcc5v0-sys not ready > platform f800.pcie: probe deferral - wait for supplier > gpio2@ff78 > platform sdio-pwrseq: probe deferral - wait for supplier gpio0@ff72 > platform vcc1v8-s3: probe deferral - wait for supplier pmic@1b > platform vcca0v9-s3: probe deferral - supplier vcc1v8-s3 not ready > platform vcca1v8-s3: probe deferral - supplier vcc1v8-s3 not ready > platform f800.pcie: probe deferral - wait for supplier > gpio2@ff78 > platform sdio-pwrseq: probe deferral - wait for supplier gpio0@ff72 >
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On 13/01/2021 11:11, Marc Zyngier wrote: > On 2021-01-07 20:05, Greg Kroah-Hartman wrote: >> On Thu, Dec 17, 2020 at 07:16:58PM -0800, Saravana Kannan wrote: >>> As discussed in LPC 2020, cyclic dependencies in firmware that couldn't >>> be broken using logic was one of the last remaining reasons >>> fw_devlink=on couldn't be set by default. >>> >>> This series changes fw_devlink so that when a cyclic dependency is found >>> in firmware, the links between those devices fallback to permissive mode >>> behavior. This way, the rest of the system still benefits from >>> fw_devlink, but the ambiguous cases fallback to permissive mode. >>> >>> Setting fw_devlink=on by default brings a bunch of benefits (currently, >>> only for systems with device tree firmware): >>> * Significantly cuts down deferred probes. >>> * Device probe is effectively attempted in graph order. >>> * Makes it much easier to load drivers as modules without having to >>> worry about functional dependencies between modules (depmod is still >>> needed for symbol dependencies). >>> >>> Greg/Rafael, >>> >>> Can we get this pulled into 5.11-rc1 or -rc2 soon please? I expect to >>> see some issues due to device drivers that aren't following best >>> practices (they don't expose the device to driver core). Want to >>> identify those early on and try to have them fixed before 5.11 release. >>> See [1] for an example of such a case. >> >> Now queued up in my tree, will show up in linux-next in a few days, >> let's see what breaks! :) >> >> And it is scheduled for 5.12-rc1, not 5.11, sorry. > > For the record, this breaks my rk3399 board, (NanoPC-T4) as no mass > storage can be discovered (it lives on PCIe): > > (initramfs) find /sys -name 'waiting_for_supplier'| xargs grep .| egrep > -v ':0$' > /sys/devices/platform/ff3d.i2c/i2c-4/4-0022/waiting_for_supplier:1 > /sys/devices/platform/f800.pcie/waiting_for_supplier:1 > /sys/devices/platform/fe32.mmc/waiting_for_supplier:1 > /sys/devices/platform/sdio-pwrseq/waiting_for_supplier:1 > /sys/devices/platform/ff3c.i2c/i2c-0/0-001b/waiting_for_supplier:1 > > Enabling the debug prints in device_links_check_suppliers(), I end up with > the dump below (apologies for the size). I am seeing the same problem on Tegra30 Cardhu A04 where several regulators are continuously deferred and prevents the board from booting ... [2.518334] platform panel: probe deferral - supplier regulator@11 not ready [2.525503] platform regulator@1: probe deferral - supplier 4-002d not ready [2.533141] platform regulator@3: probe deferral - supplier regulator@101 not ready [2.540856] platform regulator@5: probe deferral - supplier regulator@101 not ready [2.548589] platform regulator@6: probe deferral - supplier regulator@101 not ready [2.556316] platform regulator@7: probe deferral - supplier regulator@101 not ready [2.564041] platform regulator@8: probe deferral - supplier regulator@101 not ready [2.571743] platform regulator@9: probe deferral - supplier regulator@101 not ready [2.579463] platform regulator@10: probe deferral - supplier regulator@101 not ready [2.587273] platform regulator@11: probe deferral - supplier regulator@101 not ready [2.595088] platform regulator@12: probe deferral - supplier regulator@104 not ready [2.603837] platform regulator@102: probe deferral - supplier regulator@104 not ready [2.611726] platform regulator@103: probe deferral - supplier regulator@104 not ready [2.620137] platform 3000.pcie: probe deferral - supplier regulator@5 not ready Cheers Jon -- nvpublic
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On 2021-01-13 11:44, Nicolas Saenz Julienne wrote: On Thu, 2020-12-17 at 19:16 -0800, Saravana Kannan wrote: As discussed in LPC 2020, cyclic dependencies in firmware that couldn't be broken using logic was one of the last remaining reasons fw_devlink=on couldn't be set by default. This series changes fw_devlink so that when a cyclic dependency is found in firmware, the links between those devices fallback to permissive mode behavior. This way, the rest of the system still benefits from fw_devlink, but the ambiguous cases fallback to permissive mode. Setting fw_devlink=on by default brings a bunch of benefits (currently, only for systems with device tree firmware): * Significantly cuts down deferred probes. * Device probe is effectively attempted in graph order. * Makes it much easier to load drivers as modules without having to worry about functional dependencies between modules (depmod is still needed for symbol dependencies). FWIW I don't see any issues with this on Raspberry Pi 4 :). Keep bragging! ;-) M. -- Jazz is not dead. It just smells funny...
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On Thu, 2020-12-17 at 19:16 -0800, Saravana Kannan wrote: > As discussed in LPC 2020, cyclic dependencies in firmware that couldn't > be broken using logic was one of the last remaining reasons > fw_devlink=on couldn't be set by default. > > This series changes fw_devlink so that when a cyclic dependency is found > in firmware, the links between those devices fallback to permissive mode > behavior. This way, the rest of the system still benefits from > fw_devlink, but the ambiguous cases fallback to permissive mode. > > Setting fw_devlink=on by default brings a bunch of benefits (currently, > only for systems with device tree firmware): > * Significantly cuts down deferred probes. > * Device probe is effectively attempted in graph order. > * Makes it much easier to load drivers as modules without having to > worry about functional dependencies between modules (depmod is still > needed for symbol dependencies). FWIW I don't see any issues with this on Raspberry Pi 4 :). Regards, Nicolas signature.asc Description: This is a digitally signed message part
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On 18/12/2020 03:16, Saravana Kannan wrote: > As discussed in LPC 2020, cyclic dependencies in firmware that couldn't > be broken using logic was one of the last remaining reasons > fw_devlink=on couldn't be set by default. > > This series changes fw_devlink so that when a cyclic dependency is found > in firmware, the links between those devices fallback to permissive mode > behavior. This way, the rest of the system still benefits from > fw_devlink, but the ambiguous cases fallback to permissive mode. > > Setting fw_devlink=on by default brings a bunch of benefits (currently, > only for systems with device tree firmware): > * Significantly cuts down deferred probes. > * Device probe is effectively attempted in graph order. > * Makes it much easier to load drivers as modules without having to > worry about functional dependencies between modules (depmod is still > needed for symbol dependencies). One issue we have come across with this is the of_mdio.c driver. On Tegra194 Jetson Xavier I am seeing the following ... boot: logs: [ 4.194791] WARNING KERN WARNING: CPU: 0 PID: 1 at /dvs/git/dirty/git-master_l4t-upstream/kernel/drivers/base/core.c:1189 device_links_driver_bound+0x240/0x260 boot: logs: [ 4.207683] WARNING KERN Modules linked in: boot: logs: [ 4.210691] WARNING KERN CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.11.0-rc3-next-20210112-gdf869cab4b35 #1 boot: logs: [ 4.219221] WARNING KERN Hardware name: NVIDIA Jetson AGX Xavier Developer Kit (DT) boot: logs: [ 4.225628] WARNING KERN pstate: 8049 (Nzcv daif +PAN -UAO -TCO BTYPE=--) boot: logs: [ 4.231542] WARNING KERN pc : device_links_driver_bound+0x240/0x260 boot: logs: [ 4.236587] WARNING KERN lr : device_links_driver_bound+0xf8/0x260 boot: logs: [ 4.241560] WARNING KERN sp : 800011f4b980 boot: logs: [ 4.244819] WARNING KERN x29: 800011f4b980 x28: 8208a0a0 boot: logs: [ 4.250051] WARNING KERN x27: 8208a080 x26: boot: logs: [ 4.255271] WARNING KERN x25: 0003 x24: 800011b99000 boot: logs: [ 4.260489] WARNING KERN x23: 0001 x22: 800011df14f0 boot: logs: [ 4.265706] WARNING KERN x21: 800011f4b9f8 x20: 800011df1000 boot: logs: [ 4.270934] WARNING KERN x19: 8208a000 x18: 0005 boot: logs: [ 4.276166] WARNING KERN x17: 0007 x16: 0001 boot: logs: [ 4.281382] WARNING KERN x15: 80030c90 x14: 805c9df8 boot: logs: [ 4.286618] WARNING KERN x13: x12: 80030c90 boot: logs: [ 4.291847] WARNING KERN x11: 805c9da8 x10: 0040 boot: logs: [ 4.297061] WARNING KERN x9 : 80030c98 x8 : boot: logs: [ 4.302291] WARNING KERN x7 : 0009 x6 : boot: logs: [ 4.307509] WARNING KERN x5 : 8010 x4 : boot: logs: [ 4.312739] WARNING KERN x3 : 800011df1e38 x2 : 80908c10 boot: logs: [ 4.317956] WARNING KERN x1 : 0001 x0 : 809ca400 boot: logs: [ 4.323183] WARNING KERN Call trace: boot: logs: [ 4.325593] WARNING KERN device_links_driver_bound+0x240/0x260 boot: logs: [ 4.330301] WARNING KERN driver_bound+0x70/0xd0 boot: logs: [ 4.333740] WARNING KERN device_bind_driver+0x50/0x60 boot: logs: [ 4.337671] WARNING KERN phy_attach_direct+0x258/0x2e0 boot: logs: [ 4.341718] WARNING KERN phylink_of_phy_connect+0x7c/0x140 boot: logs: [ 4.346081] WARNING KERN stmmac_open+0xb04/0xc70 boot: logs: [ 4.349612] WARNING KERN __dev_open+0xe0/0x190 boot: logs: [ 4.352972] WARNING KERN __dev_change_flags+0x16c/0x1b8 boot: logs: [ 4.357081] WARNING KERN dev_change_flags+0x20/0x60 boot: logs: [ 4.360856] WARNING KERN ip_auto_config+0x2a0/0xfe8 boot: logs: [ 4.364633] WARNING KERN do_one_initcall+0x58/0x1b8 boot: logs: [ 4.368405] WARNING KERN kernel_init_freeable+0x1ec/0x240 boot: logs: [ 4.372698] WARNING KERN kernel_init+0x10/0x110 boot: logs: [ 4.376130] WARNING KERN ret_from_fork+0x10/0x18 So looking at this change does this mean that the of_mdio needs to be converted to a proper driver? I would have thought that this will be seen on several platforms. Cheers Jon -- nvpublic
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On 2021-01-07 20:05, Greg Kroah-Hartman wrote: On Thu, Dec 17, 2020 at 07:16:58PM -0800, Saravana Kannan wrote: As discussed in LPC 2020, cyclic dependencies in firmware that couldn't be broken using logic was one of the last remaining reasons fw_devlink=on couldn't be set by default. This series changes fw_devlink so that when a cyclic dependency is found in firmware, the links between those devices fallback to permissive mode behavior. This way, the rest of the system still benefits from fw_devlink, but the ambiguous cases fallback to permissive mode. Setting fw_devlink=on by default brings a bunch of benefits (currently, only for systems with device tree firmware): * Significantly cuts down deferred probes. * Device probe is effectively attempted in graph order. * Makes it much easier to load drivers as modules without having to worry about functional dependencies between modules (depmod is still needed for symbol dependencies). Greg/Rafael, Can we get this pulled into 5.11-rc1 or -rc2 soon please? I expect to see some issues due to device drivers that aren't following best practices (they don't expose the device to driver core). Want to identify those early on and try to have them fixed before 5.11 release. See [1] for an example of such a case. Now queued up in my tree, will show up in linux-next in a few days, let's see what breaks! :) And it is scheduled for 5.12-rc1, not 5.11, sorry. For the record, this breaks my rk3399 board, (NanoPC-T4) as no mass storage can be discovered (it lives on PCIe): (initramfs) find /sys -name 'waiting_for_supplier'| xargs grep .| egrep -v ':0$' /sys/devices/platform/ff3d.i2c/i2c-4/4-0022/waiting_for_supplier:1 /sys/devices/platform/f800.pcie/waiting_for_supplier:1 /sys/devices/platform/fe32.mmc/waiting_for_supplier:1 /sys/devices/platform/sdio-pwrseq/waiting_for_supplier:1 /sys/devices/platform/ff3c.i2c/i2c-0/0-001b/waiting_for_supplier:1 Enabling the debug prints in device_links_check_suppliers(), I end up with the dump below (apologies for the size). This seems to all hang on the GPIO banks, but it is pretty unclear what is wrong with them. Happy to test things further. M. platform vcc3v3-sys: probe deferral - supplier vcc12v0-sys not ready platform vcc5v0-sys: probe deferral - supplier vcc12v0-sys not ready platform vcc1v8-s3: probe deferral - wait for supplier pmic@1b platform vcc3v0-sd: probe deferral - supplier vcc3v3-sys not ready platform vcca0v9-s3: probe deferral - supplier vcc1v8-s3 not ready platform vcca1v8-s3: probe deferral - supplier vcc1v8-s3 not ready platform vbus-typec: probe deferral - supplier vcc5v0-sys not ready platform vcc5v0-host0: probe deferral - supplier vcc5v0-sys not ready platform f800.pcie: probe deferral - wait for supplier gpio2@ff78 platform sdio-pwrseq: probe deferral - wait for supplier gpio0@ff72 platform vcc1v8-s3: probe deferral - wait for supplier pmic@1b platform vcca0v9-s3: probe deferral - supplier vcc1v8-s3 not ready platform vcca1v8-s3: probe deferral - supplier vcc1v8-s3 not ready platform f800.pcie: probe deferral - wait for supplier gpio2@ff78 platform sdio-pwrseq: probe deferral - wait for supplier gpio0@ff72 platform vcc1v8-s3: probe deferral - wait for supplier pmic@1b platform vcca0v9-s3: probe deferral - supplier vcc1v8-s3 not ready platform vcca1v8-s3: probe deferral - supplier vcc1v8-s3 not ready platform f800.pcie: probe deferral - wait for supplier gpio2@ff78 platform sdio-pwrseq: probe deferral - wait for supplier gpio0@ff72 platform vcc1v8-s3: probe deferral - wait for supplier pmic@1b platform vcca0v9-s3: probe deferral - supplier vcc1v8-s3 not ready platform vcca1v8-s3: probe deferral - supplier vcc1v8-s3 not ready platform f800.pcie: probe deferral - wait for supplier gpio2@ff78 platform sdio-pwrseq: probe deferral - wait for supplier gpio0@ff72 platform vcc1v8-s3: probe deferral - wait for supplier pmic@1b platform vcca0v9-s3: probe deferral - supplier vcc1v8-s3 not ready platform vcca1v8-s3: probe deferral - supplier vcc1v8-s3 not ready platform f800.pcie: probe deferral - wait for supplier gpio2@ff78 platform sdio-pwrseq: probe deferral - wait for supplier gpio0@ff72 platform vcc1v8-s3: probe deferral - wait for supplier pmic@1b platform vcca0v9-s3: probe deferral - supplier vcc1v8-s3 not ready platform vcca1v8-s3: probe deferral - supplier vcc1v8-s3 not ready platform f800.pcie: probe deferral - wait for supplier gpio2@ff78 platform sdio-pwrseq: probe deferral - wait for supplier gpio0@ff72 platform vcc1v8-s3: probe deferral - wait for supplier pmic@1b platform vcca0v9-s3: probe deferral - supplier vcc1v8-s3 not ready platform vcca1v8-s3: probe deferral - supplier vcc1v8-s3 not ready platform fe32.mmc: probe deferral - wait for supplier pmic@1b platform f800.pcie: probe deferral - wait for supplier
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On Thu, Jan 7, 2021 at 12:04 PM Greg Kroah-Hartman wrote: > > On Thu, Dec 17, 2020 at 07:16:58PM -0800, Saravana Kannan wrote: > > As discussed in LPC 2020, cyclic dependencies in firmware that couldn't > > be broken using logic was one of the last remaining reasons > > fw_devlink=on couldn't be set by default. > > > > This series changes fw_devlink so that when a cyclic dependency is found > > in firmware, the links between those devices fallback to permissive mode > > behavior. This way, the rest of the system still benefits from > > fw_devlink, but the ambiguous cases fallback to permissive mode. > > > > Setting fw_devlink=on by default brings a bunch of benefits (currently, > > only for systems with device tree firmware): > > * Significantly cuts down deferred probes. > > * Device probe is effectively attempted in graph order. > > * Makes it much easier to load drivers as modules without having to > > worry about functional dependencies between modules (depmod is still > > needed for symbol dependencies). > > > > Greg/Rafael, > > > > Can we get this pulled into 5.11-rc1 or -rc2 soon please? I expect to > > see some issues due to device drivers that aren't following best > > practices (they don't expose the device to driver core). Want to > > identify those early on and try to have them fixed before 5.11 release. > > See [1] for an example of such a case. > > Now queued up in my tree, will show up in linux-next in a few days, > let's see what breaks! :) > > And it is scheduled for 5.12-rc1, not 5.11, sorry. Thanks. Not too worried about the actual version. I just want things to start breaking as soon as possible if they are going to break. -Saravana
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On Thu, Dec 17, 2020 at 07:16:58PM -0800, Saravana Kannan wrote: > As discussed in LPC 2020, cyclic dependencies in firmware that couldn't > be broken using logic was one of the last remaining reasons > fw_devlink=on couldn't be set by default. > > This series changes fw_devlink so that when a cyclic dependency is found > in firmware, the links between those devices fallback to permissive mode > behavior. This way, the rest of the system still benefits from > fw_devlink, but the ambiguous cases fallback to permissive mode. > > Setting fw_devlink=on by default brings a bunch of benefits (currently, > only for systems with device tree firmware): > * Significantly cuts down deferred probes. > * Device probe is effectively attempted in graph order. > * Makes it much easier to load drivers as modules without having to > worry about functional dependencies between modules (depmod is still > needed for symbol dependencies). > > Greg/Rafael, > > Can we get this pulled into 5.11-rc1 or -rc2 soon please? I expect to > see some issues due to device drivers that aren't following best > practices (they don't expose the device to driver core). Want to > identify those early on and try to have them fixed before 5.11 release. > See [1] for an example of such a case. Now queued up in my tree, will show up in linux-next in a few days, let's see what breaks! :) And it is scheduled for 5.12-rc1, not 5.11, sorry. thanks, greg k-h
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On Fri, Dec 18, 2020 at 4:17 AM Saravana Kannan wrote: > > As discussed in LPC 2020, cyclic dependencies in firmware that couldn't > be broken using logic was one of the last remaining reasons > fw_devlink=on couldn't be set by default. > > This series changes fw_devlink so that when a cyclic dependency is found > in firmware, the links between those devices fallback to permissive mode > behavior. This way, the rest of the system still benefits from > fw_devlink, but the ambiguous cases fallback to permissive mode. > > Setting fw_devlink=on by default brings a bunch of benefits (currently, > only for systems with device tree firmware): > * Significantly cuts down deferred probes. > * Device probe is effectively attempted in graph order. > * Makes it much easier to load drivers as modules without having to > worry about functional dependencies between modules (depmod is still > needed for symbol dependencies). > > Greg/Rafael, > > Can we get this pulled into 5.11-rc1 or -rc2 soon please? Honestly, I'd rather not (but it's up to Greg). This is a new series posted during the merge window, so it should not be looked at even according to the rules. Personally, I don't have the time to look at it now. > I expect to see some issues due to device drivers that aren't following best > practices (they don't expose the device to driver core). Want to > identify those early on and try to have them fixed before 5.11 release. > See [1] for an example of such a case. So it should be posted right after -rc1 and spend a whole cycle in linux-next. > If we do end up have to revert anything, it'll just be Patch 5/5 (a one > liner). Which totally doesn't matter IMV.
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On Thu, 17 Dec 2020 19:16:58 -0800 Saravana Kannan wrote: > > > As discussed in LPC 2020, cyclic dependencies in firmware that couldn't > be broken using logic was one of the last remaining reasons > fw_devlink=on couldn't be set by default. > > This series changes fw_devlink so that when a cyclic dependency is found > in firmware, the links between those devices fallback to permissive mode > behavior. This way, the rest of the system still benefits from > fw_devlink, but the ambiguous cases fallback to permissive mode. > > Setting fw_devlink=on by default brings a bunch of benefits (currently, > only for systems with device tree firmware): > * Significantly cuts down deferred probes. > * Device probe is effectively attempted in graph order. > * Makes it much easier to load drivers as modules without having to > worry about functional dependencies between modules (depmod is still > needed for symbol dependencies). > > Greg/Rafael, > > Can we get this pulled into 5.11-rc1 or -rc2 soon please? I expect to > see some issues due to device drivers that aren't following best > practices (they don't expose the device to driver core). Want to > identify those early on and try to have them fixed before 5.11 release. > See [1] for an example of such a case. > > If we do end up have to revert anything, it'll just be Patch 5/5 (a one > liner). > > Marc, > > You had hit issues with fw_devlink=on before on some of your systems. > Want to give this a shot? > > Jisheng, > > Want to fix up one of those gpio drivers you were having problems with? > Hi Saravana, I didn't send fix for the gpio-dwapb.c in last development window, so can send patch once 5.11-rc1 is released. thanks
Re: [PATCH v1 0/5] Enable fw_devlink=on by default
On Thu, Dec 17, 2020 at 7:17 PM Saravana Kannan wrote: > > As discussed in LPC 2020, cyclic dependencies in firmware that couldn't > be broken using logic was one of the last remaining reasons > fw_devlink=on couldn't be set by default. > > This series changes fw_devlink so that when a cyclic dependency is found > in firmware, the links between those devices fallback to permissive mode > behavior. This way, the rest of the system still benefits from > fw_devlink, but the ambiguous cases fallback to permissive mode. > > Setting fw_devlink=on by default brings a bunch of benefits (currently, > only for systems with device tree firmware): > * Significantly cuts down deferred probes. > * Device probe is effectively attempted in graph order. > * Makes it much easier to load drivers as modules without having to > worry about functional dependencies between modules (depmod is still > needed for symbol dependencies). > > Greg/Rafael, > > Can we get this pulled into 5.11-rc1 or -rc2 soon please? I expect to > see some issues due to device drivers that aren't following best > practices (they don't expose the device to driver core). Want to > identify those early on and try to have them fixed before 5.11 release. > See [1] for an example of such a case. > > If we do end up have to revert anything, it'll just be Patch 5/5 (a one > liner). > > Marc, > > You had hit issues with fw_devlink=on before on some of your systems. > Want to give this a shot? Marc, If you decide to test this, please also pull in this patch. It should fix all your interrupt issues. https://lore.kernel.org/lkml/20201218210750.3455872-1-sarava...@google.com/ -Saravana
