[PATCH 0/6] platform/chrome: get rid of BUG_ON()

[PATCH 0/6] platform/chrome: get rid of BUG_ON()

[Tzung-Bi Shih]

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.

Tzung-Bi Shih (6):
  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 BUG_ON()

 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   | 19 ++++++++++++++-----
 6 files changed, 39 insertions(+), 13 deletions(-)

-- 
2.36.0.512.ge40c2bad7a-goog

[PATCH 1/6] platform/chrome: cros_ec_proto: drop unneeded BUG_ON() in prepare_packet()

[Tzung-Bi Shih]

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().

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
 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.512.ge40c2bad7a-goog

Re: [PATCH 1/6] platform/chrome: cros_ec_proto: drop unneeded BUG_ON() in prepare_packet()

[Guenter Roeck]

On Thu, May 12, 2022 at 1:36 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> 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().
>
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>

An alternative would be to return -EPROTO, but given the context this
is just a waste of code space.

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

> ---
>  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.512.ge40c2bad7a-goog
>

[PATCH 2/6] platform/chrome: correct cros_ec_prepare_tx() usage

[Tzung-Bi Shih]

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.

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
 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.512.ge40c2bad7a-goog

Re: [PATCH 2/6] platform/chrome: correct cros_ec_prepare_tx() usage

[Guenter Roeck]

On Thu, May 12, 2022 at 1:36 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> 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.
>
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>

Makes sense in the context of the next patch.

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

> ---
>  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.512.ge40c2bad7a-goog
>

[PATCH 3/6] platform/chrome: cros_ec_proto: drop BUG_ON() in cros_ec_prepare_tx()

[Tzung-Bi Shih]

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

Drop the BUG_ON() and return -EINVAL instead.

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
 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.512.ge40c2bad7a-goog

Re: [PATCH 3/6] platform/chrome: cros_ec_proto: drop BUG_ON() in cros_ec_prepare_tx()

[Guenter Roeck]

On Thu, May 12, 2022 at 1:36 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> It is overkill to crash the kernel if the given message is oversize.
>
> Drop the BUG_ON() and return -EINVAL instead.
>
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>

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

> ---
>  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.512.ge40c2bad7a-goog
>

[PATCH 4/6] platform/chrome: cros_ec_proto: drop BUG_ON() in cros_ec_get_host_event()

[Tzung-Bi Shih]

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.

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
 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.512.ge40c2bad7a-goog

Re: [PATCH 4/6] platform/chrome: cros_ec_proto: drop BUG_ON() in cros_ec_get_host_event()

[Guenter Roeck]

On Thu, May 12, 2022 at 1:36 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> 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.
>
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>

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

> ---
>  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.512.ge40c2bad7a-goog
>

[PATCH 5/6] platform/chrome: cros_ec_i2c: drop BUG_ON() in cros_ec_pkt_xfer_i2c()

[Tzung-Bi Shih]

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

Drop the BUG_ON() and return -EINVAL instead.

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
 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.512.ge40c2bad7a-goog

Re: [PATCH 5/6] platform/chrome: cros_ec_i2c: drop BUG_ON() in cros_ec_pkt_xfer_i2c()

[Guenter Roeck]

On Thu, May 12, 2022 at 1:36 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> It is overkill to crash the kernel if the given message is oversize.
>
> Drop the BUG_ON() and return -EINVAL instead.
>
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>

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

> ---
>  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.512.ge40c2bad7a-goog
>

[PATCH 6/6] platform/chrome: cros_ec_spi: drop BUG_ON()

[Tzung-Bi Shih]

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>
---
 drivers/platform/chrome/cros_ec_spi.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
index 589f18e9537d..0a938f317adc 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);
@@ -237,7 +239,8 @@ 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);
+	if (todo < 0 || todo > ec_dev->din_size)
+		return -EINVAL;
 	todo = min(todo, need_len);
 	memmove(ec_dev->din, ptr, todo);
 	ptr = ec_dev->din + todo;
@@ -305,7 +308,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);
@@ -345,7 +349,8 @@ 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);
+	if (todo < 0 || todo > ec_dev->din_size)
+		return -EINVAL;
 	todo = min(todo, need_len);
 	memmove(ec_dev->din, ptr, todo);
 	ptr = ec_dev->din + todo;
-- 
2.36.0.512.ge40c2bad7a-goog

Re: [PATCH 6/6] platform/chrome: cros_ec_spi: drop BUG_ON()

[Guenter Roeck]

On Thu, May 12, 2022 at 1:37 AM 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>

Wonder if the return code should be -EPROTO instead, but I don't have
a strong opinion.

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

> ---
>  drivers/platform/chrome/cros_ec_spi.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
> index 589f18e9537d..0a938f317adc 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);
> @@ -237,7 +239,8 @@ 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);
> +       if (todo < 0 || todo > ec_dev->din_size)
> +               return -EINVAL;
>         todo = min(todo, need_len);
>         memmove(ec_dev->din, ptr, todo);
>         ptr = ec_dev->din + todo;
> @@ -305,7 +308,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);
> @@ -345,7 +349,8 @@ 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);
> +       if (todo < 0 || todo > ec_dev->din_size)
> +               return -EINVAL;
>         todo = min(todo, need_len);
>         memmove(ec_dev->din, ptr, todo);
>         ptr = ec_dev->din + todo;
> --
> 2.36.0.512.ge40c2bad7a-goog
>

Re: [PATCH 6/6] platform/chrome: cros_ec_spi: drop BUG_ON()

[Tzung-Bi Shih]

On Thu, May 12, 2022 at 08:14:10AM -0700, Guenter Roeck wrote:
> On Thu, May 12, 2022 at 1:37 AM 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>
> 
> Wonder if the return code should be -EPROTO instead, but I don't have
> a strong opinion.

Thanks for the review.  I am going to split the patch into 2 smaller pieces.

For those related to preamble bytes, at the first glance, they could use
-EPROTO.  But, no, they are irrelevant to the protocol.  I would drop them.
See [1].

For those `din` isn't large enough, I would keep using -EINVAL as they look
more like the `ec_dev` didn't configure correctly.  See [2].

[1]: https://patchwork.kernel.org/project/chrome-platform/patch/20220513044143.1045728-7-tzungbi@kernel.org/
[2]: https://patchwork.kernel.org/project/chrome-platform/patch/20220513044143.1045728-8-tzungbi@kernel.org/