Nouveau - Sep 2016 - [PATCH 2/3] drm/nouveau/led: guard against a division by 0

Reviewed-by: Karol Herbst <karolherbst at gmail.com>

2016-09-16 9:34 GMT+02:00 Martin Peres <martin.peres at
free.fr>:
> Signed-off-by: Martin Peres <martin.peres at free.fr>
> ---
>  drm/nouveau/nouveau_led.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drm/nouveau/nouveau_led.c b/drm/nouveau/nouveau_led.c
> index 5e28b5f..1f731da 100644
> --- a/drm/nouveau/nouveau_led.c
> +++ b/drm/nouveau/nouveau_led.c
> @@ -44,7 +44,10 @@ nouveau_led_get_brightness(struct led_classdev *led)
>         div =  nvif_rd32(device, 0x61c880) & 0x00ffffff;
>         duty = nvif_rd32(device, 0x61c884) & 0x00ffffff;
>
> -       return duty * LED_FULL / div;
> +       if (div > 0)
> +               return duty * LED_FULL / div;
> +       else
> +               return 0;
minor nitpick: you can drop the else, I don't mind though. Maybe it
would be clearer to do it the other way around though.

Or to do:

if (unlikely(div <= 0)) return 0;
return duty * LED_FULL / div;
>  }
>
>  static void
> --
> 2.8.0
>
> _______________________________________________
> Nouveau mailing list
> Nouveau at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

On 16/09/16 10:42, Karol Herbst wrote:
> Reviewed-by: Karol Herbst <karolherbst at gmail.com>
>
> 2016-09-16 9:34 GMT+02:00 Martin Peres <martin.peres at free.fr>:
>> Signed-off-by: Martin Peres <martin.peres at free.fr>
>> ---
>>   drm/nouveau/nouveau_led.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drm/nouveau/nouveau_led.c b/drm/nouveau/nouveau_led.c
>> index 5e28b5f..1f731da 100644
>> --- a/drm/nouveau/nouveau_led.c
>> +++ b/drm/nouveau/nouveau_led.c
>> @@ -44,7 +44,10 @@ nouveau_led_get_brightness(struct led_classdev *led)
>>          div =  nvif_rd32(device, 0x61c880) & 0x00ffffff;
>>          duty = nvif_rd32(device, 0x61c884) & 0x00ffffff;
>>
>> -       return duty * LED_FULL / div;
>> +       if (div > 0)
>> +               return duty * LED_FULL / div;
>> +       else
>> +               return 0;
> minor nitpick: you can drop the else, I don't mind though. Maybe it
> would be clearer to do it the other way around though.
>
> Or to do:
>
> if (unlikely(div <= 0)) return 0;
> return duty * LED_FULL / div;
The unlikely is a good idea, not a fan of single lines if conditions 
though :s

2016-09-16 11:14 GMT+02:00 Martin Peres <martin.peres at
free.fr>:
> On 16/09/16 10:42, Karol Herbst wrote:
>>
>> Reviewed-by: Karol Herbst <karolherbst at gmail.com>
>>
>> 2016-09-16 9:34 GMT+02:00 Martin Peres <martin.peres at free.fr>:
>>>
>>> Signed-off-by: Martin Peres <martin.peres at free.fr>
>>> ---
>>>   drm/nouveau/nouveau_led.c | 5 ++++-
>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drm/nouveau/nouveau_led.c b/drm/nouveau/nouveau_led.c
>>> index 5e28b5f..1f731da 100644
>>> --- a/drm/nouveau/nouveau_led.c
>>> +++ b/drm/nouveau/nouveau_led.c
>>> @@ -44,7 +44,10 @@ nouveau_led_get_brightness(struct led_classdev
*led)
>>>          div =  nvif_rd32(device, 0x61c880) & 0x00ffffff;
>>>          duty = nvif_rd32(device, 0x61c884) & 0x00ffffff;
>>>
>>> -       return duty * LED_FULL / div;
>>> +       if (div > 0)
>>> +               return duty * LED_FULL / div;
>>> +       else
>>> +               return 0;
>>
>> minor nitpick: you can drop the else, I don't mind though. Maybe it
>> would be clearer to do it the other way around though.
>>
>> Or to do:
>>
>> if (unlikely(div <= 0)) return 0;
>> return duty * LED_FULL / div;
>
>
> The unlikely is a good idea, not a fan of single lines if conditions though
> :s
the single line was me being lazy.

if (unlikely(div <= 0))
        return 0;
return duty * LED_FULL / div;

:p