| Message ID | 20220830065413.638-1-thunder.leizhen@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] ARM: Add sanity check for dev->periphid in amba_probe() | expand |
On Mon, Aug 29, 2022 at 11:59 PM Zhen Lei <thunder.leizhen@huawei.com> 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 <thunder.leizhen@huawei.com> > --- > 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/20220818172852.3548-1-isaacmanjarres@google.com/ > 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 <thunder.leizhen@huawei.com> 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 <thunder.leizhen@huawei.com> > > --- > > 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/20220818172852.3548-1-isaacmanjarres@google.com/ > > 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.
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.
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 <thunder.leizhen@huawei.com> > --- > 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/20220818172852.3548-1-isaacmanjarres@google.com/ > 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 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 <thunder.leizhen@huawei.com> 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 <thunder.leizhen@huawei.com> >>> --- >>> 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/20220818172852.3548-1-isaacmanjarres@google.com/ >>> 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. >
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 <thunder.leizhen@huawei.com> >> --- >> 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/20220818172852.3548-1-isaacmanjarres@google.com/ >> 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 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 <thunder.leizhen@huawei.com> 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 <thunder.leizhen@huawei.com> > >>> --- > >>> 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/20220818172852.3548-1-isaacmanjarres@google.com/ > >>> 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().
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.
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 <thunder.leizhen@huawei.com> 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 <thunder.leizhen@huawei.com> >>>>> --- >>>>> 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/20220818172852.3548-1-isaacmanjarres@google.com/ >>>>> 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 >
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/20220817184026.3468620-1-isaacmanjarres@google.com/
On Tue, Aug 30, 2022 at 12:37 PM Russell King (Oracle) <linux@armlinux.org.uk> 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/20220817184026.3468620-1-isaacmanjarres@google.com/ 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) > <linux@armlinux.org.uk> 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/20220817184026.3468620-1-isaacmanjarres@google.com/ > > 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 > . >
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
On Tue, Aug 30, 2022 at 3:36 AM Russell King (Oracle) <linux@armlinux.org.uk> 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/20220817184026.3468620-1-isaacmanjarres@google.com/ 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 <saravanak@google.com> wrote: > > On Tue, Aug 30, 2022 at 3:36 AM Russell King (Oracle) > <linux@armlinux.org.uk> 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/20220817184026.3468620-1-isaacmanjarres@google.com/ > > 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
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)
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 <thunder.leizhen@huawei.com> --- 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/20220818172852.3548-1-isaacmanjarres@google.com/ 2. Move the operations of sanity checking and reading dev->periphid, updating uevent into new function amba_prepare_periphid().