[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 at ziswiler.com>
> wrote:
> > From: Marcel Ziswiler <marcel.ziswiler at 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 at 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