In the function oaktrail_lvds_mode_set, I don't think that the following code makes any sense:
/* Find the connector we're trying to set up */ list_for_each_entry(connector, &mode_config->connector_list, head) { if (!connector->encoder || connector->encoder->crtc != crtc) continue; }
if (!connector) { DRM_ERROR("Couldn't find connector when setting mode"); return; }
drm_connector_property_get_value( connector, dev->mode_config.scaling_mode_property, &v);
The initial loop is a no-op, because it always continues. The test !connector can never be true, because at the end of a list_for_each_entry connector points to the list head, and calling drm_connector_property_get_value on the list head probably does not make sense.
julia
On Sun, 8 Jul 2012 10:39:43 +0200 (CEST) Julia Lawall julia.lawall@lip6.fr wrote:
In the function oaktrail_lvds_mode_set, I don't think that the following code makes any sense:
/* Find the connector we're trying to set up */ list_for_each_entry(connector, &mode_config->connector_list, head) { if (!connector->encoder || connector->encoder->crtc != crtc) continue; } if (!connector) { DRM_ERROR("Couldn't find connector when setting mode"); return; } drm_connector_property_get_value( connector, dev->mode_config.scaling_mode_property, &v);The initial loop is a no-op, because it always continues. The test !connector can never be true, because at the end of a list_for_each_entry connector points to the list head, and calling drm_connector_property_get_value on the list head probably does not make sense.
We test !connector->encoder rather than !connector ?
Alan
On Sun, 8 Jul 2012, Alan Cox wrote:
On Sun, 8 Jul 2012 10:39:43 +0200 (CEST) Julia Lawall julia.lawall@lip6.fr wrote:
In the function oaktrail_lvds_mode_set, I don't think that the following code makes any sense:
/* Find the connector we're trying to set up */ list_for_each_entry(connector, &mode_config->connector_list, head) { if (!connector->encoder || connector->encoder->crtc != crtc) continue; } if (!connector) { DRM_ERROR("Couldn't find connector when setting mode"); return; } drm_connector_property_get_value( connector, dev->mode_config.scaling_mode_property, &v);The initial loop is a no-op, because it always continues. The test !connector can never be true, because at the end of a list_for_each_entry connector points to the list head, and calling drm_connector_property_get_value on the list head probably does not make sense.
We test !connector->encoder rather than !connector ?
There is a test of !connector->encoder inside the loop. But the whole loop body is a no-op because of the continue. The only effect of the loop is to set connector to the list head, which is the exit condition for the loop.
The test of !connector is afterwards, but connector will not be NULL at that point, it will point to the list head.
julia
On Sun, Jul 8, 2012 at 10:16 PM, Alan Cox alan@lxorguk.ukuu.org.uk wrote:
On Sun, 8 Jul 2012 10:39:43 +0200 (CEST) Julia Lawall julia.lawall@lip6.fr wrote:
In the function oaktrail_lvds_mode_set, I don't think that the following code makes any sense:
/* Find the connector we're trying to set up */ list_for_each_entry(connector, &mode_config->connector_list, head) { if (!connector->encoder || connector->encoder->crtc != crtc) continue; } if (!connector) { DRM_ERROR("Couldn't find connector when setting mode"); return; } drm_connector_property_get_value( connector, dev->mode_config.scaling_mode_property, &v);The initial loop is a no-op, because it always continues. The test !connector can never be true, because at the end of a list_for_each_entry connector points to the list head, and calling drm_connector_property_get_value on the list head probably does not make sense.
We test !connector->encoder rather than !connector ?
It seems we should break on : if (connector->encoder && connector->encoder == encoder)
Then do a check after list iteration: if (!connector || connector->encoder != encoder) DRM_ERROR("Couldn't find connector when setting mode");
But my attention span is less than a jiffie right now... will look at it with fresh eyes tomorrow.
-Patrik
On Mon, 9 Jul 2012, Patrik Jakobsson wrote:
On Sun, Jul 8, 2012 at 10:16 PM, Alan Cox alan@lxorguk.ukuu.org.uk wrote:
On Sun, 8 Jul 2012 10:39:43 +0200 (CEST) Julia Lawall julia.lawall@lip6.fr wrote:
In the function oaktrail_lvds_mode_set, I don't think that the following code makes any sense:
/* Find the connector we're trying to set up */ list_for_each_entry(connector, &mode_config->connector_list, head) { if (!connector->encoder || connector->encoder->crtc != crtc) continue; } if (!connector) { DRM_ERROR("Couldn't find connector when setting mode"); return; } drm_connector_property_get_value( connector, dev->mode_config.scaling_mode_property, &v);The initial loop is a no-op, because it always continues. The test !connector can never be true, because at the end of a list_for_each_entry connector points to the list head, and calling drm_connector_property_get_value on the list head probably does not make sense.
We test !connector->encoder rather than !connector ?
It seems we should break on : if (connector->encoder && connector->encoder == encoder)
Then do a check after list iteration: if (!connector || connector->encoder != encoder) DRM_ERROR("Couldn't find connector when setting mode");
Possible. The !connector is still not needed, but the overall logic seems better.
julia
dri-devel@lists.freedesktop.org
-
Alan Cox -
Julia Lawall -
Patrik Jakobsson