Re: [BUG] nf: xt_LED: led-always-blink invisible

[Bryan Wu <cooloney@gmail.com>]

On Mon, Jun 30, 2014 at 3:54 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Cc'ing LED subsystem developers.
>
> On Thu, Jun 19, 2014 at 11:17:49AM +0200, Jiří Prchal wrote:
>> Hi all,
>> probably I found bug in kernel 3.14.0 in xt_LED module when set
>> led-always-blink. If it is set, then between switch led OFF and ON
>> is almost zero time. So blink is invisible. I did some fix, but I'm
>> not sure if this way would be good. Please, help to direct me. This
>> is not final patch so don't punish me for coding style.
>
> Yes, coding style needs to be fixed.
>
> On top of that, I need a second opinion on this since I'm not familiar
> with the led subsystem.
>
> @Richard, Bryan: Would you comment on this, please?
>
> Thanks.
>
>> diff --git a/net/netfilter/xt_LED.c b/net/netfilter/xt_LED.c
>> index 993de2b..430584b 100644
>> --- a/net/netfilter/xt_LED.c
>> +++ b/net/netfilter/xt_LED.c
>> @@ -54,30 +54,32 @@ static unsigned int
>>  led_tg(struct sk_buff *skb, const struct xt_action_param *par)
>>  {
>>       const struct xt_led_info *ledinfo = par->targinfo;
>>       struct xt_led_info_internal *ledinternal = ledinfo->internal_data;
>> +        unsigned long t=50;
>>
>>       /*
>>        * If "always blink" is enabled, and there's still some time until the
>>        * LED will switch off, briefly switch it off now.
>>        */
>>       if ((ledinfo->delay > 0) && ledinfo->always_blink &&
>>           timer_pending(&ledinternal->timer))
>> -             led_trigger_event(&ledinternal->netfilter_led_trigger, LED_OFF);
>> -
>> -     led_trigger_event(&ledinternal->netfilter_led_trigger, LED_FULL);
>> +             //led_trigger_event(&ledinternal->netfilter_led_trigger, LED_OFF);
>> +          led_trigger_blink_oneshot(&ledinternal->netfilter_led_trigger, &t, &t, 1);

For this kind of package activity trigger. led_trigger_blink_oneshot()
is the right API to use.
And what's always_blink for? I think for each package event we detects
here we blink once LED.

Thanks,
-Bryan

>> +        else
>> +                led_trigger_event(&ledinternal->netfilter_led_trigger, LED_FULL);
>>
>>       /* If there's a positive delay, start/update the timer */
>>       if (ledinfo->delay > 0) {
>>               mod_timer(&ledinternal->timer,
>>                         jiffies + msecs_to_jiffies(ledinfo->delay));
>>
>>       /* Otherwise if there was no delay given, blink as fast as possible */
>>       } else if (ledinfo->delay == 0) {
>>               led_trigger_event(&ledinternal->netfilter_led_trigger, LED_OFF);
>>       }
>>
>>       /* else the delay is negative, which means switch on and stay on */
>>
>>       return XT_CONTINUE;
>>  }
>>
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html