| Message ID | 20190124142957.15040-3-marcel@ziswiler.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | While converting SATA on Apalis iMX6 to use driver model I noticed it | expand |
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
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);