Re: [PATCH-next] Fix unintentional integer overflow

[Advait Dhamorikar]

Hi Christian,

I am not sure if I correctly understood what you meant,  just to clarify

When you say this
>No, all of this are numerical problems where not taken into account the
>size of the destination type.

>Saying that all of that are basically just style cleanups which doesn't
>need to be back-ported in any way, so please drop the Fixes: tag.

>And you should probably change the subject line to something like
>"drm/amdgpu: cleanup shift coding style".

Are you just talking about this message?
>> There are a few instances where we can use 1U instead of int as
>> harvest_config uses unsigned int
>>(adev->jpeg.harvest_config & (1 << i)
>> However I think they should be fixed in a separate patch?

Or is it intended for the complete previous "Fix unintentional
overflow" patch as well?
And I should just send a v3 with the two changes?

Thanks and regards,
Advait

On Mon, 7 Oct 2024 at 19:26, Christian König
<ckoenig.leichtzumerken@xxxxxxxxx> wrote:
>
> Am 05.10.24 um 09:05 schrieb Advait Dhamorikar:
> > Hi Sathish,
> >
> >> Please collate the changes together with Lijo's suggestion as well,
> >> "1ULL <<" instead of typecast, there are 3 occurrences of the error in
> >> f0b19b84d391.
> > I could only observe two instances of this error in f0b19b84d391 at:
> > 'mask = (1 << (adev->jpeg.num_jpeg_inst * adev->jpeg.num_jpeg_rings)) - 1;`
> > and `mask |= 1 << ((i * adev->jpeg.num_jpeg_rings) + j);`
> >
> > There are a few instances where we can use 1U instead of int as
> > harvest_config uses unsigned int
> > (adev->jpeg.harvest_config & (1 << i)
> > However I think they should be fixed in a separate patch?
>
> No, all of this are numerical problems where not taken into account the
> size of the destination type.
>
> Saying that all of that are basically just style cleanups which doesn't
> need to be back-ported in any way, so please drop the Fixes: tag.
>
> And you should probably change the subject line to something like
> "drm/amdgpu: cleanup shift coding style".
>
> Regards,
> Christian.
>
> >
> > Thanks and regards,
> > Advait
> >
> > On Sat, 5 Oct 2024 at 09:05, Sundararaju, Sathishkumar <sasundar@xxxxxxx> wrote:
> >>
> >>
> >> On 10/4/2024 11:30 PM, Alex Deucher wrote:
> >>> On Fri, Oct 4, 2024 at 5:15 AM Sundararaju, Sathishkumar
> >>> <sasundar@xxxxxxx> wrote:
> >>>> All occurrences of this error fix should have been together in a single patch both in _get and _set callbacks corresponding to f0b19b84d391, please avoid separate patch for each occurrence.
> >>>>
> >>>> Sorry Alex, I missed to note this yesterday.
> >>> I've dropped the patch.  Please pick it up once it's fixed up appropriately.
> >> Thanks Alex.
> >>
> >> Hi Advait,
> >> Please collate the changes together with Lijo's suggestion as well,
> >> "1ULL <<" instead of typecast, there are 3 occurrences of the error in
> >> f0b19b84d391.
> >>
> >> Regards,
> >> Sathish
> >>> Thanks,
> >>>
> >>> Alex
> >>>
> >>>> Regards,
> >>>> Sathish
> >>>>
> >>>>
> >>>> On 10/4/2024 1:46 PM, Advait Dhamorikar wrote:
> >>>>
> >>>> Fix shift-count-overflow when creating mask.
> >>>> The expression's value may not be what the
> >>>> programmer intended, because the expression is
> >>>> evaluated using a narrower integer type.
> >>>>
> >>>> Fixes: f0b19b84d391 ("drm/amdgpu: add amdgpu_jpeg_sched_mask debugfs")
> >>>> Signed-off-by: Advait Dhamorikar <advaitdhamorikar@xxxxxxxxx>
> >>>> ---
> >>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c | 2 +-
> >>>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
> >>>> index 95e2796919fc..7df402c45f40 100644
> >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
> >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
> >>>> @@ -388,7 +388,7 @@ static int amdgpu_debugfs_jpeg_sched_mask_get(void *data, u64 *val)
> >>>>     for (j = 0; j < adev->jpeg.num_jpeg_rings; ++j) {
> >>>>     ring = &adev->jpeg.inst[i].ring_dec[j];
> >>>>     if (ring->sched.ready)
> >>>> - mask |= 1 << ((i * adev->jpeg.num_jpeg_rings) + j);
> >>>> + mask |= (u64)1 << ((i * adev->jpeg.num_jpeg_rings) + j);
> >>>>     }
> >>>>     }
> >>>>     *val = mask;
>