drm: sysfs files error handling
Subsystems (3)
PATCH 1/1 drm: sysfs files error handling
In the original code we used "j" as an iterator but we used "i" as an index.
- for (j = 0; j < i; j++)
- device_remove_file(&connector->kdev,
- &connector_attrs[i]);
Smatch complained about that because "i" was potentially passed the end of the array. Which makes sense if we should be using "j" there.
I also thought that we should remove the files for &connector_attrs_opt1 but to do that I had to add separate iterators for &connector_attrs and &connector_attrs_opt1.
---
Found by static analysis and compile tested only.
drivers/gpu/drm/drm_sysfs.c +12 -9
@@ -353,7 +353,10 @@ static struct bin_attribute edid_attr = {int drm_sysfs_connector_add (struct drm_connector *connector) {struct drm_device *dev = connector->dev; -int ret =0 , i, j; +int attr_cnt =0 ; +int opt_cnt =0 ; +int i; +int ret =0 ;/* We shouldn't get called more than once for the same connector */ BUG_ON (device_is_registered (&connector->kdev));
@@ -376,8 +379,8 @@ int drm_sysfs_connector_add(struct drm_connector *connector)/* Standard attributes */ -for (i =0 ; i <ARRAY_SIZE (connector_attrs); i++) { - ret =device_create_file (&connector->kdev, &connector_attrs[i]); +for (attr_cnt =0 ; attr_cnt <ARRAY_SIZE (connector_attrs); attr_cnt++) { + ret =device_create_file (&connector->kdev, &connector_attrs[attr_cnt]);if (ret) goto err_out_files; }
@@ -393,8 +396,8 @@ int drm_sysfs_connector_add(struct drm_connector *connector)case DRM_MODE_CONNECTOR_SVIDEO :case DRM_MODE_CONNECTOR_Component:case DRM_MODE_CONNECTOR_TV : -for (i =0 ; i <ARRAY_SIZE (connector_attrs_opt1); i++) { - ret =device_create_file (&connector->kdev, &connector_attrs_opt1[i]); +for (opt_cnt =0 ; opt_cnt <ARRAY_SIZE (connector_attrs_opt1); opt_cnt++) { + ret =device_create_file (&connector->kdev, &connector_attrs_opt1[opt_cnt]);if (ret) goto err_out_files; }
@@ -413,10 +416,10 @@ int drm_sysfs_connector_add(struct drm_connector *connector)return 0 ; err_out_files: -if (i >0 ) -for (j =0 ; j < i; j++) -device_remove_file (&connector->kdev, - &connector_attrs[i]); +for (i =0 ; i < opt_cnt; i++) +device_remove_file (&connector->kdev, &connector_attrs_opt1[i]); +for (i =0 ; i < attr_cnt; i++) +device_remove_file (&connector->kdev, &connector_attrs[i]);device_unregister (&connector->kdev); out:
drivers/gpu/drm/drm_sysfs.c int drm_sysfs_connector_add(struct drm_connector *connector) · line 419
Quoted 7 lines · Dan Carpenter
In the original code we used "j" as an iterator but we used "i" as an index.
drivers/gpu/drm/drm_sysfs.c +0
-3-for (j =0 ; j< i; j++) -device_remove_file (&connector->kdev, -&connector_attrs[i]);
I guess this really should be a attribute group anyways?
Typically when there's such a open coded loop it means the wrong interfaces are being used.
-Andi
drivers/gpu/drm/drm_sysfs.c int drm_sysfs_connector_add(struct drm_connector *connector) · line 419
Quoted 7 lines · Dan Carpenter
In the original code we used "j" as an iterator but we used "i" as an index.
drivers/gpu/drm/drm_sysfs.c +0
-3-for (j =0 ; j< i; j++) -device_remove_file (&connector->kdev, -&connector_attrs[i]);
I guess this really should be a attribute group anyways?
Typically when there's such a open coded loop it means the wrong interfaces are being used.
-Andi
------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
--
Quoted 5 lines · Andi Kleen
Quoted 4 lines · Dan Carpenter
In the original code we used "j" as an iterator but we used "i" as an index.
Quoted 8 lines
drivers/gpu/drm/drm_sysfs.c int drm_sysfs_connector_add(struct drm_connector *connector) · line 419
Quoted 3 lines
drivers/gpu/drm/drm_sysfs.c +0
-3-for (j =0 ; j< i; j++) -device_remove_file (&connector->kdev, -&connector_attrs[i]);I guess this really should be a attribute group anyways?
Typically when there's such a open coded loop it means the wrong interfaces are being used.
My graphics card is crap and doesn't use this code at all. I'd feel uncomfortable those changes without being able to test it.
So while, you are probably right, someone else should probably do that.
regards,
dan carpenter
Quoted 1 line
-Andi