The main transfer function for cros_ec_spi can be called by more than
one client at a time. Make sure that those clients don't stomp on
each other by locking the bus for the duration of the transfer
function.
Signed-off-by: Doug Anderson <[email protected]>
---
drivers/mfd/cros_ec_spi.c | 26 +++++++++++++++++++++-----
1 file changed, 21 insertions(+), 5 deletions(-)
diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c
index c185eb6..a2a605d 100644
--- a/drivers/mfd/cros_ec_spi.c
+++ b/drivers/mfd/cros_ec_spi.c
@@ -65,11 +65,13 @@
* if no record
* @end_of_msg_delay: used to set the delay_usecs on the spi_transfer that
* is sent when we want to turn off CS at the end of a transaction.
+ * @lock: mutex to ensure only one user of cros_ec_command_spi_xfer at a time
*/
struct cros_ec_spi {
struct spi_device *spi;
s64 last_transfer_ns;
unsigned int end_of_msg_delay;
+ struct mutex lock;
};
static void debug_packet(struct device *dev, const char *name, u8 *ptr,
@@ -208,6 +210,13 @@ static int cros_ec_command_spi_xfer(struct cros_ec_device *ec_dev,
int ret = 0, final_ret;
struct timespec ts;
+ /*
+ * We have the shared ec_dev buffer plus we do lots of separate spi_sync
+ * calls, so we need to make sure only one person is using this at a
+ * time.
+ */
+ mutex_lock(&ec_spi->lock);
+
len = cros_ec_prepare_tx(ec_dev, ec_msg);
dev_dbg(ec_dev->dev, "prepared, len=%d\n", len);
@@ -260,7 +269,7 @@ static int cros_ec_command_spi_xfer(struct cros_ec_device *ec_dev,
ret = final_ret;
if (ret < 0) {
dev_err(ec_dev->dev, "spi transfer failed: %d\n", ret);
- return ret;
+ goto exit;
}
/* check response error code */
@@ -269,14 +278,16 @@ static int cros_ec_command_spi_xfer(struct cros_ec_device *ec_dev,
dev_warn(ec_dev->dev, "command 0x%02x returned an error %d\n",
ec_msg->cmd, ptr[0]);
debug_packet(ec_dev->dev, "in_err", ptr, len);
- return -EINVAL;
+ ret = -EINVAL;
+ goto exit;
}
len = ptr[1];
sum = ptr[0] + ptr[1];
if (len > ec_msg->in_len) {
dev_err(ec_dev->dev, "packet too long (%d bytes, expected %d)",
len, ec_msg->in_len);
- return -ENOSPC;
+ ret = -ENOSPC;
+ goto exit;
}
/* copy response packet payload and compute checksum */
@@ -293,10 +304,14 @@ static int cros_ec_command_spi_xfer(struct cros_ec_device *ec_dev,
dev_err(ec_dev->dev,
"bad packet checksum, expected %02x, got %02x\n",
sum, ptr[len + 2]);
- return -EBADMSG;
+ ret = -EBADMSG;
+ goto exit;
}
- return 0;
+ ret = 0;
+exit:
+ mutex_unlock(&ec_spi->lock);
+ return ret;
}
static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)
@@ -327,6 +342,7 @@ static int cros_ec_spi_probe(struct spi_device *spi)
if (ec_spi == NULL)
return -ENOMEM;
ec_spi->spi = spi;
+ mutex_init(&ec_spi->lock);
ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
if (!ec_dev)
return -ENOMEM;
--
1.9.1.423.g4596e3a
Hi Doug,
On 17 April 2014 11:59, Doug Anderson <[email protected]> wrote:
> The main transfer function for cros_ec_spi can be called by more than
> one client at a time. Make sure that those clients don't stomp on
> each other by locking the bus for the duration of the transfer
> function.
Is there no lock at the cros_ec layer?
Regards,
Simon
Simon,
On Fri, Apr 18, 2014 at 10:28 AM, Simon Glass <[email protected]> wrote:
> Hi Doug,
>
> On 17 April 2014 11:59, Doug Anderson <[email protected]> wrote:
>> The main transfer function for cros_ec_spi can be called by more than
>> one client at a time. Make sure that those clients don't stomp on
>> each other by locking the bus for the duration of the transfer
>> function.
>
> Is there no lock at the cros_ec layer?
Not with what's upstream.
Locally in the Chromium OS tree:
* You can see that Bill removed the dev_lock at
<https://chromium-review.googlesource.com/#/c/57051/> since it wasn't
used.
* Andrew moved to a common locking scheme later in
<https://chromium-review.googlesource.com/#/c/170747/> (thus adding
roughly the same lock back and using it), but in order to do that
we've got a dozen pathces in between, some of which are big
reorganizations. This includes at least:
6820b91 CHROMIUM: cros_ec: Tweak struct cros_ec_device for clarity
5e8e562 CHROMIUM: Use struct cros_ec_command to communicate with the EC
9e7db82 CHROMIUM: cleanup: remove unused fields from struct cros_ec_device
866e62d CHROMIUM: cleanup: Remove EC wrapper functions.
8a372b2 cros_ec: move locking into cros_ec_cmd_xfer
981c4aa cros_ec: stop calling ->cmd_xfer() directly
I think we should take in some of those other changes but I'd rather
get correctness in first (since people are wanting to use this driver
in upstream kernel) and get cleanups posted after this lands. I was
also trying not to spam the list with a 20-patch series...
-Doug
Hi Doug,
On 18 April 2014 15:15, Doug Anderson <[email protected]> wrote:
>
> Simon,
>
> On Fri, Apr 18, 2014 at 10:28 AM, Simon Glass <[email protected]> wrote:
> > Hi Doug,
> >
> > On 17 April 2014 11:59, Doug Anderson <[email protected]> wrote:
> >> The main transfer function for cros_ec_spi can be called by more than
> >> one client at a time. Make sure that those clients don't stomp on
> >> each other by locking the bus for the duration of the transfer
> >> function.
> >
> > Is there no lock at the cros_ec layer?
>
> Not with what's upstream.
>
> Locally in the Chromium OS tree:
>
> * You can see that Bill removed the dev_lock at
> <https://chromium-review.googlesource.com/#/c/57051/> since it wasn't
> used.
>
> * Andrew moved to a common locking scheme later in
> <https://chromium-review.googlesource.com/#/c/170747/> (thus adding
> roughly the same lock back and using it), but in order to do that
> we've got a dozen pathces in between, some of which are big
> reorganizations. This includes at least:
>
> 6820b91 CHROMIUM: cros_ec: Tweak struct cros_ec_device for clarity
> 5e8e562 CHROMIUM: Use struct cros_ec_command to communicate with the EC
> 9e7db82 CHROMIUM: cleanup: remove unused fields from struct cros_ec_device
> 866e62d CHROMIUM: cleanup: Remove EC wrapper functions.
> 8a372b2 cros_ec: move locking into cros_ec_cmd_xfer
> 981c4aa cros_ec: stop calling ->cmd_xfer() directly
>
> I think we should take in some of those other changes but I'd rather
> get correctness in first (since people are wanting to use this driver
> in upstream kernel) and get cleanups posted after this lands. I was
> also trying not to spam the list with a 20-patch series...
That explains it, thank you. I should have guessed that for myself.
Reviewed-by: Simon Glass <[email protected]>
Regards,
Simon