From: Kefeng Wang <hidden> Date: 2021-08-16 07:43:23
Patch 1 and 2 make some cleanup, and patch 3 use of_irq_get() instead of
irq_of_parse_and_map() to get irq number, return -EPROBE_DEFER if the irq
domain is not yet created, amba_device_add() will properly to handle the
no IRQ domain issue via deferred probe.
Kefeng Wang (3):
amba: Drop unused functions about APB/AHB devices add
Revert "ARM: amba: make use of -1 IRQs warn"
amba: Properly handle device probe without IRQ domain
drivers/amba/bus.c | 100 ++++++++++-----------------------------
drivers/of/platform.c | 6 +--
include/linux/amba/bus.h | 18 -------
3 files changed, 27 insertions(+), 97 deletions(-)
--
2.26.2
From: Kefeng Wang <hidden> Date: 2021-08-16 07:43:19
After commit 77a7300abad7 ("of/irq: Get rid of NO_IRQ usage"),
no irq case has been removed, irq_of_parse_and_map() will return
0 in all cases when get error from parse and map an interrupt into
linux virq space.
amba_device_register() is only used on no-DT initialization, see
s3c64xx_pl080_init() arch/arm/mach-s3c/pl080.c
ep93xx_init_devices() arch/arm/mach-ep93xx/core.c
They won't set -1 to irq[0], so no need the warn.
This reverts commit 2eac58d5026e4ec8b17ff8b62877fea9e1d2f1b3.
Cc: Russell King <linux@armlinux.org.uk>
Signed-off-by: Kefeng Wang <redacted>
---
drivers/amba/bus.c | 3 ---
1 file changed, 3 deletions(-)
From: Kefeng Wang <hidden> Date: 2021-08-16 07:43:25
of_amba_device_create() uses irq_of_parse_and_map() to translate
a DT interrupt specification into a Linux virtual interrupt number.
But it doesn't properly handle the case where the interrupt controller
is not yet available, eg, when pl011 interrupt is connected to MBIGEN
interrupt controller, because the mbigen initialization is too late,
which will lead to no IRQ due to no IRQ domain found, log is shown below,
"irq: no irq domain found for uart0 !"
use of_irq_get() to return -EPROBE_DEFER as above, and in the function
amba_device_try_add()/amba_device_add(), it will properly handle in such
case, also return 0 in other fail cases to be consistent as before.
Cc: Russell King <linux@armlinux.org.uk>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
Reported-by: Ruizhe Lin <redacted>
Signed-off-by: Kefeng Wang <redacted>
---
drivers/amba/bus.c | 27 +++++++++++++++++++++++++++
drivers/of/platform.c | 6 +-----
2 files changed, 28 insertions(+), 5 deletions(-)
From: Rob Herring <robh@kernel.org> Date: 2021-08-17 22:27:42
On Mon, Aug 16, 2021 at 03:46:16PM +0800, Kefeng Wang wrote:
Patch 1 and 2 make some cleanup, and patch 3 use of_irq_get() instead of
irq_of_parse_and_map() to get irq number, return -EPROBE_DEFER if the irq
domain is not yet created, amba_device_add() will properly to handle the
no IRQ domain issue via deferred probe.
Kefeng Wang (3):
amba: Drop unused functions about APB/AHB devices add
Revert "ARM: amba: make use of -1 IRQs warn"
amba: Properly handle device probe without IRQ domain
drivers/amba/bus.c | 100 ++++++++++-----------------------------
drivers/of/platform.c | 6 +--
include/linux/amba/bus.h | 18 -------
3 files changed, 27 insertions(+), 97 deletions(-)
From: Kefeng Wang <hidden> Date: 2021-08-23 02:19:36
On 2021/8/18 6:27, Rob Herring wrote:
On Mon, Aug 16, 2021 at 03:46:16PM +0800, Kefeng Wang wrote:
quoted
Patch 1 and 2 make some cleanup, and patch 3 use of_irq_get() instead of
irq_of_parse_and_map() to get irq number, return -EPROBE_DEFER if the irq
domain is not yet created, amba_device_add() will properly to handle the
no IRQ domain issue via deferred probe.
Kefeng Wang (3):
amba: Drop unused functions about APB/AHB devices add
Revert "ARM: amba: make use of -1 IRQs warn"
amba: Properly handle device probe without IRQ domain
drivers/amba/bus.c | 100 ++++++++++-----------------------------
drivers/of/platform.c | 6 +--
include/linux/amba/bus.h | 18 -------
3 files changed, 27 insertions(+), 97 deletions(-)
Reviewed-by: Rob Herring <robh@kernel.org>
Thanks Rob.
Hi Russell, should I send the patches to the ARM patch system?
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2021-08-23 09:05:32
On Mon, Aug 23, 2021 at 10:19:23AM +0800, Kefeng Wang wrote:
On 2021/8/18 6:27, Rob Herring wrote:
quoted
On Mon, Aug 16, 2021 at 03:46:16PM +0800, Kefeng Wang wrote:
quoted
Patch 1 and 2 make some cleanup, and patch 3 use of_irq_get() instead of
irq_of_parse_and_map() to get irq number, return -EPROBE_DEFER if the irq
domain is not yet created, amba_device_add() will properly to handle the
no IRQ domain issue via deferred probe.
Kefeng Wang (3):
amba: Drop unused functions about APB/AHB devices add
Revert "ARM: amba: make use of -1 IRQs warn"
amba: Properly handle device probe without IRQ domain
drivers/amba/bus.c | 100 ++++++++++-----------------------------
drivers/of/platform.c | 6 +--
include/linux/amba/bus.h | 18 -------
3 files changed, 27 insertions(+), 97 deletions(-)
Reviewed-by: Rob Herring <robh@kernel.org>
Thanks Rob.
Hi Russell, should I send the patches to the ARM patch system?
Yes please - I'll try to squeeze it in for this cycle but it's getting
a tad late for that. Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
From: Kefeng Wang <hidden> Date: 2021-08-23 10:57:48
On 2021/8/23 17:05, Russell King (Oracle) wrote:
On Mon, Aug 23, 2021 at 10:19:23AM +0800, Kefeng Wang wrote:
quoted
On 2021/8/18 6:27, Rob Herring wrote:
quoted
On Mon, Aug 16, 2021 at 03:46:16PM +0800, Kefeng Wang wrote:
quoted
Patch 1 and 2 make some cleanup, and patch 3 use of_irq_get() instead of
irq_of_parse_and_map() to get irq number, return -EPROBE_DEFER if the irq
domain is not yet created, amba_device_add() will properly to handle the
no IRQ domain issue via deferred probe.
Kefeng Wang (3):
amba: Drop unused functions about APB/AHB devices add
Revert "ARM: amba: make use of -1 IRQs warn"
amba: Properly handle device probe without IRQ domain
drivers/amba/bus.c | 100 ++++++++++-----------------------------
drivers/of/platform.c | 6 +--
include/linux/amba/bus.h | 18 -------
3 files changed, 27 insertions(+), 97 deletions(-)
Reviewed-by: Rob Herring <robh@kernel.org>
Thanks Rob.
Hi Russell, should I send the patches to the ARM patch system?
Yes please - I'll try to squeeze it in for this cycle but it's getting
a tad late for that. Thanks.
Done, but the sequence of patches is reordered at ARM patch system, (using git send-email
and deliver patch1/2/3 in order).
BTW, could you give me some direction the following patchset[1] too if you have time, I have
addressed your comments and resend, but there's been no new feedback for a long time.
If it is too late for this cycle, I could resend them after 5.15-rc1.
Many thanks.
[1] https://lore.kernel.org/linux-arm-kernel/20210610123556.171328-1-wangkefeng.wang@huawei.com/
From: Rob Herring <robh+dt@kernel.org> Date: 2021-08-24 20:05:39
+Saravana
Saravana mentioned to me there may be some issues with this one...
On Mon, Aug 16, 2021 at 2:43 AM Kefeng Wang [off-list ref] wrote:
quoted hunk
of_amba_device_create() uses irq_of_parse_and_map() to translate
a DT interrupt specification into a Linux virtual interrupt number.
But it doesn't properly handle the case where the interrupt controller
is not yet available, eg, when pl011 interrupt is connected to MBIGEN
interrupt controller, because the mbigen initialization is too late,
which will lead to no IRQ due to no IRQ domain found, log is shown below,
"irq: no irq domain found for uart0 !"
use of_irq_get() to return -EPROBE_DEFER as above, and in the function
amba_device_try_add()/amba_device_add(), it will properly handle in such
case, also return 0 in other fail cases to be consistent as before.
Cc: Russell King <linux@armlinux.org.uk>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
Reported-by: Ruizhe Lin <redacted>
Signed-off-by: Kefeng Wang <redacted>
---
drivers/amba/bus.c | 27 +++++++++++++++++++++++++++
drivers/of/platform.c | 6 +-----
2 files changed, 28 insertions(+), 5 deletions(-)
On Tue, Aug 24, 2021 at 1:05 PM Rob Herring [off-list ref] wrote:
+Saravana
Saravana mentioned to me there may be some issues with this one...
On Mon, Aug 16, 2021 at 2:43 AM Kefeng Wang [off-list ref] wrote:
quoted
of_amba_device_create() uses irq_of_parse_and_map() to translate
a DT interrupt specification into a Linux virtual interrupt number.
But it doesn't properly handle the case where the interrupt controller
is not yet available, eg, when pl011 interrupt is connected to MBIGEN
interrupt controller, because the mbigen initialization is too late,
which will lead to no IRQ due to no IRQ domain found, log is shown below,
"irq: no irq domain found for uart0 !"
use of_irq_get() to return -EPROBE_DEFER as above, and in the function
amba_device_try_add()/amba_device_add(), it will properly handle in such
case, also return 0 in other fail cases to be consistent as before.
Cc: Russell King <linux@armlinux.org.uk>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
Reported-by: Ruizhe Lin <redacted>
Signed-off-by: Kefeng Wang <redacted>
---
drivers/amba/bus.c | 27 +++++++++++++++++++++++++++
drivers/of/platform.c | 6 +-----
2 files changed, 28 insertions(+), 5 deletions(-)
@@ -371,12 +372,38 @@ static void amba_device_release(struct device *dev)kfree(d);}+staticintof_amba_device_decode_irq(structamba_device*dev)+{+structdevice_node*node=dev->dev.of_node;+inti,irq=0;++if(IS_ENABLED(CONFIG_OF_IRQ)&&node){+/* Decode the IRQs and address ranges */+for(i=0;i<AMBA_NR_IRQS;i++){+irq=of_irq_get(node,i);+if(irq<0){+if(irq==-EPROBE_DEFER)+returnirq;+irq=0;+}++dev->irq[i]=irq;+}+}++return0;+}+staticintamba_device_try_add(structamba_device*dev,structresource*parent){u32size;void__iomem*tmp;inti,ret;+ret=of_amba_device_decode_irq(dev);+if(ret)+gotoerr_out;+
Similar to other resources the AMBA bus "gets" for the device, I think
this should be moved into amba_probe() and not here. There's no reason
to delay the addition of the device (and loading its module) because
the IRQ isn't ready yet.
-Saravana
quoted
ret = request_resource(parent, &dev->res);
if (ret)
goto err_out;
From: Kefeng Wang <hidden> Date: 2021-08-25 04:05:20
On 2021/8/25 4:08, Saravana Kannan wrote:
On Tue, Aug 24, 2021 at 1:05 PM Rob Herring [off-list ref] wrote:
quoted
+Saravana
Saravana mentioned to me there may be some issues with this one...
On Mon, Aug 16, 2021 at 2:43 AM Kefeng Wang [off-list ref] wrote:
quoted
of_amba_device_create() uses irq_of_parse_and_map() to translate
a DT interrupt specification into a Linux virtual interrupt number.
But it doesn't properly handle the case where the interrupt controller
is not yet available, eg, when pl011 interrupt is connected to MBIGEN
interrupt controller, because the mbigen initialization is too late,
which will lead to no IRQ due to no IRQ domain found, log is shown below,
"irq: no irq domain found for uart0 !"
use of_irq_get() to return -EPROBE_DEFER as above, and in the function
amba_device_try_add()/amba_device_add(), it will properly handle in such
case, also return 0 in other fail cases to be consistent as before.
Cc: Russell King <linux@armlinux.org.uk>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
Reported-by: Ruizhe Lin <redacted>
Signed-off-by: Kefeng Wang <redacted>
---
drivers/amba/bus.c | 27 +++++++++++++++++++++++++++
drivers/of/platform.c | 6 +-----
2 files changed, 28 insertions(+), 5 deletions(-)
@@ -371,12 +372,38 @@ static void amba_device_release(struct device *dev)kfree(d);}+staticintof_amba_device_decode_irq(structamba_device*dev)+{+structdevice_node*node=dev->dev.of_node;+inti,irq=0;++if(IS_ENABLED(CONFIG_OF_IRQ)&&node){+/* Decode the IRQs and address ranges */+for(i=0;i<AMBA_NR_IRQS;i++){+irq=of_irq_get(node,i);+if(irq<0){+if(irq==-EPROBE_DEFER)+returnirq;+irq=0;+}++dev->irq[i]=irq;+}+}++return0;+}+staticintamba_device_try_add(structamba_device*dev,structresource*parent){u32size;void__iomem*tmp;inti,ret;+ret=of_amba_device_decode_irq(dev);+if(ret)+gotoerr_out;+
Similar to other resources the AMBA bus "gets" for the device, I think
this should be moved into amba_probe() and not here. There's no reason
to delay the addition of the device (and loading its module) because
the IRQ isn't ready yet.
The following code in the amba_device_try_add() will be called, it uses irq[0]
and irq[1], so I put of_amba_device_decode_irq() into amba_device_try_add().
470 if (dev->irq[0])
471 ret = device_create_file(&dev->dev, &dev_attr_irq0);
472 if (ret == 0 && dev->irq[1])
473 ret = device_create_file(&dev->dev, &dev_attr_irq1);
474 if (ret == 0)
475 return ret;
of_amba_device_decode_irq() in amba_device_try_add() won't lead to issue,
only delay the device add, right?
If make it into amba_probe(), the above code should be moved too, could we
make a new patch to move both of them, or don't move them?
On Tue, Aug 24, 2021 at 9:05 PM Kefeng Wang [off-list ref] wrote:
On 2021/8/25 4:08, Saravana Kannan wrote:
quoted
On Tue, Aug 24, 2021 at 1:05 PM Rob Herring [off-list ref] wrote:
quoted
+Saravana
Saravana mentioned to me there may be some issues with this one...
On Mon, Aug 16, 2021 at 2:43 AM Kefeng Wang [off-list ref] wrote:
quoted
of_amba_device_create() uses irq_of_parse_and_map() to translate
a DT interrupt specification into a Linux virtual interrupt number.
But it doesn't properly handle the case where the interrupt controller
is not yet available, eg, when pl011 interrupt is connected to MBIGEN
interrupt controller, because the mbigen initialization is too late,
which will lead to no IRQ due to no IRQ domain found, log is shown below,
"irq: no irq domain found for uart0 !"
use of_irq_get() to return -EPROBE_DEFER as above, and in the function
amba_device_try_add()/amba_device_add(), it will properly handle in such
case, also return 0 in other fail cases to be consistent as before.
Cc: Russell King <linux@armlinux.org.uk>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
Reported-by: Ruizhe Lin <redacted>
Signed-off-by: Kefeng Wang <redacted>
---
drivers/amba/bus.c | 27 +++++++++++++++++++++++++++
drivers/of/platform.c | 6 +-----
2 files changed, 28 insertions(+), 5 deletions(-)
@@ -371,12 +372,38 @@ static void amba_device_release(struct device *dev)kfree(d);}+staticintof_amba_device_decode_irq(structamba_device*dev)+{+structdevice_node*node=dev->dev.of_node;+inti,irq=0;++if(IS_ENABLED(CONFIG_OF_IRQ)&&node){+/* Decode the IRQs and address ranges */+for(i=0;i<AMBA_NR_IRQS;i++){+irq=of_irq_get(node,i);+if(irq<0){+if(irq==-EPROBE_DEFER)+returnirq;+irq=0;+}++dev->irq[i]=irq;+}+}++return0;+}+staticintamba_device_try_add(structamba_device*dev,structresource*parent){u32size;void__iomem*tmp;inti,ret;+ret=of_amba_device_decode_irq(dev);+if(ret)+gotoerr_out;+
Similar to other resources the AMBA bus "gets" for the device, I think
this should be moved into amba_probe() and not here. There's no reason
to delay the addition of the device (and loading its module) because
the IRQ isn't ready yet.
The following code in the amba_device_try_add() will be called, it uses irq[0]
and irq[1], so I put of_amba_device_decode_irq() into amba_device_try_add().
470 if (dev->irq[0])
471 ret = device_create_file(&dev->dev, &dev_attr_irq0);
472 if (ret == 0 && dev->irq[1])
473 ret = device_create_file(&dev->dev, &dev_attr_irq1);
474 if (ret == 0)
475 return ret;
of_amba_device_decode_irq() in amba_device_try_add() won't lead to issue,
only delay the device add, right?
But delaying the device add is the issue. For example, adding a device
could trigger the loading of the corresponding module using uevents.
But now this change would delay that step. That can have other
unintended consequences -- slowing down boot, what if the driver was
working fine without the IRQ, etc.
If make it into amba_probe(), the above code should be moved too, could we
make a new patch to move both of them, or don't move them?
I'd say move them both. If Russell hasn't already picked this up, then
I'd say redo your Patch 3/3.
Btw, I've been working on [1] cleaning up the one-off deferred probe
solution that we have for amba devices. That causes a bunch of other
headaches. Your patch 3/3 takes us further in the wrong direction by
adding more reasons for delaying the addition of the device.
-Saravana
[1] - https://lore.kernel.org/lkml/CAGETcx8b228nDUho3cX9AAQ-pXOfZTMv8cj2vhdx9yc_pk8q+A@mail.gmail.com/
From: Kefeng Wang <hidden> Date: 2021-08-25 08:40:01
On 2021/8/25 16:04, Saravana Kannan wrote:
On Tue, Aug 24, 2021 at 9:05 PM Kefeng Wang [off-list ref] wrote:
quoted
On 2021/8/25 4:08, Saravana Kannan wrote:
quoted
On Tue, Aug 24, 2021 at 1:05 PM Rob Herring [off-list ref] wrote:
quoted
+Saravana
Saravana mentioned to me there may be some issues with this one...
On Mon, Aug 16, 2021 at 2:43 AM Kefeng Wang [off-list ref] wrote:
quoted
of_amba_device_create() uses irq_of_parse_and_map() to translate
a DT interrupt specification into a Linux virtual interrupt number.
But it doesn't properly handle the case where the interrupt controller
is not yet available, eg, when pl011 interrupt is connected to MBIGEN
interrupt controller, because the mbigen initialization is too late,
which will lead to no IRQ due to no IRQ domain found, log is shown below,
"irq: no irq domain found for uart0 !"
use of_irq_get() to return -EPROBE_DEFER as above, and in the function
amba_device_try_add()/amba_device_add(), it will properly handle in such
case, also return 0 in other fail cases to be consistent as before.
Cc: Russell King <linux@armlinux.org.uk>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
Reported-by: Ruizhe Lin <redacted>
Signed-off-by: Kefeng Wang <redacted>
---
drivers/amba/bus.c | 27 +++++++++++++++++++++++++++
drivers/of/platform.c | 6 +-----
2 files changed, 28 insertions(+), 5 deletions(-)
@@ -371,12 +372,38 @@ static void amba_device_release(struct device *dev)kfree(d);}+staticintof_amba_device_decode_irq(structamba_device*dev)+{+structdevice_node*node=dev->dev.of_node;+inti,irq=0;++if(IS_ENABLED(CONFIG_OF_IRQ)&&node){+/* Decode the IRQs and address ranges */+for(i=0;i<AMBA_NR_IRQS;i++){+irq=of_irq_get(node,i);+if(irq<0){+if(irq==-EPROBE_DEFER)+returnirq;+irq=0;+}++dev->irq[i]=irq;+}+}++return0;+}+staticintamba_device_try_add(structamba_device*dev,structresource*parent){u32size;void__iomem*tmp;inti,ret;+ret=of_amba_device_decode_irq(dev);+if(ret)+gotoerr_out;+
Similar to other resources the AMBA bus "gets" for the device, I think
this should be moved into amba_probe() and not here. There's no reason
to delay the addition of the device (and loading its module) because
the IRQ isn't ready yet.
The following code in the amba_device_try_add() will be called, it uses irq[0]
and irq[1], so I put of_amba_device_decode_irq() into amba_device_try_add().
470 if (dev->irq[0])
471 ret = device_create_file(&dev->dev, &dev_attr_irq0);
472 if (ret == 0 && dev->irq[1])
473 ret = device_create_file(&dev->dev, &dev_attr_irq1);
474 if (ret == 0)
475 return ret;
of_amba_device_decode_irq() in amba_device_try_add() won't lead to issue,
only delay the device add, right?
But delaying the device add is the issue. For example, adding a device
could trigger the loading of the corresponding module using uevents.
But now this change would delay that step. That can have other
unintended consequences -- slowing down boot, what if the driver was
working fine without the IRQ, etc.
quoted
If make it into amba_probe(), the above code should be moved too, could we
make a new patch to move both of them, or don't move them?
I'd say move them both. If Russell hasn't already picked this up, then
I'd say redo your Patch 3/3.
Sure，I will update it and resend.
Btw, I've been working on [1] cleaning up the one-off deferred probe
solution that we have for amba devices. That causes a bunch of other
headaches. Your patch 3/3 takes us further in the wrong direction by
adding more reasons for delaying the addition of the device.
From: Rob Herring <robh+dt@kernel.org> Date: 2021-08-25 12:33:56
On Tue, Aug 24, 2021 at 11:05 PM Kefeng Wang [off-list ref] wrote:
On 2021/8/25 4:08, Saravana Kannan wrote:
quoted
On Tue, Aug 24, 2021 at 1:05 PM Rob Herring [off-list ref] wrote:
quoted
+Saravana
Saravana mentioned to me there may be some issues with this one...
On Mon, Aug 16, 2021 at 2:43 AM Kefeng Wang [off-list ref] wrote:
quoted
of_amba_device_create() uses irq_of_parse_and_map() to translate
a DT interrupt specification into a Linux virtual interrupt number.
But it doesn't properly handle the case where the interrupt controller
is not yet available, eg, when pl011 interrupt is connected to MBIGEN
interrupt controller, because the mbigen initialization is too late,
which will lead to no IRQ due to no IRQ domain found, log is shown below,
"irq: no irq domain found for uart0 !"
use of_irq_get() to return -EPROBE_DEFER as above, and in the function
amba_device_try_add()/amba_device_add(), it will properly handle in such
case, also return 0 in other fail cases to be consistent as before.
Cc: Russell King <linux@armlinux.org.uk>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
Reported-by: Ruizhe Lin <redacted>
Signed-off-by: Kefeng Wang <redacted>
---
drivers/amba/bus.c | 27 +++++++++++++++++++++++++++
drivers/of/platform.c | 6 +-----
2 files changed, 28 insertions(+), 5 deletions(-)
@@ -371,12 +372,38 @@ static void amba_device_release(struct device *dev)kfree(d);}+staticintof_amba_device_decode_irq(structamba_device*dev)+{+structdevice_node*node=dev->dev.of_node;+inti,irq=0;++if(IS_ENABLED(CONFIG_OF_IRQ)&&node){+/* Decode the IRQs and address ranges */+for(i=0;i<AMBA_NR_IRQS;i++){+irq=of_irq_get(node,i);+if(irq<0){+if(irq==-EPROBE_DEFER)+returnirq;+irq=0;+}++dev->irq[i]=irq;+}+}++return0;+}+staticintamba_device_try_add(structamba_device*dev,structresource*parent){u32size;void__iomem*tmp;inti,ret;+ret=of_amba_device_decode_irq(dev);+if(ret)+gotoerr_out;+
Similar to other resources the AMBA bus "gets" for the device, I think
this should be moved into amba_probe() and not here. There's no reason
to delay the addition of the device (and loading its module) because
the IRQ isn't ready yet.
The following code in the amba_device_try_add() will be called, it uses irq[0]
and irq[1], so I put of_amba_device_decode_irq() into amba_device_try_add().
470 if (dev->irq[0])
471 ret = device_create_file(&dev->dev, &dev_attr_irq0);
472 if (ret == 0 && dev->irq[1])
473 ret = device_create_file(&dev->dev, &dev_attr_irq1);
474 if (ret == 0)
475 return ret;
From: Kefeng Wang <hidden> Date: 2021-08-25 14:41:32
On 2021/8/25 20:33, Rob Herring wrote:
On Tue, Aug 24, 2021 at 11:05 PM Kefeng Wang [off-list ref] wrote:
...
quoted
quoted
Similar to other resources the AMBA bus "gets" for the device, I think
this should be moved into amba_probe() and not here. There's no reason
to delay the addition of the device (and loading its module) because
the IRQ isn't ready yet.
The following code in the amba_device_try_add() will be called, it uses irq[0]
and irq[1], so I put of_amba_device_decode_irq() into amba_device_try_add().
470 if (dev->irq[0])
471 ret = device_create_file(&dev->dev, &dev_attr_irq0);
472 if (ret == 0 && dev->irq[1])
473 ret = device_create_file(&dev->dev, &dev_attr_irq1);
474 if (ret == 0)
475 return ret;
I wonder if we could just remove these. Why does userspace need them
in the first place? It's only an ABI if someone notices. Looking at
the history, AMBA bus was added in 2003 with just 'irq' and then
changed (ABI break) in 2004 to 'irq0' and 'irq1'.
Rob
#include <linux/platform_device.h>
#include <linux/reset.h>
-#include <asm/irq.h>
-
#define to_amba_driver(d) container_of(d, struct amba_driver, drv)
/* called on periphid match and class 0x9 coresight device. */
if (ret)
goto err_release;
- if (dev->irq[0])
- ret = device_create_file(&dev->dev, &dev_attr_irq0);
- if (ret == 0 && dev->irq[1])
- ret = device_create_file(&dev->dev, &dev_attr_irq1);
and do some cleanup about error handling in the next version.
From: Kefeng Wang <hidden> Date: 2021-08-26 02:45:28
On 2021/8/25 16:04, Saravana Kannan wrote:
On Tue, Aug 24, 2021 at 9:05 PM Kefeng Wang [off-list ref] wrote:
quoted
On 2021/8/25 4:08, Saravana Kannan wrote:
quoted
On Tue, Aug 24, 2021 at 1:05 PM Rob Herring [off-list ref] wrote:
quoted
+Saravana
Saravana mentioned to me there may be some issues with this one...
On Mon, Aug 16, 2021 at 2:43 AM Kefeng Wang [off-list ref] wrote:
quoted
of_amba_device_create() uses irq_of_parse_and_map() to translate
a DT interrupt specification into a Linux virtual interrupt number.
But it doesn't properly handle the case where the interrupt controller
is not yet available, eg, when pl011 interrupt is connected to MBIGEN
interrupt controller, because the mbigen initialization is too late,
which will lead to no IRQ due to no IRQ domain found, log is shown below,
"irq: no irq domain found for uart0 !"
use of_irq_get() to return -EPROBE_DEFER as above, and in the function
amba_device_try_add()/amba_device_add(), it will properly handle in such
case, also return 0 in other fail cases to be consistent as before.
Cc: Russell King <linux@armlinux.org.uk>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
Reported-by: Ruizhe Lin <redacted>
Signed-off-by: Kefeng Wang <redacted>
---
drivers/amba/bus.c | 27 +++++++++++++++++++++++++++
drivers/of/platform.c | 6 +-----
2 files changed, 28 insertions(+), 5 deletions(-)
@@ -371,12 +372,38 @@ static void amba_device_release(struct device *dev)kfree(d);}+staticintof_amba_device_decode_irq(structamba_device*dev)+{+structdevice_node*node=dev->dev.of_node;+inti,irq=0;++if(IS_ENABLED(CONFIG_OF_IRQ)&&node){+/* Decode the IRQs and address ranges */+for(i=0;i<AMBA_NR_IRQS;i++){+irq=of_irq_get(node,i);+if(irq<0){+if(irq==-EPROBE_DEFER)+returnirq;+irq=0;+}++dev->irq[i]=irq;+}+}++return0;+}+staticintamba_device_try_add(structamba_device*dev,structresource*parent){u32size;void__iomem*tmp;inti,ret;+ret=of_amba_device_decode_irq(dev);+if(ret)+gotoerr_out;+
Similar to other resources the AMBA bus "gets" for the device, I think
this should be moved into amba_probe() and not here. There's no reason
to delay the addition of the device (and loading its module) because
the IRQ isn't ready yet.
The following code in the amba_device_try_add() will be called, it uses irq[0]
and irq[1], so I put of_amba_device_decode_irq() into amba_device_try_add().
470 if (dev->irq[0])
471 ret = device_create_file(&dev->dev, &dev_attr_irq0);
472 if (ret == 0 && dev->irq[1])
473 ret = device_create_file(&dev->dev, &dev_attr_irq1);
474 if (ret == 0)
475 return ret;
of_amba_device_decode_irq() in amba_device_try_add() won't lead to issue,
only delay the device add, right?
But delaying the device add is the issue. For example, adding a device
could trigger the loading of the corresponding module using uevents.
But now this change would delay that step. That can have other
unintended consequences -- slowing down boot, what if the driver was
working fine without the IRQ, etc.
quoted
If make it into amba_probe(), the above code should be moved too, could we
make a new patch to move both of them, or don't move them?
I'd say move them both. If Russell hasn't already picked this up, then
I'd say redo your Patch 3/3.
I will resend with put it into amba_probe.
Btw, I've been working on [1] cleaning up the one-off deferred probe
solution that we have for amba devices. That causes a bunch of other
headaches. Your patch 3/3 takes us further in the wrong direction by
adding more reasons for delaying the addition of the device.
Got it, and I could resend all combine your patch(due to context conflict
when changing same function) if you no object.
On Wed, Aug 25, 2021 at 7:45 PM Kefeng Wang [off-list ref] wrote:
On 2021/8/25 16:04, Saravana Kannan wrote:
quoted
On Tue, Aug 24, 2021 at 9:05 PM Kefeng Wang [off-list ref] wrote:
quoted
On 2021/8/25 4:08, Saravana Kannan wrote:
quoted
On Tue, Aug 24, 2021 at 1:05 PM Rob Herring [off-list ref] wrote:
quoted
+Saravana
Saravana mentioned to me there may be some issues with this one...
On Mon, Aug 16, 2021 at 2:43 AM Kefeng Wang [off-list ref] wrote:
quoted
of_amba_device_create() uses irq_of_parse_and_map() to translate
a DT interrupt specification into a Linux virtual interrupt number.
But it doesn't properly handle the case where the interrupt controller
is not yet available, eg, when pl011 interrupt is connected to MBIGEN
interrupt controller, because the mbigen initialization is too late,
which will lead to no IRQ due to no IRQ domain found, log is shown below,
"irq: no irq domain found for uart0 !"
use of_irq_get() to return -EPROBE_DEFER as above, and in the function
amba_device_try_add()/amba_device_add(), it will properly handle in such
case, also return 0 in other fail cases to be consistent as before.
Cc: Russell King <linux@armlinux.org.uk>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
Reported-by: Ruizhe Lin <redacted>
Signed-off-by: Kefeng Wang <redacted>
---
drivers/amba/bus.c | 27 +++++++++++++++++++++++++++
drivers/of/platform.c | 6 +-----
2 files changed, 28 insertions(+), 5 deletions(-)
@@ -371,12 +372,38 @@ static void amba_device_release(struct device *dev)kfree(d);}+staticintof_amba_device_decode_irq(structamba_device*dev)+{+structdevice_node*node=dev->dev.of_node;+inti,irq=0;++if(IS_ENABLED(CONFIG_OF_IRQ)&&node){+/* Decode the IRQs and address ranges */+for(i=0;i<AMBA_NR_IRQS;i++){+irq=of_irq_get(node,i);+if(irq<0){+if(irq==-EPROBE_DEFER)+returnirq;+irq=0;+}++dev->irq[i]=irq;+}+}++return0;+}+staticintamba_device_try_add(structamba_device*dev,structresource*parent){u32size;void__iomem*tmp;inti,ret;+ret=of_amba_device_decode_irq(dev);+if(ret)+gotoerr_out;+
Similar to other resources the AMBA bus "gets" for the device, I think
this should be moved into amba_probe() and not here. There's no reason
to delay the addition of the device (and loading its module) because
the IRQ isn't ready yet.
The following code in the amba_device_try_add() will be called, it uses irq[0]
and irq[1], so I put of_amba_device_decode_irq() into amba_device_try_add().
470 if (dev->irq[0])
471 ret = device_create_file(&dev->dev, &dev_attr_irq0);
472 if (ret == 0 && dev->irq[1])
473 ret = device_create_file(&dev->dev, &dev_attr_irq1);
474 if (ret == 0)
475 return ret;
of_amba_device_decode_irq() in amba_device_try_add() won't lead to issue,
only delay the device add, right?
But delaying the device add is the issue. For example, adding a device
could trigger the loading of the corresponding module using uevents.
But now this change would delay that step. That can have other
unintended consequences -- slowing down boot, what if the driver was
working fine without the IRQ, etc.
quoted
If make it into amba_probe(), the above code should be moved too, could we
make a new patch to move both of them, or don't move them?
I'd say move them both. If Russell hasn't already picked this up, then
I'd say redo your Patch 3/3.
I will resend with put it into amba_probe.
quoted
Btw, I've been working on [1] cleaning up the one-off deferred probe
solution that we have for amba devices. That causes a bunch of other
headaches. Your patch 3/3 takes us further in the wrong direction by
adding more reasons for delaying the addition of the device.
Got it, and I could resend all combine your patch(due to context conflict
when changing same function) if you no object.
If you want to resolve the conflict with my patch and resend it while
keeping me as the author, I would definitely appreciate it.
-Saravana
From: Kefeng Wang <hidden> Date: 2021-08-26 06:22:30
On 2021/8/26 12:45, Saravana Kannan wrote:
On Wed, Aug 25, 2021 at 7:45 PM Kefeng Wang [off-list ref] wrote:
quoted
On 2021/8/25 16:04, Saravana Kannan wrote:
quoted
On Tue, Aug 24, 2021 at 9:05 PM Kefeng Wang [off-list ref] wrote:
quoted
On 2021/8/25 4:08, Saravana Kannan wrote:
quoted
On Tue, Aug 24, 2021 at 1:05 PM Rob Herring [off-list ref] wrote:
quoted
+Saravana
Saravana mentioned to me there may be some issues with this one...
On Mon, Aug 16, 2021 at 2:43 AM Kefeng Wang [off-list ref] wrote:
quoted
of_amba_device_create() uses irq_of_parse_and_map() to translate
a DT interrupt specification into a Linux virtual interrupt number.
But it doesn't properly handle the case where the interrupt controller
is not yet available, eg, when pl011 interrupt is connected to MBIGEN
interrupt controller, because the mbigen initialization is too late,
which will lead to no IRQ due to no IRQ domain found, log is shown below,
"irq: no irq domain found for uart0 !"
use of_irq_get() to return -EPROBE_DEFER as above, and in the function
amba_device_try_add()/amba_device_add(), it will properly handle in such
case, also return 0 in other fail cases to be consistent as before.
Cc: Russell King <linux@armlinux.org.uk>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
Reported-by: Ruizhe Lin <redacted>
Signed-off-by: Kefeng Wang <redacted>
---
...
quoted
quoted
quoted
quoted
Similar to other resources the AMBA bus "gets" for the device, I think
this should be moved into amba_probe() and not here. There's no reason
to delay the addition of the device (and loading its module) because
the IRQ isn't ready yet.
The following code in the amba_device_try_add() will be called, it uses irq[0]
and irq[1], so I put of_amba_device_decode_irq() into amba_device_try_add().
470 if (dev->irq[0])
471 ret = device_create_file(&dev->dev, &dev_attr_irq0);
472 if (ret == 0 && dev->irq[1])
473 ret = device_create_file(&dev->dev, &dev_attr_irq1);
474 if (ret == 0)
475 return ret;
of_amba_device_decode_irq() in amba_device_try_add() won't lead to issue,
only delay the device add, right?
But delaying the device add is the issue. For example, adding a device
could trigger the loading of the corresponding module using uevents.
But now this change would delay that step. That can have other
unintended consequences -- slowing down boot, what if the driver was
working fine without the IRQ, etc.
quoted
If make it into amba_probe(), the above code should be moved too, could we
make a new patch to move both of them, or don't move them?
I'd say move them both. If Russell hasn't already picked this up, then
I'd say redo your Patch 3/3.
I will resend with put it into amba_probe.
quoted
Btw, I've been working on [1] cleaning up the one-off deferred probe
solution that we have for amba devices. That causes a bunch of other
headaches. Your patch 3/3 takes us further in the wrong direction by
adding more reasons for delaying the addition of the device.
Got it, and I could resend all combine your patch(due to context conflict
when changing same function) if you no object.
If you want to resolve the conflict with my patch and resend it while
keeping me as the author, I would definitely appreciate it.
Yes, I will keep it, and rebase my patch based on it.
From: Kefeng Wang <hidden> Date: 2021-08-26 08:22:49
quoted
quoted
Btw, I've been working on [1] cleaning up the one-off deferred probe
solution that we have for amba devices. That causes a bunch of other
headaches. Your patch 3/3 takes us further in the wrong direction by
adding more reasons for delaying the addition of the device.
On Thu, Aug 26, 2021 at 1:22 AM Kefeng Wang [off-list ref] wrote:
quoted
quoted
quoted
Btw, I've been working on [1] cleaning up the one-off deferred probe
solution that we have for amba devices. That causes a bunch of other
headaches. Your patch 3/3 takes us further in the wrong direction by
adding more reasons for delaying the addition of the device.
Hi Saravana, I try the link[1], but with it, there is a crash when boot
(qemu-system-arm -M vexpress-a15),
Hi,
It's hard to make sense of the logs. Looks like two different threads
might be printing to the log at the same time? Can you please enable
the config that prints the thread ID (forgot what it's called) and
collect this again? With what I could tell the crash seems to be
happening somewhere in platform_match(), but that's not related to
this patch at all?
-Saravana
From: Kefeng Wang <hidden> Date: 2021-08-27 14:38:42
On 2021/8/27 8:04, Saravana Kannan wrote:
On Thu, Aug 26, 2021 at 1:22 AM Kefeng Wang [off-list ref] wrote:
quoted
quoted
quoted
quoted
Btw, I've been working on [1] cleaning up the one-off deferred probe
solution that we have for amba devices. That causes a bunch of other
headaches. Your patch 3/3 takes us further in the wrong direction by
adding more reasons for delaying the addition of the device.
Hi Saravana, I try the link[1], but with it, there is a crash when boot
(qemu-system-arm -M vexpress-a15),
Hi,
It's hard to make sense of the logs. Looks like two different threads
might be printing to the log at the same time? Can you please enable
the config that prints the thread ID (forgot what it's called) and
collect this again? With what I could tell the crash seems to be
happening somewhere in platform_match(), but that's not related to
this patch at all?
Can you reproduce it? it is very likely related(without your patch, the boot is fine),
the NULL ptr is about serio, it is registed from amba driver.
ambakmi_driver_init
-- amba_kmi_probe
-- __serio_register_port
/* If primecell ID isn't hard-coded, figure it out */
if (!dev->periphid) {
ret = amba_read_periphid(dev);
+ dev_info(&dev->dev, "%s, amba_read_periphid ret = %d\n", __func__, ret);
if (ret && ret != -EPROBE_DEFER)
goto err_release;
/*
On Fri, Aug 27, 2021 at 7:38 AM Kefeng Wang [off-list ref] wrote:
On 2021/8/27 8:04, Saravana Kannan wrote:
quoted
On Thu, Aug 26, 2021 at 1:22 AM Kefeng Wang [off-list ref] wrote:
quoted
quoted
quoted
quoted
Btw, I've been working on [1] cleaning up the one-off deferred probe
solution that we have for amba devices. That causes a bunch of other
headaches. Your patch 3/3 takes us further in the wrong direction by
adding more reasons for delaying the addition of the device.
Hi Saravana, I try the link[1], but with it, there is a crash when boot
(qemu-system-arm -M vexpress-a15),
I'm assuming it's this one?
arch/arm/boot/dts/vexpress-v2p-ca15_a7.dts
quoted
Hi,
It's hard to make sense of the logs. Looks like two different threads
might be printing to the log at the same time? Can you please enable
the config that prints the thread ID (forgot what it's called) and
collect this again? With what I could tell the crash seems to be
happening somewhere in platform_match(), but that's not related to
this patch at all?
Can you reproduce it? it is very likely related(without your patch, the
boot is fine),
Sorry, I haven't ever setup qemu and booted vexpress. Thanks for your help.
the NULL ptr is about serio, it is registed from amba driver.
ambakmi_driver_init
-- amba_kmi_probe
-- __serio_register_port
Thanks for the pointer. I took a look at the logs and the code. It's
very strange. As you can see from the backtrace, platform_match() is
being called for the device_add() from serio_handle_event(). But the
device that gets added there is on the serio_bus which obviously
should be using the serio_bus_match.
+Dmitry and input maillist, is there some known issue about serio ?
I add some debug, the full log is attached.
[ 2.958355][ T41] input: AT Raw Set 2 keyboard as
/devices/platform/bus@8000000/bus@8000000:motherboard-bus/bus@8000000:motherboard-bus:iofpga-bus@300000000/1c060000.kmi/serio0/input/input0
[ 2.977441][ T41] serio serio1: pdev c1e05508, pdev->name (null),
drv c1090fc0, drv->name vexpress-reset
Based on the logs you added, it's pretty clear we are getting to
platform_match(). It's also strange that the drv->name is
vexpress-reset
[ 2.977928][ T41] 8<--- cut here ---
[ 2.978162][ T41] Unhandled fault: page domain fault (0x01b) at
0x00000000
[ 2.978494][ T41] pgd = (ptrval)
[ 2.978819][ T41] [00000000] *pgd=00000000
[ 2.979881][ T41] Internal error: : 1b [#1] SMP ARM
[ 2.980385][ T41] Modules linked in:
[ 2.980810][ T41] CPU: 0 PID: 41 Comm: kworker/0:2 Not tainted
5.14.0-rc7+ #213
[ 2.981229][ T41] Hardware name: ARM-Versatile Express
[ 2.981780][ T41] Workqueue: events_long serio_handle_event
[ 2.982737][ T41] PC is at strcmp+0x18/0x44
[ 2.983030][ T41] LR is at platform_match+0xdc/0xf0
[ 2.983283][ T41] pc : [<c0560bcc>] lr : [<c0646358>] psr:
600b0013
[ 2.983572][ T41] sp : c1675d68 ip : c1675d78 fp : c1675d74
[ 2.983832][ T41] r10: 00000000 r9 : 00000000 r8 : 00000001
[ 2.984095][ T41] r7 : c1e05518 r6 : c1675df4 r5 : c1e05518 r4 :
c1090fc0
[ 2.984395][ T41] r3 : c0a5e180 r2 : 6bede3db r1 : c0b82a04 r0 :
00000000
[ 2.984906][ T41] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA
ARM Segment none
---- 8< ---- cleaning up a bunch of register dumps
[ 3.003113][ T41] Backtrace:
[ 3.003451][ T41] [<c0560bb4>] (strcmp) from [<c0646358>] (platform_match+0xdc/0xf0)
[ 3.003963][ T41] [<c064627c>] (platform_match) from [<c06437d4>] (__device_attach_driver+0x3c/0xf4)
[ 3.004769][ T41] [<c0643798>] (__device_attach_driver) from [<c0641180>] (bus_for_each_drv+0x68/0xc8)
[ 3.005481][ T41] [<c0641118>] (bus_for_each_drv) from [<c0642f40>] (__device_attach+0xf0/0x16c)
[ 3.006152][ T41] [<c0642e50>] (__device_attach) from [<c06439d4>] (device_initial_probe+0x1c/0x20)
[ 3.006853][ T41] [<c06439b8>] (device_initial_probe) from [<c0642030>] (bus_probe_device+0x94/0x9c)
[ 3.007259][ T41] [<c0641f9c>] (bus_probe_device) from [<c063f9cc>] (device_add+0x408/0x8b8)
[ 3.007900][ T41] [<c063f5c4>] (device_add) from [<c071c1cc>] (serio_handle_event+0x1b8/0x234)
[ 3.008824][ T41] [<c071c014>] (serio_handle_event) from [<c01475a4>] (process_one_work+0x238/0x594)
[ 3.009737][ T41] [<c014736c>] (process_one_work) from [<c014795c>] (worker_thread+0x5c/0x5f4)
[ 3.010638][ T41] [<c0147900>] (worker_thread) from [<c014feb4>] (kthread+0x178/0x194)
[ 3.011496][ T41] [<c014fd3c>] (kthread) from [<c0100150>] (ret_from_fork+0x14/0x24)
[ 3.011860][ T41] Exception stack(0xc1675fb0 to 0xc1675ff8)
But the platform_match() is happening for the device_add() from
serio_event_handle() that's adding a device to the serio_bus and it
should be using serio_bus_match().
I haven't reached any conclusion yet, but my current thought process
is that it's either:
1. My patch is somehow causing list corruption. But I don't directly
touch any list in my change (other than deleting a list entirely), so
it's not clear how that would be happening.
2. Without my patch, these AMBA device's probe would be delayed at
least until 5 seconds or possibly later. I'm wondering if my patch is
catching some bad timing assumptions in other code.
You might be able to test out theory (2) by DEFERRED_DEVICE_TIMEOUT to
a much smaller number. Say 500ms or 100ms. If it doesn't crash, it
doesn't mean it's not (2), but if it does, then we know for sure it's
(2).
I'll continue debugging further.
-Saravana
@@ -237,6 +237,7 @@ static int amba_match(struct device *dev, struct
device_driver *drv)
if (!pcdev->periphid) {
int ret = amba_read_periphid(pcdev);
+ dev_info(dev, "%s, amba_read_periphid ret = %d\n",
__func__, ret);
if (ret)
return ret;
@@ -522,6 +523,7 @@ int amba_device_add(struct amba_device *dev, struct
resource *parent)
/* If primecell ID isn't hard-coded, figure it out */
if (!dev->periphid) {
ret = amba_read_periphid(dev);
+ dev_info(&dev->dev, "%s, amba_read_periphid ret = %d\n",
__func__, ret);
if (ret && ret != -EPROBE_DEFER)
goto err_release;
/*
From: Kefeng Wang <hidden> Date: 2021-08-28 01:09:22
On 2021/8/28 3:09, Saravana Kannan wrote:
On Fri, Aug 27, 2021 at 7:38 AM Kefeng Wang [off-list ref] wrote:
quoted
On 2021/8/27 8:04, Saravana Kannan wrote:
quoted
On Thu, Aug 26, 2021 at 1:22 AM Kefeng Wang [off-list ref] wrote:
quoted
quoted
quoted
quoted
Btw, I've been working on [1] cleaning up the one-off deferred probe
solution that we have for amba devices. That causes a bunch of other
headaches. Your patch 3/3 takes us further in the wrong direction by
adding more reasons for delaying the addition of the device.
Hi Saravana, I try the link[1], but with it, there is a crash when boot
(qemu-system-arm -M vexpress-a15),
I'm assuming it's this one?
arch/arm/boot/dts/vexpress-v2p-ca15_a7.dts
Hi,
It's hard to make sense of the logs. Looks like two different threads
might be printing to the log at the same time? Can you please enable
the config that prints the thread ID (forgot what it's called) and
collect this again? With what I could tell the crash seems to be
happening somewhere in platform_match(), but that's not related to
this patch at all?
Can you reproduce it? it is very likely related(without your patch, the
boot is fine),
Sorry, I haven't ever setup qemu and booted vexpress. Thanks for your help.
quoted
the NULL ptr is about serio, it is registed from amba driver.
ambakmi_driver_init
-- amba_kmi_probe
-- __serio_register_port
Thanks for the pointer. I took a look at the logs and the code. It's
very strange. As you can see from the backtrace, platform_match() is
being called for the device_add() from serio_handle_event(). But the
device that gets added there is on the serio_bus which obviously
should be using the serio_bus_match.
Yes, I am confused too.
quoted
+Dmitry and input maillist, is there some known issue about serio ?
I add some debug, the full log is attached.
[ 2.958355][ T41] input: AT Raw Set 2 keyboard as
/devices/platform/bus@8000000/bus@8000000:motherboard-bus/bus@8000000:motherboard-bus:iofpga-bus@300000000/1c060000.kmi/serio0/input/input0
[ 2.977441][ T41] serio serio1: pdev c1e05508, pdev->name (null),
drv c1090fc0, drv->name vexpress-reset
Based on the logs you added, it's pretty clear we are getting to
platform_match(). It's also strange that the drv->name is
vexpress-reset
...
quoted
[ 3.003113][ T41] Backtrace:
[ 3.003451][ T41] [<c0560bb4>] (strcmp) from [<c0646358>] (platform_match+0xdc/0xf0)
[ 3.003963][ T41] [<c064627c>] (platform_match) from [<c06437d4>] (__device_attach_driver+0x3c/0xf4)
[ 3.004769][ T41] [<c0643798>] (__device_attach_driver) from [<c0641180>] (bus_for_each_drv+0x68/0xc8)
[ 3.005481][ T41] [<c0641118>] (bus_for_each_drv) from [<c0642f40>] (__device_attach+0xf0/0x16c)
[ 3.006152][ T41] [<c0642e50>] (__device_attach) from [<c06439d4>] (device_initial_probe+0x1c/0x20)
[ 3.006853][ T41] [<c06439b8>] (device_initial_probe) from [<c0642030>] (bus_probe_device+0x94/0x9c)
[ 3.007259][ T41] [<c0641f9c>] (bus_probe_device) from [<c063f9cc>] (device_add+0x408/0x8b8)
[ 3.007900][ T41] [<c063f5c4>] (device_add) from [<c071c1cc>] (serio_handle_event+0x1b8/0x234)
[ 3.008824][ T41] [<c071c014>] (serio_handle_event) from [<c01475a4>] (process_one_work+0x238/0x594)
[ 3.009737][ T41] [<c014736c>] (process_one_work) from [<c014795c>] (worker_thread+0x5c/0x5f4)
[ 3.010638][ T41] [<c0147900>] (worker_thread) from [<c014feb4>] (kthread+0x178/0x194)
[ 3.011496][ T41] [<c014fd3c>] (kthread) from [<c0100150>] (ret_from_fork+0x14/0x24)
[ 3.011860][ T41] Exception stack(0xc1675fb0 to 0xc1675ff8)
But the platform_match() is happening for the device_add() from
serio_event_handle() that's adding a device to the serio_bus and it
should be using serio_bus_match().
I haven't reached any conclusion yet, but my current thought process
is that it's either:
1. My patch is somehow causing list corruption. But I don't directly
touch any list in my change (other than deleting a list entirely), so
it's not clear how that would be happening.
Maybe some concurrent driver load？
2. Without my patch, these AMBA device's probe would be delayed at
least until 5 seconds or possibly later. I'm wondering if my patch is
catching some bad timing assumptions in other code.
After Rob's patch, It will retry soon.
commit 039599c92d3b2e73689e8b6e519d653fd4770abb
Author: Rob Herring [off-list ref]
Date: Wed Apr 29 15:58:12 2020 -0500
amba: Retry adding deferred devices at late_initcall
If amba bus devices defer when adding, the amba bus code simply retries
adding the devices every 5 seconds. This doesn't work well as it
completely unsynchronized with starting the init process which can
happen in less than 5 secs. Add a retry during late_initcall. If the
amba devices are added, then deferred probe takes over. If the
dependencies have not probed at this point, then there's no improvement
over previous behavior. To completely solve this, we'd need to retry
after every successful probe as deferred probe does.
The list_empty() check now happens outside the mutex, but the mutex
wasn't necessary in the first place.
This needed to use deferred probe instead of fragile initcall ordering
on 32-bit VExpress systems where the apb_pclk has a number of probe
dependencies (vexpress-sysregs, vexpress-config).
You might be able to test out theory (2) by DEFERRED_DEVICE_TIMEOUT to
a much smaller number. Say 500ms or 100ms. If it doesn't crash, it
doesn't mean it's not (2), but if it does, then we know for sure it's
(2).
ok, I will try this one, but due to above patch, it may not work.
On Fri, Aug 27, 2021 at 6:09 PM Kefeng Wang [off-list ref] wrote:
On 2021/8/28 3:09, Saravana Kannan wrote:
quoted
On Fri, Aug 27, 2021 at 7:38 AM Kefeng Wang [off-list ref] wrote:
quoted
On 2021/8/27 8:04, Saravana Kannan wrote:
quoted
On Thu, Aug 26, 2021 at 1:22 AM Kefeng Wang [off-list ref] wrote:
quoted
quoted
quoted
quoted
Btw, I've been working on [1] cleaning up the one-off deferred probe
solution that we have for amba devices. That causes a bunch of other
headaches. Your patch 3/3 takes us further in the wrong direction by
adding more reasons for delaying the addition of the device.
Hi Saravana, I try the link[1], but with it, there is a crash when boot
(qemu-system-arm -M vexpress-a15),
I'm assuming it's this one?
arch/arm/boot/dts/vexpress-v2p-ca15_a7.dts
Hi,
It's hard to make sense of the logs. Looks like two different threads
might be printing to the log at the same time? Can you please enable
the config that prints the thread ID (forgot what it's called) and
collect this again? With what I could tell the crash seems to be
happening somewhere in platform_match(), but that's not related to
this patch at all?
Can you reproduce it? it is very likely related(without your patch, the
boot is fine),
Sorry, I haven't ever setup qemu and booted vexpress. Thanks for your help.
quoted
the NULL ptr is about serio, it is registed from amba driver.
ambakmi_driver_init
-- amba_kmi_probe
-- __serio_register_port
Thanks for the pointer. I took a look at the logs and the code. It's
very strange. As you can see from the backtrace, platform_match() is
being called for the device_add() from serio_handle_event(). But the
device that gets added there is on the serio_bus which obviously
should be using the serio_bus_match.
Yes, I am confused too.
quoted
quoted
+Dmitry and input maillist, is there some known issue about serio ?
I add some debug, the full log is attached.
[ 2.958355][ T41] input: AT Raw Set 2 keyboard as
/devices/platform/bus@8000000/bus@8000000:motherboard-bus/bus@8000000:motherboard-bus:iofpga-bus@300000000/1c060000.kmi/serio0/input/input0
[ 2.977441][ T41] serio serio1: pdev c1e05508, pdev->name (null),
drv c1090fc0, drv->name vexpress-reset
Based on the logs you added, it's pretty clear we are getting to
platform_match(). It's also strange that the drv->name is
vexpress-reset
...
quoted
quoted
[ 3.003113][ T41] Backtrace:
[ 3.003451][ T41] [<c0560bb4>] (strcmp) from [<c0646358>] (platform_match+0xdc/0xf0)
[ 3.003963][ T41] [<c064627c>] (platform_match) from [<c06437d4>] (__device_attach_driver+0x3c/0xf4)
[ 3.004769][ T41] [<c0643798>] (__device_attach_driver) from [<c0641180>] (bus_for_each_drv+0x68/0xc8)
[ 3.005481][ T41] [<c0641118>] (bus_for_each_drv) from [<c0642f40>] (__device_attach+0xf0/0x16c)
[ 3.006152][ T41] [<c0642e50>] (__device_attach) from [<c06439d4>] (device_initial_probe+0x1c/0x20)
[ 3.006853][ T41] [<c06439b8>] (device_initial_probe) from [<c0642030>] (bus_probe_device+0x94/0x9c)
[ 3.007259][ T41] [<c0641f9c>] (bus_probe_device) from [<c063f9cc>] (device_add+0x408/0x8b8)
[ 3.007900][ T41] [<c063f5c4>] (device_add) from [<c071c1cc>] (serio_handle_event+0x1b8/0x234)
[ 3.008824][ T41] [<c071c014>] (serio_handle_event) from [<c01475a4>] (process_one_work+0x238/0x594)
[ 3.009737][ T41] [<c014736c>] (process_one_work) from [<c014795c>] (worker_thread+0x5c/0x5f4)
[ 3.010638][ T41] [<c0147900>] (worker_thread) from [<c014feb4>] (kthread+0x178/0x194)
[ 3.011496][ T41] [<c014fd3c>] (kthread) from [<c0100150>] (ret_from_fork+0x14/0x24)
[ 3.011860][ T41] Exception stack(0xc1675fb0 to 0xc1675ff8)
But the platform_match() is happening for the device_add() from
serio_event_handle() that's adding a device to the serio_bus and it
should be using serio_bus_match().
I haven't reached any conclusion yet, but my current thought process
is that it's either:
1. My patch is somehow causing list corruption. But I don't directly
touch any list in my change (other than deleting a list entirely), so
it's not clear how that would be happening.
Maybe some concurrent driver load？
quoted
2. Without my patch, these AMBA device's probe would be delayed at
least until 5 seconds or possibly later. I'm wondering if my patch is
catching some bad timing assumptions in other code.
After Rob's patch, It will retry soon.
commit 039599c92d3b2e73689e8b6e519d653fd4770abb
Author: Rob Herring [off-list ref]
Date: Wed Apr 29 15:58:12 2020 -0500
amba: Retry adding deferred devices at late_initcall
If amba bus devices defer when adding, the amba bus code simply retries
adding the devices every 5 seconds. This doesn't work well as it
completely unsynchronized with starting the init process which can
happen in less than 5 secs. Add a retry during late_initcall. If the
amba devices are added, then deferred probe takes over. If the
dependencies have not probed at this point, then there's no improvement
over previous behavior. To completely solve this, we'd need to retry
after every successful probe as deferred probe does.
The list_empty() check now happens outside the mutex, but the mutex
wasn't necessary in the first place.
This needed to use deferred probe instead of fragile initcall ordering
on 32-bit VExpress systems where the apb_pclk has a number of probe
dependencies (vexpress-sysregs, vexpress-config).
quoted
You might be able to test out theory (2) by DEFERRED_DEVICE_TIMEOUT to
a much smaller number. Say 500ms or 100ms. If it doesn't crash, it
doesn't mean it's not (2), but if it does, then we know for sure it's
(2).
ok, I will try this one, but due to above patch, it may not work.
From: Kefeng Wang <hidden> Date: 2021-09-10 07:59:49
On 2021/9/9 11:30, Saravana Kannan wrote:
On Fri, Aug 27, 2021 at 6:09 PM Kefeng Wang [off-list ref] wrote:
quoted
On 2021/8/28 3:09, Saravana Kannan wrote:
quoted
On Fri, Aug 27, 2021 at 7:38 AM Kefeng Wang [off-list ref] wrote:
quoted
On 2021/8/27 8:04, Saravana Kannan wrote:
quoted
On Thu, Aug 26, 2021 at 1:22 AM Kefeng Wang [off-list ref] wrote:
quoted
quoted
quoted
quoted
Btw, I've been working on [1] cleaning up the one-off deferred probe
solution that we have for amba devices. That causes a bunch of other
headaches. Your patch 3/3 takes us further in the wrong direction by
adding more reasons for delaying the addition of the device.
Hi Saravana, I try the link[1], but with it, there is a crash when boot
(qemu-system-arm -M vexpress-a15),
I'm assuming it's this one?
arch/arm/boot/dts/vexpress-v2p-ca15_a7.dts
Hi,
It's hard to make sense of the logs. Looks like two different threads
might be printing to the log at the same time? Can you please enable
the config that prints the thread ID (forgot what it's called) and
collect this again? With what I could tell the crash seems to be
happening somewhere in platform_match(), but that's not related to
this patch at all?
Can you reproduce it? it is very likely related(without your patch, the
boot is fine),
Sorry, I haven't ever setup qemu and booted vexpress. Thanks for your help.
quoted
the NULL ptr is about serio, it is registed from amba driver.
ambakmi_driver_init
-- amba_kmi_probe
-- __serio_register_port
Thanks for the pointer. I took a look at the logs and the code. It's
very strange. As you can see from the backtrace, platform_match() is
being called for the device_add() from serio_handle_event(). But the
device that gets added there is on the serio_bus which obviously
should be using the serio_bus_match.
Yes, I am confused too.
quoted
quoted
+Dmitry and input maillist, is there some known issue about serio ?
I add some debug, the full log is attached.
[ 2.958355][ T41] input: AT Raw Set 2 keyboard as
/devices/platform/bus@8000000/bus@8000000:motherboard-bus/bus@8000000:motherboard-bus:iofpga-bus@300000000/1c060000.kmi/serio0/input/input0
[ 2.977441][ T41] serio serio1: pdev c1e05508, pdev->name (null),
drv c1090fc0, drv->name vexpress-reset
Based on the logs you added, it's pretty clear we are getting to
platform_match(). It's also strange that the drv->name is
vexpress-reset
...
quoted
quoted
[ 3.003113][ T41] Backtrace:
[ 3.003451][ T41] [<c0560bb4>] (strcmp) from [<c0646358>] (platform_match+0xdc/0xf0)
[ 3.003963][ T41] [<c064627c>] (platform_match) from [<c06437d4>] (__device_attach_driver+0x3c/0xf4)
[ 3.004769][ T41] [<c0643798>] (__device_attach_driver) from [<c0641180>] (bus_for_each_drv+0x68/0xc8)
[ 3.005481][ T41] [<c0641118>] (bus_for_each_drv) from [<c0642f40>] (__device_attach+0xf0/0x16c)
[ 3.006152][ T41] [<c0642e50>] (__device_attach) from [<c06439d4>] (device_initial_probe+0x1c/0x20)
[ 3.006853][ T41] [<c06439b8>] (device_initial_probe) from [<c0642030>] (bus_probe_device+0x94/0x9c)
[ 3.007259][ T41] [<c0641f9c>] (bus_probe_device) from [<c063f9cc>] (device_add+0x408/0x8b8)
[ 3.007900][ T41] [<c063f5c4>] (device_add) from [<c071c1cc>] (serio_handle_event+0x1b8/0x234)
[ 3.008824][ T41] [<c071c014>] (serio_handle_event) from [<c01475a4>] (process_one_work+0x238/0x594)
[ 3.009737][ T41] [<c014736c>] (process_one_work) from [<c014795c>] (worker_thread+0x5c/0x5f4)
[ 3.010638][ T41] [<c0147900>] (worker_thread) from [<c014feb4>] (kthread+0x178/0x194)
[ 3.011496][ T41] [<c014fd3c>] (kthread) from [<c0100150>] (ret_from_fork+0x14/0x24)
[ 3.011860][ T41] Exception stack(0xc1675fb0 to 0xc1675ff8)
But the platform_match() is happening for the device_add() from
serio_event_handle() that's adding a device to the serio_bus and it
should be using serio_bus_match().
I haven't reached any conclusion yet, but my current thought process
is that it's either:
1. My patch is somehow causing list corruption. But I don't directly
touch any list in my change (other than deleting a list entirely), so
it's not clear how that would be happening.
Maybe some concurrent driver load？
quoted
2. Without my patch, these AMBA device's probe would be delayed at
least until 5 seconds or possibly later. I'm wondering if my patch is
catching some bad timing assumptions in other code.
After Rob's patch, It will retry soon.
commit 039599c92d3b2e73689e8b6e519d653fd4770abb
Author: Rob Herring [off-list ref]
Date: Wed Apr 29 15:58:12 2020 -0500
amba: Retry adding deferred devices at late_initcall
If amba bus devices defer when adding, the amba bus code simply retries
adding the devices every 5 seconds. This doesn't work well as it
completely unsynchronized with starting the init process which can
happen in less than 5 secs. Add a retry during late_initcall. If the
amba devices are added, then deferred probe takes over. If the
dependencies have not probed at this point, then there's no improvement
over previous behavior. To completely solve this, we'd need to retry
after every successful probe as deferred probe does.
The list_empty() check now happens outside the mutex, but the mutex
wasn't necessary in the first place.
This needed to use deferred probe instead of fragile initcall ordering
on 32-bit VExpress systems where the apb_pclk has a number of probe
dependencies (vexpress-sysregs, vexpress-config).
quoted
You might be able to test out theory (2) by DEFERRED_DEVICE_TIMEOUT to
a much smaller number. Say 500ms or 100ms. If it doesn't crash, it
doesn't mean it's not (2), but if it does, then we know for sure it's
(2).
ok, I will try this one, but due to above patch, it may not work.
Were you able to find anything more?
I can't find any clue， and have no time to check this for now, is there any news from your side?
On Fri, Sep 10, 2021 at 12:59 AM Kefeng Wang [off-list ref] wrote:
On 2021/9/9 11:30, Saravana Kannan wrote:
quoted
On Fri, Aug 27, 2021 at 6:09 PM Kefeng Wang [off-list ref] wrote:
quoted
On 2021/8/28 3:09, Saravana Kannan wrote:
quoted
On Fri, Aug 27, 2021 at 7:38 AM Kefeng Wang [off-list ref] wrote:
quoted
On 2021/8/27 8:04, Saravana Kannan wrote:
quoted
On Thu, Aug 26, 2021 at 1:22 AM Kefeng Wang [off-list ref] wrote:
quoted
quoted
quoted
quoted
Btw, I've been working on [1] cleaning up the one-off deferred probe
solution that we have for amba devices. That causes a bunch of other
headaches. Your patch 3/3 takes us further in the wrong direction by
adding more reasons for delaying the addition of the device.
Hi Saravana, I try the link[1], but with it, there is a crash when boot
(qemu-system-arm -M vexpress-a15),
I'm assuming it's this one?
arch/arm/boot/dts/vexpress-v2p-ca15_a7.dts
Hi,
It's hard to make sense of the logs. Looks like two different threads
might be printing to the log at the same time? Can you please enable
the config that prints the thread ID (forgot what it's called) and
collect this again? With what I could tell the crash seems to be
happening somewhere in platform_match(), but that's not related to
this patch at all?
Can you reproduce it? it is very likely related(without your patch, the
boot is fine),
Sorry, I haven't ever setup qemu and booted vexpress. Thanks for your help.
quoted
the NULL ptr is about serio, it is registed from amba driver.
ambakmi_driver_init
-- amba_kmi_probe
-- __serio_register_port
Thanks for the pointer. I took a look at the logs and the code. It's
very strange. As you can see from the backtrace, platform_match() is
being called for the device_add() from serio_handle_event(). But the
device that gets added there is on the serio_bus which obviously
should be using the serio_bus_match.
Yes, I am confused too.
quoted
quoted
+Dmitry and input maillist, is there some known issue about serio ?
I add some debug, the full log is attached.
[ 2.958355][ T41] input: AT Raw Set 2 keyboard as
/devices/platform/bus@8000000/bus@8000000:motherboard-bus/bus@8000000:motherboard-bus:iofpga-bus@300000000/1c060000.kmi/serio0/input/input0
[ 2.977441][ T41] serio serio1: pdev c1e05508, pdev->name (null),
drv c1090fc0, drv->name vexpress-reset
Based on the logs you added, it's pretty clear we are getting to
platform_match(). It's also strange that the drv->name is
vexpress-reset
...
quoted
quoted
[ 3.003113][ T41] Backtrace:
[ 3.003451][ T41] [<c0560bb4>] (strcmp) from [<c0646358>] (platform_match+0xdc/0xf0)
[ 3.003963][ T41] [<c064627c>] (platform_match) from [<c06437d4>] (__device_attach_driver+0x3c/0xf4)
[ 3.004769][ T41] [<c0643798>] (__device_attach_driver) from [<c0641180>] (bus_for_each_drv+0x68/0xc8)
[ 3.005481][ T41] [<c0641118>] (bus_for_each_drv) from [<c0642f40>] (__device_attach+0xf0/0x16c)
[ 3.006152][ T41] [<c0642e50>] (__device_attach) from [<c06439d4>] (device_initial_probe+0x1c/0x20)
[ 3.006853][ T41] [<c06439b8>] (device_initial_probe) from [<c0642030>] (bus_probe_device+0x94/0x9c)
[ 3.007259][ T41] [<c0641f9c>] (bus_probe_device) from [<c063f9cc>] (device_add+0x408/0x8b8)
[ 3.007900][ T41] [<c063f5c4>] (device_add) from [<c071c1cc>] (serio_handle_event+0x1b8/0x234)
[ 3.008824][ T41] [<c071c014>] (serio_handle_event) from [<c01475a4>] (process_one_work+0x238/0x594)
[ 3.009737][ T41] [<c014736c>] (process_one_work) from [<c014795c>] (worker_thread+0x5c/0x5f4)
[ 3.010638][ T41] [<c0147900>] (worker_thread) from [<c014feb4>] (kthread+0x178/0x194)
[ 3.011496][ T41] [<c014fd3c>] (kthread) from [<c0100150>] (ret_from_fork+0x14/0x24)
[ 3.011860][ T41] Exception stack(0xc1675fb0 to 0xc1675ff8)
But the platform_match() is happening for the device_add() from
serio_event_handle() that's adding a device to the serio_bus and it
should be using serio_bus_match().
I haven't reached any conclusion yet, but my current thought process
is that it's either:
1. My patch is somehow causing list corruption. But I don't directly
touch any list in my change (other than deleting a list entirely), so
it's not clear how that would be happening.
Maybe some concurrent driver load？
quoted
2. Without my patch, these AMBA device's probe would be delayed at
least until 5 seconds or possibly later. I'm wondering if my patch is
catching some bad timing assumptions in other code.
After Rob's patch, It will retry soon.
commit 039599c92d3b2e73689e8b6e519d653fd4770abb
Author: Rob Herring [off-list ref]
Date: Wed Apr 29 15:58:12 2020 -0500
amba: Retry adding deferred devices at late_initcall
If amba bus devices defer when adding, the amba bus code simply retries
adding the devices every 5 seconds. This doesn't work well as it
completely unsynchronized with starting the init process which can
happen in less than 5 secs. Add a retry during late_initcall. If the
amba devices are added, then deferred probe takes over. If the
dependencies have not probed at this point, then there's no improvement
over previous behavior. To completely solve this, we'd need to retry
after every successful probe as deferred probe does.
The list_empty() check now happens outside the mutex, but the mutex
wasn't necessary in the first place.
This needed to use deferred probe instead of fragile initcall ordering
on 32-bit VExpress systems where the apb_pclk has a number of probe
dependencies (vexpress-sysregs, vexpress-config).
quoted
You might be able to test out theory (2) by DEFERRED_DEVICE_TIMEOUT to
a much smaller number. Say 500ms or 100ms. If it doesn't crash, it
doesn't mean it's not (2), but if it does, then we know for sure it's
(2).
ok, I will try this one, but due to above patch, it may not work.
Were you able to find anything more?
I can't find any clue， and have no time to check this for now, is there
any news from your side?
To close out this thread, the issue was due to a UAF bug in driver
core that was fixed by:
https://lore.kernel.org/all/20220513112444.45112-1-schspa@gmail.com/
With that fix, there wouldn't have been a crash, but amba driver
registration would have failed (because match returned
non-EPROBE_DEFER error).
-Saravana
On Fri, Sep 10, 2021 at 12:59 AM Kefeng Wang [off-list ref] wrote:
quoted
On 2021/9/9 11:30, Saravana Kannan wrote:
quoted
On Fri, Aug 27, 2021 at 6:09 PM Kefeng Wang [off-list ref] wrote:
quoted
On 2021/8/28 3:09, Saravana Kannan wrote:
quoted
On Fri, Aug 27, 2021 at 7:38 AM Kefeng Wang [off-list ref] wrote:
quoted
On 2021/8/27 8:04, Saravana Kannan wrote:
quoted
On Thu, Aug 26, 2021 at 1:22 AM Kefeng Wang [off-list ref] wrote:
quoted
quoted
quoted
quoted
Btw, I've been working on [1] cleaning up the one-off deferred probe
solution that we have for amba devices. That causes a bunch of other
headaches. Your patch 3/3 takes us further in the wrong direction by
adding more reasons for delaying the addition of the device.
Hi Saravana, I try the link[1], but with it, there is a crash when boot
(qemu-system-arm -M vexpress-a15),
I'm assuming it's this one?
arch/arm/boot/dts/vexpress-v2p-ca15_a7.dts
Hi,
It's hard to make sense of the logs. Looks like two different threads
might be printing to the log at the same time? Can you please enable
the config that prints the thread ID (forgot what it's called) and
collect this again? With what I could tell the crash seems to be
happening somewhere in platform_match(), but that's not related to
this patch at all?
Can you reproduce it? it is very likely related(without your patch, the
boot is fine),
Sorry, I haven't ever setup qemu and booted vexpress. Thanks for your help.
quoted
the NULL ptr is about serio, it is registed from amba driver.
ambakmi_driver_init
-- amba_kmi_probe
-- __serio_register_port
Thanks for the pointer. I took a look at the logs and the code. It's
very strange. As you can see from the backtrace, platform_match() is
being called for the device_add() from serio_handle_event(). But the
device that gets added there is on the serio_bus which obviously
should be using the serio_bus_match.
Yes, I am confused too.
quoted
quoted
+Dmitry and input maillist, is there some known issue about serio ?
I add some debug, the full log is attached.
[ 2.958355][ T41] input: AT Raw Set 2 keyboard as
/devices/platform/bus@8000000/bus@8000000:motherboard-bus/bus@8000000:motherboard-bus:iofpga-bus@300000000/1c060000.kmi/serio0/input/input0
[ 2.977441][ T41] serio serio1: pdev c1e05508, pdev->name (null),
drv c1090fc0, drv->name vexpress-reset
Based on the logs you added, it's pretty clear we are getting to
platform_match(). It's also strange that the drv->name is
vexpress-reset
...
quoted
quoted
[ 3.003113][ T41] Backtrace:
[ 3.003451][ T41] [<c0560bb4>] (strcmp) from [<c0646358>] (platform_match+0xdc/0xf0)
[ 3.003963][ T41] [<c064627c>] (platform_match) from [<c06437d4>] (__device_attach_driver+0x3c/0xf4)
[ 3.004769][ T41] [<c0643798>] (__device_attach_driver) from [<c0641180>] (bus_for_each_drv+0x68/0xc8)
[ 3.005481][ T41] [<c0641118>] (bus_for_each_drv) from [<c0642f40>] (__device_attach+0xf0/0x16c)
[ 3.006152][ T41] [<c0642e50>] (__device_attach) from [<c06439d4>] (device_initial_probe+0x1c/0x20)
[ 3.006853][ T41] [<c06439b8>] (device_initial_probe) from [<c0642030>] (bus_probe_device+0x94/0x9c)
[ 3.007259][ T41] [<c0641f9c>] (bus_probe_device) from [<c063f9cc>] (device_add+0x408/0x8b8)
[ 3.007900][ T41] [<c063f5c4>] (device_add) from [<c071c1cc>] (serio_handle_event+0x1b8/0x234)
[ 3.008824][ T41] [<c071c014>] (serio_handle_event) from [<c01475a4>] (process_one_work+0x238/0x594)
[ 3.009737][ T41] [<c014736c>] (process_one_work) from [<c014795c>] (worker_thread+0x5c/0x5f4)
[ 3.010638][ T41] [<c0147900>] (worker_thread) from [<c014feb4>] (kthread+0x178/0x194)
[ 3.011496][ T41] [<c014fd3c>] (kthread) from [<c0100150>] (ret_from_fork+0x14/0x24)
[ 3.011860][ T41] Exception stack(0xc1675fb0 to 0xc1675ff8)
But the platform_match() is happening for the device_add() from
serio_event_handle() that's adding a device to the serio_bus and it
should be using serio_bus_match().
I haven't reached any conclusion yet, but my current thought process
is that it's either:
1. My patch is somehow causing list corruption. But I don't directly
touch any list in my change (other than deleting a list entirely), so
it's not clear how that would be happening.
Maybe some concurrent driver load？
quoted
2. Without my patch, these AMBA device's probe would be delayed at
least until 5 seconds or possibly later. I'm wondering if my patch is
catching some bad timing assumptions in other code.
After Rob's patch, It will retry soon.
commit 039599c92d3b2e73689e8b6e519d653fd4770abb
Author: Rob Herring [off-list ref]
Date: Wed Apr 29 15:58:12 2020 -0500
amba: Retry adding deferred devices at late_initcall
If amba bus devices defer when adding, the amba bus code simply retries
adding the devices every 5 seconds. This doesn't work well as it
completely unsynchronized with starting the init process which can
happen in less than 5 secs. Add a retry during late_initcall. If the
amba devices are added, then deferred probe takes over. If the
dependencies have not probed at this point, then there's no improvement
over previous behavior. To completely solve this, we'd need to retry
after every successful probe as deferred probe does.
The list_empty() check now happens outside the mutex, but the mutex
wasn't necessary in the first place.
This needed to use deferred probe instead of fragile initcall ordering
on 32-bit VExpress systems where the apb_pclk has a number of probe
dependencies (vexpress-sysregs, vexpress-config).
quoted
You might be able to test out theory (2) by DEFERRED_DEVICE_TIMEOUT to
a much smaller number. Say 500ms or 100ms. If it doesn't crash, it
doesn't mean it's not (2), but if it does, then we know for sure it's
(2).
ok, I will try this one, but due to above patch, it may not work.
Were you able to find anything more?
I can't find any clue， and have no time to check this for now, is there
any news from your side?
Hi, Saravana and Kefeng:
I've spent the whole afternoon trying to figure this out, and the fix
patch has been cc you two.