| Message ID | 0cdc2dce-ca89-451a-9774-1482ab2f4762@moroto.mountain (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | drm/bridge: nxp-ptn3460: fix i2c_master_send() error checking | expand |
On 04/12/2023 13:29, Dan Carpenter wrote: > The i2c_master_send/recv() functions return negative error codes or the > number of bytes that were able to be sent/received. This code has > two problems. 1) Instead of checking if all the bytes were sent or > received, it checks that at least one byte was sent or received. > 2) If there was a partial send/receive then we should return a negative > error code but this code returns success. > > Fixes: a9fe713d7d45 ("drm/bridge: Add PTN3460 bridge driver") > Cc: stable@vger.kernel.org > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > This is from static analysis and code review. It's always a concern > when you add stricter error handling that something will break. > > drivers/gpu/drm/bridge/nxp-ptn3460.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c > index d81920227a8a..9b7eb8c669c1 100644 > --- a/drivers/gpu/drm/bridge/nxp-ptn3460.c > +++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c > @@ -56,13 +56,13 @@ static int ptn3460_read_bytes(struct ptn3460_bridge *ptn_bridge, char addr, > ret = i2c_master_send(ptn_bridge->client, &addr, 1); > if (ret <= 0) { > DRM_ERROR("Failed to send i2c command, ret=%d\n", ret); > - return ret; > + return ret ?: -EIO; > } > > ret = i2c_master_recv(ptn_bridge->client, buf, len); > - if (ret <= 0) { > + if (ret != len) { This is impossible, i2c_transfer_buffer_flags() returns len as-is if no error, so ret can only be negative or equal to len. The original code is right. > DRM_ERROR("Failed to recv i2c data, ret=%d\n", ret); > - return ret; > + return ret < 0 ? ret : -EIO; > } > > return 0; > @@ -78,9 +78,9 @@ static int ptn3460_write_byte(struct ptn3460_bridge *ptn_bridge, char addr, > buf[1] = val; > > ret = i2c_master_send(ptn_bridge->client, buf, ARRAY_SIZE(buf)); > - if (ret <= 0) { > + if (ret != ARRAY_SIZE(buf)) { Ditto > DRM_ERROR("Failed to send i2c command, ret=%d\n", ret); > - return ret; > + return ret < 0 ? ret : -EIO; > } > > return 0;
On Mon, Dec 04, 2023 at 02:53:05PM +0100, Neil Armstrong wrote: > On 04/12/2023 13:29, Dan Carpenter wrote: > > The i2c_master_send/recv() functions return negative error codes or the > > number of bytes that were able to be sent/received. This code has > > two problems. 1) Instead of checking if all the bytes were sent or > > received, it checks that at least one byte was sent or received. > > 2) If there was a partial send/receive then we should return a negative > > error code but this code returns success. > > > > Fixes: a9fe713d7d45 ("drm/bridge: Add PTN3460 bridge driver") > > Cc: stable@vger.kernel.org > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > --- > > This is from static analysis and code review. It's always a concern > > when you add stricter error handling that something will break. > > > > drivers/gpu/drm/bridge/nxp-ptn3460.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c > > index d81920227a8a..9b7eb8c669c1 100644 > > --- a/drivers/gpu/drm/bridge/nxp-ptn3460.c > > +++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c > > @@ -56,13 +56,13 @@ static int ptn3460_read_bytes(struct ptn3460_bridge *ptn_bridge, char addr, > > ret = i2c_master_send(ptn_bridge->client, &addr, 1); > > if (ret <= 0) { > > DRM_ERROR("Failed to send i2c command, ret=%d\n", ret); > > - return ret; > > + return ret ?: -EIO; > > } > > ret = i2c_master_recv(ptn_bridge->client, buf, len); > > - if (ret <= 0) { > > + if (ret != len) { > > This is impossible, i2c_transfer_buffer_flags() returns len as-is if no error, so > ret can only be negative or equal to len. The original code is right. It works, but it's not "right". The <= 0 could be changed to < 0. The "len" variable is EDID_LENGTH (128). regards, dan carpenter
On 04/12/2023 17:59, Dan Carpenter wrote: > On Mon, Dec 04, 2023 at 02:53:05PM +0100, Neil Armstrong wrote: >> On 04/12/2023 13:29, Dan Carpenter wrote: >>> The i2c_master_send/recv() functions return negative error codes or the >>> number of bytes that were able to be sent/received. This code has >>> two problems. 1) Instead of checking if all the bytes were sent or >>> received, it checks that at least one byte was sent or received. >>> 2) If there was a partial send/receive then we should return a negative >>> error code but this code returns success. >>> >>> Fixes: a9fe713d7d45 ("drm/bridge: Add PTN3460 bridge driver") >>> Cc: stable@vger.kernel.org >>> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> >>> --- >>> This is from static analysis and code review. It's always a concern >>> when you add stricter error handling that something will break. >>> >>> drivers/gpu/drm/bridge/nxp-ptn3460.c | 10 +++++----- >>> 1 file changed, 5 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c >>> index d81920227a8a..9b7eb8c669c1 100644 >>> --- a/drivers/gpu/drm/bridge/nxp-ptn3460.c >>> +++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c >>> @@ -56,13 +56,13 @@ static int ptn3460_read_bytes(struct ptn3460_bridge *ptn_bridge, char addr, >>> ret = i2c_master_send(ptn_bridge->client, &addr, 1); >>> if (ret <= 0) { >>> DRM_ERROR("Failed to send i2c command, ret=%d\n", ret); >>> - return ret; >>> + return ret ?: -EIO; >>> } >>> ret = i2c_master_recv(ptn_bridge->client, buf, len); >>> - if (ret <= 0) { >>> + if (ret != len) { >> >> This is impossible, i2c_transfer_buffer_flags() returns len as-is if no error, so >> ret can only be negative or equal to len. The original code is right. > > It works, but it's not "right". The <= 0 could be changed to < 0. The > "len" variable is EDID_LENGTH (128). So indeed, switching to < 0 is the most reasonable, no need to change the ret value in this case. Neil > > regards, > dan carpenter >
On Mon, 4 Dec 2023 15:29:00 +0300, Dan Carpenter wrote: > The i2c_master_send/recv() functions return negative error codes or the > number of bytes that were able to be sent/received. This code has > two problems. 1) Instead of checking if all the bytes were sent or > received, it checks that at least one byte was sent or received. > 2) If there was a partial send/receive then we should return a negative > error code but this code returns success. > > [...] Applied, thanks! [1/1] drm/bridge: nxp-ptn3460: fix i2c_master_send() error checking https://cgit.freedesktop.org/drm/drm-misc/commit/?id=914437992876 Rob
On Tue, Dec 05, 2023 at 02:48:26PM +0100, Robert Foss wrote: > On Mon, 4 Dec 2023 15:29:00 +0300, Dan Carpenter wrote: > > The i2c_master_send/recv() functions return negative error codes or the > > number of bytes that were able to be sent/received. This code has > > two problems. 1) Instead of checking if all the bytes were sent or > > received, it checks that at least one byte was sent or received. > > 2) If there was a partial send/receive then we should return a negative > > error code but this code returns success. > > > > [...] > > Applied, thanks! > > [1/1] drm/bridge: nxp-ptn3460: fix i2c_master_send() error checking > https://cgit.freedesktop.org/drm/drm-misc/commit/?id=914437992876 > Wait. That was unexpected. Neil's review comments were correct. I was planning to send a v2 patch which was just a cleanup. regards, dan carpenter
On Tue, Dec 5, 2023, 15:01 Dan Carpenter <dan.carpenter@linaro.org> wrote: > On Tue, Dec 05, 2023 at 02:48:26PM +0100, Robert Foss wrote: > > On Mon, 4 Dec 2023 15:29:00 +0300, Dan Carpenter wrote: > > > The i2c_master_send/recv() functions return negative error codes or the > > > number of bytes that were able to be sent/received. This code has > > > two problems. 1) Instead of checking if all the bytes were sent or > > > received, it checks that at least one byte was sent or received. > > > 2) If there was a partial send/receive then we should return a negative > > > error code but this code returns success. > > > > > > [...] > > > > Applied, thanks! > > > > [1/1] drm/bridge: nxp-ptn3460: fix i2c_master_send() error checking > > https://cgit.freedesktop.org/drm/drm-misc/commit/?id=914437992876 > > > > Wait. That was unexpected. Neil's review comments were correct. I was > planning to send a v2 patch which was just a cleanup. > Sorry Dan, I was too quick on the draw. Can you send a fixup and I'll apply it too? > regards, > dan carpenter > >
On Tue, Dec 05, 2023 at 03:04:49PM +0100, Robert Foss wrote: > On Tue, Dec 5, 2023, 15:01 Dan Carpenter <dan.carpenter@linaro.org> wrote: > > > On Tue, Dec 05, 2023 at 02:48:26PM +0100, Robert Foss wrote: > > > On Mon, 4 Dec 2023 15:29:00 +0300, Dan Carpenter wrote: > > > > The i2c_master_send/recv() functions return negative error codes or the > > > > number of bytes that were able to be sent/received. This code has > > > > two problems. 1) Instead of checking if all the bytes were sent or > > > > received, it checks that at least one byte was sent or received. > > > > 2) If there was a partial send/receive then we should return a negative > > > > error code but this code returns success. > > > > > > > > [...] > > > > > > Applied, thanks! > > > > > > [1/1] drm/bridge: nxp-ptn3460: fix i2c_master_send() error checking > > > https://cgit.freedesktop.org/drm/drm-misc/commit/?id=914437992876 > > > > > > > Wait. That was unexpected. Neil's review comments were correct. I was > > planning to send a v2 patch which was just a cleanup. > > > > Sorry Dan, I was too quick on the draw. Can you send a fixup and I'll apply > it too? > Sure. I will do that. regards, dan carpenter
diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c index d81920227a8a..9b7eb8c669c1 100644 --- a/drivers/gpu/drm/bridge/nxp-ptn3460.c +++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c @@ -56,13 +56,13 @@ static int ptn3460_read_bytes(struct ptn3460_bridge *ptn_bridge, char addr, ret = i2c_master_send(ptn_bridge->client, &addr, 1); if (ret <= 0) { DRM_ERROR("Failed to send i2c command, ret=%d\n", ret); - return ret; + return ret ?: -EIO; } ret = i2c_master_recv(ptn_bridge->client, buf, len); - if (ret <= 0) { + if (ret != len) { DRM_ERROR("Failed to recv i2c data, ret=%d\n", ret); - return ret; + return ret < 0 ? ret : -EIO; } return 0; @@ -78,9 +78,9 @@ static int ptn3460_write_byte(struct ptn3460_bridge *ptn_bridge, char addr, buf[1] = val; ret = i2c_master_send(ptn_bridge->client, buf, ARRAY_SIZE(buf)); - if (ret <= 0) { + if (ret != ARRAY_SIZE(buf)) { DRM_ERROR("Failed to send i2c command, ret=%d\n", ret); - return ret; + return ret < 0 ? ret : -EIO; } return 0;
The i2c_master_send/recv() functions return negative error codes or the number of bytes that were able to be sent/received. This code has two problems. 1) Instead of checking if all the bytes were sent or received, it checks that at least one byte was sent or received. 2) If there was a partial send/receive then we should return a negative error code but this code returns success. Fixes: a9fe713d7d45 ("drm/bridge: Add PTN3460 bridge driver") Cc: stable@vger.kernel.org Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- This is from static analysis and code review. It's always a concern when you add stricter error handling that something will break. drivers/gpu/drm/bridge/nxp-ptn3460.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)