Re: [bug report] platform/chrome: cros_ec_uart: Add transport layer

[Tzung-Bi Shih <tzungbi@kernel.org>]

On Fri, Jan 06, 2023 at 11:54:27AM +0300, Dan Carpenter wrote:
> Hello Bhanu Prakash Maiya,
> 
> The patch 04a8bdd135cc: "platform/chrome: cros_ec_uart: Add transport
> layer" from Dec 27, 2022, leads to the following Smatch static
> checker warning:
> 
> 	drivers/platform/chrome/cros_ec_uart.c:152 cros_ec_uart_pkt_xfer()
> 	warn: 'ret' possible negative type promoted to high
> 
> drivers/platform/chrome/cros_ec_uart.c
>     130 static int cros_ec_uart_pkt_xfer(struct cros_ec_device *ec_dev,
>     131                                  struct cros_ec_command *ec_msg)
>     132 {
>     133         struct cros_ec_uart *ec_uart = ec_dev->priv;
>     134         struct serdev_device *serdev = ec_uart->serdev;
>     135         struct response_info *resp = &ec_uart->response;
>     136         struct ec_host_response *host_response;
>     137         unsigned int len;
>                 ^^^^^^^^^^^^^^^^
> 
>     138         int ret, i;
>     139         u8 sum;
>     140 
>     141         len = cros_ec_prepare_tx(ec_dev, ec_msg);
>     142         dev_dbg(ec_dev->dev, "Prepared len=%d\n", len);
>     143 
>     144         /* Setup for incoming response */
>     145         resp->data = ec_dev->din;
>     146         resp->max_size = ec_dev->din_size;
>     147         resp->size = 0;
>     148         resp->exp_len = 0;
>     149         resp->status = 0;
>     150 
>     151         ret = serdev_device_write_buf(serdev, ec_dev->dout, len);
> --> 152         if (ret < len) {
> 
> If serdev_device_write_buf() returns negative then type promotion means
> this condition is false.  Write it like:
> 
> 	if (ret < 0 || ret != len) {
> 		dev_err(ec_dev->dev, "Unable to write data\n");
> 		if (ret >= 0)
> 			ret = -EIO;
> 		goto exit;
> 	}

Fixed in https://patchwork.kernel.org/project/chrome-platform/patch/20230109081554.3792547-1-tzungbi@kernel.org/.

> 
>     153                 dev_err(ec_dev->dev, "Unable to write data\n");
>     154                 ret = -EIO;
>     155                 goto exit;
>     156         }
>     157 
>     158         ret = wait_event_timeout(resp->wait_queue, resp->status,
>     159                                  msecs_to_jiffies(EC_MSG_DEADLINE_MS));
>     160         if (ret == 0) {
>     161                 dev_warn(ec_dev->dev, "Timed out waiting for response.\n");
>     162                 ret = -ETIMEDOUT;
>     163                 goto exit;
>     164         }
>     165 
>     166         if (resp->status < 0) {
>     167                 ret = resp->status;
>     168                 dev_warn(ec_dev->dev, "Error response received: %d\n", ret);
>     169                 goto exit;
>     170         }
>     171 
>     172         host_response = (struct ec_host_response *)ec_dev->din;
>     173         ec_msg->result = host_response->result;
>     174 
>     175         if (host_response->data_len > ec_msg->insize) {
>     176                 dev_err(ec_dev->dev, "Resp too long (%d bytes, expected %d)\n",
>     177                         host_response->data_len, ec_msg->insize);
>     178                 ret = -ENOSPC;
> 
> 
> ret = -EINVAL;  (Unless you are discussing harddrives).

It looks like platform/chrome used the error number for a while for the
case:

$ grep -R ENOSPC drivers/platform/chrome/
drivers/platform/chrome/cros_ec_lpc.c:          ret = -ENOSPC;
drivers/platform/chrome/cros_ec_i2c.c:          ret = -ENOSPC;
drivers/platform/chrome/cros_ec_uart.c:         ret = -ENOSPC;
drivers/platform/chrome/cros_ec_ishtp.c:                return -ENOSPC;
drivers/platform/chrome/cros_ec_spi.c:          ret = -ENOSPC;

I wouldn't feel bother if we keep using the error number.  Otherwise, we
should change all of them.