* [lm-sensors] [PATCH v2] hwmon: (ads7871) Fix ads7871_probe error @ 2010-08-17 12:41 ` Axel Lin 0 siblings, 0 replies; 6+ messages in thread From: Axel Lin @ 2010-08-17 12:41 UTC (permalink / raw) To: linux-kernel; +Cc: Jean Delvare, Paul Thomas, lm-sensors This patch moves hwmon_device_register() to the end of ads7871_probe(). So we can make sure the device exists and is properly initialized before the device is exposed to user-space. Signed-off-by: Axel Lin <axel.lin@gmail.com> --- drivers/hwmon/ads7871.c | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/hwmon/ads7871.c b/drivers/hwmon/ads7871.c index b300a20..45f5829 100644 --- a/drivers/hwmon/ads7871.c +++ b/drivers/hwmon/ads7871.c @@ -176,12 +176,6 @@ static int __devinit ads7871_probe(struct spi_device *spi) if (status < 0) goto error_free; - pdata->hwmon_dev = hwmon_device_register(&spi->dev); - if (IS_ERR(pdata->hwmon_dev)) { - err = PTR_ERR(pdata->hwmon_dev); - goto error_remove; - } - spi_set_drvdata(spi, pdata); /* Configure the SPI bus */ @@ -204,6 +198,12 @@ static int __devinit ads7871_probe(struct spi_device *spi) goto error_remove; } + pdata->hwmon_dev = hwmon_device_register(&spi->dev); + if (IS_ERR(pdata->hwmon_dev)) { + err = PTR_ERR(pdata->hwmon_dev); + goto error_remove; + } + return 0; error_remove: -- 1.7.0.4 _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2] hwmon: (ads7871) Fix ads7871_probe error path @ 2010-08-17 12:41 ` Axel Lin 0 siblings, 0 replies; 6+ messages in thread From: Axel Lin @ 2010-08-17 12:41 UTC (permalink / raw) To: linux-kernel; +Cc: Jean Delvare, Paul Thomas, lm-sensors This patch moves hwmon_device_register() to the end of ads7871_probe(). So we can make sure the device exists and is properly initialized before the device is exposed to user-space. Signed-off-by: Axel Lin <axel.lin@gmail.com> --- drivers/hwmon/ads7871.c | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/hwmon/ads7871.c b/drivers/hwmon/ads7871.c index b300a20..45f5829 100644 --- a/drivers/hwmon/ads7871.c +++ b/drivers/hwmon/ads7871.c @@ -176,12 +176,6 @@ static int __devinit ads7871_probe(struct spi_device *spi) if (status < 0) goto error_free; - pdata->hwmon_dev = hwmon_device_register(&spi->dev); - if (IS_ERR(pdata->hwmon_dev)) { - err = PTR_ERR(pdata->hwmon_dev); - goto error_remove; - } - spi_set_drvdata(spi, pdata); /* Configure the SPI bus */ @@ -204,6 +198,12 @@ static int __devinit ads7871_probe(struct spi_device *spi) goto error_remove; } + pdata->hwmon_dev = hwmon_device_register(&spi->dev); + if (IS_ERR(pdata->hwmon_dev)) { + err = PTR_ERR(pdata->hwmon_dev); + goto error_remove; + } + return 0; error_remove: -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [lm-sensors] [PATCH v2] hwmon: (ads7871) Fix ads7871_probe 2010-08-17 12:41 ` [PATCH v2] hwmon: (ads7871) Fix ads7871_probe error path Axel Lin @ 2010-08-17 14:10 ` Jean Delvare -1 siblings, 0 replies; 6+ messages in thread From: Jean Delvare @ 2010-08-17 14:10 UTC (permalink / raw) To: Axel Lin; +Cc: linux-kernel, Paul Thomas, lm-sensors On Tue, 17 Aug 2010 20:41:24 +0800, Axel Lin wrote: > This patch moves hwmon_device_register() to the end of ads7871_probe(). > So we can make sure the device exists and is properly initialized > before the device is exposed to user-space. > > Signed-off-by: Axel Lin <axel.lin@gmail.com> > --- > drivers/hwmon/ads7871.c | 12 ++++++------ > 1 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/hwmon/ads7871.c b/drivers/hwmon/ads7871.c > index b300a20..45f5829 100644 > --- a/drivers/hwmon/ads7871.c > +++ b/drivers/hwmon/ads7871.c > @@ -176,12 +176,6 @@ static int __devinit ads7871_probe(struct spi_device *spi) > if (status < 0) > goto error_free; > > - pdata->hwmon_dev = hwmon_device_register(&spi->dev); > - if (IS_ERR(pdata->hwmon_dev)) { > - err = PTR_ERR(pdata->hwmon_dev); > - goto error_remove; > - } > - > spi_set_drvdata(spi, pdata); > > /* Configure the SPI bus */ > @@ -204,6 +198,12 @@ static int __devinit ads7871_probe(struct spi_device *spi) > goto error_remove; > } > > + pdata->hwmon_dev = hwmon_device_register(&spi->dev); > + if (IS_ERR(pdata->hwmon_dev)) { > + err = PTR_ERR(pdata->hwmon_dev); > + goto error_remove; > + } > + > return 0; > > error_remove: Applied, thanks. -- Jean Delvare _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] hwmon: (ads7871) Fix ads7871_probe error path @ 2010-08-17 14:10 ` Jean Delvare 0 siblings, 0 replies; 6+ messages in thread From: Jean Delvare @ 2010-08-17 14:10 UTC (permalink / raw) To: Axel Lin; +Cc: linux-kernel, Paul Thomas, lm-sensors On Tue, 17 Aug 2010 20:41:24 +0800, Axel Lin wrote: > This patch moves hwmon_device_register() to the end of ads7871_probe(). > So we can make sure the device exists and is properly initialized > before the device is exposed to user-space. > > Signed-off-by: Axel Lin <axel.lin@gmail.com> > --- > drivers/hwmon/ads7871.c | 12 ++++++------ > 1 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/hwmon/ads7871.c b/drivers/hwmon/ads7871.c > index b300a20..45f5829 100644 > --- a/drivers/hwmon/ads7871.c > +++ b/drivers/hwmon/ads7871.c > @@ -176,12 +176,6 @@ static int __devinit ads7871_probe(struct spi_device *spi) > if (status < 0) > goto error_free; > > - pdata->hwmon_dev = hwmon_device_register(&spi->dev); > - if (IS_ERR(pdata->hwmon_dev)) { > - err = PTR_ERR(pdata->hwmon_dev); > - goto error_remove; > - } > - > spi_set_drvdata(spi, pdata); > > /* Configure the SPI bus */ > @@ -204,6 +198,12 @@ static int __devinit ads7871_probe(struct spi_device *spi) > goto error_remove; > } > > + pdata->hwmon_dev = hwmon_device_register(&spi->dev); > + if (IS_ERR(pdata->hwmon_dev)) { > + err = PTR_ERR(pdata->hwmon_dev); > + goto error_remove; > + } > + > return 0; > > error_remove: Applied, thanks. -- Jean Delvare ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [lm-sensors] [PATCH v2] hwmon: (ads7871) Fix ads7871_probe 2010-08-17 14:10 ` [PATCH v2] hwmon: (ads7871) Fix ads7871_probe error path Jean Delvare @ 2010-08-22 11:58 ` Jean Delvare -1 siblings, 0 replies; 6+ messages in thread From: Jean Delvare @ 2010-08-22 11:58 UTC (permalink / raw) To: Axel Lin, Paul Thomas; +Cc: linux-kernel, lm-sensors Axel, Paul, On Tue, 17 Aug 2010 16:10:48 +0200, Jean Delvare wrote: > On Tue, 17 Aug 2010 20:41:24 +0800, Axel Lin wrote: > > This patch moves hwmon_device_register() to the end of ads7871_probe(). > > So we can make sure the device exists and is properly initialized > > before the device is exposed to user-space. > > > > Signed-off-by: Axel Lin <axel.lin@gmail.com> > > --- > > drivers/hwmon/ads7871.c | 12 ++++++------ > > 1 files changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/hwmon/ads7871.c b/drivers/hwmon/ads7871.c > > index b300a20..45f5829 100644 > > --- a/drivers/hwmon/ads7871.c > > +++ b/drivers/hwmon/ads7871.c > > @@ -176,12 +176,6 @@ static int __devinit ads7871_probe(struct spi_device *spi) > > if (status < 0) > > goto error_free; > > > > - pdata->hwmon_dev = hwmon_device_register(&spi->dev); > > - if (IS_ERR(pdata->hwmon_dev)) { > > - err = PTR_ERR(pdata->hwmon_dev); > > - goto error_remove; > > - } > > - > > spi_set_drvdata(spi, pdata); > > > > /* Configure the SPI bus */ > > @@ -204,6 +198,12 @@ static int __devinit ads7871_probe(struct spi_device *spi) > > goto error_remove; > > } > > > > + pdata->hwmon_dev = hwmon_device_register(&spi->dev); > > + if (IS_ERR(pdata->hwmon_dev)) { > > + err = PTR_ERR(pdata->hwmon_dev); > > + goto error_remove; > > + } > > + > > return 0; > > > > error_remove: > > Applied, thanks. Hmm, looking at the code again as I was about to send the patch to Linus, it seems that the above fix isn't sufficient. One problem is that it makes little sense to create sysfs attributes for the device if the probe is going to fail. So sysfs file creation should be delayed until after the device presence check. A second, more important problem is that no error code is returned if sysfs_create_group() fails. The probe function will return success (0) while the device is not registered, which means trouble when the remove function is called later. The second problem is caused by the presence of two local variables carrying error values: err and status. The latter should be dropped to avoid the confusion, and the former shouldn't be initialized to 0, so that the compiler has a chance to warn about such issues. Axel, Paul, can either of you please send an updated patch as a replacement for this one, fixing the two remaining issues? Thanks. As a side note, I also noticed that struct ads7871_data has an unused member "update_lock". Someone familiar with the device and driver should check whether it should be used somewhere, or if it can be dropped. -- Jean Delvare _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] hwmon: (ads7871) Fix ads7871_probe error path @ 2010-08-22 11:58 ` Jean Delvare 0 siblings, 0 replies; 6+ messages in thread From: Jean Delvare @ 2010-08-22 11:58 UTC (permalink / raw) To: Axel Lin, Paul Thomas; +Cc: linux-kernel, lm-sensors Axel, Paul, On Tue, 17 Aug 2010 16:10:48 +0200, Jean Delvare wrote: > On Tue, 17 Aug 2010 20:41:24 +0800, Axel Lin wrote: > > This patch moves hwmon_device_register() to the end of ads7871_probe(). > > So we can make sure the device exists and is properly initialized > > before the device is exposed to user-space. > > > > Signed-off-by: Axel Lin <axel.lin@gmail.com> > > --- > > drivers/hwmon/ads7871.c | 12 ++++++------ > > 1 files changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/hwmon/ads7871.c b/drivers/hwmon/ads7871.c > > index b300a20..45f5829 100644 > > --- a/drivers/hwmon/ads7871.c > > +++ b/drivers/hwmon/ads7871.c > > @@ -176,12 +176,6 @@ static int __devinit ads7871_probe(struct spi_device *spi) > > if (status < 0) > > goto error_free; > > > > - pdata->hwmon_dev = hwmon_device_register(&spi->dev); > > - if (IS_ERR(pdata->hwmon_dev)) { > > - err = PTR_ERR(pdata->hwmon_dev); > > - goto error_remove; > > - } > > - > > spi_set_drvdata(spi, pdata); > > > > /* Configure the SPI bus */ > > @@ -204,6 +198,12 @@ static int __devinit ads7871_probe(struct spi_device *spi) > > goto error_remove; > > } > > > > + pdata->hwmon_dev = hwmon_device_register(&spi->dev); > > + if (IS_ERR(pdata->hwmon_dev)) { > > + err = PTR_ERR(pdata->hwmon_dev); > > + goto error_remove; > > + } > > + > > return 0; > > > > error_remove: > > Applied, thanks. Hmm, looking at the code again as I was about to send the patch to Linus, it seems that the above fix isn't sufficient. One problem is that it makes little sense to create sysfs attributes for the device if the probe is going to fail. So sysfs file creation should be delayed until after the device presence check. A second, more important problem is that no error code is returned if sysfs_create_group() fails. The probe function will return success (0) while the device is not registered, which means trouble when the remove function is called later. The second problem is caused by the presence of two local variables carrying error values: err and status. The latter should be dropped to avoid the confusion, and the former shouldn't be initialized to 0, so that the compiler has a chance to warn about such issues. Axel, Paul, can either of you please send an updated patch as a replacement for this one, fixing the two remaining issues? Thanks. As a side note, I also noticed that struct ads7871_data has an unused member "update_lock". Someone familiar with the device and driver should check whether it should be used somewhere, or if it can be dropped. -- Jean Delvare ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-08-22 11:58 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-08-17 12:41 [lm-sensors] [PATCH v2] hwmon: (ads7871) Fix ads7871_probe error Axel Lin 2010-08-17 12:41 ` [PATCH v2] hwmon: (ads7871) Fix ads7871_probe error path Axel Lin 2010-08-17 14:10 ` [lm-sensors] [PATCH v2] hwmon: (ads7871) Fix ads7871_probe Jean Delvare 2010-08-17 14:10 ` [PATCH v2] hwmon: (ads7871) Fix ads7871_probe error path Jean Delvare 2010-08-22 11:58 ` [lm-sensors] [PATCH v2] hwmon: (ads7871) Fix ads7871_probe Jean Delvare 2010-08-22 11:58 ` [PATCH v2] hwmon: (ads7871) Fix ads7871_probe error path Jean Delvare
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.