| Message ID | d1f1eb7f-1de7-4d73-a7d0-00cc1eac3d5d@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/xe/query: Refactor copy_to_user() usage in four functions | expand |
On Wed, 18 Sep 2024, Markus Elfring <Markus.Elfring@web.de> wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Wed, 18 Sep 2024 09:43:07 +0200 > > Assign return values from copy_to_user() calls to additional local variables > so that four kfree() calls and return statements can be omitted accordingly. > > This issue was transformed by using the Coccinelle software. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > drivers/gpu/drm/xe/xe_query.c | 32 ++++++++++++-------------------- > 1 file changed, 12 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c > index 5246a4a2740e..6195e720176d 100644 > --- a/drivers/gpu/drm/xe/xe_query.c > +++ b/drivers/gpu/drm/xe/xe_query.c > @@ -220,13 +220,11 @@ static int query_engines(struct xe_device *xe, > > engines->num_engines = i; > > - if (copy_to_user(query_ptr, engines, size)) { > + { Please don't leave blocks like this behind when you remove the if. BR, Jani. > + unsigned long ctu = copy_to_user(query_ptr, engines, size); > kfree(engines); > - return -EFAULT; > + return ctu ? -EFAULT : 0; > } > - kfree(engines); > - > - return 0; > } > > static size_t calc_mem_regions_size(struct xe_device *xe) > @@ -344,13 +342,11 @@ static int query_config(struct xe_device *xe, struct drm_xe_device_query *query) > config->info[DRM_XE_QUERY_CONFIG_MAX_EXEC_QUEUE_PRIORITY] = > xe_exec_queue_device_get_max_priority(xe); > > - if (copy_to_user(query_ptr, config, size)) { > + { > + unsigned long ctu = copy_to_user(query_ptr, config, size); > kfree(config); > - return -EFAULT; > + return ctu ? -EFAULT : 0; > } > - kfree(config); > - > - return 0; > } > > static int query_gt_list(struct xe_device *xe, struct drm_xe_device_query *query) > @@ -414,13 +410,11 @@ static int query_gt_list(struct xe_device *xe, struct drm_xe_device_query *query > REG_FIELD_GET(GMD_ID_REVID, gt->info.gmdid); > } > > - if (copy_to_user(query_ptr, gt_list, size)) { > + { > + unsigned long ctu = copy_to_user(query_ptr, gt_list, size); > kfree(gt_list); > - return -EFAULT; > + return ctu ? -EFAULT : 0; > } > - kfree(gt_list); > - > - return 0; > } > > static int query_hwconfig(struct xe_device *xe, > @@ -444,13 +438,11 @@ static int query_hwconfig(struct xe_device *xe, > > xe_guc_hwconfig_copy(>->uc.guc, hwconfig); > > - if (copy_to_user(query_ptr, hwconfig, size)) { > + { > + unsigned long ctu = copy_to_user(query_ptr, hwconfig, size); > kfree(hwconfig); > - return -EFAULT; > + return ctu ? -EFAULT : 0; > } > - kfree(hwconfig); > - > - return 0; > } > > static size_t calc_topo_query_size(struct xe_device *xe) > -- > 2.46.0 >
>> Assign return values from copy_to_user() calls to additional local variables >> so that four kfree() calls and return statements can be omitted accordingly. … >> +++ b/drivers/gpu/drm/xe/xe_query.c >> @@ -220,13 +220,11 @@ static int query_engines(struct xe_device *xe, >> >> engines->num_engines = i; >> >> - if (copy_to_user(query_ptr, engines, size)) { >> + { > > Please don't leave blocks like this behind when you remove the if. … >> + unsigned long ctu = copy_to_user(query_ptr, engines, size); >> kfree(engines); >> - return -EFAULT; >> + return ctu ? -EFAULT : 0; >> } >> - kfree(engines); >> - >> - return 0; >> } … Would you tolerate the shown variable definition without the proposed compound statement? Regards, Markus
diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c index 5246a4a2740e..6195e720176d 100644 --- a/drivers/gpu/drm/xe/xe_query.c +++ b/drivers/gpu/drm/xe/xe_query.c @@ -220,13 +220,11 @@ static int query_engines(struct xe_device *xe, engines->num_engines = i; - if (copy_to_user(query_ptr, engines, size)) { + { + unsigned long ctu = copy_to_user(query_ptr, engines, size); kfree(engines); - return -EFAULT; + return ctu ? -EFAULT : 0; } - kfree(engines); - - return 0; } static size_t calc_mem_regions_size(struct xe_device *xe) @@ -344,13 +342,11 @@ static int query_config(struct xe_device *xe, struct drm_xe_device_query *query) config->info[DRM_XE_QUERY_CONFIG_MAX_EXEC_QUEUE_PRIORITY] = xe_exec_queue_device_get_max_priority(xe); - if (copy_to_user(query_ptr, config, size)) { + { + unsigned long ctu = copy_to_user(query_ptr, config, size); kfree(config); - return -EFAULT; + return ctu ? -EFAULT : 0; } - kfree(config); - - return 0; } static int query_gt_list(struct xe_device *xe, struct drm_xe_device_query *query) @@ -414,13 +410,11 @@ static int query_gt_list(struct xe_device *xe, struct drm_xe_device_query *query REG_FIELD_GET(GMD_ID_REVID, gt->info.gmdid); } - if (copy_to_user(query_ptr, gt_list, size)) { + { + unsigned long ctu = copy_to_user(query_ptr, gt_list, size); kfree(gt_list); - return -EFAULT; + return ctu ? -EFAULT : 0; } - kfree(gt_list); - - return 0; } static int query_hwconfig(struct xe_device *xe, @@ -444,13 +438,11 @@ static int query_hwconfig(struct xe_device *xe, xe_guc_hwconfig_copy(>->uc.guc, hwconfig); - if (copy_to_user(query_ptr, hwconfig, size)) { + { + unsigned long ctu = copy_to_user(query_ptr, hwconfig, size); kfree(hwconfig); - return -EFAULT; + return ctu ? -EFAULT : 0; } - kfree(hwconfig); - - return 0; } static size_t calc_topo_query_size(struct xe_device *xe)