* [bug report] platform/chrome: cros_ec_uart: Add transport layer
@ 2023-01-06 8:54 Dan Carpenter
2023-01-10 3:25 ` Tzung-Bi Shih
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2023-01-06 8:54 UTC (permalink / raw)
To: bhanumaiya; +Cc: chrome-platform
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;
}
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).
179 goto exit;
180 }
181
182 /* Validate checksum */
183 sum = 0;
184 for (i = 0; i < sizeof(*host_response) + host_response->data_len; i++)
185 sum += ec_dev->din[i];
186
187 if (sum) {
188 dev_err(ec_dev->dev, "Bad packet checksum calculated %x\n", sum);
189 ret = -EBADMSG;
190 goto exit;
191 }
192
193 memcpy(ec_msg->data, ec_dev->din + sizeof(*host_response), host_response->data_len);
194
195 ret = host_response->data_len;
196
197 exit:
198 /* Invalidate response buffer to guard against out of band rx data */
199 resp->data = NULL;
200
201 if (ec_msg->command == EC_CMD_REBOOT_EC)
202 msleep(EC_REBOOT_DELAY_MS);
203
204 return ret;
205 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] platform/chrome: cros_ec_uart: Add transport layer
2023-01-06 8:54 [bug report] platform/chrome: cros_ec_uart: Add transport layer Dan Carpenter
@ 2023-01-10 3:25 ` Tzung-Bi Shih
2023-01-10 5:16 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Tzung-Bi Shih @ 2023-01-10 3:25 UTC (permalink / raw)
To: Dan Carpenter; +Cc: bhanumaiya, chrome-platform
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.
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] platform/chrome: cros_ec_uart: Add transport layer
2023-01-10 3:25 ` Tzung-Bi Shih
@ 2023-01-10 5:16 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2023-01-10 5:16 UTC (permalink / raw)
To: Tzung-Bi Shih; +Cc: bhanumaiya, chrome-platform
On Tue, Jan 10, 2023 at 11:25:24AM +0800, Tzung-Bi Shih wrote:
> > 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.
I don't really care about this code, but generally don't let consistency
hold you back from being correct. If 99 are wrong and 1 is correct
that's at least better than 100 wrong and none correct.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-01-10 5:16 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-06 8:54 [bug report] platform/chrome: cros_ec_uart: Add transport layer Dan Carpenter
2023-01-10 3:25 ` Tzung-Bi Shih
2023-01-10 5:16 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox