Fix a few minor issues durring error handling, all of these were pointed at by Coverity reports.
Imre Deak (3): drm: Tune up error message during format->bpp/cpp conversion drm/mst: Fix error handling during MST sideband message reception drm: Avoid dereferencing a NULL mstb in drm_dp_mst_handle_up_req()
drivers/gpu/drm/drm_crtc.c | 10 +++++++--- drivers/gpu/drm/drm_dp_mst_topology.c | 16 +++++++++++++--- 2 files changed, 20 insertions(+), 6 deletions(-)
Returning a 0 bpp/cpp value from these functions isn't ever valid. In many cases it can also lead to a div-by-zero possibly at some later point in time, so make sure we catch such errors as soon as possible via louder error reporting.
CC: Dave Airlie airlied@redhat.com Signed-off-by: Imre Deak imre.deak@intel.com --- drivers/gpu/drm/drm_crtc.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 70f9c68..3a32606 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -5610,8 +5610,8 @@ void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth, *bpp = 32; break; default: - DRM_DEBUG_KMS("unsupported pixel format %s\n", - drm_get_format_name(format)); + WARN(1, "unsupported pixel format %s\n", + drm_get_format_name(format)); *depth = 0; *bpp = 0; break; @@ -5666,8 +5666,12 @@ int drm_format_plane_cpp(uint32_t format, int plane) unsigned int depth; int bpp;
- if (plane >= drm_format_num_planes(format)) + if (plane >= drm_format_num_planes(format)) { + WARN(1, "invalid plane %d for format %s\n", + plane, drm_get_format_name(format)); + return 0; + }
switch (format) { case DRM_FORMAT_YUYV:
On Thu, May 12, 2016 at 04:00:38PM +0300, Imre Deak wrote:
Returning a 0 bpp/cpp value from these functions isn't ever valid. In many cases it can also lead to a div-by-zero possibly at some later point in time, so make sure we catch such errors as soon as possible via louder error reporting.
CC: Dave Airlie airlied@redhat.com Signed-off-by: Imre Deak imre.deak@intel.com
drivers/gpu/drm/drm_crtc.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 70f9c68..3a32606 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -5610,8 +5610,8 @@ void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth, *bpp = 32; break; default:
DRM_DEBUG_KMS("unsupported pixel format %s\n",drm_get_format_name(format));
WARN(1, "unsupported pixel format %s\n",drm_get_format_name(format));
NAK. This will happen every time drm_helper_mode_fill_fb_struct() is called with a non-RGB format.
*depth = 0; *bpp = 0; break;@@ -5666,8 +5666,12 @@ int drm_format_plane_cpp(uint32_t format, int plane) unsigned int depth; int bpp;
- if (plane >= drm_format_num_planes(format))
- if (plane >= drm_format_num_planes(format)) {
WARN(1, "invalid plane %d for format %s\n",plane, drm_get_format_name(format));
We have this check in many places. Should either convert all or none.
return 0;
}
switch (format) { case DRM_FORMAT_YUYV:
-- 2.5.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, 2016-05-12 at 16:10 +0300, Ville Syrjälä wrote:
On Thu, May 12, 2016 at 04:00:38PM +0300, Imre Deak wrote:
Returning a 0 bpp/cpp value from these functions isn't ever valid. In many cases it can also lead to a div-by-zero possibly at some later point in time, so make sure we catch such errors as soon as possible via louder error reporting.
CC: Dave Airlie airlied@redhat.com Signed-off-by: Imre Deak imre.deak@intel.com
drivers/gpu/drm/drm_crtc.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 70f9c68..3a32606 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -5610,8 +5610,8 @@ void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth, *bpp = 32; break; default:
DRM_DEBUG_KMS("unsupported pixel format %s\n",drm_get_format_name(format));
WARN(1, "unsupported pixel format %s\n",drm_get_format_name(format));NAK. This will happen every time drm_helper_mode_fill_fb_struct() is called with a non-RGB format.
Yep, missed that. So how about handling here those formats explicitly, and emitting a warning only for truly unsupported formats?
*depth = 0; *bpp = 0; break; @@ -5666,8 +5666,12 @@ int drm_format_plane_cpp(uint32_t format, int plane) unsigned int depth; int bpp;
- if (plane >= drm_format_num_planes(format))
- if (plane >= drm_format_num_planes(format)) {
WARN(1, "invalid plane %d for format %s\n",plane, drm_get_format_name(format));We have this check in many places. Should either convert all or none.
Ok, I can also change drm_format_plane_width() and drm_format_plane_height(). Couldn't spot any other places.
return 0;
- }
switch (format) { case DRM_FORMAT_YUYV: -- 2.5.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, May 12, 2016 at 04:43:05PM +0300, Imre Deak wrote:
On Thu, 2016-05-12 at 16:10 +0300, Ville Syrjälä wrote:
On Thu, May 12, 2016 at 04:00:38PM +0300, Imre Deak wrote:
Returning a 0 bpp/cpp value from these functions isn't ever valid. In many cases it can also lead to a div-by-zero possibly at some later point in time, so make sure we catch such errors as soon as possible via louder error reporting.
CC: Dave Airlie airlied@redhat.com Signed-off-by: Imre Deak imre.deak@intel.com
drivers/gpu/drm/drm_crtc.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 70f9c68..3a32606 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -5610,8 +5610,8 @@ void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth, *bpp = 32; break; default:
DRM_DEBUG_KMS("unsupported pixel format %s\n",drm_get_format_name(format));
WARN(1, "unsupported pixel format %s\n",drm_get_format_name(format));NAK. This will happen every time drm_helper_mode_fill_fb_struct() is called with a non-RGB format.
Yep, missed that. So how about handling here those formats explicitly, and emitting a warning only for truly unsupported formats?
More work to keep this list updated, and it wouldn't prevent any div-by-zero with those formats. So I don't really see a point in that.
*depth = 0; *bpp = 0; break; @@ -5666,8 +5666,12 @@ int drm_format_plane_cpp(uint32_t format, int plane) unsigned int depth; int bpp;
- if (plane >= drm_format_num_planes(format))
- if (plane >= drm_format_num_planes(format)) {
WARN(1, "invalid plane %d for format %s\n",plane, drm_get_format_name(format));We have this check in many places. Should either convert all or none.
Ok, I can also change drm_format_plane_width() and drm_format_plane_height(). Couldn't spot any other places.
I thought we might have more. I guess not.
return 0;
- }
switch (format) { case DRM_FORMAT_YUYV: -- 2.5.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, 2016-05-12 at 16:52 +0300, Ville Syrjälä wrote:
On Thu, May 12, 2016 at 04:43:05PM +0300, Imre Deak wrote:
On Thu, 2016-05-12 at 16:10 +0300, Ville Syrjälä wrote:
On Thu, May 12, 2016 at 04:00:38PM +0300, Imre Deak wrote:
Returning a 0 bpp/cpp value from these functions isn't ever valid. In many cases it can also lead to a div-by-zero possibly at some later point in time, so make sure we catch such errors as soon as possible via louder error reporting.
CC: Dave Airlie airlied@redhat.com Signed-off-by: Imre Deak imre.deak@intel.com
drivers/gpu/drm/drm_crtc.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 70f9c68..3a32606 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -5610,8 +5610,8 @@ void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth, *bpp = 32; break; default:
DRM_DEBUG_KMS("unsupported pixel format %s\n",drm_get_format_name(format));
WARN(1, "unsupported pixel format %s\n",drm_get_format_name(format));NAK. This will happen every time drm_helper_mode_fill_fb_struct() is called with a non-RGB format.
Yep, missed that. So how about handling here those formats explicitly, and emitting a warning only for truly unsupported formats?
More work to keep this list updated, and it wouldn't prevent any div-by-zero with those formats. So I don't really see a point in that.
It would avoid the incorrect 'unsupported pixel format' message for those.
*depth = 0; *bpp = 0; break; @@ -5666,8 +5666,12 @@ int drm_format_plane_cpp(uint32_t format, int plane) unsigned int depth; int bpp;
- if (plane >= drm_format_num_planes(format))
- if (plane >= drm_format_num_planes(format)) {
WARN(1, "invalid plane %d for format %s\n",plane, drm_get_format_name(format));We have this check in many places. Should either convert all or none.
Ok, I can also change drm_format_plane_width() and drm_format_plane_height(). Couldn't spot any other places.
I thought we might have more. I guess not.
return 0;
- }
switch (format) { case DRM_FORMAT_YUYV: -- 2.5.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, May 12, 2016 at 05:00:06PM +0300, Imre Deak wrote:
On Thu, 2016-05-12 at 16:52 +0300, Ville Syrjälä wrote:
On Thu, May 12, 2016 at 04:43:05PM +0300, Imre Deak wrote:
On Thu, 2016-05-12 at 16:10 +0300, Ville Syrjälä wrote:
On Thu, May 12, 2016 at 04:00:38PM +0300, Imre Deak wrote:
Returning a 0 bpp/cpp value from these functions isn't ever valid. In many cases it can also lead to a div-by-zero possibly at some later point in time, so make sure we catch such errors as soon as possible via louder error reporting.
CC: Dave Airlie airlied@redhat.com Signed-off-by: Imre Deak imre.deak@intel.com
drivers/gpu/drm/drm_crtc.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 70f9c68..3a32606 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -5610,8 +5610,8 @@ void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth, *bpp = 32; break; default:
DRM_DEBUG_KMS("unsupported pixel format %s\n",drm_get_format_name(format));
WARN(1, "unsupported pixel format %s\n",drm_get_format_name(format));NAK. This will happen every time drm_helper_mode_fill_fb_struct() is called with a non-RGB format.
Yep, missed that. So how about handling here those formats explicitly, and emitting a warning only for truly unsupported formats?
More work to keep this list updated, and it wouldn't prevent any div-by-zero with those formats. So I don't really see a point in that.
It would avoid the incorrect 'unsupported pixel format' message for those.
If that's the entire concern, delete it. New drivers shouldn't use these functions any more anyway. -Daniel
Returning 0 from these functions isn't ever valid. In many cases it can also lead to a div-by-zero possibly at some later point in time, so make sure we catch such errors as soon as possible via louder error reporting.
v2: - Print the same WARN whenever we check for the same condition (Ville) - Don't change drm_fb_get_bpp_depth(), for non-RGB formats we return bpp=0, depth=0 normally. (Ville, Daniel)
CC: Dave Airlie airlied@redhat.com CC: Ville Syrjälä ville.syrjala@linux.intel.com CC: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Imre Deak imre.deak@intel.com --- drivers/gpu/drm/drm_crtc.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 70f9c68..990a9de 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -5544,6 +5544,13 @@ int drm_mode_destroy_dumb_ioctl(struct drm_device *dev, return dev->driver->dumb_destroy(file_priv, dev, args->handle); }
+static bool check_format_plane_valid(uint32_t format, int plane) +{ + return !WARN(plane >= drm_format_num_planes(format), + "invalid plane %d for format %s\n", + plane, drm_get_format_name(format)); +} + /** * drm_fb_get_bpp_depth - get the bpp/depth values for format * @format: pixel format (DRM_FORMAT_*) @@ -5666,7 +5673,7 @@ int drm_format_plane_cpp(uint32_t format, int plane) unsigned int depth; int bpp;
- if (plane >= drm_format_num_planes(format)) + if (!check_format_plane_valid(format, plane)) return 0;
switch (format) { @@ -5771,7 +5778,7 @@ EXPORT_SYMBOL(drm_format_vert_chroma_subsampling); */ int drm_format_plane_width(int width, uint32_t format, int plane) { - if (plane >= drm_format_num_planes(format)) + if (!check_format_plane_valid(format, plane)) return 0;
if (plane == 0) @@ -5792,7 +5799,7 @@ EXPORT_SYMBOL(drm_format_plane_width); */ int drm_format_plane_height(int height, uint32_t format, int plane) { - if (plane >= drm_format_num_planes(format)) + if (!check_format_plane_valid(format, plane)) return 0;
if (plane == 0)
Handle any error due to partial reads, timeouts etc. to avoid parsing uninitialized data subsequently. Also bail out if the parsing itself fails.
CC: Dave Airlie airlied@redhat.com Signed-off-by: Imre Deak imre.deak@intel.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index a13edf5..12c0ab3 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -2204,11 +2204,19 @@ static void drm_dp_get_one_sb_msg(struct drm_dp_mst_topology_mgr *mgr, bool up) ret = drm_dp_dpcd_read(mgr->aux, basereg + curreply, replyblock, len); if (ret != len) { - DRM_DEBUG_KMS("failed to read a chunk\n"); + DRM_DEBUG_KMS("failed to read a chunk (len %d, ret %d)\n", + len, ret); + + return; } + ret = drm_dp_sideband_msg_build(msg, replyblock, len, false); - if (ret == false) + if (!ret) { DRM_DEBUG_KMS("failed to build sideband msg\n"); + + return; + } + curreply += len; replylen -= len; }
On Thu, 2016-05-12 at 16:00 +0300, Imre Deak wrote:
Handle any error due to partial reads, timeouts etc. to avoid parsing uninitialized data subsequently. Also bail out if the parsing itself fails.
CC: Dave Airlie airlied@redhat.com Signed-off-by: Imre Deak imre.deak@intel.com
drivers/gpu/drm/drm_dp_mst_topology.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index a13edf5..12c0ab3 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -2204,11 +2204,19 @@ static void drm_dp_get_one_sb_msg(struct drm_dp_mst_topology_mgr *mgr, bool up) ret = drm_dp_dpcd_read(mgr->aux, basereg + curreply, replyblock, len); if (ret != len) {
DRM_DEBUG_KMS("failed to read a chunk\n");
DRM_DEBUG_KMS("failed to read a chunk (len %d, ret%d)\n",
len, ret);
I'd get rid of the whitespace here…
return;}
ret = drm_dp_sideband_msg_build(msg, replyblock, len, false);
if (ret == false)
if (!ret) {DRM_DEBUG_KMS("failed to build sideband msg\n");
And here, to match the rest of the file. Otherwise looks good to me;
Reviewed-by: Lyude cpaul@redhat.com
return;}curreply += len; replylen -= len; }
Handle any error due to partial reads, timeouts etc. to avoid parsing uninitialized data subsequently. Also bail out if the parsing itself fails.
v2: - Remove blank lines before returns to match the rest of file (Lyude)
CC: Dave Airlie airlied@redhat.com Signed-off-by: Imre Deak imre.deak@intel.com Reviewed-by: Lyude cpaul@redhat.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index a13edf5..9e64493 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -2204,11 +2204,17 @@ static void drm_dp_get_one_sb_msg(struct drm_dp_mst_topology_mgr *mgr, bool up) ret = drm_dp_dpcd_read(mgr->aux, basereg + curreply, replyblock, len); if (ret != len) { - DRM_DEBUG_KMS("failed to read a chunk\n"); + DRM_DEBUG_KMS("failed to read a chunk (len %d, ret %d)\n", + len, ret); + return; } + ret = drm_dp_sideband_msg_build(msg, replyblock, len, false); - if (ret == false) + if (!ret) { DRM_DEBUG_KMS("failed to build sideband msg\n"); + return; + } + curreply += len; replylen -= len; }
In case of an unknown broadcast message is sent mstb will remain unset, so check for this.
CC: Dave Airlie airlied@redhat.com Signed-off-by: Imre Deak imre.deak@intel.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 12c0ab3..412b9ca 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -2334,7 +2334,9 @@ static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr) DRM_DEBUG_KMS("Got RSN: pn: %d avail_pbn %d\n", msg.u.resource_stat.port_number, msg.u.resource_stat.available_pbn); }
- drm_dp_put_mst_branch_device(mstb); + if (mstb) + drm_dp_put_mst_branch_device(mstb); + memset(&mgr->up_req_recv, 0, sizeof(struct drm_dp_sideband_msg_rx)); } return ret;
On Thu, May 12, 2016 at 04:00:40PM +0300, Imre Deak wrote:
In case of an unknown broadcast message is sent mstb will remain unset, so check for this.
CC: Dave Airlie airlied@redhat.com Signed-off-by: Imre Deak imre.deak@intel.com
Could someone review 2/3 and 3/3 in this patchset? 1/3 was NAK'd and otherwise isn't needed anymore.
Thanks, Imre
drivers/gpu/drm/drm_dp_mst_topology.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 12c0ab3..412b9ca 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -2334,7 +2334,9 @@ static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr) DRM_DEBUG_KMS("Got RSN: pn: %d avail_pbn %d\n", msg.u.resource_stat.port_number, msg.u.resource_stat.available_pbn); }
drm_dp_put_mst_branch_device(mstb);
if (mstb)drm_dp_put_mst_branch_device(mstb);- memset(&mgr->up_req_recv, 0, sizeof(struct drm_dp_sideband_msg_rx)); } return ret;
-- 2.5.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Looks good to me.
Reviewed-by: Lyude lyude@redhat.com
On Mon, 2017-07-17 at 15:10 +0300, Imre Deak wrote:
On Thu, May 12, 2016 at 04:00:40PM +0300, Imre Deak wrote:
In case of an unknown broadcast message is sent mstb will remain unset, so check for this.
CC: Dave Airlie airlied@redhat.com Signed-off-by: Imre Deak imre.deak@intel.com
Could someone review 2/3 and 3/3 in this patchset? 1/3 was NAK'd and otherwise isn't needed anymore.
Thanks, Imre
drivers/gpu/drm/drm_dp_mst_topology.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 12c0ab3..412b9ca 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -2334,7 +2334,9 @@ static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr) DRM_DEBUG_KMS("Got RSN: pn: %d avail_pbn %d\n", msg.u.resource_stat.port_number, msg.u.resource_stat.available_pbn); }
drm_dp_put_mst_branch_device(mstb);
if (mstb)drm_dp_put_mst_branch_device(mstb);- memset(&mgr->up_req_recv, 0, sizeof(struct
drm_dp_sideband_msg_rx)); } return ret; -- 2.5.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Imre Deak -
Lyude Paul -
Lyude Paul -
Ville Syrjälä