Re: [PATCH v2] iio: adc: exynos: do not rely on 'users' counter in ISR
On Tue, Oct 06, 2020 at 09:39:07PM +0200, Michał Mirosław wrote: > On Mon, Oct 05, 2020 at 09:12:14PM -0700, dmitry.torok...@gmail.com wrote: > > The order in which 'users' counter is decremented vs calling drivers' > > close() method is implementation specific, and we should not rely on > > it. Let's introduce driver private flag and use it to signal ISR > > to exit when device is being closed. > > > > This has a side-effect of fixing issue of accessing inut->users > > outside of input->mutex protection. > [...] > > Reviewed-by: Michał Mirosław > (after with a fix mentioned below) > > > --- a/drivers/iio/adc/exynos_adc.c > > +++ b/drivers/iio/adc/exynos_adc.c > [...] > > @@ -712,6 +715,7 @@ static int exynos_adc_ts_open(struct input_dev *dev) > > { > > struct exynos_adc *info = input_get_drvdata(dev); > > > > + WRITE_ONCE(info->ts_enabled, true); > > enable_irq(info->tsirq); > > > > return 0; > > @@ -721,6 +725,7 @@ static void exynos_adc_ts_close(struct input_dev *dev) > > { > > struct exynos_adc *info = input_get_drvdata(dev); > > > > + WRITE_ONCE(info->ts_enabled, true); > > disable_irq(info->tsirq); > > Shouldn't 'true' be 'false' here? I swear if we disable cut-n-paste functionality there will be markable reduction in bug rates... Thanks for noticing this! -- Dmitry
Re: [PATCH v2] iio: adc: exynos: do not rely on 'users' counter in ISR
On Mon, Oct 05, 2020 at 09:12:14PM -0700, dmitry.torok...@gmail.com wrote: > The order in which 'users' counter is decremented vs calling drivers' > close() method is implementation specific, and we should not rely on > it. Let's introduce driver private flag and use it to signal ISR > to exit when device is being closed. > > This has a side-effect of fixing issue of accessing inut->users > outside of input->mutex protection. [...] Reviewed-by: Michał Mirosław (after with a fix mentioned below) > --- a/drivers/iio/adc/exynos_adc.c > +++ b/drivers/iio/adc/exynos_adc.c [...] > @@ -712,6 +715,7 @@ static int exynos_adc_ts_open(struct input_dev *dev) > { > struct exynos_adc *info = input_get_drvdata(dev); > > + WRITE_ONCE(info->ts_enabled, true); > enable_irq(info->tsirq); > > return 0; > @@ -721,6 +725,7 @@ static void exynos_adc_ts_close(struct input_dev *dev) > { > struct exynos_adc *info = input_get_drvdata(dev); > > + WRITE_ONCE(info->ts_enabled, true); > disable_irq(info->tsirq); Shouldn't 'true' be 'false' here? Best Regards, Michał Mirosław
Re: [PATCH v2] iio: adc: exynos: do not rely on 'users' counter in ISR
On Tue, 6 Oct 2020 at 06:12, wrote: > > The order in which 'users' counter is decremented vs calling drivers' > close() method is implementation specific, and we should not rely on > it. Let's introduce driver private flag and use it to signal ISR > to exit when device is being closed. > > This has a side-effect of fixing issue of accessing inut->users > outside of input->mutex protection. > > Reported-by: Andrzej Pietrasiewicz > Signed-off-by: Dmitry Torokhov > --- > > v2: switched from ordinary read/write to READ_ONCE/WRITE_ONCE per Michał > Mirosław > > drivers/iio/adc/exynos_adc.c | 7 ++- > 1 file changed, 6 insertions(+), 1 deletion(-) Acked-by: Krzysztof Kozlowski Best regards, Krzysztof