Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753227AbbKZJOe (ORCPT ); Thu, 26 Nov 2015 04:14:34 -0500 Received: from mail-wm0-f53.google.com ([74.125.82.53]:35270 "EHLO mail-wm0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752873AbbKZJO0 (ORCPT ); Thu, 26 Nov 2015 04:14:26 -0500 Date: Thu, 26 Nov 2015 09:14:22 +0000 From: Lee Jones To: Nicolas Boichat Cc: linux-kernel@vger.kernel.org, Javier Martinez Canillas , Olof Johansson , dianders@chromium.org, rspangler@chromium.org, gwendal@chromium.org, linux-spi@vger.kernel.org, Mark Brown Subject: Re: [PATCH v3] mfd: cros ec: Lock the SPI bus while holding chipselect Message-ID: <20151126091422.GR12874@x1> References: <1448430667-4213-1-git-send-email-drinkcat@chromium.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1448430667-4213-1-git-send-email-drinkcat@chromium.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4087 Lines: 132 On Wed, 25 Nov 2015, Nicolas Boichat wrote: > cros_ec_cmd_xfer_spi and cros_ec_pkt_xfer_spi generally work like > this: > - Pull CS down (active), wait a bit, then send a command > - Wait for response (multiple requests) > - Wait a while, pull CS up (inactive) > > These operations, individually, lock the SPI bus, but there is > nothing preventing the SPI framework from interleaving messages > intended for other devices as the bus is unlocked in between. > > This is a problem as the EC expects CS to be held low for the > whole duration. > > Solution: Lock the SPI bus during the whole transaction, to make > sure that no other messages can be interleaved. > > Signed-off-by: Nicolas Boichat > --- > > v2: Move bus_unlock earlier in the functions. > v3: Remove comments. > > Applies on top on linux-next/master (20151124) > > drivers/mfd/cros_ec_spi.c | 30 ++++++++++++++++++------------ > 1 file changed, 18 insertions(+), 12 deletions(-) Applied, thanks. > diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c > index 6a0f6ec..d6af52d 100644 > --- a/drivers/mfd/cros_ec_spi.c > +++ b/drivers/mfd/cros_ec_spi.c > @@ -113,7 +113,7 @@ static int terminate_request(struct cros_ec_device *ec_dev) > trans.delay_usecs = ec_spi->end_of_msg_delay; > spi_message_add_tail(&trans, &msg); > > - ret = spi_sync(ec_spi->spi, &msg); > + ret = spi_sync_locked(ec_spi->spi, &msg); > > /* Reset end-of-response timer */ > ec_spi->last_transfer_ns = ktime_get_ns(); > @@ -147,7 +147,7 @@ static int receive_n_bytes(struct cros_ec_device *ec_dev, u8 *buf, int n) > > spi_message_init(&msg); > spi_message_add_tail(&trans, &msg); > - ret = spi_sync(ec_spi->spi, &msg); > + ret = spi_sync_locked(ec_spi->spi, &msg); > if (ret < 0) > dev_err(ec_dev->dev, "spi transfer failed: %d\n", ret); > > @@ -391,10 +391,10 @@ static int cros_ec_pkt_xfer_spi(struct cros_ec_device *ec_dev, > } > > rx_buf = kzalloc(len, GFP_KERNEL); > - if (!rx_buf) { > - ret = -ENOMEM; > - goto exit; > - } > + if (!rx_buf) > + return -ENOMEM; > + > + spi_bus_lock(ec_spi->spi->master); > > /* > * Leave a gap between CS assertion and clocking of data to allow the > @@ -414,7 +414,7 @@ static int cros_ec_pkt_xfer_spi(struct cros_ec_device *ec_dev, > trans.len = len; > trans.cs_change = 1; > spi_message_add_tail(&trans, &msg); > - ret = spi_sync(ec_spi->spi, &msg); > + ret = spi_sync_locked(ec_spi->spi, &msg); > > /* Get the response */ > if (!ret) { > @@ -440,6 +440,9 @@ static int cros_ec_pkt_xfer_spi(struct cros_ec_device *ec_dev, > } > > final_ret = terminate_request(ec_dev); > + > + spi_bus_unlock(ec_spi->spi->master); > + > if (!ret) > ret = final_ret; > if (ret < 0) > @@ -520,10 +523,10 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev, > } > > rx_buf = kzalloc(len, GFP_KERNEL); > - if (!rx_buf) { > - ret = -ENOMEM; > - goto exit; > - } > + if (!rx_buf) > + return -ENOMEM; > + > + spi_bus_lock(ec_spi->spi->master); > > /* Transmit phase - send our message */ > debug_packet(ec_dev->dev, "out", ec_dev->dout, len); > @@ -534,7 +537,7 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev, > trans.cs_change = 1; > spi_message_init(&msg); > spi_message_add_tail(&trans, &msg); > - ret = spi_sync(ec_spi->spi, &msg); > + ret = spi_sync_locked(ec_spi->spi, &msg); > > /* Get the response */ > if (!ret) { > @@ -560,6 +563,9 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev, > } > > final_ret = terminate_request(ec_dev); > + > + spi_bus_unlock(ec_spi->spi->master); > + > if (!ret) > ret = final_ret; > if (ret < 0) -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/