Commit f2d3b9a46e0e ("ARM: 9220/1: amba: Remove deferred device addition")
forcibly invokes device_add() even if dev->periphid is not ready. Although
it will be remedied in amba_match(): dev->periphid will be initialized
if everything is in place; Otherwise, return -EPROBE_DEFER to block
__driver_attach() from further execution. But not all drivers have .match
hook, such as pl031, the dev->bus->probe will be called directly in
__driver_attach(). Unfortunately, if dev->periphid is still not
initialized, the following exception will be triggered.
8<--- cut here ---
Unable to handle kernel NULL pointer dereference at virtual address 00000008
[00000008] *pgd=00000000
Internal error: Oops: 5 [#1] SMP ARM
Modules linked in:
CPU: 1 PID: 1 Comm: swapper/0 Not tainted 6.0.0-rc2+ #7
Hardware name: ARM-Versatile Express
PC is at pl031_probe+0x8/0x208
LR is at amba_probe+0xf0/0x160
pc : 80698df8 lr : 8050eb54 psr: 80000013
sp : c0825df8 ip : 00000000 fp : 811fda38
r10: 00000000 r9 : 80d72470 r8 : fffffdfb
r7 : 811fd800 r6 : be7eb330 r5 : 00000000 r4 : 811fd900
r3 : 80698df0 r2 : 37000000 r1 : 00000000 r0 : 811fd800
Flags: Nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
Control: 10c5387d Table: 6000406a DAC: 00000051
... ...
pl031_probe from amba_probe+0xf0/0x160
amba_probe from really_probe+0x118/0x290
really_probe from __driver_probe_device+0x84/0xe4
__driver_probe_device from driver_probe_device+0x30/0xd0
driver_probe_device from __driver_attach+0x8c/0xfc
__driver_attach from bus_for_each_dev+0x70/0xb0
bus_for_each_dev from bus_add_driver+0x168/0x1f4
bus_add_driver from driver_register+0x7c/0x118
driver_register from do_one_initcall+0x44/0x1ec
do_one_initcall from kernel_init_freeable+0x238/0x288
kernel_init_freeable from kernel_init+0x18/0x12c
kernel_init from ret_from_fork+0x14/0x2c
... ...
---[ end trace 0000000000000000 ]---
Therefore, take the same action as in amba_match(): return -EPROBE_DEFER
if dev->periphid is not ready in amba_probe().
Fixes: f2d3b9a46e0e ("ARM: 9220/1: amba: Remove deferred device addition")
Signed-off-by: Zhen Lei <[email protected]>
---
KernelVersion: v6.0-rc3
drivers/amba/bus.c | 24 +++++++++++++++++++++---
1 file changed, 21 insertions(+), 3 deletions(-)
v1 --> v2:
1. Update this patch based on:
https://lore.kernel.org/lkml/[email protected]/
2. Move the operations of sanity checking and reading dev->periphid,
updating uevent into new function amba_prepare_periphid().
diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 110a535648d2e1f..8e4c7e190880206 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -204,10 +204,9 @@ static int amba_read_periphid(struct amba_device *dev)
return ret;
}
-static int amba_match(struct device *dev, struct device_driver *drv)
+static int amba_prepare_periphid(struct device *dev)
{
struct amba_device *pcdev = to_amba_device(dev);
- struct amba_driver *pcdrv = to_amba_driver(drv);
mutex_lock(&pcdev->periphid_lock);
if (!pcdev->periphid) {
@@ -228,6 +227,19 @@ static int amba_match(struct device *dev, struct device_driver *drv)
}
mutex_unlock(&pcdev->periphid_lock);
+ return 0;
+}
+
+static int amba_match(struct device *dev, struct device_driver *drv)
+{
+ struct amba_device *pcdev = to_amba_device(dev);
+ struct amba_driver *pcdrv = to_amba_driver(drv);
+ int ret;
+
+ ret = amba_prepare_periphid(dev);
+ if (ret)
+ return ret;
+
/* When driver_override is set, only bind to the matching driver */
if (pcdev->driver_override)
return !strcmp(pcdev->driver_override, drv->name);
@@ -278,9 +290,15 @@ static int amba_probe(struct device *dev)
{
struct amba_device *pcdev = to_amba_device(dev);
struct amba_driver *pcdrv = to_amba_driver(dev->driver);
- const struct amba_id *id = amba_lookup(pcdrv->id_table, pcdev);
+ const struct amba_id *id;
int ret;
+ ret = amba_prepare_periphid(dev);
+ if (ret)
+ return ret;
+
+ id = amba_lookup(pcdrv->id_table, pcdev);
+
do {
ret = of_amba_device_decode_irq(pcdev);
if (ret)
--
2.25.1
On Mon, Aug 29, 2022 at 11:59 PM Zhen Lei <[email protected]> wrote:
>
> Commit f2d3b9a46e0e ("ARM: 9220/1: amba: Remove deferred device addition")
> forcibly invokes device_add() even if dev->periphid is not ready. Although
> it will be remedied in amba_match(): dev->periphid will be initialized
> if everything is in place; Otherwise, return -EPROBE_DEFER to block
> __driver_attach() from further execution. But not all drivers have .match
> hook, such as pl031, the dev->bus->probe will be called directly in
> __driver_attach(). Unfortunately, if dev->periphid is still not
> initialized, the following exception will be triggered.
>
> 8<--- cut here ---
> Unable to handle kernel NULL pointer dereference at virtual address 00000008
> [00000008] *pgd=00000000
> Internal error: Oops: 5 [#1] SMP ARM
> Modules linked in:
> CPU: 1 PID: 1 Comm: swapper/0 Not tainted 6.0.0-rc2+ #7
> Hardware name: ARM-Versatile Express
> PC is at pl031_probe+0x8/0x208
> LR is at amba_probe+0xf0/0x160
> pc : 80698df8 lr : 8050eb54 psr: 80000013
> sp : c0825df8 ip : 00000000 fp : 811fda38
> r10: 00000000 r9 : 80d72470 r8 : fffffdfb
> r7 : 811fd800 r6 : be7eb330 r5 : 00000000 r4 : 811fd900
> r3 : 80698df0 r2 : 37000000 r1 : 00000000 r0 : 811fd800
> Flags: Nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
> Control: 10c5387d Table: 6000406a DAC: 00000051
> ... ...
> pl031_probe from amba_probe+0xf0/0x160
> amba_probe from really_probe+0x118/0x290
> really_probe from __driver_probe_device+0x84/0xe4
> __driver_probe_device from driver_probe_device+0x30/0xd0
> driver_probe_device from __driver_attach+0x8c/0xfc
> __driver_attach from bus_for_each_dev+0x70/0xb0
> bus_for_each_dev from bus_add_driver+0x168/0x1f4
> bus_add_driver from driver_register+0x7c/0x118
> driver_register from do_one_initcall+0x44/0x1ec
> do_one_initcall from kernel_init_freeable+0x238/0x288
> kernel_init_freeable from kernel_init+0x18/0x12c
> kernel_init from ret_from_fork+0x14/0x2c
> ... ...
> ---[ end trace 0000000000000000 ]---
>
> Therefore, take the same action as in amba_match(): return -EPROBE_DEFER
> if dev->periphid is not ready in amba_probe().
>
> Fixes: f2d3b9a46e0e ("ARM: 9220/1: amba: Remove deferred device addition")
> Signed-off-by: Zhen Lei <[email protected]>
> ---
> KernelVersion: v6.0-rc3
> drivers/amba/bus.c | 24 +++++++++++++++++++++---
> 1 file changed, 21 insertions(+), 3 deletions(-)
>
> v1 --> v2:
> 1. Update this patch based on:
> https://lore.kernel.org/lkml/[email protected]/
> 2. Move the operations of sanity checking and reading dev->periphid,
> updating uevent into new function amba_prepare_periphid().
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 110a535648d2e1f..8e4c7e190880206 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -204,10 +204,9 @@ static int amba_read_periphid(struct amba_device *dev)
> return ret;
> }
>
> -static int amba_match(struct device *dev, struct device_driver *drv)
> +static int amba_prepare_periphid(struct device *dev)
> {
> struct amba_device *pcdev = to_amba_device(dev);
> - struct amba_driver *pcdrv = to_amba_driver(drv);
>
> mutex_lock(&pcdev->periphid_lock);
> if (!pcdev->periphid) {
> @@ -228,6 +227,19 @@ static int amba_match(struct device *dev, struct device_driver *drv)
> }
> mutex_unlock(&pcdev->periphid_lock);
>
> + return 0;
> +}
> +
> +static int amba_match(struct device *dev, struct device_driver *drv)
> +{
> + struct amba_device *pcdev = to_amba_device(dev);
> + struct amba_driver *pcdrv = to_amba_driver(drv);
> + int ret;
> +
> + ret = amba_prepare_periphid(dev);
> + if (ret)
> + return ret;
> +
> /* When driver_override is set, only bind to the matching driver */
> if (pcdev->driver_override)
> return !strcmp(pcdev->driver_override, drv->name);
> @@ -278,9 +290,15 @@ static int amba_probe(struct device *dev)
> {
> struct amba_device *pcdev = to_amba_device(dev);
> struct amba_driver *pcdrv = to_amba_driver(dev->driver);
> - const struct amba_id *id = amba_lookup(pcdrv->id_table, pcdev);
> + const struct amba_id *id;
> int ret;
>
> + ret = amba_prepare_periphid(dev);
> + if (ret)
> + return ret;
> +
> + id = amba_lookup(pcdrv->id_table, pcdev);
> +
> do {
> ret = of_amba_device_decode_irq(pcdev);
> if (ret)
Let's wait for Isaac to review this. He has been looking at the
locking issue for a bit and there were some tricky cases.
-Saravana
On Tue, Aug 30, 2022 at 12:20:00AM -0700, Saravana Kannan wrote:
> On Mon, Aug 29, 2022 at 11:59 PM Zhen Lei <[email protected]> wrote:
> >
> > Commit f2d3b9a46e0e ("ARM: 9220/1: amba: Remove deferred device addition")
> > forcibly invokes device_add() even if dev->periphid is not ready. Although
> > it will be remedied in amba_match(): dev->periphid will be initialized
> > if everything is in place; Otherwise, return -EPROBE_DEFER to block
> > __driver_attach() from further execution. But not all drivers have .match
> > hook, such as pl031, the dev->bus->probe will be called directly in
> > __driver_attach(). Unfortunately, if dev->periphid is still not
> > initialized, the following exception will be triggered.
> >
> > 8<--- cut here ---
> > Unable to handle kernel NULL pointer dereference at virtual address 00000008
> > [00000008] *pgd=00000000
> > Internal error: Oops: 5 [#1] SMP ARM
> > Modules linked in:
> > CPU: 1 PID: 1 Comm: swapper/0 Not tainted 6.0.0-rc2+ #7
> > Hardware name: ARM-Versatile Express
> > PC is at pl031_probe+0x8/0x208
> > LR is at amba_probe+0xf0/0x160
> > pc : 80698df8 lr : 8050eb54 psr: 80000013
> > sp : c0825df8 ip : 00000000 fp : 811fda38
> > r10: 00000000 r9 : 80d72470 r8 : fffffdfb
> > r7 : 811fd800 r6 : be7eb330 r5 : 00000000 r4 : 811fd900
> > r3 : 80698df0 r2 : 37000000 r1 : 00000000 r0 : 811fd800
> > Flags: Nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
> > Control: 10c5387d Table: 6000406a DAC: 00000051
> > ... ...
> > pl031_probe from amba_probe+0xf0/0x160
> > amba_probe from really_probe+0x118/0x290
> > really_probe from __driver_probe_device+0x84/0xe4
> > __driver_probe_device from driver_probe_device+0x30/0xd0
> > driver_probe_device from __driver_attach+0x8c/0xfc
> > __driver_attach from bus_for_each_dev+0x70/0xb0
> > bus_for_each_dev from bus_add_driver+0x168/0x1f4
> > bus_add_driver from driver_register+0x7c/0x118
> > driver_register from do_one_initcall+0x44/0x1ec
> > do_one_initcall from kernel_init_freeable+0x238/0x288
> > kernel_init_freeable from kernel_init+0x18/0x12c
> > kernel_init from ret_from_fork+0x14/0x2c
> > ... ...
> > ---[ end trace 0000000000000000 ]---
> >
> > Therefore, take the same action as in amba_match(): return -EPROBE_DEFER
> > if dev->periphid is not ready in amba_probe().
> >
> > Fixes: f2d3b9a46e0e ("ARM: 9220/1: amba: Remove deferred device addition")
> > Signed-off-by: Zhen Lei <[email protected]>
> > ---
> > KernelVersion: v6.0-rc3
> > drivers/amba/bus.c | 24 +++++++++++++++++++++---
> > 1 file changed, 21 insertions(+), 3 deletions(-)
> >
> > v1 --> v2:
> > 1. Update this patch based on:
> > https://lore.kernel.org/lkml/[email protected]/
> > 2. Move the operations of sanity checking and reading dev->periphid,
> > updating uevent into new function amba_prepare_periphid().
> >
> > diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> > index 110a535648d2e1f..8e4c7e190880206 100644
> > --- a/drivers/amba/bus.c
> > +++ b/drivers/amba/bus.c
> > @@ -204,10 +204,9 @@ static int amba_read_periphid(struct amba_device *dev)
> > return ret;
> > }
> >
> > -static int amba_match(struct device *dev, struct device_driver *drv)
> > +static int amba_prepare_periphid(struct device *dev)
> > {
> > struct amba_device *pcdev = to_amba_device(dev);
> > - struct amba_driver *pcdrv = to_amba_driver(drv);
> >
> > mutex_lock(&pcdev->periphid_lock);
> > if (!pcdev->periphid) {
> > @@ -228,6 +227,19 @@ static int amba_match(struct device *dev, struct device_driver *drv)
> > }
> > mutex_unlock(&pcdev->periphid_lock);
> >
> > + return 0;
> > +}
> > +
> > +static int amba_match(struct device *dev, struct device_driver *drv)
> > +{
> > + struct amba_device *pcdev = to_amba_device(dev);
> > + struct amba_driver *pcdrv = to_amba_driver(drv);
> > + int ret;
> > +
> > + ret = amba_prepare_periphid(dev);
> > + if (ret)
> > + return ret;
> > +
> > /* When driver_override is set, only bind to the matching driver */
> > if (pcdev->driver_override)
> > return !strcmp(pcdev->driver_override, drv->name);
> > @@ -278,9 +290,15 @@ static int amba_probe(struct device *dev)
> > {
> > struct amba_device *pcdev = to_amba_device(dev);
> > struct amba_driver *pcdrv = to_amba_driver(dev->driver);
> > - const struct amba_id *id = amba_lookup(pcdrv->id_table, pcdev);
> > + const struct amba_id *id;
> > int ret;
> >
> > + ret = amba_prepare_periphid(dev);
> > + if (ret)
> > + return ret;
> > +
> > + id = amba_lookup(pcdrv->id_table, pcdev);
> > +
> > do {
> > ret = of_amba_device_decode_irq(pcdev);
> > if (ret)
>
> Let's wait for Isaac to review this. He has been looking at the
> locking issue for a bit and there were some tricky cases.
How can we get to amba_probe() if amba_match() has not returned a
positive match for an ID? Surely if that happens, the driver model
is failed - since a probe function should not be called unless the
driver matches the device.
This patch, and 9229/1 both feel like they're working around some
brokenness elsewhere in the kernel.
The description in 9229/1, specifically "But not all drivers have .match
hook, such as pl031, the dev->bus->probe will be called directly in
__driver_attach()." AMBA drivers do not have a .match hook - the bus
does, which all AMBA drivers must be a member of. If amba_match fails,
there is no way that amba_probe() should ever be called.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Tue, Aug 30, 2022 at 10:47:31AM +0100, Russell King (Oracle) wrote:
> How can we get to amba_probe() if amba_match() has not returned a
> positive match for an ID? Surely if that happens, the driver model
> is failed - since a probe function should not be called unless the
> driver matches the device.
>
> This patch, and 9229/1 both feel like they're working around some
> brokenness elsewhere in the kernel.
>
> The description in 9229/1, specifically "But not all drivers have .match
Sorry, meant 9238/1.
> hook, such as pl031, the dev->bus->probe will be called directly in
> __driver_attach()." AMBA drivers do not have a .match hook - the bus
> does, which all AMBA drivers must be a member of. If amba_match fails,
> there is no way that amba_probe() should ever be called.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Please don't send the patch system patches that have not had any chance
of review. The patch system is supposed to be for patches that are to
be applied.
Sending patches that are yet to be reviewed makes extra work for me.
Thanks.
On Tue, Aug 30, 2022 at 02:54:13PM +0800, Zhen Lei wrote:
> Commit f2d3b9a46e0e ("ARM: 9220/1: amba: Remove deferred device addition")
> forcibly invokes device_add() even if dev->periphid is not ready. Although
> it will be remedied in amba_match(): dev->periphid will be initialized
> if everything is in place; Otherwise, return -EPROBE_DEFER to block
> __driver_attach() from further execution. But not all drivers have .match
> hook, such as pl031, the dev->bus->probe will be called directly in
> __driver_attach(). Unfortunately, if dev->periphid is still not
> initialized, the following exception will be triggered.
>
> 8<--- cut here ---
> Unable to handle kernel NULL pointer dereference at virtual address 00000008
> [00000008] *pgd=00000000
> Internal error: Oops: 5 [#1] SMP ARM
> Modules linked in:
> CPU: 1 PID: 1 Comm: swapper/0 Not tainted 6.0.0-rc2+ #7
> Hardware name: ARM-Versatile Express
> PC is at pl031_probe+0x8/0x208
> LR is at amba_probe+0xf0/0x160
> pc : 80698df8 lr : 8050eb54 psr: 80000013
> sp : c0825df8 ip : 00000000 fp : 811fda38
> r10: 00000000 r9 : 80d72470 r8 : fffffdfb
> r7 : 811fd800 r6 : be7eb330 r5 : 00000000 r4 : 811fd900
> r3 : 80698df0 r2 : 37000000 r1 : 00000000 r0 : 811fd800
> Flags: Nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
> Control: 10c5387d Table: 6000406a DAC: 00000051
> ... ...
> pl031_probe from amba_probe+0xf0/0x160
> amba_probe from really_probe+0x118/0x290
> really_probe from __driver_probe_device+0x84/0xe4
> __driver_probe_device from driver_probe_device+0x30/0xd0
> driver_probe_device from __driver_attach+0x8c/0xfc
> __driver_attach from bus_for_each_dev+0x70/0xb0
> bus_for_each_dev from bus_add_driver+0x168/0x1f4
> bus_add_driver from driver_register+0x7c/0x118
> driver_register from do_one_initcall+0x44/0x1ec
> do_one_initcall from kernel_init_freeable+0x238/0x288
> kernel_init_freeable from kernel_init+0x18/0x12c
> kernel_init from ret_from_fork+0x14/0x2c
> ... ...
> ---[ end trace 0000000000000000 ]---
>
> Therefore, take the same action as in amba_match(): return -EPROBE_DEFER
> if dev->periphid is not ready in amba_probe().
>
> Fixes: f2d3b9a46e0e ("ARM: 9220/1: amba: Remove deferred device addition")
> Signed-off-by: Zhen Lei <[email protected]>
> ---
> KernelVersion: v6.0-rc3
> drivers/amba/bus.c | 24 +++++++++++++++++++++---
> 1 file changed, 21 insertions(+), 3 deletions(-)
>
> v1 --> v2:
> 1. Update this patch based on:
> https://lore.kernel.org/lkml/[email protected]/
> 2. Move the operations of sanity checking and reading dev->periphid,
> updating uevent into new function amba_prepare_periphid().
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 110a535648d2e1f..8e4c7e190880206 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -204,10 +204,9 @@ static int amba_read_periphid(struct amba_device *dev)
> return ret;
> }
>
> -static int amba_match(struct device *dev, struct device_driver *drv)
> +static int amba_prepare_periphid(struct device *dev)
> {
> struct amba_device *pcdev = to_amba_device(dev);
> - struct amba_driver *pcdrv = to_amba_driver(drv);
>
> mutex_lock(&pcdev->periphid_lock);
> if (!pcdev->periphid) {
> @@ -228,6 +227,19 @@ static int amba_match(struct device *dev, struct device_driver *drv)
> }
> mutex_unlock(&pcdev->periphid_lock);
>
> + return 0;
> +}
> +
> +static int amba_match(struct device *dev, struct device_driver *drv)
> +{
> + struct amba_device *pcdev = to_amba_device(dev);
> + struct amba_driver *pcdrv = to_amba_driver(drv);
> + int ret;
> +
> + ret = amba_prepare_periphid(dev);
> + if (ret)
> + return ret;
> +
> /* When driver_override is set, only bind to the matching driver */
> if (pcdev->driver_override)
> return !strcmp(pcdev->driver_override, drv->name);
> @@ -278,9 +290,15 @@ static int amba_probe(struct device *dev)
> {
> struct amba_device *pcdev = to_amba_device(dev);
> struct amba_driver *pcdrv = to_amba_driver(dev->driver);
> - const struct amba_id *id = amba_lookup(pcdrv->id_table, pcdev);
> + const struct amba_id *id;
> int ret;
>
> + ret = amba_prepare_periphid(dev);
> + if (ret)
> + return ret;
> +
> + id = amba_lookup(pcdrv->id_table, pcdev);
> +
> do {
> ret = of_amba_device_decode_irq(pcdev);
> if (ret)
> --
> 2.25.1
>
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On 2022/8/30 17:47, Russell King (Oracle) wrote:
> On Tue, Aug 30, 2022 at 12:20:00AM -0700, Saravana Kannan wrote:
>> On Mon, Aug 29, 2022 at 11:59 PM Zhen Lei <[email protected]> wrote:
>>>
>>> Commit f2d3b9a46e0e ("ARM: 9220/1: amba: Remove deferred device addition")
>>> forcibly invokes device_add() even if dev->periphid is not ready. Although
>>> it will be remedied in amba_match(): dev->periphid will be initialized
>>> if everything is in place; Otherwise, return -EPROBE_DEFER to block
>>> __driver_attach() from further execution. But not all drivers have .match
>>> hook, such as pl031, the dev->bus->probe will be called directly in
>>> __driver_attach(). Unfortunately, if dev->periphid is still not
>>> initialized, the following exception will be triggered.
>>>
>>> 8<--- cut here ---
>>> Unable to handle kernel NULL pointer dereference at virtual address 00000008
>>> [00000008] *pgd=00000000
>>> Internal error: Oops: 5 [#1] SMP ARM
>>> Modules linked in:
>>> CPU: 1 PID: 1 Comm: swapper/0 Not tainted 6.0.0-rc2+ #7
>>> Hardware name: ARM-Versatile Express
>>> PC is at pl031_probe+0x8/0x208
>>> LR is at amba_probe+0xf0/0x160
>>> pc : 80698df8 lr : 8050eb54 psr: 80000013
>>> sp : c0825df8 ip : 00000000 fp : 811fda38
>>> r10: 00000000 r9 : 80d72470 r8 : fffffdfb
>>> r7 : 811fd800 r6 : be7eb330 r5 : 00000000 r4 : 811fd900
>>> r3 : 80698df0 r2 : 37000000 r1 : 00000000 r0 : 811fd800
>>> Flags: Nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
>>> Control: 10c5387d Table: 6000406a DAC: 00000051
>>> ... ...
>>> pl031_probe from amba_probe+0xf0/0x160
>>> amba_probe from really_probe+0x118/0x290
>>> really_probe from __driver_probe_device+0x84/0xe4
>>> __driver_probe_device from driver_probe_device+0x30/0xd0
>>> driver_probe_device from __driver_attach+0x8c/0xfc
>>> __driver_attach from bus_for_each_dev+0x70/0xb0
>>> bus_for_each_dev from bus_add_driver+0x168/0x1f4
>>> bus_add_driver from driver_register+0x7c/0x118
>>> driver_register from do_one_initcall+0x44/0x1ec
>>> do_one_initcall from kernel_init_freeable+0x238/0x288
>>> kernel_init_freeable from kernel_init+0x18/0x12c
>>> kernel_init from ret_from_fork+0x14/0x2c
>>> ... ...
>>> ---[ end trace 0000000000000000 ]---
>>>
>>> Therefore, take the same action as in amba_match(): return -EPROBE_DEFER
>>> if dev->periphid is not ready in amba_probe().
>>>
>>> Fixes: f2d3b9a46e0e ("ARM: 9220/1: amba: Remove deferred device addition")
>>> Signed-off-by: Zhen Lei <[email protected]>
>>> ---
>>> KernelVersion: v6.0-rc3
>>> drivers/amba/bus.c | 24 +++++++++++++++++++++---
>>> 1 file changed, 21 insertions(+), 3 deletions(-)
>>>
>>> v1 --> v2:
>>> 1. Update this patch based on:
>>> https://lore.kernel.org/lkml/[email protected]/
>>> 2. Move the operations of sanity checking and reading dev->periphid,
>>> updating uevent into new function amba_prepare_periphid().
>>>
>>> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
>>> index 110a535648d2e1f..8e4c7e190880206 100644
>>> --- a/drivers/amba/bus.c
>>> +++ b/drivers/amba/bus.c
>>> @@ -204,10 +204,9 @@ static int amba_read_periphid(struct amba_device *dev)
>>> return ret;
>>> }
>>>
>>> -static int amba_match(struct device *dev, struct device_driver *drv)
>>> +static int amba_prepare_periphid(struct device *dev)
>>> {
>>> struct amba_device *pcdev = to_amba_device(dev);
>>> - struct amba_driver *pcdrv = to_amba_driver(drv);
>>>
>>> mutex_lock(&pcdev->periphid_lock);
>>> if (!pcdev->periphid) {
>>> @@ -228,6 +227,19 @@ static int amba_match(struct device *dev, struct device_driver *drv)
>>> }
>>> mutex_unlock(&pcdev->periphid_lock);
>>>
>>> + return 0;
>>> +}
>>> +
>>> +static int amba_match(struct device *dev, struct device_driver *drv)
>>> +{
>>> + struct amba_device *pcdev = to_amba_device(dev);
>>> + struct amba_driver *pcdrv = to_amba_driver(drv);
>>> + int ret;
>>> +
>>> + ret = amba_prepare_periphid(dev);
>>> + if (ret)
>>> + return ret;
>>> +
>>> /* When driver_override is set, only bind to the matching driver */
>>> if (pcdev->driver_override)
>>> return !strcmp(pcdev->driver_override, drv->name);
>>> @@ -278,9 +290,15 @@ static int amba_probe(struct device *dev)
>>> {
>>> struct amba_device *pcdev = to_amba_device(dev);
>>> struct amba_driver *pcdrv = to_amba_driver(dev->driver);
>>> - const struct amba_id *id = amba_lookup(pcdrv->id_table, pcdev);
>>> + const struct amba_id *id;
>>> int ret;
>>>
>>> + ret = amba_prepare_periphid(dev);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + id = amba_lookup(pcdrv->id_table, pcdev);
>>> +
>>> do {
>>> ret = of_amba_device_decode_irq(pcdev);
>>> if (ret)
>>
>> Let's wait for Isaac to review this. He has been looking at the
>> locking issue for a bit and there were some tricky cases.
>
> How can we get to amba_probe() if amba_match() has not returned a
> positive match for an ID? Surely if that happens, the driver model
Always return true.
__driver_attach
driver_match_device
static inline int driver_match_device(struct device_driver *drv,
struct device *dev)
{
return drv->bus->match ? drv->bus->match(dev, drv) : 1;
}
> is failed - since a probe function should not be called unless the
> driver matches the device.
>
> This patch, and 9229/1 both feel like they're working around some
> brokenness elsewhere in the kernel.
>
> The description in 9229/1, specifically "But not all drivers have .match
> hook, such as pl031, the dev->bus->probe will be called directly in
> __driver_attach()." AMBA drivers do not have a .match hook - the bus
> does, which all AMBA drivers must be a member of. If amba_match fails,
> there is no way that amba_probe() should ever be called.
>
--
Regards,
Zhen Lei
On 2022/8/30 17:50, Russell King (Oracle) wrote:
> Please don't send the patch system patches that have not had any chance
> of review. The patch system is supposed to be for patches that are to
> be applied.
Okay, I get it now.
>
> Sending patches that are yet to be reviewed makes extra work for me.
I'm so sorry.
>
> Thanks.
>
> On Tue, Aug 30, 2022 at 02:54:13PM +0800, Zhen Lei wrote:
>> Commit f2d3b9a46e0e ("ARM: 9220/1: amba: Remove deferred device addition")
>> forcibly invokes device_add() even if dev->periphid is not ready. Although
>> it will be remedied in amba_match(): dev->periphid will be initialized
>> if everything is in place; Otherwise, return -EPROBE_DEFER to block
>> __driver_attach() from further execution. But not all drivers have .match
>> hook, such as pl031, the dev->bus->probe will be called directly in
>> __driver_attach(). Unfortunately, if dev->periphid is still not
>> initialized, the following exception will be triggered.
>>
>> 8<--- cut here ---
>> Unable to handle kernel NULL pointer dereference at virtual address 00000008
>> [00000008] *pgd=00000000
>> Internal error: Oops: 5 [#1] SMP ARM
>> Modules linked in:
>> CPU: 1 PID: 1 Comm: swapper/0 Not tainted 6.0.0-rc2+ #7
>> Hardware name: ARM-Versatile Express
>> PC is at pl031_probe+0x8/0x208
>> LR is at amba_probe+0xf0/0x160
>> pc : 80698df8 lr : 8050eb54 psr: 80000013
>> sp : c0825df8 ip : 00000000 fp : 811fda38
>> r10: 00000000 r9 : 80d72470 r8 : fffffdfb
>> r7 : 811fd800 r6 : be7eb330 r5 : 00000000 r4 : 811fd900
>> r3 : 80698df0 r2 : 37000000 r1 : 00000000 r0 : 811fd800
>> Flags: Nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
>> Control: 10c5387d Table: 6000406a DAC: 00000051
>> ... ...
>> pl031_probe from amba_probe+0xf0/0x160
>> amba_probe from really_probe+0x118/0x290
>> really_probe from __driver_probe_device+0x84/0xe4
>> __driver_probe_device from driver_probe_device+0x30/0xd0
>> driver_probe_device from __driver_attach+0x8c/0xfc
>> __driver_attach from bus_for_each_dev+0x70/0xb0
>> bus_for_each_dev from bus_add_driver+0x168/0x1f4
>> bus_add_driver from driver_register+0x7c/0x118
>> driver_register from do_one_initcall+0x44/0x1ec
>> do_one_initcall from kernel_init_freeable+0x238/0x288
>> kernel_init_freeable from kernel_init+0x18/0x12c
>> kernel_init from ret_from_fork+0x14/0x2c
>> ... ...
>> ---[ end trace 0000000000000000 ]---
>>
>> Therefore, take the same action as in amba_match(): return -EPROBE_DEFER
>> if dev->periphid is not ready in amba_probe().
>>
>> Fixes: f2d3b9a46e0e ("ARM: 9220/1: amba: Remove deferred device addition")
>> Signed-off-by: Zhen Lei <[email protected]>
>> ---
>> KernelVersion: v6.0-rc3
>> drivers/amba/bus.c | 24 +++++++++++++++++++++---
>> 1 file changed, 21 insertions(+), 3 deletions(-)
>>
>> v1 --> v2:
>> 1. Update this patch based on:
>> https://lore.kernel.org/lkml/[email protected]/
>> 2. Move the operations of sanity checking and reading dev->periphid,
>> updating uevent into new function amba_prepare_periphid().
>>
>> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
>> index 110a535648d2e1f..8e4c7e190880206 100644
>> --- a/drivers/amba/bus.c
>> +++ b/drivers/amba/bus.c
>> @@ -204,10 +204,9 @@ static int amba_read_periphid(struct amba_device *dev)
>> return ret;
>> }
>>
>> -static int amba_match(struct device *dev, struct device_driver *drv)
>> +static int amba_prepare_periphid(struct device *dev)
>> {
>> struct amba_device *pcdev = to_amba_device(dev);
>> - struct amba_driver *pcdrv = to_amba_driver(drv);
>>
>> mutex_lock(&pcdev->periphid_lock);
>> if (!pcdev->periphid) {
>> @@ -228,6 +227,19 @@ static int amba_match(struct device *dev, struct device_driver *drv)
>> }
>> mutex_unlock(&pcdev->periphid_lock);
>>
>> + return 0;
>> +}
>> +
>> +static int amba_match(struct device *dev, struct device_driver *drv)
>> +{
>> + struct amba_device *pcdev = to_amba_device(dev);
>> + struct amba_driver *pcdrv = to_amba_driver(drv);
>> + int ret;
>> +
>> + ret = amba_prepare_periphid(dev);
>> + if (ret)
>> + return ret;
>> +
>> /* When driver_override is set, only bind to the matching driver */
>> if (pcdev->driver_override)
>> return !strcmp(pcdev->driver_override, drv->name);
>> @@ -278,9 +290,15 @@ static int amba_probe(struct device *dev)
>> {
>> struct amba_device *pcdev = to_amba_device(dev);
>> struct amba_driver *pcdrv = to_amba_driver(dev->driver);
>> - const struct amba_id *id = amba_lookup(pcdrv->id_table, pcdev);
>> + const struct amba_id *id;
>> int ret;
>>
>> + ret = amba_prepare_periphid(dev);
>> + if (ret)
>> + return ret;
>> +
>> + id = amba_lookup(pcdrv->id_table, pcdev);
>> +
>> do {
>> ret = of_amba_device_decode_irq(pcdev);
>> if (ret)
>> --
>> 2.25.1
>>
>>
>
--
Regards,
Zhen Lei
On Tue, Aug 30, 2022 at 06:03:31PM +0800, Leizhen (ThunderTown) wrote:
>
>
> On 2022/8/30 17:47, Russell King (Oracle) wrote:
> > On Tue, Aug 30, 2022 at 12:20:00AM -0700, Saravana Kannan wrote:
> >> On Mon, Aug 29, 2022 at 11:59 PM Zhen Lei <[email protected]> wrote:
> >>>
> >>> Commit f2d3b9a46e0e ("ARM: 9220/1: amba: Remove deferred device addition")
> >>> forcibly invokes device_add() even if dev->periphid is not ready. Although
> >>> it will be remedied in amba_match(): dev->periphid will be initialized
> >>> if everything is in place; Otherwise, return -EPROBE_DEFER to block
> >>> __driver_attach() from further execution. But not all drivers have .match
> >>> hook, such as pl031, the dev->bus->probe will be called directly in
> >>> __driver_attach(). Unfortunately, if dev->periphid is still not
> >>> initialized, the following exception will be triggered.
> >>>
> >>> 8<--- cut here ---
> >>> Unable to handle kernel NULL pointer dereference at virtual address 00000008
> >>> [00000008] *pgd=00000000
> >>> Internal error: Oops: 5 [#1] SMP ARM
> >>> Modules linked in:
> >>> CPU: 1 PID: 1 Comm: swapper/0 Not tainted 6.0.0-rc2+ #7
> >>> Hardware name: ARM-Versatile Express
> >>> PC is at pl031_probe+0x8/0x208
> >>> LR is at amba_probe+0xf0/0x160
> >>> pc : 80698df8 lr : 8050eb54 psr: 80000013
> >>> sp : c0825df8 ip : 00000000 fp : 811fda38
> >>> r10: 00000000 r9 : 80d72470 r8 : fffffdfb
> >>> r7 : 811fd800 r6 : be7eb330 r5 : 00000000 r4 : 811fd900
> >>> r3 : 80698df0 r2 : 37000000 r1 : 00000000 r0 : 811fd800
> >>> Flags: Nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
> >>> Control: 10c5387d Table: 6000406a DAC: 00000051
> >>> ... ...
> >>> pl031_probe from amba_probe+0xf0/0x160
> >>> amba_probe from really_probe+0x118/0x290
> >>> really_probe from __driver_probe_device+0x84/0xe4
> >>> __driver_probe_device from driver_probe_device+0x30/0xd0
> >>> driver_probe_device from __driver_attach+0x8c/0xfc
> >>> __driver_attach from bus_for_each_dev+0x70/0xb0
> >>> bus_for_each_dev from bus_add_driver+0x168/0x1f4
> >>> bus_add_driver from driver_register+0x7c/0x118
> >>> driver_register from do_one_initcall+0x44/0x1ec
> >>> do_one_initcall from kernel_init_freeable+0x238/0x288
> >>> kernel_init_freeable from kernel_init+0x18/0x12c
> >>> kernel_init from ret_from_fork+0x14/0x2c
> >>> ... ...
> >>> ---[ end trace 0000000000000000 ]---
> >>>
> >>> Therefore, take the same action as in amba_match(): return -EPROBE_DEFER
> >>> if dev->periphid is not ready in amba_probe().
> >>>
> >>> Fixes: f2d3b9a46e0e ("ARM: 9220/1: amba: Remove deferred device addition")
> >>> Signed-off-by: Zhen Lei <[email protected]>
> >>> ---
> >>> KernelVersion: v6.0-rc3
> >>> drivers/amba/bus.c | 24 +++++++++++++++++++++---
> >>> 1 file changed, 21 insertions(+), 3 deletions(-)
> >>>
> >>> v1 --> v2:
> >>> 1. Update this patch based on:
> >>> https://lore.kernel.org/lkml/[email protected]/
> >>> 2. Move the operations of sanity checking and reading dev->periphid,
> >>> updating uevent into new function amba_prepare_periphid().
> >>>
> >>> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> >>> index 110a535648d2e1f..8e4c7e190880206 100644
> >>> --- a/drivers/amba/bus.c
> >>> +++ b/drivers/amba/bus.c
> >>> @@ -204,10 +204,9 @@ static int amba_read_periphid(struct amba_device *dev)
> >>> return ret;
> >>> }
> >>>
> >>> -static int amba_match(struct device *dev, struct device_driver *drv)
> >>> +static int amba_prepare_periphid(struct device *dev)
> >>> {
> >>> struct amba_device *pcdev = to_amba_device(dev);
> >>> - struct amba_driver *pcdrv = to_amba_driver(drv);
> >>>
> >>> mutex_lock(&pcdev->periphid_lock);
> >>> if (!pcdev->periphid) {
> >>> @@ -228,6 +227,19 @@ static int amba_match(struct device *dev, struct device_driver *drv)
> >>> }
> >>> mutex_unlock(&pcdev->periphid_lock);
> >>>
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int amba_match(struct device *dev, struct device_driver *drv)
> >>> +{
> >>> + struct amba_device *pcdev = to_amba_device(dev);
> >>> + struct amba_driver *pcdrv = to_amba_driver(drv);
> >>> + int ret;
> >>> +
> >>> + ret = amba_prepare_periphid(dev);
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> /* When driver_override is set, only bind to the matching driver */
> >>> if (pcdev->driver_override)
> >>> return !strcmp(pcdev->driver_override, drv->name);
> >>> @@ -278,9 +290,15 @@ static int amba_probe(struct device *dev)
> >>> {
> >>> struct amba_device *pcdev = to_amba_device(dev);
> >>> struct amba_driver *pcdrv = to_amba_driver(dev->driver);
> >>> - const struct amba_id *id = amba_lookup(pcdrv->id_table, pcdev);
> >>> + const struct amba_id *id;
> >>> int ret;
> >>>
> >>> + ret = amba_prepare_periphid(dev);
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> + id = amba_lookup(pcdrv->id_table, pcdev);
> >>> +
> >>> do {
> >>> ret = of_amba_device_decode_irq(pcdev);
> >>> if (ret)
> >>
> >> Let's wait for Isaac to review this. He has been looking at the
> >> locking issue for a bit and there were some tricky cases.
> >
> > How can we get to amba_probe() if amba_match() has not returned a
> > positive match for an ID? Surely if that happens, the driver model
>
> Always return true.
>
> __driver_attach
> driver_match_device
>
> static inline int driver_match_device(struct device_driver *drv,
> struct device *dev)
> {
> return drv->bus->match ? drv->bus->match(dev, drv) : 1;
> }
You seem to be misunderstanding something rather fundamental here.
For an amba driver, drv->bus will always be pointing at amba_bustype.
That always has a "match" operation. Therefore, the default of '1'
above will *never* be used for an AMBA driver.
If drv->bus does not point at amba_bustype, then amba_probe() will
not be called for "drv".
Therefore, amba_match() must always be called before amba_probe().
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Tue, Aug 30, 2022 at 02:54:13PM +0800, Zhen Lei wrote:
> Commit f2d3b9a46e0e ("ARM: 9220/1: amba: Remove deferred device addition")
> forcibly invokes device_add() even if dev->periphid is not ready. Although
> it will be remedied in amba_match(): dev->periphid will be initialized
> if everything is in place; Otherwise, return -EPROBE_DEFER to block
> __driver_attach() from further execution. But not all drivers have .match
> hook, such as pl031, the dev->bus->probe will be called directly in
> __driver_attach(). Unfortunately, if dev->periphid is still not
> initialized, the following exception will be triggered.
Please check whether 9229/1 fixes this issue for you.
http://www.armlinux.org.uk/developer/patches/download.php?id=9229/1
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Tue, Aug 30, 2022 at 06:31:14PM +0800, Leizhen (ThunderTown) wrote:
> On 2022/8/30 18:07, Russell King (Oracle) wrote:
> > For an amba driver, drv->bus will always be pointing at amba_bustype.
> > That always has a "match" operation. Therefore, the default of '1'
> > above will *never* be used for an AMBA driver.
> >
> > If drv->bus does not point at amba_bustype, then amba_probe() will
> > not be called for "drv".
> >
> > Therefore, amba_match() must always be called before amba_probe().
>
> Oh, I was careless. I think it's drv->match. But the processing flow
> will continue to go to "dev->bus->probe".
>
> __driver_attach():
> ret = driver_match_device(drv, dev);
> if (ret == 0) {
> /* no match */
> return 0;
> } else if (ret == -EPROBE_DEFER) { <------no return in this branch
> dev_dbg(dev, "Device match requests probe deferral\n");
> dev->can_match = true;
> driver_deferred_probe_add(dev);
> } else if (ret < 0) {
> dev_dbg(dev, "Bus failed to match device: %d\n", ret);
> return ret;
> } /* ret > 0 means positive match */
>
> ... ...
> driver_probe_device(drv, dev);
> ......
> dev->bus->probe
And that makes no sense, is an already known issue, and there is a patch
to fix it:
https://lore.kernel.org/all/[email protected]/
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On 2022/8/30 18:07, Russell King (Oracle) wrote:
> On Tue, Aug 30, 2022 at 06:03:31PM +0800, Leizhen (ThunderTown) wrote:
>>
>>
>> On 2022/8/30 17:47, Russell King (Oracle) wrote:
>>> On Tue, Aug 30, 2022 at 12:20:00AM -0700, Saravana Kannan wrote:
>>>> On Mon, Aug 29, 2022 at 11:59 PM Zhen Lei <[email protected]> wrote:
>>>>>
>>>>> Commit f2d3b9a46e0e ("ARM: 9220/1: amba: Remove deferred device addition")
>>>>> forcibly invokes device_add() even if dev->periphid is not ready. Although
>>>>> it will be remedied in amba_match(): dev->periphid will be initialized
>>>>> if everything is in place; Otherwise, return -EPROBE_DEFER to block
>>>>> __driver_attach() from further execution. But not all drivers have .match
>>>>> hook, such as pl031, the dev->bus->probe will be called directly in
>>>>> __driver_attach(). Unfortunately, if dev->periphid is still not
>>>>> initialized, the following exception will be triggered.
>>>>>
>>>>> 8<--- cut here ---
>>>>> Unable to handle kernel NULL pointer dereference at virtual address 00000008
>>>>> [00000008] *pgd=00000000
>>>>> Internal error: Oops: 5 [#1] SMP ARM
>>>>> Modules linked in:
>>>>> CPU: 1 PID: 1 Comm: swapper/0 Not tainted 6.0.0-rc2+ #7
>>>>> Hardware name: ARM-Versatile Express
>>>>> PC is at pl031_probe+0x8/0x208
>>>>> LR is at amba_probe+0xf0/0x160
>>>>> pc : 80698df8 lr : 8050eb54 psr: 80000013
>>>>> sp : c0825df8 ip : 00000000 fp : 811fda38
>>>>> r10: 00000000 r9 : 80d72470 r8 : fffffdfb
>>>>> r7 : 811fd800 r6 : be7eb330 r5 : 00000000 r4 : 811fd900
>>>>> r3 : 80698df0 r2 : 37000000 r1 : 00000000 r0 : 811fd800
>>>>> Flags: Nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
>>>>> Control: 10c5387d Table: 6000406a DAC: 00000051
>>>>> ... ...
>>>>> pl031_probe from amba_probe+0xf0/0x160
>>>>> amba_probe from really_probe+0x118/0x290
>>>>> really_probe from __driver_probe_device+0x84/0xe4
>>>>> __driver_probe_device from driver_probe_device+0x30/0xd0
>>>>> driver_probe_device from __driver_attach+0x8c/0xfc
>>>>> __driver_attach from bus_for_each_dev+0x70/0xb0
>>>>> bus_for_each_dev from bus_add_driver+0x168/0x1f4
>>>>> bus_add_driver from driver_register+0x7c/0x118
>>>>> driver_register from do_one_initcall+0x44/0x1ec
>>>>> do_one_initcall from kernel_init_freeable+0x238/0x288
>>>>> kernel_init_freeable from kernel_init+0x18/0x12c
>>>>> kernel_init from ret_from_fork+0x14/0x2c
>>>>> ... ...
>>>>> ---[ end trace 0000000000000000 ]---
>>>>>
>>>>> Therefore, take the same action as in amba_match(): return -EPROBE_DEFER
>>>>> if dev->periphid is not ready in amba_probe().
>>>>>
>>>>> Fixes: f2d3b9a46e0e ("ARM: 9220/1: amba: Remove deferred device addition")
>>>>> Signed-off-by: Zhen Lei <[email protected]>
>>>>> ---
>>>>> KernelVersion: v6.0-rc3
>>>>> drivers/amba/bus.c | 24 +++++++++++++++++++++---
>>>>> 1 file changed, 21 insertions(+), 3 deletions(-)
>>>>>
>>>>> v1 --> v2:
>>>>> 1. Update this patch based on:
>>>>> https://lore.kernel.org/lkml/[email protected]/
>>>>> 2. Move the operations of sanity checking and reading dev->periphid,
>>>>> updating uevent into new function amba_prepare_periphid().
>>>>>
>>>>> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
>>>>> index 110a535648d2e1f..8e4c7e190880206 100644
>>>>> --- a/drivers/amba/bus.c
>>>>> +++ b/drivers/amba/bus.c
>>>>> @@ -204,10 +204,9 @@ static int amba_read_periphid(struct amba_device *dev)
>>>>> return ret;
>>>>> }
>>>>>
>>>>> -static int amba_match(struct device *dev, struct device_driver *drv)
>>>>> +static int amba_prepare_periphid(struct device *dev)
>>>>> {
>>>>> struct amba_device *pcdev = to_amba_device(dev);
>>>>> - struct amba_driver *pcdrv = to_amba_driver(drv);
>>>>>
>>>>> mutex_lock(&pcdev->periphid_lock);
>>>>> if (!pcdev->periphid) {
>>>>> @@ -228,6 +227,19 @@ static int amba_match(struct device *dev, struct device_driver *drv)
>>>>> }
>>>>> mutex_unlock(&pcdev->periphid_lock);
>>>>>
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static int amba_match(struct device *dev, struct device_driver *drv)
>>>>> +{
>>>>> + struct amba_device *pcdev = to_amba_device(dev);
>>>>> + struct amba_driver *pcdrv = to_amba_driver(drv);
>>>>> + int ret;
>>>>> +
>>>>> + ret = amba_prepare_periphid(dev);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> +
>>>>> /* When driver_override is set, only bind to the matching driver */
>>>>> if (pcdev->driver_override)
>>>>> return !strcmp(pcdev->driver_override, drv->name);
>>>>> @@ -278,9 +290,15 @@ static int amba_probe(struct device *dev)
>>>>> {
>>>>> struct amba_device *pcdev = to_amba_device(dev);
>>>>> struct amba_driver *pcdrv = to_amba_driver(dev->driver);
>>>>> - const struct amba_id *id = amba_lookup(pcdrv->id_table, pcdev);
>>>>> + const struct amba_id *id;
>>>>> int ret;
>>>>>
>>>>> + ret = amba_prepare_periphid(dev);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> +
>>>>> + id = amba_lookup(pcdrv->id_table, pcdev);
>>>>> +
>>>>> do {
>>>>> ret = of_amba_device_decode_irq(pcdev);
>>>>> if (ret)
>>>>
>>>> Let's wait for Isaac to review this. He has been looking at the
>>>> locking issue for a bit and there were some tricky cases.
>>>
>>> How can we get to amba_probe() if amba_match() has not returned a
>>> positive match for an ID? Surely if that happens, the driver model
>>
>> Always return true.
>>
>> __driver_attach
>> driver_match_device
>>
>> static inline int driver_match_device(struct device_driver *drv,
>> struct device *dev)
>> {
>> return drv->bus->match ? drv->bus->match(dev, drv) : 1;
>> }
>
> You seem to be misunderstanding something rather fundamental here.
>
> For an amba driver, drv->bus will always be pointing at amba_bustype.
> That always has a "match" operation. Therefore, the default of '1'
> above will *never* be used for an AMBA driver.
>
> If drv->bus does not point at amba_bustype, then amba_probe() will
> not be called for "drv".
>
> Therefore, amba_match() must always be called before amba_probe().
Oh, I was careless. I think it's drv->match. But the processing flow
will continue to go to "dev->bus->probe".
__driver_attach():
ret = driver_match_device(drv, dev);
if (ret == 0) {
/* no match */
return 0;
} else if (ret == -EPROBE_DEFER) { <------no return in this branch
dev_dbg(dev, "Device match requests probe deferral\n");
dev->can_match = true;
driver_deferred_probe_add(dev);
} else if (ret < 0) {
dev_dbg(dev, "Bus failed to match device: %d\n", ret);
return ret;
} /* ret > 0 means positive match */
... ...
driver_probe_device(drv, dev);
......
dev->bus->probe
>
--
Regards,
Zhen Lei
On Tue, Aug 30, 2022 at 12:37 PM Russell King (Oracle)
<[email protected]> wrote:
> > ... ...
> > driver_probe_device(drv, dev);
> > ......
> > dev->bus->probe
>
> And that makes no sense, is an already known issue, and there is a patch
> to fix it:
>
> https://lore.kernel.org/all/[email protected]/
This patch to the driver core fixes my QEMU issue as well, I'll try
to reply in-thread. Thanks for pointing this out Russell!
Yours,
Linus Walleij
On 2022/8/30 19:19, Linus Walleij wrote:
> On Tue, Aug 30, 2022 at 12:37 PM Russell King (Oracle)
> <[email protected]> wrote:
>
>>> ... ...
>>> driver_probe_device(drv, dev);
>>> ......
>>> dev->bus->probe
>>
>> And that makes no sense, is an already known issue, and there is a patch
>> to fix it:
>>
>> https://lore.kernel.org/all/[email protected]/
>
> This patch to the driver core fixes my QEMU issue as well, I'll try
> to reply in-thread. Thanks for pointing this out Russell!
OK, so that my patch can be dropped.
>
> Yours,
> Linus Walleij
> .
>
--
Regards,
Zhen Lei
On 2022/8/30 18:07, Leizhen (ThunderTown) wrote:
>
> On 2022/8/30 17:50, Russell King (Oracle) wrote:
>> Please don't send the patch system patches that have not had any chance
>> of review. The patch system is supposed to be for patches that are to
>> be applied.
> Okay, I get it now.
>
>> Sending patches that are yet to be reviewed makes extra work for me.
> I'm so sorry.
>
Hi, Russell:
Do you have time to look at these two patches?
https://lkml.org/lkml/2022/8/3/140
https://lkml.org/lkml/2022/8/11/1266
--
Regards,
Zhen Lei
On Tue, Aug 30, 2022 at 3:36 AM Russell King (Oracle)
<[email protected]> wrote:
>
> On Tue, Aug 30, 2022 at 06:31:14PM +0800, Leizhen (ThunderTown) wrote:
> > On 2022/8/30 18:07, Russell King (Oracle) wrote:
> > > For an amba driver, drv->bus will always be pointing at amba_bustype.
> > > That always has a "match" operation. Therefore, the default of '1'
> > > above will *never* be used for an AMBA driver.
> > >
> > > If drv->bus does not point at amba_bustype, then amba_probe() will
> > > not be called for "drv".
> > >
> > > Therefore, amba_match() must always be called before amba_probe().
> >
> > Oh, I was careless. I think it's drv->match. But the processing flow
> > will continue to go to "dev->bus->probe".
> >
> > __driver_attach():
> > ret = driver_match_device(drv, dev);
> > if (ret == 0) {
> > /* no match */
> > return 0;
> > } else if (ret == -EPROBE_DEFER) { <------no return in this branch
> > dev_dbg(dev, "Device match requests probe deferral\n");
> > dev->can_match = true;
> > driver_deferred_probe_add(dev);
> > } else if (ret < 0) {
> > dev_dbg(dev, "Bus failed to match device: %d\n", ret);
> > return ret;
> > } /* ret > 0 means positive match */
> >
> > ... ...
> > driver_probe_device(drv, dev);
> > ......
> > dev->bus->probe
>
> And that makes no sense, is an already known issue, and there is a patch
> to fix it:
>
> https://lore.kernel.org/all/[email protected]/
Russell,
Thanks for discussing this further and pointing out the other fix. I
assumed Leizhen was talking about an instance of
device_driver_attach() which allows probe to be called without match.
Thankfully that function is used only by some specific
frameworks/buses types. I did a cursory check and I don't see any
intersection with we amba.
-Saravana
P.S: I hate that function exists, as it just throws a wrench in the
whole driver core design and adds corner cases to a lot of generic
driver core design.
On Tue, Aug 30, 2022 at 10:34 AM Saravana Kannan <[email protected]> wrote:
>
> On Tue, Aug 30, 2022 at 3:36 AM Russell King (Oracle)
> <[email protected]> wrote:
> >
> > On Tue, Aug 30, 2022 at 06:31:14PM +0800, Leizhen (ThunderTown) wrote:
> > > On 2022/8/30 18:07, Russell King (Oracle) wrote:
> > > > For an amba driver, drv->bus will always be pointing at amba_bustype.
> > > > That always has a "match" operation. Therefore, the default of '1'
> > > > above will *never* be used for an AMBA driver.
> > > >
> > > > If drv->bus does not point at amba_bustype, then amba_probe() will
> > > > not be called for "drv".
> > > >
> > > > Therefore, amba_match() must always be called before amba_probe().
> > >
> > > Oh, I was careless. I think it's drv->match. But the processing flow
> > > will continue to go to "dev->bus->probe".
> > >
> > > __driver_attach():
> > > ret = driver_match_device(drv, dev);
> > > if (ret == 0) {
> > > /* no match */
> > > return 0;
> > > } else if (ret == -EPROBE_DEFER) { <------no return in this branch
> > > dev_dbg(dev, "Device match requests probe deferral\n");
> > > dev->can_match = true;
> > > driver_deferred_probe_add(dev);
> > > } else if (ret < 0) {
> > > dev_dbg(dev, "Bus failed to match device: %d\n", ret);
> > > return ret;
> > > } /* ret > 0 means positive match */
> > >
> > > ... ...
> > > driver_probe_device(drv, dev);
> > > ......
> > > dev->bus->probe
> >
> > And that makes no sense, is an already known issue, and there is a patch
> > to fix it:
> >
> > https://lore.kernel.org/all/[email protected]/
>
> Russell,
>
> Thanks for discussing this further and pointing out the other fix. I
> assumed Leizhen was talking about an instance of
> device_driver_attach() which allows probe to be called without match.
> Thankfully that function is used only by some specific
> frameworks/buses types. I did a cursory check and I don't see any
> intersection with we amba.
>
> -Saravana
> P.S: I hate that function exists, as it just throws a wrench in the
> whole driver core design and adds corner cases to a lot of generic
> driver core design.
Oops, meant to say device_bind_driver().
So through out my email:
s/device_driver_attach/device_bind_driver/
-Saravana