[U-Boot] [PATCH 0/3] While converting SATA on Apalis iMX6 to use driver model I noticed it
crashing in case no SATA driver is actually there:
Apalis iMX6 # sata init data abort pc : [<8ff78e1a>] lr : [<8ff78e1b>] reloc pc : [<1781be1a>] lr : [<1781be1b>] sp : 8df53068 ip : 00000001 fp : 00000002 r10: 8df612e8 r9 : 8df5ceb0 r8 : 8ffc2b28 r7 : 8ff746ed r6 : 00000000 r5 : 68abf82a r4 : 00000000 r3 : 8ff73d39 r2 : c0000000 r1 : 00000000 r0 : 0000000c Flags: nzcv IRQs off FIQs off Mode SVC_32 Code: 47700025 6803b570 6bdd4604 f0254805 (68abf82a) Resetting CPU ...
resetting ...
Investigating this I identified 3 places where no proper checking is done.
This patch set adds such checks leading to a graceful error instead:
Apalis iMX6 # sata init Cannot probe SATA device 0 (err=-19)
Marcel Ziswiler (3): dm: device: fail uclass_find_first_device() if list_empty dm: sata: add null pointer check for dev cmd: sata: add null pointer check for dev
cmd/sata.c | 4 ++++ drivers/ata/sata.c | 27 +++++++++++++++++++++------ drivers/core/uclass.c | 2 +- 3 files changed, 26 insertions(+), 7 deletions(-)
From: Marcel Ziswiler marcel.ziswiler@toradex.com
While uclass_find_device() fails with -ENODEV in case of list_empty strangely uclass_find_first_device() returns 0.
Fix uclass_find_first_device() to also fail with -ENODEV instead.
Signed-off-by: Marcel Ziswiler marcel.ziswiler@toradex.com
---
drivers/core/uclass.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c index a622f07941..fc3157de39 100644 --- a/drivers/core/uclass.c +++ b/drivers/core/uclass.c @@ -225,7 +225,7 @@ int uclass_find_first_device(enum uclass_id id, struct udevice **devp) if (ret) return ret; if (list_empty(&uc->dev_head)) - return 0; + return -ENODEV;
*devp = list_first_entry(&uc->dev_head, struct udevice, uclass_node);
Hi Marcel,
On Fri, 25 Jan 2019 at 03:30, Marcel Ziswiler marcel@ziswiler.com wrote:
From: Marcel Ziswiler marcel.ziswiler@toradex.com
While uclass_find_device() fails with -ENODEV in case of list_empty strangely uclass_find_first_device() returns 0.
Fix uclass_find_first_device() to also fail with -ENODEV instead.
The fix sees OK to me. I assume that 'make qcheck' still passes.
But can you please update dm_test_uclass_devices_find() to test this behaviour?
Signed-off-by: Marcel Ziswiler marcel.ziswiler@toradex.com
drivers/core/uclass.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c index a622f07941..fc3157de39 100644 --- a/drivers/core/uclass.c +++ b/drivers/core/uclass.c @@ -225,7 +225,7 @@ int uclass_find_first_device(enum uclass_id id, struct udevice **devp) if (ret) return ret; if (list_empty(&uc->dev_head))
return 0;
return -ENODEV; *devp = list_first_entry(&uc->dev_head, struct udevice, uclass_node);-- 2.20.1
Regards, Simon
Hi Simon
On Fri, 2019-01-25 at 09:18 +1300, Simon Glass wrote:
Hi Marcel,
On Fri, 25 Jan 2019 at 03:30, Marcel Ziswiler marcel@ziswiler.com wrote:
From: Marcel Ziswiler marcel.ziswiler@toradex.com
While uclass_find_device() fails with -ENODEV in case of list_empty strangely uclass_find_first_device() returns 0.
Fix uclass_find_first_device() to also fail with -ENODEV instead.
The fix sees OK to me. I assume that 'make qcheck' still passes.
Yes, certainly.
This means it did not check for this so far.
But can you please update dm_test_uclass_devices_find() to test this behaviour?
OK, will do so for v2.
Signed-off-by: Marcel Ziswiler marcel.ziswiler@toradex.com
drivers/core/uclass.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c index a622f07941..fc3157de39 100644 --- a/drivers/core/uclass.c +++ b/drivers/core/uclass.c @@ -225,7 +225,7 @@ int uclass_find_first_device(enum uclass_id id, struct udevice **devp) if (ret) return ret; if (list_empty(&uc->dev_head))
return 0;
return -ENODEV; *devp = list_first_entry(&uc->dev_head, struct udevice,uclass_node);
-- 2.20.1
Regards, Simon
Cheers
Marcel
From: Marcel Ziswiler marcel.ziswiler@toradex.com
Given ahci_get_ops() being a macro not checking anything make sure we only call it if we do indeed have a dev pointer.
Signed-off-by: Marcel Ziswiler marcel.ziswiler@toradex.com
---
drivers/ata/sata.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-)
diff --git a/drivers/ata/sata.c b/drivers/ata/sata.c index e384b805b2..4e41f09c87 100644 --- a/drivers/ata/sata.c +++ b/drivers/ata/sata.c @@ -20,9 +20,14 @@ struct blk_desc sata_dev_desc[CONFIG_SYS_SATA_MAX_DEVICE];
int sata_reset(struct udevice *dev) { - struct ahci_ops *ops = ahci_get_ops(dev); + struct ahci_ops *ops = NULL;
- if (!ops->reset) + if (!dev) + return -ENODEV; + + ops = ahci_get_ops(dev); + + if (!ops || !ops->reset) return -ENOSYS;
return ops->reset(dev); @@ -30,9 +35,14 @@ int sata_reset(struct udevice *dev)
int sata_dm_port_status(struct udevice *dev, int port) { - struct ahci_ops *ops = ahci_get_ops(dev); + struct ahci_ops *ops = NULL; + + if (!dev) + return -ENODEV;
- if (!ops->port_status) + ops = ahci_get_ops(dev); + + if (!ops || !ops->port_status) return -ENOSYS;
return ops->port_status(dev, port); @@ -40,9 +50,14 @@ int sata_dm_port_status(struct udevice *dev, int port)
int sata_scan(struct udevice *dev) { - struct ahci_ops *ops = ahci_get_ops(dev); + struct ahci_ops *ops = NULL; + + if (!dev) + return -ENODEV; + + ops = ahci_get_ops(dev);
- if (!ops->scan) + if (!ops || !ops->scan) return -ENOSYS;
return ops->scan(dev);
Hi Marcel,
On Fri, 25 Jan 2019 at 03:30, Marcel Ziswiler marcel@ziswiler.com wrote:
From: Marcel Ziswiler marcel.ziswiler@toradex.com
Given ahci_get_ops() being a macro not checking anything make sure we only call it if we do indeed have a dev pointer.
Signed-off-by: Marcel Ziswiler marcel.ziswiler@toradex.com
drivers/ata/sata.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-)
diff --git a/drivers/ata/sata.c b/drivers/ata/sata.c index e384b805b2..4e41f09c87 100644 --- a/drivers/ata/sata.c +++ b/drivers/ata/sata.c @@ -20,9 +20,14 @@ struct blk_desc sata_dev_desc[CONFIG_SYS_SATA_MAX_DEVICE];
int sata_reset(struct udevice *dev) {
struct ahci_ops *ops = ahci_get_ops(dev);
struct ahci_ops *ops = NULL;
if (!ops->reset)
if (!dev)return -ENODEV;
This should not happen, i.e. we should not call a DM function with a NULL pointer. That is the caller's problem.
Similarly with ops. It's OK to check for one of the operations being NULL, but the operation pointer itself should never be NULL.
These checks add to code side with no benefit in the normal case.
We did talk about checking this in the uclass or in DM core, but I don't believe a patch was sent.
ops = ahci_get_ops(dev);if (!ops || !ops->reset) return -ENOSYS; return ops->reset(dev);@@ -30,9 +35,14 @@ int sata_reset(struct udevice *dev)
[..]
Regards, Simon
Hi Simon
On Fri, 2019-01-25 at 09:18 +1300, Simon Glass wrote:
Hi Marcel,
On Fri, 25 Jan 2019 at 03:30, Marcel Ziswiler marcel@ziswiler.com wrote:
From: Marcel Ziswiler marcel.ziswiler@toradex.com
Given ahci_get_ops() being a macro not checking anything make sure we only call it if we do indeed have a dev pointer.
Signed-off-by: Marcel Ziswiler marcel.ziswiler@toradex.com
drivers/ata/sata.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-)
diff --git a/drivers/ata/sata.c b/drivers/ata/sata.c index e384b805b2..4e41f09c87 100644 --- a/drivers/ata/sata.c +++ b/drivers/ata/sata.c @@ -20,9 +20,14 @@ struct blk_desc sata_dev_desc[CONFIG_SYS_SATA_MAX_DEVICE];
int sata_reset(struct udevice *dev) {
struct ahci_ops *ops = ahci_get_ops(dev);
struct ahci_ops *ops = NULL;
if (!ops->reset)
if (!dev)return -ENODEV;This should not happen, i.e. we should not call a DM function with a NULL pointer. That is the caller's problem.
Similarly with ops. It's OK to check for one of the operations being NULL, but the operation pointer itself should never be NULL.
These checks add to code side with no benefit in the normal case.
OK, sorry. I was not aware what exactly the overall error handling decision was on this.
In theory, of course just one of them patches in this series is enough to fix the particular issue I stumbled upon. I was just wondering whether or not adding some more checks may be useful for the next one hitting such issue.
I understand now and will therefore drop this one.
We did talk about checking this in the uclass or in DM core, but I don't believe a patch was sent.
Sure.
ops = ahci_get_ops(dev);if (!ops || !ops->reset) return -ENOSYS; return ops->reset(dev);@@ -30,9 +35,14 @@ int sata_reset(struct udevice *dev)
[..]
Regards, Simon
Cheers
Marcel
From: Marcel Ziswiler marcel.ziswiler@toradex.com
Calling sata_scan() with a null pointer probably won't make much sense.
Signed-off-by: Marcel Ziswiler marcel.ziswiler@toradex.com
---
cmd/sata.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/cmd/sata.c b/cmd/sata.c index 6d62ba8f74..a73cc54bd3 100644 --- a/cmd/sata.c +++ b/cmd/sata.c @@ -60,6 +60,10 @@ int sata_probe(int devnum) printf("Cannot probe SATA device %d (err=%d)\n", devnum, rc); return CMD_RET_FAILURE; } + if (!dev) { + printf("No SATA device found!\n"); + return CMD_RET_FAILURE; + } rc = sata_scan(dev); if (rc) { printf("Cannot scan SATA device %d (err=%d)\n", devnum, rc);
On Fri, 25 Jan 2019 at 03:30, Marcel Ziswiler marcel@ziswiler.com wrote:
From: Marcel Ziswiler marcel.ziswiler@toradex.com
Calling sata_scan() with a null pointer probably won't make much sense.
Signed-off-by: Marcel Ziswiler marcel.ziswiler@toradex.com
cmd/sata.c | 4 ++++ 1 file changed, 4 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
participants (3)
-
Marcel Ziswiler -
Marcel Ziswiler -
Simon Glass