chrome-platform.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/7] platform/chrome: get rid of BUG_ON()
@ 2022-05-13  4:41 Tzung-Bi Shih
  2022-05-13  4:41 ` [PATCH v2 1/7] platform/chrome: cros_ec_proto: drop unneeded BUG_ON() in prepare_packet() Tzung-Bi Shih
                   ` (8 more replies)
  0 siblings, 9 replies; 12+ messages in thread
From: Tzung-Bi Shih @ 2022-05-13  4:41 UTC (permalink / raw)
  To: bleung, groeck; +Cc: chrome-platform, tzungbi, linux-kernel

The series gets rid of BUG_ON()s in drivers/platform/chrome/.  Most of them
can be replaced by returning proper return code.

The 2nd patch makes callers of cros_ec_prepare_tx() to take care of the
return code.

The 3rd patch turns cros_ec_prepare_tx() to return error code if any.

Changes from v1:
(https://patchwork.kernel.org/project/chrome-platform/cover/20220512083627.885338-1-tzungbi@kernel.org/)
- Split the original 6th patch into 2 smaller patches. 

Tzung-Bi Shih (7):
  platform/chrome: cros_ec_proto: drop unneeded BUG_ON() in
    prepare_packet()
  platform/chrome: correct cros_ec_prepare_tx() usage
  platform/chrome: cros_ec_proto: drop BUG_ON() in cros_ec_prepare_tx()
  platform/chrome: cros_ec_proto: drop BUG_ON() in
    cros_ec_get_host_event()
  platform/chrome: cros_ec_i2c: drop BUG_ON() in cros_ec_pkt_xfer_i2c()
  platform/chrome: cros_ec_spi: drop unneeded BUG_ON()
  platform/chrome: cros_ec_spi: drop BUG_ON() if `din` isn't large
    enough

 drivers/platform/chrome/cros_ec_i2c.c   | 12 ++++++++++--
 drivers/platform/chrome/cros_ec_ishtp.c |  4 +++-
 drivers/platform/chrome/cros_ec_lpc.c   |  2 ++
 drivers/platform/chrome/cros_ec_proto.c | 13 ++++++++-----
 drivers/platform/chrome/cros_ec_rpmsg.c |  2 ++
 drivers/platform/chrome/cros_ec_spi.c   | 15 ++++++++++-----
 6 files changed, 35 insertions(+), 13 deletions(-)

-- 
2.36.0.550.gb090851708-goog


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 1/7] platform/chrome: cros_ec_proto: drop unneeded BUG_ON() in prepare_packet()
  2022-05-13  4:41 [PATCH v2 0/7] platform/chrome: get rid of BUG_ON() Tzung-Bi Shih
@ 2022-05-13  4:41 ` Tzung-Bi Shih
  2022-05-13  4:41 ` [PATCH v2 2/7] platform/chrome: correct cros_ec_prepare_tx() usage Tzung-Bi Shih
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 12+ messages in thread
From: Tzung-Bi Shih @ 2022-05-13  4:41 UTC (permalink / raw)
  To: bleung, groeck; +Cc: chrome-platform, tzungbi, linux-kernel

prepare_packet() gets called if `ec_dev->proto_version` > 2.  For now, it
must be equivalent to EC_HOST_REQUEST_VERSION.

Drop the BUG_ON().

Reviewed-by: Guenter Roeck <groeck@chromium.org>
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
Changes from v1:
- Add R-b tag.

 drivers/platform/chrome/cros_ec_proto.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index ac1419881ff3..db1c8ba43171 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -60,7 +60,6 @@ static int prepare_packet(struct cros_ec_device *ec_dev,
 	int i;
 	u8 csum = 0;
 
-	BUG_ON(ec_dev->proto_version != EC_HOST_REQUEST_VERSION);
 	BUG_ON(msg->outsize + sizeof(*request) > ec_dev->dout_size);
 
 	out = ec_dev->dout;
-- 
2.36.0.550.gb090851708-goog


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 2/7] platform/chrome: correct cros_ec_prepare_tx() usage
  2022-05-13  4:41 [PATCH v2 0/7] platform/chrome: get rid of BUG_ON() Tzung-Bi Shih
  2022-05-13  4:41 ` [PATCH v2 1/7] platform/chrome: cros_ec_proto: drop unneeded BUG_ON() in prepare_packet() Tzung-Bi Shih
@ 2022-05-13  4:41 ` Tzung-Bi Shih
  2022-05-13  4:41 ` [PATCH v2 3/7] platform/chrome: cros_ec_proto: drop BUG_ON() in cros_ec_prepare_tx() Tzung-Bi Shih
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 12+ messages in thread
From: Tzung-Bi Shih @ 2022-05-13  4:41 UTC (permalink / raw)
  To: bleung, groeck; +Cc: chrome-platform, tzungbi, linux-kernel

cros_ec_prepare_tx() returns either:
- >= 0 for number of prepared bytes.
- < 0 for -errno.

Correct the comment and make sure all callers check the return code.

Reviewed-by: Guenter Roeck <groeck@chromium.org>
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
Changes from v1:
- Add R-b tag.

 drivers/platform/chrome/cros_ec_i2c.c   | 2 ++
 drivers/platform/chrome/cros_ec_ishtp.c | 4 +++-
 drivers/platform/chrome/cros_ec_lpc.c   | 2 ++
 drivers/platform/chrome/cros_ec_proto.c | 2 +-
 drivers/platform/chrome/cros_ec_rpmsg.c | 2 ++
 drivers/platform/chrome/cros_ec_spi.c   | 4 ++++
 6 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_i2c.c b/drivers/platform/chrome/cros_ec_i2c.c
index 22feb0fd4ce7..a4f305f1eb0e 100644
--- a/drivers/platform/chrome/cros_ec_i2c.c
+++ b/drivers/platform/chrome/cros_ec_i2c.c
@@ -89,6 +89,8 @@ static int cros_ec_pkt_xfer_i2c(struct cros_ec_device *ec_dev,
 
 	ec_dev->dout++;
 	ret = cros_ec_prepare_tx(ec_dev, msg);
+	if (ret < 0)
+		goto done;
 	ec_dev->dout--;
 
 	/* send command to EC and read answer */
diff --git a/drivers/platform/chrome/cros_ec_ishtp.c b/drivers/platform/chrome/cros_ec_ishtp.c
index 4020b8354bae..cb2031cf7106 100644
--- a/drivers/platform/chrome/cros_ec_ishtp.c
+++ b/drivers/platform/chrome/cros_ec_ishtp.c
@@ -521,7 +521,9 @@ static int cros_ec_pkt_xfer_ish(struct cros_ec_device *ec_dev,
 	out_msg->hdr.status = 0;
 
 	ec_dev->dout += OUT_MSG_EC_REQUEST_PREAMBLE;
-	cros_ec_prepare_tx(ec_dev, msg);
+	rv = cros_ec_prepare_tx(ec_dev, msg);
+	if (rv < 0)
+		goto end_error;
 	ec_dev->dout -= OUT_MSG_EC_REQUEST_PREAMBLE;
 
 	dev_dbg(dev,
diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index 8eeef85a96b1..7677ab3c0ead 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -147,6 +147,8 @@ static int cros_ec_pkt_xfer_lpc(struct cros_ec_device *ec,
 	u8 *dout;
 
 	ret = cros_ec_prepare_tx(ec, msg);
+	if (ret < 0)
+		goto done;
 
 	/* Write buffer */
 	cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_PACKET, ret, ec->dout);
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index db1c8ba43171..2d6d3fbfa905 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -164,7 +164,7 @@ static int send_command(struct cros_ec_device *ec_dev,
  * only SPI uses it. Once LPC uses the same protocol it can start using it.
  * I2C could use it now, with a refactor of the existing code.
  *
- * Return: 0 on success or negative error code.
+ * Return: number of prepared bytes on success or negative error code.
  */
 int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
 		       struct cros_ec_command *msg)
diff --git a/drivers/platform/chrome/cros_ec_rpmsg.c b/drivers/platform/chrome/cros_ec_rpmsg.c
index d96d15b8ca94..39d3b50a7c09 100644
--- a/drivers/platform/chrome/cros_ec_rpmsg.c
+++ b/drivers/platform/chrome/cros_ec_rpmsg.c
@@ -89,6 +89,8 @@ static int cros_ec_pkt_xfer_rpmsg(struct cros_ec_device *ec_dev,
 
 	ec_msg->result = 0;
 	len = cros_ec_prepare_tx(ec_dev, ec_msg);
+	if (len < 0)
+		return len;
 	dev_dbg(ec_dev->dev, "prepared, len=%d\n", len);
 
 	reinit_completion(&ec_rpmsg->xfer_ack);
diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
index 8493af0f680e..589f18e9537d 100644
--- a/drivers/platform/chrome/cros_ec_spi.c
+++ b/drivers/platform/chrome/cros_ec_spi.c
@@ -401,6 +401,8 @@ static int do_cros_ec_pkt_xfer_spi(struct cros_ec_device *ec_dev,
 	unsigned long delay;
 
 	len = cros_ec_prepare_tx(ec_dev, ec_msg);
+	if (len < 0)
+		return len;
 	dev_dbg(ec_dev->dev, "prepared, len=%d\n", len);
 
 	/* If it's too soon to do another transaction, wait */
@@ -544,6 +546,8 @@ static int do_cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
 	unsigned long delay;
 
 	len = cros_ec_prepare_tx(ec_dev, ec_msg);
+	if (len < 0)
+		return len;
 	dev_dbg(ec_dev->dev, "prepared, len=%d\n", len);
 
 	/* If it's too soon to do another transaction, wait */
-- 
2.36.0.550.gb090851708-goog


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 3/7] platform/chrome: cros_ec_proto: drop BUG_ON() in cros_ec_prepare_tx()
  2022-05-13  4:41 [PATCH v2 0/7] platform/chrome: get rid of BUG_ON() Tzung-Bi Shih
  2022-05-13  4:41 ` [PATCH v2 1/7] platform/chrome: cros_ec_proto: drop unneeded BUG_ON() in prepare_packet() Tzung-Bi Shih
  2022-05-13  4:41 ` [PATCH v2 2/7] platform/chrome: correct cros_ec_prepare_tx() usage Tzung-Bi Shih
@ 2022-05-13  4:41 ` Tzung-Bi Shih
  2022-05-13  4:41 ` [PATCH v2 4/7] platform/chrome: cros_ec_proto: drop BUG_ON() in cros_ec_get_host_event() Tzung-Bi Shih
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 12+ messages in thread
From: Tzung-Bi Shih @ 2022-05-13  4:41 UTC (permalink / raw)
  To: bleung, groeck; +Cc: chrome-platform, tzungbi, linux-kernel

It is overkill to crash the kernel if the given message is oversize.

Drop the BUG_ON() and return -EINVAL instead.

Reviewed-by: Guenter Roeck <groeck@chromium.org>
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
Changes from v1:
- Add R-b tag.

 drivers/platform/chrome/cros_ec_proto.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 2d6d3fbfa905..9ce3374846ff 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -60,7 +60,8 @@ static int prepare_packet(struct cros_ec_device *ec_dev,
 	int i;
 	u8 csum = 0;
 
-	BUG_ON(msg->outsize + sizeof(*request) > ec_dev->dout_size);
+	if (msg->outsize + sizeof(*request) > ec_dev->dout_size)
+		return -EINVAL;
 
 	out = ec_dev->dout;
 	request = (struct ec_host_request *)out;
@@ -176,7 +177,9 @@ int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
 	if (ec_dev->proto_version > 2)
 		return prepare_packet(ec_dev, msg);
 
-	BUG_ON(msg->outsize > EC_PROTO2_MAX_PARAM_SIZE);
+	if (msg->outsize > EC_PROTO2_MAX_PARAM_SIZE)
+		return -EINVAL;
+
 	out = ec_dev->dout;
 	out[0] = EC_CMD_VERSION0 + msg->version;
 	out[1] = msg->command;
-- 
2.36.0.550.gb090851708-goog


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 4/7] platform/chrome: cros_ec_proto: drop BUG_ON() in cros_ec_get_host_event()
  2022-05-13  4:41 [PATCH v2 0/7] platform/chrome: get rid of BUG_ON() Tzung-Bi Shih
                   ` (2 preceding siblings ...)
  2022-05-13  4:41 ` [PATCH v2 3/7] platform/chrome: cros_ec_proto: drop BUG_ON() in cros_ec_prepare_tx() Tzung-Bi Shih
@ 2022-05-13  4:41 ` Tzung-Bi Shih
  2022-05-13  4:41 ` [PATCH v2 5/7] platform/chrome: cros_ec_i2c: drop BUG_ON() in cros_ec_pkt_xfer_i2c() Tzung-Bi Shih
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 12+ messages in thread
From: Tzung-Bi Shih @ 2022-05-13  4:41 UTC (permalink / raw)
  To: bleung, groeck; +Cc: chrome-platform, tzungbi, linux-kernel

It is overkill to crash the kernel if the `ec_dev` doesn't support MKBP
event but gets called into cros_ec_get_host_event().

Drop the BUG_ON() and return error (0 in the case) instead.

Reviewed-by: Guenter Roeck <groeck@chromium.org>
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
Changes from v1:
- Add R-b tag.

 drivers/platform/chrome/cros_ec_proto.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 9ce3374846ff..ff767dccdf0f 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -817,7 +817,8 @@ u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev)
 {
 	u32 host_event;
 
-	BUG_ON(!ec_dev->mkbp_event_supported);
+	if (!ec_dev->mkbp_event_supported)
+		return 0;
 
 	if (ec_dev->event_data.event_type != EC_MKBP_EVENT_HOST_EVENT)
 		return 0;
-- 
2.36.0.550.gb090851708-goog


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 5/7] platform/chrome: cros_ec_i2c: drop BUG_ON() in cros_ec_pkt_xfer_i2c()
  2022-05-13  4:41 [PATCH v2 0/7] platform/chrome: get rid of BUG_ON() Tzung-Bi Shih
                   ` (3 preceding siblings ...)
  2022-05-13  4:41 ` [PATCH v2 4/7] platform/chrome: cros_ec_proto: drop BUG_ON() in cros_ec_get_host_event() Tzung-Bi Shih
@ 2022-05-13  4:41 ` Tzung-Bi Shih
  2022-05-13  4:41 ` [PATCH v2 6/7] platform/chrome: cros_ec_spi: drop unneeded BUG_ON() Tzung-Bi Shih
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 12+ messages in thread
From: Tzung-Bi Shih @ 2022-05-13  4:41 UTC (permalink / raw)
  To: bleung, groeck; +Cc: chrome-platform, tzungbi, linux-kernel

It is overkill to crash the kernel if the given message is oversize.

Drop the BUG_ON() and return -EINVAL instead.

Reviewed-by: Guenter Roeck <groeck@chromium.org>
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
Changes from v1:
- Add R-b tag.

 drivers/platform/chrome/cros_ec_i2c.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_i2c.c b/drivers/platform/chrome/cros_ec_i2c.c
index a4f305f1eb0e..9f5b95763173 100644
--- a/drivers/platform/chrome/cros_ec_i2c.c
+++ b/drivers/platform/chrome/cros_ec_i2c.c
@@ -72,13 +72,19 @@ static int cros_ec_pkt_xfer_i2c(struct cros_ec_device *ec_dev,
 	i2c_msg[1].flags = I2C_M_RD;
 
 	packet_len = msg->insize + response_header_size;
-	BUG_ON(packet_len > ec_dev->din_size);
+	if (packet_len > ec_dev->din_size) {
+		ret = -EINVAL;
+		goto done;
+	}
 	in_buf = ec_dev->din;
 	i2c_msg[1].len = packet_len;
 	i2c_msg[1].buf = (char *) in_buf;
 
 	packet_len = msg->outsize + request_header_size;
-	BUG_ON(packet_len > ec_dev->dout_size);
+	if (packet_len > ec_dev->dout_size) {
+		ret = -EINVAL;
+		goto done;
+	}
 	out_buf = ec_dev->dout;
 	i2c_msg[0].len = packet_len;
 	i2c_msg[0].buf = (char *) out_buf;
-- 
2.36.0.550.gb090851708-goog


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 6/7] platform/chrome: cros_ec_spi: drop unneeded BUG_ON()
  2022-05-13  4:41 [PATCH v2 0/7] platform/chrome: get rid of BUG_ON() Tzung-Bi Shih
                   ` (4 preceding siblings ...)
  2022-05-13  4:41 ` [PATCH v2 5/7] platform/chrome: cros_ec_i2c: drop BUG_ON() in cros_ec_pkt_xfer_i2c() Tzung-Bi Shih
@ 2022-05-13  4:41 ` Tzung-Bi Shih
  2022-05-13 13:19   ` Guenter Roeck
  2022-05-13  4:41 ` [PATCH v2 7/7] platform/chrome: cros_ec_spi: drop BUG_ON() if `din` isn't large enough Tzung-Bi Shih
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 12+ messages in thread
From: Tzung-Bi Shih @ 2022-05-13  4:41 UTC (permalink / raw)
  To: bleung, groeck; +Cc: chrome-platform, tzungbi, linux-kernel

In the context, the following conditions are always false:

- `todo` < 0
Suppose that EC_SPI_FRAME_START is found at the last byte of transfer.

In the case, `ptr` == `end` - 1.  As a result, `todo` must be 0.

- `todo` > `ec_dev->din_size`
Suppose that there is no preamble bytes.  EC_SPI_FRAME_START is found at
the first byte of transfer.

In the case, `end` == `ptr` + EC_MSG_PREAMBLE_COUNT.
As a result, `todo` == EC_MSG_PREAMBLE_COUNT - 1.
However, it already checked `ec_dev->din_size` < EC_MSG_PREAMBLE_COUNT at
the beginning of function.

Drop the unneeded BUG_ON().

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
Changes from v1:
- Separate from the original 6th patch.
- Drop the BUG_ON() instead of returning -EINVAL.

 drivers/platform/chrome/cros_ec_spi.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
index 589f18e9537d..5264615f46af 100644
--- a/drivers/platform/chrome/cros_ec_spi.c
+++ b/drivers/platform/chrome/cros_ec_spi.c
@@ -237,7 +237,6 @@ static int cros_ec_spi_receive_packet(struct cros_ec_device *ec_dev,
 	 * start of our buffer
 	 */
 	todo = end - ++ptr;
-	BUG_ON(todo < 0 || todo > ec_dev->din_size);
 	todo = min(todo, need_len);
 	memmove(ec_dev->din, ptr, todo);
 	ptr = ec_dev->din + todo;
@@ -345,7 +344,6 @@ static int cros_ec_spi_receive_response(struct cros_ec_device *ec_dev,
 	 * start of our buffer
 	 */
 	todo = end - ++ptr;
-	BUG_ON(todo < 0 || todo > ec_dev->din_size);
 	todo = min(todo, need_len);
 	memmove(ec_dev->din, ptr, todo);
 	ptr = ec_dev->din + todo;
-- 
2.36.0.550.gb090851708-goog


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 7/7] platform/chrome: cros_ec_spi: drop BUG_ON() if `din` isn't large enough
  2022-05-13  4:41 [PATCH v2 0/7] platform/chrome: get rid of BUG_ON() Tzung-Bi Shih
                   ` (5 preceding siblings ...)
  2022-05-13  4:41 ` [PATCH v2 6/7] platform/chrome: cros_ec_spi: drop unneeded BUG_ON() Tzung-Bi Shih
@ 2022-05-13  4:41 ` Tzung-Bi Shih
  2022-05-13 13:20   ` Guenter Roeck
  2022-05-16  2:10 ` [PATCH v2 0/7] platform/chrome: get rid of BUG_ON() patchwork-bot+chrome-platform
  2022-05-18  5:20 ` patchwork-bot+chrome-platform
  8 siblings, 1 reply; 12+ messages in thread
From: Tzung-Bi Shih @ 2022-05-13  4:41 UTC (permalink / raw)
  To: bleung, groeck; +Cc: chrome-platform, tzungbi, linux-kernel

It is overkill to crash the kernel if the `din` buffer is going to full
or overflow.

Drop the BUG_ON() and return -EINVAL instead.

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
Changes from v1:
- Separate from the original 6th patch.

 drivers/platform/chrome/cros_ec_spi.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
index 5264615f46af..7360b3ff6e4f 100644
--- a/drivers/platform/chrome/cros_ec_spi.c
+++ b/drivers/platform/chrome/cros_ec_spi.c
@@ -160,7 +160,8 @@ static int receive_n_bytes(struct cros_ec_device *ec_dev, u8 *buf, int n)
 	struct spi_message msg;
 	int ret;
 
-	BUG_ON(buf - ec_dev->din + n > ec_dev->din_size);
+	if (buf - ec_dev->din + n > ec_dev->din_size)
+		return -EINVAL;
 
 	memset(&trans, 0, sizeof(trans));
 	trans.cs_change = 1;
@@ -197,7 +198,8 @@ static int cros_ec_spi_receive_packet(struct cros_ec_device *ec_dev,
 	unsigned long deadline;
 	int todo;
 
-	BUG_ON(ec_dev->din_size < EC_MSG_PREAMBLE_COUNT);
+	if (ec_dev->din_size < EC_MSG_PREAMBLE_COUNT)
+		return -EINVAL;
 
 	/* Receive data until we see the header byte */
 	deadline = jiffies + msecs_to_jiffies(EC_MSG_DEADLINE_MS);
@@ -304,7 +306,8 @@ static int cros_ec_spi_receive_response(struct cros_ec_device *ec_dev,
 	unsigned long deadline;
 	int todo;
 
-	BUG_ON(ec_dev->din_size < EC_MSG_PREAMBLE_COUNT);
+	if (ec_dev->din_size < EC_MSG_PREAMBLE_COUNT)
+		return -EINVAL;
 
 	/* Receive data until we see the header byte */
 	deadline = jiffies + msecs_to_jiffies(EC_MSG_DEADLINE_MS);
-- 
2.36.0.550.gb090851708-goog


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 6/7] platform/chrome: cros_ec_spi: drop unneeded BUG_ON()
  2022-05-13  4:41 ` [PATCH v2 6/7] platform/chrome: cros_ec_spi: drop unneeded BUG_ON() Tzung-Bi Shih
@ 2022-05-13 13:19   ` Guenter Roeck
  0 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2022-05-13 13:19 UTC (permalink / raw)
  To: Tzung-Bi Shih; +Cc: Benson Leung, Guenter Roeck, chrome-platform, linux-kernel

On Thu, May 12, 2022 at 9:42 PM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> In the context, the following conditions are always false:
>
> - `todo` < 0
> Suppose that EC_SPI_FRAME_START is found at the last byte of transfer.
>
> In the case, `ptr` == `end` - 1.  As a result, `todo` must be 0.
>
> - `todo` > `ec_dev->din_size`
> Suppose that there is no preamble bytes.  EC_SPI_FRAME_START is found at
> the first byte of transfer.
>
> In the case, `end` == `ptr` + EC_MSG_PREAMBLE_COUNT.
> As a result, `todo` == EC_MSG_PREAMBLE_COUNT - 1.
> However, it already checked `ec_dev->din_size` < EC_MSG_PREAMBLE_COUNT at
> the beginning of function.
>
> Drop the unneeded BUG_ON().
>
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>

Reviewed-by: Guenter Roeck <groeck@chromium.org>

> ---
> Changes from v1:
> - Separate from the original 6th patch.
> - Drop the BUG_ON() instead of returning -EINVAL.
>
>  drivers/platform/chrome/cros_ec_spi.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
> index 589f18e9537d..5264615f46af 100644
> --- a/drivers/platform/chrome/cros_ec_spi.c
> +++ b/drivers/platform/chrome/cros_ec_spi.c
> @@ -237,7 +237,6 @@ static int cros_ec_spi_receive_packet(struct cros_ec_device *ec_dev,
>          * start of our buffer
>          */
>         todo = end - ++ptr;
> -       BUG_ON(todo < 0 || todo > ec_dev->din_size);
>         todo = min(todo, need_len);
>         memmove(ec_dev->din, ptr, todo);
>         ptr = ec_dev->din + todo;
> @@ -345,7 +344,6 @@ static int cros_ec_spi_receive_response(struct cros_ec_device *ec_dev,
>          * start of our buffer
>          */
>         todo = end - ++ptr;
> -       BUG_ON(todo < 0 || todo > ec_dev->din_size);
>         todo = min(todo, need_len);
>         memmove(ec_dev->din, ptr, todo);
>         ptr = ec_dev->din + todo;
> --
> 2.36.0.550.gb090851708-goog
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 7/7] platform/chrome: cros_ec_spi: drop BUG_ON() if `din` isn't large enough
  2022-05-13  4:41 ` [PATCH v2 7/7] platform/chrome: cros_ec_spi: drop BUG_ON() if `din` isn't large enough Tzung-Bi Shih
@ 2022-05-13 13:20   ` Guenter Roeck
  0 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2022-05-13 13:20 UTC (permalink / raw)
  To: Tzung-Bi Shih; +Cc: Benson Leung, Guenter Roeck, chrome-platform, linux-kernel

On Thu, May 12, 2022 at 9:42 PM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> It is overkill to crash the kernel if the `din` buffer is going to full
> or overflow.
>
> Drop the BUG_ON() and return -EINVAL instead.
>
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>

Reviewed-by: Guenter Roeck <groeck@chromium.org>

> ---
> Changes from v1:
> - Separate from the original 6th patch.
>
>  drivers/platform/chrome/cros_ec_spi.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
> index 5264615f46af..7360b3ff6e4f 100644
> --- a/drivers/platform/chrome/cros_ec_spi.c
> +++ b/drivers/platform/chrome/cros_ec_spi.c
> @@ -160,7 +160,8 @@ static int receive_n_bytes(struct cros_ec_device *ec_dev, u8 *buf, int n)
>         struct spi_message msg;
>         int ret;
>
> -       BUG_ON(buf - ec_dev->din + n > ec_dev->din_size);
> +       if (buf - ec_dev->din + n > ec_dev->din_size)
> +               return -EINVAL;
>
>         memset(&trans, 0, sizeof(trans));
>         trans.cs_change = 1;
> @@ -197,7 +198,8 @@ static int cros_ec_spi_receive_packet(struct cros_ec_device *ec_dev,
>         unsigned long deadline;
>         int todo;
>
> -       BUG_ON(ec_dev->din_size < EC_MSG_PREAMBLE_COUNT);
> +       if (ec_dev->din_size < EC_MSG_PREAMBLE_COUNT)
> +               return -EINVAL;
>
>         /* Receive data until we see the header byte */
>         deadline = jiffies + msecs_to_jiffies(EC_MSG_DEADLINE_MS);
> @@ -304,7 +306,8 @@ static int cros_ec_spi_receive_response(struct cros_ec_device *ec_dev,
>         unsigned long deadline;
>         int todo;
>
> -       BUG_ON(ec_dev->din_size < EC_MSG_PREAMBLE_COUNT);
> +       if (ec_dev->din_size < EC_MSG_PREAMBLE_COUNT)
> +               return -EINVAL;
>
>         /* Receive data until we see the header byte */
>         deadline = jiffies + msecs_to_jiffies(EC_MSG_DEADLINE_MS);
> --
> 2.36.0.550.gb090851708-goog
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/7] platform/chrome: get rid of BUG_ON()
  2022-05-13  4:41 [PATCH v2 0/7] platform/chrome: get rid of BUG_ON() Tzung-Bi Shih
                   ` (6 preceding siblings ...)
  2022-05-13  4:41 ` [PATCH v2 7/7] platform/chrome: cros_ec_spi: drop BUG_ON() if `din` isn't large enough Tzung-Bi Shih
@ 2022-05-16  2:10 ` patchwork-bot+chrome-platform
  2022-05-18  5:20 ` patchwork-bot+chrome-platform
  8 siblings, 0 replies; 12+ messages in thread
From: patchwork-bot+chrome-platform @ 2022-05-16  2:10 UTC (permalink / raw)
  To: Tzung-Bi Shih; +Cc: bleung, groeck, chrome-platform, linux-kernel

Hello:

This series was applied to chrome-platform/linux.git (for-kernelci)
by Tzung-Bi Shih <tzungbi@kernel.org>:

On Fri, 13 May 2022 12:41:36 +0800 you wrote:
> The series gets rid of BUG_ON()s in drivers/platform/chrome/.  Most of them
> can be replaced by returning proper return code.
> 
> The 2nd patch makes callers of cros_ec_prepare_tx() to take care of the
> return code.
> 
> The 3rd patch turns cros_ec_prepare_tx() to return error code if any.
> 
> [...]

Here is the summary with links:
  - [v2,1/7] platform/chrome: cros_ec_proto: drop unneeded BUG_ON() in prepare_packet()
    https://git.kernel.org/chrome-platform/c/42701e7c0cd2
  - [v2,2/7] platform/chrome: correct cros_ec_prepare_tx() usage
    https://git.kernel.org/chrome-platform/c/71d3ae7fb640
  - [v2,3/7] platform/chrome: cros_ec_proto: drop BUG_ON() in cros_ec_prepare_tx()
    https://git.kernel.org/chrome-platform/c/c2dcb1b06053
  - [v2,4/7] platform/chrome: cros_ec_proto: drop BUG_ON() in cros_ec_get_host_event()
    https://git.kernel.org/chrome-platform/c/20a264c97bc8
  - [v2,5/7] platform/chrome: cros_ec_i2c: drop BUG_ON() in cros_ec_pkt_xfer_i2c()
    https://git.kernel.org/chrome-platform/c/8bff946c4199
  - [v2,6/7] platform/chrome: cros_ec_spi: drop unneeded BUG_ON()
    https://git.kernel.org/chrome-platform/c/ddec8e9e90ce
  - [v2,7/7] platform/chrome: cros_ec_spi: drop BUG_ON() if `din` isn't large enough
    https://git.kernel.org/chrome-platform/c/bbd43a37ec7a

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/7] platform/chrome: get rid of BUG_ON()
  2022-05-13  4:41 [PATCH v2 0/7] platform/chrome: get rid of BUG_ON() Tzung-Bi Shih
                   ` (7 preceding siblings ...)
  2022-05-16  2:10 ` [PATCH v2 0/7] platform/chrome: get rid of BUG_ON() patchwork-bot+chrome-platform
@ 2022-05-18  5:20 ` patchwork-bot+chrome-platform
  8 siblings, 0 replies; 12+ messages in thread
From: patchwork-bot+chrome-platform @ 2022-05-18  5:20 UTC (permalink / raw)
  To: Tzung-Bi Shih; +Cc: bleung, groeck, chrome-platform, linux-kernel

Hello:

This series was applied to chrome-platform/linux.git (for-next)
by Tzung-Bi Shih <tzungbi@kernel.org>:

On Fri, 13 May 2022 12:41:36 +0800 you wrote:
> The series gets rid of BUG_ON()s in drivers/platform/chrome/.  Most of them
> can be replaced by returning proper return code.
> 
> The 2nd patch makes callers of cros_ec_prepare_tx() to take care of the
> return code.
> 
> The 3rd patch turns cros_ec_prepare_tx() to return error code if any.
> 
> [...]

Here is the summary with links:
  - [v2,1/7] platform/chrome: cros_ec_proto: drop unneeded BUG_ON() in prepare_packet()
    https://git.kernel.org/chrome-platform/c/42701e7c0cd2
  - [v2,2/7] platform/chrome: correct cros_ec_prepare_tx() usage
    https://git.kernel.org/chrome-platform/c/71d3ae7fb640
  - [v2,3/7] platform/chrome: cros_ec_proto: drop BUG_ON() in cros_ec_prepare_tx()
    https://git.kernel.org/chrome-platform/c/c2dcb1b06053
  - [v2,4/7] platform/chrome: cros_ec_proto: drop BUG_ON() in cros_ec_get_host_event()
    https://git.kernel.org/chrome-platform/c/20a264c97bc8
  - [v2,5/7] platform/chrome: cros_ec_i2c: drop BUG_ON() in cros_ec_pkt_xfer_i2c()
    https://git.kernel.org/chrome-platform/c/8bff946c4199
  - [v2,6/7] platform/chrome: cros_ec_spi: drop unneeded BUG_ON()
    https://git.kernel.org/chrome-platform/c/ddec8e9e90ce
  - [v2,7/7] platform/chrome: cros_ec_spi: drop BUG_ON() if `din` isn't large enough
    https://git.kernel.org/chrome-platform/c/bbd43a37ec7a

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2022-05-18  5:20 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-13  4:41 [PATCH v2 0/7] platform/chrome: get rid of BUG_ON() Tzung-Bi Shih
2022-05-13  4:41 ` [PATCH v2 1/7] platform/chrome: cros_ec_proto: drop unneeded BUG_ON() in prepare_packet() Tzung-Bi Shih
2022-05-13  4:41 ` [PATCH v2 2/7] platform/chrome: correct cros_ec_prepare_tx() usage Tzung-Bi Shih
2022-05-13  4:41 ` [PATCH v2 3/7] platform/chrome: cros_ec_proto: drop BUG_ON() in cros_ec_prepare_tx() Tzung-Bi Shih
2022-05-13  4:41 ` [PATCH v2 4/7] platform/chrome: cros_ec_proto: drop BUG_ON() in cros_ec_get_host_event() Tzung-Bi Shih
2022-05-13  4:41 ` [PATCH v2 5/7] platform/chrome: cros_ec_i2c: drop BUG_ON() in cros_ec_pkt_xfer_i2c() Tzung-Bi Shih
2022-05-13  4:41 ` [PATCH v2 6/7] platform/chrome: cros_ec_spi: drop unneeded BUG_ON() Tzung-Bi Shih
2022-05-13 13:19   ` Guenter Roeck
2022-05-13  4:41 ` [PATCH v2 7/7] platform/chrome: cros_ec_spi: drop BUG_ON() if `din` isn't large enough Tzung-Bi Shih
2022-05-13 13:20   ` Guenter Roeck
2022-05-16  2:10 ` [PATCH v2 0/7] platform/chrome: get rid of BUG_ON() patchwork-bot+chrome-platform
2022-05-18  5:20 ` patchwork-bot+chrome-platform

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).