| Message ID | 1529852721-17828-1-git-send-email-akinobu.mita@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 25 Jun 2018 00:05:21 +0900 Akinobu Mita <akinobu.mita@gmail.com> wrote: > When the buffer is enabled for ina2xx driver, a dedicated kthread is > invoked to capture mesurement data. When the buffer is disabled, the > kthread is stopped. > > However if the kthread gets register access errors, it immediately exits > and when the malfunctional buffer is disabled, the stale task_struct > pointer is accessed as there is no kthread to be stopped. > > A similar issue in the usbip driver is prevented by kthread_get_run and > kthread_stop_put helpers by increasing usage count of the task_struct. > This change applies the same solution. > > Cc: Stefan Brüns <stefan.bruens@rwth-aachen.de> > Cc: Jonathan Cameron <jic23@kernel.org> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> Seems fine, but this is a fix so should have an appropriate fixes tag. Feel free to send one in reply to this thread rather than a v2. Without a fixes tag it can be very hard to know exactly where a patch 'should' apply. I also have little visibility on how important backporting htis issue is. What would actually trigger the issue and is it likely to be seen in the wild? Thanks, Jonathan > --- > drivers/iio/adc/ina2xx-adc.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c > index 0635a79..d123962 100644 > --- a/drivers/iio/adc/ina2xx-adc.c > +++ b/drivers/iio/adc/ina2xx-adc.c > @@ -30,6 +30,7 @@ > #include <linux/module.h> > #include <linux/of_device.h> > #include <linux/regmap.h> > +#include <linux/sched/task.h> > #include <linux/util_macros.h> > > #include <linux/platform_data/ina2xx.h> > @@ -826,6 +827,7 @@ static int ina2xx_buffer_enable(struct iio_dev *indio_dev) > { > struct ina2xx_chip_info *chip = iio_priv(indio_dev); > unsigned int sampling_us = SAMPLING_PERIOD(chip); > + struct task_struct *task; > > dev_dbg(&indio_dev->dev, "Enabling buffer w/ scan_mask %02x, freq = %d, avg =%u\n", > (unsigned int)(*indio_dev->active_scan_mask), > @@ -835,11 +837,17 @@ static int ina2xx_buffer_enable(struct iio_dev *indio_dev) > dev_dbg(&indio_dev->dev, "Async readout mode: %d\n", > chip->allow_async_readout); > > - chip->task = kthread_run(ina2xx_capture_thread, (void *)indio_dev, > - "%s:%d-%uus", indio_dev->name, indio_dev->id, > - sampling_us); > + task = kthread_create(ina2xx_capture_thread, (void *)indio_dev, > + "%s:%d-%uus", indio_dev->name, indio_dev->id, > + sampling_us); > + if (IS_ERR(task)) > + return PTR_ERR(task); > + > + get_task_struct(task); > + wake_up_process(task); > + chip->task = task; > > - return PTR_ERR_OR_ZERO(chip->task); > + return 0; > } > > static int ina2xx_buffer_disable(struct iio_dev *indio_dev) > @@ -848,6 +856,7 @@ static int ina2xx_buffer_disable(struct iio_dev *indio_dev) > > if (chip->task) { > kthread_stop(chip->task); > + put_task_struct(chip->task); > chip->task = NULL; > } > -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018年7月1日(日) 2:40 Jonathan Cameron <jic23@kernel.org>: > > On Mon, 25 Jun 2018 00:05:21 +0900 > Akinobu Mita <akinobu.mita@gmail.com> wrote: > > > When the buffer is enabled for ina2xx driver, a dedicated kthread is > > invoked to capture mesurement data. When the buffer is disabled, the > > kthread is stopped. > > > > However if the kthread gets register access errors, it immediately exits > > and when the malfunctional buffer is disabled, the stale task_struct > > pointer is accessed as there is no kthread to be stopped. > > > > A similar issue in the usbip driver is prevented by kthread_get_run and > > kthread_stop_put helpers by increasing usage count of the task_struct. > > This change applies the same solution. > > > > Cc: Stefan Brüns <stefan.bruens@rwth-aachen.de> > > Cc: Jonathan Cameron <jic23@kernel.org> > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> > Seems fine, but this is a fix so should have an appropriate fixes > tag. Feel free to send one in reply to this thread rather than a v2. It seems like this problem has been there ever since the ina2xx driver was added. Fixes: c43a102e67db ("iio: ina2xx: add support for TI INA2xx Power Monitors") > Without a fixes tag it can be very hard to know exactly where > a patch 'should' apply. I also have little visibility on > how important backporting htis issue is. What would actually trigger > the issue and is it likely to be seen in the wild? This issue was actually triggered in the system with an I2C controller that occasionally malfunctions due to a software bug in the controller driver. BTW, there is no way to notify this error to the process that is calling poll() or read() to the buffer. Maybe we should implement the mechanism for returning POLLHUP or POLLERR events. -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2 Jul 2018 17:44:42 +0900 Akinobu Mita <akinobu.mita@gmail.com> wrote: > 2018年7月1日(日) 2:40 Jonathan Cameron <jic23@kernel.org>: > > > > On Mon, 25 Jun 2018 00:05:21 +0900 > > Akinobu Mita <akinobu.mita@gmail.com> wrote: > > > > > When the buffer is enabled for ina2xx driver, a dedicated kthread is > > > invoked to capture mesurement data. When the buffer is disabled, the > > > kthread is stopped. > > > > > > However if the kthread gets register access errors, it immediately exits > > > and when the malfunctional buffer is disabled, the stale task_struct > > > pointer is accessed as there is no kthread to be stopped. > > > > > > A similar issue in the usbip driver is prevented by kthread_get_run and > > > kthread_stop_put helpers by increasing usage count of the task_struct. > > > This change applies the same solution. > > > > > > Cc: Stefan Brüns <stefan.bruens@rwth-aachen.de> > > > Cc: Jonathan Cameron <jic23@kernel.org> > > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> > > Seems fine, but this is a fix so should have an appropriate fixes > > tag. Feel free to send one in reply to this thread rather than a v2. > > It seems like this problem has been there ever since the ina2xx driver > was added. > > Fixes: c43a102e67db ("iio: ina2xx: add support for TI INA2xx Power Monitors") > Thanks, added. > > Without a fixes tag it can be very hard to know exactly where > > a patch 'should' apply. I also have little visibility on > > how important backporting htis issue is. What would actually trigger > > the issue and is it likely to be seen in the wild? > > This issue was actually triggered in the system with an I2C controller > that occasionally malfunctions due to a software bug in the controller > driver. In that case I'm not going to rush it, but rather will take it via the next merge window. > > BTW, there is no way to notify this error to the process that is calling > poll() or read() to the buffer. Maybe we should implement the mechanism > for returning POLLHUP or POLLERR events. I'm certainly not against adding something like that to notify on error cases. In general, minor hardware failures are not well handled in the kernel - there is no 'standard' way of doing it. Every now and then it is discussed but no one ever takes it any further. Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it. Thanks, Jonathan > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c index 0635a79..d123962 100644 --- a/drivers/iio/adc/ina2xx-adc.c +++ b/drivers/iio/adc/ina2xx-adc.c @@ -30,6 +30,7 @@ #include <linux/module.h> #include <linux/of_device.h> #include <linux/regmap.h> +#include <linux/sched/task.h> #include <linux/util_macros.h> #include <linux/platform_data/ina2xx.h> @@ -826,6 +827,7 @@ static int ina2xx_buffer_enable(struct iio_dev *indio_dev) { struct ina2xx_chip_info *chip = iio_priv(indio_dev); unsigned int sampling_us = SAMPLING_PERIOD(chip); + struct task_struct *task; dev_dbg(&indio_dev->dev, "Enabling buffer w/ scan_mask %02x, freq = %d, avg =%u\n", (unsigned int)(*indio_dev->active_scan_mask), @@ -835,11 +837,17 @@ static int ina2xx_buffer_enable(struct iio_dev *indio_dev) dev_dbg(&indio_dev->dev, "Async readout mode: %d\n", chip->allow_async_readout); - chip->task = kthread_run(ina2xx_capture_thread, (void *)indio_dev, - "%s:%d-%uus", indio_dev->name, indio_dev->id, - sampling_us); + task = kthread_create(ina2xx_capture_thread, (void *)indio_dev, + "%s:%d-%uus", indio_dev->name, indio_dev->id, + sampling_us); + if (IS_ERR(task)) + return PTR_ERR(task); + + get_task_struct(task); + wake_up_process(task); + chip->task = task; - return PTR_ERR_OR_ZERO(chip->task); + return 0; } static int ina2xx_buffer_disable(struct iio_dev *indio_dev) @@ -848,6 +856,7 @@ static int ina2xx_buffer_disable(struct iio_dev *indio_dev) if (chip->task) { kthread_stop(chip->task); + put_task_struct(chip->task); chip->task = NULL; }
When the buffer is enabled for ina2xx driver, a dedicated kthread is invoked to capture mesurement data. When the buffer is disabled, the kthread is stopped. However if the kthread gets register access errors, it immediately exits and when the malfunctional buffer is disabled, the stale task_struct pointer is accessed as there is no kthread to be stopped. A similar issue in the usbip driver is prevented by kthread_get_run and kthread_stop_put helpers by increasing usage count of the task_struct. This change applies the same solution. Cc: Stefan Brüns <stefan.bruens@rwth-aachen.de> Cc: Jonathan Cameron <jic23@kernel.org> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> --- drivers/iio/adc/ina2xx-adc.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)