Hi,
Static analysis with Coverity has found a rather old issue in drivers/gpu/drm/gma500/oaktrail_crtc.c with the following commit:
commit 9bd81acdb648509dbbc32d4da0477c9fae0d6a73 Author: Patrik Jakobsson patrik.r.jakobsson@gmail.com Date: Mon Dec 19 21:41:33 2011 +0000
gma500: Convert Oaktrail to work with new output handling
The analysis is as follows:
114 /* Find the connector we're trying to set up */ 115 list_for_each_entry(connector, &mode_config->connector_list, head) { 116 if (!connector->encoder || connector->encoder->crtc != crtc)
Continue has no effect (NO_EFFECT)useless_continue: Statement continue does not have any effect.
117 continue; 118 } 119 120 if (!connector) { 121 DRM_ERROR("Couldn't find connector when setting mode"); 122 gma_power_end(dev); 123 return; 124 }
Currently it appears the loop just iterates to the end of the list without doing anything useful. I'm not sure what the original intent was, so I'm not sure how this should be fixed.
Colin
On Fri, Jun 18, 2021 at 12:26 PM Colin Ian King colin.king@canonical.com wrote:
Hi,
Hi Colin
Static analysis with Coverity has found a rather old issue in drivers/gpu/drm/gma500/oaktrail_crtc.c with the following commit:
Relevant code is in drivers/gpu/drm/gma500/oaktrail_lvds.c
commit 9bd81acdb648509dbbc32d4da0477c9fae0d6a73 Author: Patrik Jakobsson patrik.r.jakobsson@gmail.com Date: Mon Dec 19 21:41:33 2011 +0000
gma500: Convert Oaktrail to work with new output handlingThe analysis is as follows:
114 /* Find the connector we're trying to set up */ 115 list_for_each_entry(connector, &mode_config->connector_list, head) { 116 if (!connector->encoder || connector->encoder->crtc != crtc)
Continue has no effect (NO_EFFECT)useless_continue: Statement continue does not have any effect.
117 continue; 118 } 119 120 if (!connector) { 121 DRM_ERROR("Couldn't find connector when setting mode"); 122 gma_power_end(dev); 123 return; 124 }
Currently it appears the loop just iterates to the end of the list without doing anything useful. I'm not sure what the original intent was, so I'm not sure how this should be fixed.
The code assumes there is only one correct connector so when iterating over the connectors it checks for the connectors that does _not_ match our criteria (!connector->encoder || connector->encoder->crtc
!= crtc). When the loop is done we end up with the correct connector set in the connector variable, hence the immediate check of "if (!connector) ...".
So the code is correct but perhaps unintuitive. You could do the opposite (if that makes more sense to you):
list_for_each_entry(connector, &mode_config->connector_list, head) { if (connector->encoder && connector->encoder->crtc == crtc) break; }
-Patrik
Colin
On 18/06/2021 12:19, Patrik Jakobsson wrote:
On Fri, Jun 18, 2021 at 12:26 PM Colin Ian King colin.king@canonical.com wrote:
Hi,
Hi Colin
Static analysis with Coverity has found a rather old issue in drivers/gpu/drm/gma500/oaktrail_crtc.c with the following commit:
Relevant code is in drivers/gpu/drm/gma500/oaktrail_lvds.c
commit 9bd81acdb648509dbbc32d4da0477c9fae0d6a73 Author: Patrik Jakobsson patrik.r.jakobsson@gmail.com Date: Mon Dec 19 21:41:33 2011 +0000
gma500: Convert Oaktrail to work with new output handlingThe analysis is as follows:
114 /* Find the connector we're trying to set up */ 115 list_for_each_entry(connector, &mode_config->connector_list, head) { 116 if (!connector->encoder || connector->encoder->crtc != crtc)
Continue has no effect (NO_EFFECT)useless_continue: Statement continue does not have any effect.
117 continue; 118 } 119 120 if (!connector) { 121 DRM_ERROR("Couldn't find connector when setting mode"); 122 gma_power_end(dev); 123 return; 124 }
Currently it appears the loop just iterates to the end of the list without doing anything useful. I'm not sure what the original intent was, so I'm not sure how this should be fixed.
The code assumes there is only one correct connector so when iterating over the connectors it checks for the connectors that does _not_ match our criteria (!connector->encoder || connector->encoder->crtc
!= crtc). When the loop is done we end up with the correct connector set in the connector variable, hence the immediate check of "if (!connector) ...".
So the code is correct but perhaps unintuitive. You could do the opposite (if that makes more sense to you):
list_for_each_entry(connector, &mode_config->connector_list, head) { if (connector->encoder && connector->encoder->crtc == crtc) break; }
OK, that makes sense. I'll send a fix for this shortly
-Patrik
Colin
dri-devel@lists.freedesktop.org
-
Colin Ian King -
Patrik Jakobsson