[U-Boot] [PATCH 0/3] While converting SATA on Apalis iMX6 to use driver model I noticed it

[U-Boot] [PATCH 0/3] While converting SATA on Apalis iMX6 to use driver model I noticed it

[Marcel Ziswiler]

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(-)

-- 
2.20.1

[U-Boot] [PATCH 1/3] dm: device: fail uclass_find_first_device() if list_empty

[Marcel Ziswiler]

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);
 
-- 
2.20.1

[U-Boot] [PATCH 2/3] dm: sata: add null pointer check for dev

[Marcel Ziswiler]

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);
-- 
2.20.1

[U-Boot] [PATCH 3/3] cmd: sata: add null pointer check for dev

[Marcel Ziswiler]

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);
-- 
2.20.1

[U-Boot] [PATCH 1/3] dm: device: fail uclass_find_first_device() if list_empty

[Simon Glass]

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

[U-Boot] [PATCH 2/3] dm: sata: add null pointer check for dev

[Simon Glass]

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

[U-Boot] [PATCH 3/3] cmd: sata: add null pointer check for dev

[Simon Glass]

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>

[U-Boot] [PATCH 1/3] dm: device: fail uclass_find_first_device() if list_empty

[Marcel Ziswiler]

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

[U-Boot] [PATCH 2/3] dm: sata: add null pointer check for dev

[Marcel Ziswiler]

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