From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 148CFC43334 for ; Sat, 11 Jun 2022 17:22:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=x4cc1m2ddZQpEY80XwuzB9IRbvoERovNFG8mUjqR/2Q=; b=rjRc6I9S/+lsGy +baFzw8aTr4MY0XE2+wXhmI3zlSbWkZbAanQ4QUT8NvTiCoZuNr4lZzPqotgLyqzlFo3UnvVfGfSo XgxfIt5o2hFjkF1VSfoA1pxtcuVRHk0ANXDQxBWJc+kaw2A9zBFlyicIcBC9C+oanWs5DbLITVpUp ls4zkaHGYMd/CaZlll0nwXVq/+6JavpxsODcPvLPaMAiFl3aaK0/7faMA89bWRAipaK6iUCLyWPei qxpJDxHlDjMDbZ7yt3nZLZIwkjjqv91R2Y8dlz3rK9AOptQ/X+iJ/1O+9DO9RGnBmbR8RiKUBObS9 4eqg6TNkIHwScIoE+dHg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1o04nv-00DwpN-EI; Sat, 11 Jun 2022 17:21:43 +0000 Received: from dfw.source.kernel.org ([2604:1380:4641:c500::1]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1o04nr-00Dwoi-JP for linux-arm-kernel@lists.infradead.org; Sat, 11 Jun 2022 17:21:41 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 0D6EB611BC; Sat, 11 Jun 2022 17:21:39 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 671D2C34116; Sat, 11 Jun 2022 17:21:34 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1654968098; bh=E0kQ1q0EhIyg+oOJ3MBPCoRamwPIBHnxUd7HHhol9uw=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=qg27L7Ed2K61cm6n17/z7Zy/1Tlf9rYo/vI+IlLlk1hPQXaenf2eGSTGir7nOlnEp X+aa1zh+E5VRXUX4SKiIvQfknTud3oQkX2cgG+/bNdW1Gv+zxW+vXLiG0TGA9MCa4h 9a6xjWRpIn6glfNYTN0XHwoR5AXtVEofNQlLLwlB4OT6peThZDxbt8mGkshHd68+uj ruvTCn9PpJEMKP9kPvflRbbA3zalYas3+vmueInEQrGHtlMabMzv55OOeyj45C531R 8mnfBO0YUB2780zWnRPzUrRp/EdRwL56uSK6bJfDdyrYJ9CERlVBWnejZd4BVbJm3K IBBWEQWGjFLrA== Date: Sat, 11 Jun 2022 18:30:46 +0100 From: Jonathan Cameron To: Claudiu Beznea Subject: Re: [PATCH 02/16] iio: adc: at91-sama5d2_adc: lock around oversampling and sample freq Message-ID: <20220611183046.5515c001@jic23-huawei> In-Reply-To: <20220609083213.1795019-3-claudiu.beznea@microchip.com> References: <20220609083213.1795019-1-claudiu.beznea@microchip.com> <20220609083213.1795019-3-claudiu.beznea@microchip.com> X-Mailer: Claws Mail 4.1.0 (GTK 3.24.34; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220611_102139_762513_FA98A5FF X-CRM114-Status: GOOD ( 27.39 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: devicetree@vger.kernel.org, alexandre.belloni@bootlin.com, lars@metafoo.de, ludovic.desroches@atmel.com, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, robh+dt@kernel.org, eugen.hristev@microchip.com, krzk+dt@kernel.org, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, 9 Jun 2022 11:31:59 +0300 Claudiu Beznea wrote: > .read_raw()/.write_raw() could be called asynchronously from user space > or other in kernel drivers. Without locking on st->lock these could be > called asynchronously while there is a conversion in progress. Read will > be harmless but changing registers while conversion is in progress may > lead to inconsistent results. Thus, to avoid this lock st->lock. The patch makes sense, but I'm not convinced all of the changes below involve any changes to registers. E.g. at91_adc_adjust_val_osr() is using the cached value of something in a register, but not the register itself, so please update the description to mention cached state. Other comments inline. > > Fixes: 27e177190891 ("iio:adc:at91_adc8xx: introduce new atmel adc driver") > Fixes: 6794e23fa3fe ("iio: adc: at91-sama5d2_adc: add support for oversampling resolution") > Signed-off-by: Claudiu Beznea > --- > drivers/iio/adc/at91-sama5d2_adc.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c > index 32b6f157b803..a672a520cdc0 100644 > --- a/drivers/iio/adc/at91-sama5d2_adc.c > +++ b/drivers/iio/adc/at91-sama5d2_adc.c > @@ -1542,10 +1542,11 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev, > ret = at91_adc_read_position(st, chan->channel, > &tmp_val); > *val = tmp_val; > + ret = at91_adc_adjust_val_osr(st, val); > mutex_unlock(&st->lock); > iio_device_release_direct_mode(indio_dev); > > - return at91_adc_adjust_val_osr(st, val); > + return ret; > } > if (chan->type == IIO_PRESSURE) { > ret = iio_device_claim_direct_mode(indio_dev); > @@ -1556,10 +1557,11 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev, > ret = at91_adc_read_pressure(st, chan->channel, > &tmp_val); > *val = tmp_val; > + ret = at91_adc_adjust_val_osr(st, val); > mutex_unlock(&st->lock); > iio_device_release_direct_mode(indio_dev); > > - return at91_adc_adjust_val_osr(st, val); > + return ret; > } > > /* in this case we have a voltage channel */ > @@ -1620,11 +1622,15 @@ static int at91_adc_read_raw(struct iio_dev *indio_dev, > return IIO_VAL_FRACTIONAL_LOG2; > > case IIO_CHAN_INFO_SAMP_FREQ: > + mutex_lock(&st->lock); > *val = at91_adc_get_sample_freq(st); So this is a straight read of a cached value. The only thing you 'might' arguably be protecting against is read/write tearing due to it in theory being possible to write part of the value whilst reading. I don't see that being a concern for st->current_sample_rate > + mutex_unlock(&st->lock); > return IIO_VAL_INT; > > case IIO_CHAN_INFO_OVERSAMPLING_RATIO: > + mutex_lock(&st->lock); > *val = st->oversampling_ratio; Likewise, what are you protecting against racing with this that can't just occur before or after the lock? > + mutex_unlock(&st->lock); > return IIO_VAL_INT; > > default: > @@ -1644,18 +1650,23 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev, > (val != AT91_OSR_16SAMPLES)) > return -EINVAL; > /* if no change, optimize out */ > + mutex_lock(&st->lock); > if (val == st->oversampling_ratio) > - return 0; It should be race free to check this outside the lock. Definitely valid to lock around the cached value write and the config write though. > + goto unlock; If you did want to have locking as now then flip the logic if (val != st->oversampling_ratio) { st->oversampling_ratio = val; at91_adc_config_emr(st); } mutex_unlock() .. Goto always have a cost in readability so if you can avoid them with a simple flip of logic like this it is usually a good idea. (exception is error code which should always be out of line as that is more common so what we expect to see). > st->oversampling_ratio = val; > /* update ratio */ > at91_adc_config_emr(st); > +unlock: > + mutex_unlock(&st->lock); > return 0; > case IIO_CHAN_INFO_SAMP_FREQ: > if (val < st->soc_info.min_sample_rate || > val > st->soc_info.max_sample_rate) > return -EINVAL; > > + mutex_lock(&st->lock); > at91_adc_setup_samp_freq(indio_dev, val); > + mutex_unlock(&st->lock); > return 0; > default: > return -EINVAL; _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel