| Message ID | 20240814175947.169590-1-andi.shyti@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gem: Remove unnecessary cast | expand |
On Wed, Aug 14, 2024 at 07:59:47PM +0200, Andi Shyti wrote: > The cast from "long" to "unsigned long" is unnecessary. Remove > it. I don't believe we can be that bold in this statement. Some static analyzer tools might not agree and tell that if the start or end are negative values we could have undefined behavior. > > In this case, the variables "start" and "end" are of type long > because they need to account for the possibility of negative > values. However, they are subsequently moved to "unsigned long" > since addresses are typically handled as unsigned values. right, but the static analyzer tools won't agree and complain and people will start try to add this back. Do we really need this patch? > > Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_mman.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c > index e9b2424156f0..80528ba90a56 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c > @@ -323,8 +323,8 @@ static void set_address_limits(struct vm_area_struct *area, > end = min_t(long, end, vm_end); > > /* Let's move back into the "<< PAGE_SHIFT" domain */ > - *start_vaddr = (unsigned long)start << PAGE_SHIFT; > - *end_vaddr = (unsigned long)end << PAGE_SHIFT; > + *start_vaddr = start << PAGE_SHIFT; > + *end_vaddr = end << PAGE_SHIFT; > > *pfn = (gmadr_start + i915_ggtt_offset(vma)) >> PAGE_SHIFT; > *pfn += (*start_vaddr - area->vm_start) >> PAGE_SHIFT; > -- > 2.45.2 >
Hi Rodrigo, On Thu, Aug 15, 2024 at 02:58:25PM -0400, Rodrigo Vivi wrote: > On Wed, Aug 14, 2024 at 07:59:47PM +0200, Andi Shyti wrote: > > The cast from "long" to "unsigned long" is unnecessary. Remove > > it. > > I don't believe we can be that bold in this statement. > Some static analyzer tools might not agree and tell that > if the start or end are negative values we could have > undefined behavior. Right, but we do check for negative values before. If we reach this point I'm sure this is positive and I'm also sure that a positive long fits into an unsigned long. Maybe I should have been clearer in the commit log. > > In this case, the variables "start" and "end" are of type long > > because they need to account for the possibility of negative > > values. However, they are subsequently moved to "unsigned long" > > since addresses are typically handled as unsigned values. > > right, but the static analyzer tools won't agree and complain > and people will start try to add this back. > > Do we really need this patch? It's a cleanup, like removing trailing spaces, none of them is really needed :-) Trivial removals of unnecessary casts are normally done around the kernel, but, of course we can drop this patch. Thanks, Andi
On Fri, Aug 16, 2024 at 10:20:48AM +0200, Andi Shyti wrote: > Hi Rodrigo, > > On Thu, Aug 15, 2024 at 02:58:25PM -0400, Rodrigo Vivi wrote: > > On Wed, Aug 14, 2024 at 07:59:47PM +0200, Andi Shyti wrote: > > > The cast from "long" to "unsigned long" is unnecessary. Remove > > > it. > > > > I don't believe we can be that bold in this statement. > > Some static analyzer tools might not agree and tell that > > if the start or end are negative values we could have > > undefined behavior. > > Right, but we do check for negative values before. If we reach > this point I'm sure this is positive and I'm also sure that a > positive long fits into an unsigned long. > > Maybe I should have been clearer in the commit log. > > > > In this case, the variables "start" and "end" are of type long > > > because they need to account for the possibility of negative > > > values. However, they are subsequently moved to "unsigned long" > > > since addresses are typically handled as unsigned values. > > > > right, but the static analyzer tools won't agree and complain > > and people will start try to add this back. > > > > Do we really need this patch? > > It's a cleanup, like removing trailing spaces, none of them is > really needed :-) > > Trivial removals of unnecessary casts are normally done around > the kernel, but, of course we can drop this patch. My only concern was to remove and then we start having people trying to add it back again because some static analyzer tool complained about the undefined behavior. But if you are sure that this won't happen let's go with it. Please just rephrase the commit message. > > Thanks, > Andi
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c index e9b2424156f0..80528ba90a56 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c @@ -323,8 +323,8 @@ static void set_address_limits(struct vm_area_struct *area, end = min_t(long, end, vm_end); /* Let's move back into the "<< PAGE_SHIFT" domain */ - *start_vaddr = (unsigned long)start << PAGE_SHIFT; - *end_vaddr = (unsigned long)end << PAGE_SHIFT; + *start_vaddr = start << PAGE_SHIFT; + *end_vaddr = end << PAGE_SHIFT; *pfn = (gmadr_start + i915_ggtt_offset(vma)) >> PAGE_SHIFT; *pfn += (*start_vaddr - area->vm_start) >> PAGE_SHIFT;
The cast from "long" to "unsigned long" is unnecessary. Remove it. In this case, the variables "start" and "end" are of type long because they need to account for the possibility of negative values. However, they are subsequently moved to "unsigned long" since addresses are typically handled as unsigned values. Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com> --- drivers/gpu/drm/i915/gem/i915_gem_mman.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)