| Message ID | 20190628064139.17408-1-jk@ozlabs.org |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | fsi/core: Fix error paths on CFAM init | expand |
On Fri, Jun 28, 2019 at 2:44 PM Jeremy Kerr <jk@ozlabs.org> wrote: > > Change d1dcd67825 re-worked the struct fsi_slave initialisation in > fsi_slave_init, but introduced a few inconsitencies: the slave->dev is > now registered through cdev_device_add, but we may kfree() the device > out from underneath the cdev registration. We may also leave an IDA > allocated. > > This change fixes the error paths, so that we kfree() only before the > device is registered with the core code. We also move the smode write to > before we start creating proper devices, as it's the most likely to > fail. We also remove the IDA-allocated minor on error, and properly > clean up the of_node. > > Fixes: d1dcd678257603e71cf3f3d84c70e2b6f0f14bb8 > Reported-by: Lei YU <mine260309@gmail.com> > Signed-off-by: Jeremy Kerr <jk@ozlabs.org> > --- > > Lei: Can you test this on your hardware? Thanks! > > --- > drivers/fsi/fsi-core.c | 34 +++++++++++++++++++++------------- > 1 file changed, 21 insertions(+), 13 deletions(-) > > diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c > index 1d83f3ba478b..9ba5e19d1c42 100644 > --- a/drivers/fsi/fsi-core.c > +++ b/drivers/fsi/fsi-core.c > @@ -1029,6 +1029,14 @@ static int fsi_slave_init(struct fsi_master *master, int link, uint8_t id) > > } > > + rc = fsi_slave_set_smode(slave); > + if (rc) { > + dev_warn(&master->dev, > + "can't set smode on slave:%02x:%02x %d\n", > + link, id, rc); > + goto err_free; > + } > + > /* Allocate a minor in the FSI space */ > rc = __fsi_get_new_minor(slave, fsi_dev_cfam, &slave->dev.devt, > &slave->cdev_idx); > @@ -1040,17 +1048,14 @@ static int fsi_slave_init(struct fsi_master *master, int link, uint8_t id) > rc = cdev_device_add(&slave->cdev, &slave->dev); > if (rc) { > dev_err(&slave->dev, "Error %d creating slave device\n", rc); > - goto err_free; > + goto err_free_ida; > } > > - rc = fsi_slave_set_smode(slave); > - if (rc) { > - dev_warn(&master->dev, > - "can't set smode on slave:%02x:%02x %d\n", > - link, id, rc); > - kfree(slave); > - return -ENODEV; > - } > + /* Now that we have the cdev registered with the core, any fatal > + * failures beyond this point will need to clean up through > + * cdev_device_del(). Fortunately though, nothing past here is fatal. > + */ > + > if (master->link_config) > master->link_config(master, link, > slave->t_send_delay, > @@ -1067,11 +1072,14 @@ static int fsi_slave_init(struct fsi_master *master, int link, uint8_t id) > dev_dbg(&master->dev, "failed during slave scan with: %d\n", > rc); > > - return rc; > + return 0; > > - err_free: > - put_device(&slave->dev); > - return rc; > +err_free_ida: > + fsi_free_minor(slave->dev.devt); > +err_free: > + of_node_put(slave->dev.of_node); > + kfree(slave); > + return rc semicolon missed :) > } > > /* FSI master support */ > -- > 2.20.1 >
Hi John, > > + return rc > semicolon missed :) Whoa, I must have removed that after the last test. v2 coming. Cheers, Hereny
On Fri, 2019-06-28 at 14:41 +0800, Jeremy Kerr wrote: > Change d1dcd67825 re-worked the struct fsi_slave initialisation in > fsi_slave_init, but introduced a few inconsitencies: the slave->dev > is > now registered through cdev_device_add, but we may kfree() the device > out from underneath the cdev registration. We may also leave an IDA > allocated. > > This change fixes the error paths, so that we kfree() only before the > device is registered with the core code. We also move the smode write > to > before we start creating proper devices, as it's the most likely to > fail. We also remove the IDA-allocated minor on error, and properly > clean up the of_node. > > Fixes: d1dcd678257603e71cf3f3d84c70e2b6f0f14bb8 > Reported-by: Lei YU <mine260309@gmail.com> > Signed-off-by: Jeremy Kerr <jk@ozlabs.org> > --- You want to take maintainership of drivers/fsi ? :-) > > Lei: Can you test this on your hardware? Thanks! > > --- > drivers/fsi/fsi-core.c | 34 +++++++++++++++++++++------------- > 1 file changed, 21 insertions(+), 13 deletions(-) > > diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c > index 1d83f3ba478b..9ba5e19d1c42 100644 > --- a/drivers/fsi/fsi-core.c > +++ b/drivers/fsi/fsi-core.c > @@ -1029,6 +1029,14 @@ static int fsi_slave_init(struct fsi_master > *master, int link, uint8_t id) > > } > > + rc = fsi_slave_set_smode(slave); > + if (rc) { > + dev_warn(&master->dev, > + "can't set smode on slave:%02x:%02x > %d\n", > + link, id, rc); > + goto err_free; > + } > + > /* Allocate a minor in the FSI space */ > rc = __fsi_get_new_minor(slave, fsi_dev_cfam, &slave->dev.devt, > &slave->cdev_idx); > @@ -1040,17 +1048,14 @@ static int fsi_slave_init(struct fsi_master > *master, int link, uint8_t id) > rc = cdev_device_add(&slave->cdev, &slave->dev); > if (rc) { > dev_err(&slave->dev, "Error %d creating slave > device\n", rc); > - goto err_free; > + goto err_free_ida; > } > > - rc = fsi_slave_set_smode(slave); > - if (rc) { > - dev_warn(&master->dev, > - "can't set smode on slave:%02x:%02x > %d\n", > - link, id, rc); > - kfree(slave); > - return -ENODEV; > - } > + /* Now that we have the cdev registered with the core, any > fatal > + * failures beyond this point will need to clean up through > + * cdev_device_del(). Fortunately though, nothing past here is > fatal. > + */ > + > if (master->link_config) > master->link_config(master, link, > slave->t_send_delay, > @@ -1067,11 +1072,14 @@ static int fsi_slave_init(struct fsi_master > *master, int link, uint8_t id) > dev_dbg(&master->dev, "failed during slave scan with: > %d\n", > rc); > > - return rc; > + return 0; > > - err_free: > - put_device(&slave->dev); > - return rc; > +err_free_ida: > + fsi_free_minor(slave->dev.devt); > +err_free: > + of_node_put(slave->dev.of_node); > + kfree(slave); > + return rc > } > > /* FSI master support */
diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c index 1d83f3ba478b..9ba5e19d1c42 100644 --- a/drivers/fsi/fsi-core.c +++ b/drivers/fsi/fsi-core.c @@ -1029,6 +1029,14 @@ static int fsi_slave_init(struct fsi_master *master, int link, uint8_t id) } + rc = fsi_slave_set_smode(slave); + if (rc) { + dev_warn(&master->dev, + "can't set smode on slave:%02x:%02x %d\n", + link, id, rc); + goto err_free; + } + /* Allocate a minor in the FSI space */ rc = __fsi_get_new_minor(slave, fsi_dev_cfam, &slave->dev.devt, &slave->cdev_idx); @@ -1040,17 +1048,14 @@ static int fsi_slave_init(struct fsi_master *master, int link, uint8_t id) rc = cdev_device_add(&slave->cdev, &slave->dev); if (rc) { dev_err(&slave->dev, "Error %d creating slave device\n", rc); - goto err_free; + goto err_free_ida; } - rc = fsi_slave_set_smode(slave); - if (rc) { - dev_warn(&master->dev, - "can't set smode on slave:%02x:%02x %d\n", - link, id, rc); - kfree(slave); - return -ENODEV; - } + /* Now that we have the cdev registered with the core, any fatal + * failures beyond this point will need to clean up through + * cdev_device_del(). Fortunately though, nothing past here is fatal. + */ + if (master->link_config) master->link_config(master, link, slave->t_send_delay, @@ -1067,11 +1072,14 @@ static int fsi_slave_init(struct fsi_master *master, int link, uint8_t id) dev_dbg(&master->dev, "failed during slave scan with: %d\n", rc); - return rc; + return 0; - err_free: - put_device(&slave->dev); - return rc; +err_free_ida: + fsi_free_minor(slave->dev.devt); +err_free: + of_node_put(slave->dev.of_node); + kfree(slave); + return rc } /* FSI master support */
Change d1dcd67825 re-worked the struct fsi_slave initialisation in fsi_slave_init, but introduced a few inconsitencies: the slave->dev is now registered through cdev_device_add, but we may kfree() the device out from underneath the cdev registration. We may also leave an IDA allocated. This change fixes the error paths, so that we kfree() only before the device is registered with the core code. We also move the smode write to before we start creating proper devices, as it's the most likely to fail. We also remove the IDA-allocated minor on error, and properly clean up the of_node. Fixes: d1dcd678257603e71cf3f3d84c70e2b6f0f14bb8 Reported-by: Lei YU <mine260309@gmail.com> Signed-off-by: Jeremy Kerr <jk@ozlabs.org> --- Lei: Can you test this on your hardware? Thanks! --- drivers/fsi/fsi-core.c | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-)