netfilter-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [BUG] nf: xt_LED: led-always-blink invisible
@ 2014-06-19  9:17 Jiří Prchal
  2014-06-30 10:54 ` Pablo Neira Ayuso
  0 siblings, 1 reply; 7+ messages in thread
From: Jiří Prchal @ 2014-06-19  9:17 UTC (permalink / raw)
  To: coreteam, netfilter, netfilter-devel, kadlec, kaber, pablo

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.

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);
+        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;
  }


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [BUG] nf: xt_LED: led-always-blink invisible
  2014-06-19  9:17 [BUG] nf: xt_LED: led-always-blink invisible Jiří Prchal
@ 2014-06-30 10:54 ` Pablo Neira Ayuso
  2014-06-30 23:36   ` Bryan Wu
  0 siblings, 1 reply; 7+ messages in thread
From: Pablo Neira Ayuso @ 2014-06-30 10:54 UTC (permalink / raw)
  To: Jiří Prchal
  Cc: coreteam, netfilter-devel, kadlec, kaber, Bryan Wu,
	Richard Purdie

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);
> +        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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [BUG] nf: xt_LED: led-always-blink invisible
  2014-06-30 10:54 ` Pablo Neira Ayuso
@ 2014-06-30 23:36   ` Bryan Wu
  2014-07-25  7:21     ` [PATCH] nf: xt_LED: fix too short led-always-blink Jiří Prchal
  0 siblings, 1 reply; 7+ messages in thread
From: Bryan Wu @ 2014-06-30 23:36 UTC (permalink / raw)
  To: Pablo Neira Ayuso
  Cc: Jiří Prchal, coreteam, netfilter-devel, kadlec, kaber,
	Richard Purdie

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH] nf: xt_LED: fix too short led-always-blink
  2014-06-30 23:36   ` Bryan Wu
@ 2014-07-25  7:21     ` Jiří Prchal
  2014-07-25 12:22       ` Pablo Neira Ayuso
  0 siblings, 1 reply; 7+ messages in thread
From: Jiří Prchal @ 2014-07-25  7:21 UTC (permalink / raw)
  To: Bryan Wu, Pablo Neira Ayuso
  Cc: coreteam, netfilter-devel, kadlec, kaber, Richard Purdie,
	linux-leds

If led-always-blink is set, then between switch led OFF and ON
is almost zero time. So blink is invisible. This use oneshot led trigger
with fixed time 50ms witch is enough to see blink.

Signed-off-by: Jiri <jiri.prchal@aksignal.cz>
---
  net/netfilter/xt_LED.c | 8 +++++---
  1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/xt_LED.c b/net/netfilter/xt_LED.c
index 993de2b..29b5af4 100644
--- a/net/netfilter/xt_LED.c
+++ b/net/netfilter/xt_LED.c
@@ -55,6 +55,7 @@ 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; /* always blink 50ms */

  	/*
  	 * If "always blink" is enabled, and there's still some time until the
@@ -62,9 +63,10 @@ led_tg(struct sk_buff *skb, const struct xt_action_param *par)
  	 */
  	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_blink_oneshot(&ledinternal->netfilter_led_trigger,
+					  &t, &t, 1);
+        else
+		led_trigger_event(&ledinternal->netfilter_led_trigger, LED_FULL);

  	/* If there's a positive delay, start/update the timer */
  	if (ledinfo->delay > 0) {
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] nf: xt_LED: fix too short led-always-blink
  2014-07-25  7:21     ` [PATCH] nf: xt_LED: fix too short led-always-blink Jiří Prchal
@ 2014-07-25 12:22       ` Pablo Neira Ayuso
  2014-07-25 12:40         ` [PATCH v2] " Jiří Prchal
  0 siblings, 1 reply; 7+ messages in thread
From: Pablo Neira Ayuso @ 2014-07-25 12:22 UTC (permalink / raw)
  To: Jiří Prchal
  Cc: Bryan Wu, coreteam, netfilter-devel, kadlec, kaber,
	Richard Purdie, linux-leds

Hi,

On Fri, Jul 25, 2014 at 09:21:01AM +0200, Jiří Prchal wrote:
> If led-always-blink is set, then between switch led OFF and ON
> is almost zero time. So blink is invisible. This use oneshot led trigger
> with fixed time 50ms witch is enough to see blink.
> 
> Signed-off-by: Jiri <jiri.prchal@aksignal.cz>
> ---
>  net/netfilter/xt_LED.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/net/netfilter/xt_LED.c b/net/netfilter/xt_LED.c
> index 993de2b..29b5af4 100644
> --- a/net/netfilter/xt_LED.c
> +++ b/net/netfilter/xt_LED.c
> @@ -55,6 +55,7 @@ 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; /* always blink 50ms */

Please, define:

#define XT_LED_BLINK_DELAY 50 /* ms */

And use it in this way:

        unsigned long led_delay = XT_LED_BLINK_DELAY;

so it's consistent with what we have in net/mac80211/led.c

>  	/*
>  	 * If "always blink" is enabled, and there's still some time until the
> @@ -62,9 +63,10 @@ led_tg(struct sk_buff *skb, const struct xt_action_param *par)
>  	 */
>  	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_blink_oneshot(&ledinternal->netfilter_led_trigger,
> +					  &t, &t, 1);
> +        else
   ^^^^^^^^

Make sure you use tab identations of 8-chars. I see spaces there :-)

Thanks.
--
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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2] nf: xt_LED: fix too short led-always-blink
  2014-07-25 12:22       ` Pablo Neira Ayuso
@ 2014-07-25 12:40         ` Jiří Prchal
  2014-07-25 12:59           ` Pablo Neira Ayuso
  0 siblings, 1 reply; 7+ messages in thread
From: Jiří Prchal @ 2014-07-25 12:40 UTC (permalink / raw)
  To: Pablo Neira Ayuso
  Cc: Bryan Wu, coreteam, netfilter-devel, kadlec, kaber,
	Richard Purdie, linux-leds

If led-always-blink is set, then between switch led OFF and ON
is almost zero time. So blink is invisible. This use oneshot led trigger
with fixed time 50ms witch is enough to see blink.

Signed-off-by: Jiri <jiri.prchal@aksignal.cz>

v2:
defined constant XT_LED_BLINK_DELAY and use it instead of number
---
  net/netfilter/xt_LED.c | 10 +++++++---
  1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/xt_LED.c b/net/netfilter/xt_LED.c
index 993de2b..9c12f16 100644
--- a/net/netfilter/xt_LED.c
+++ b/net/netfilter/xt_LED.c
@@ -28,6 +28,8 @@

  #include <linux/netfilter/xt_LED.h>

+#define XT_LED_BLINK_DELAY 50 /* ms */
+
  MODULE_LICENSE("GPL");
  MODULE_AUTHOR("Adam Nielsen <a.nielsen@shikadi.net>");
  MODULE_DESCRIPTION("Xtables: trigger LED devices on packet match");
@@ -55,6 +57,7 @@ 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 led_delay = XT_LED_BLINK_DELAY;

  	/*
  	 * If "always blink" is enabled, and there's still some time until the
@@ -62,9 +65,10 @@ led_tg(struct sk_buff *skb, const struct xt_action_param *par)
  	 */
  	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_blink_oneshot(&ledinternal->netfilter_led_trigger,
+					  &led_delay, &led_delay, 1);
+	else
+		led_trigger_event(&ledinternal->netfilter_led_trigger, LED_FULL);

  	/* If there's a positive delay, start/update the timer */
  	if (ledinfo->delay > 0) {
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] nf: xt_LED: fix too short led-always-blink
  2014-07-25 12:40         ` [PATCH v2] " Jiří Prchal
@ 2014-07-25 12:59           ` Pablo Neira Ayuso
  0 siblings, 0 replies; 7+ messages in thread
From: Pablo Neira Ayuso @ 2014-07-25 12:59 UTC (permalink / raw)
  To: Jiří Prchal
  Cc: Bryan Wu, coreteam, netfilter-devel, kadlec, kaber,
	Richard Purdie, linux-leds

On Fri, Jul 25, 2014 at 02:40:42PM +0200, Jiří Prchal wrote:
> If led-always-blink is set, then between switch led OFF and ON
> is almost zero time. So blink is invisible. This use oneshot led trigger
> with fixed time 50ms witch is enough to see blink.
> 
> Signed-off-by: Jiri <jiri.prchal@aksignal.cz>
> 
> v2:
> defined constant XT_LED_BLINK_DELAY and use it instead of number

Strange, this doesn't apply. Perhaps your MUA is mangling the
attachment? Or using a very old tree as reference?

$ patch -p1 < /tmp/v2-nf-xt_LED-fix-too-short-led-always-blink.patch 
patching file net/netfilter/xt_LED.c
Hunk #1 FAILED at 28.
Hunk #2 FAILED at 55.
Hunk #3 FAILED at 62.
3 out of 3 hunks FAILED -- saving rejects to file net/netfilter/xt_LED.c.rej

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2014-07-25 12:59 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-06-19  9:17 [BUG] nf: xt_LED: led-always-blink invisible Jiří Prchal
2014-06-30 10:54 ` Pablo Neira Ayuso
2014-06-30 23:36   ` Bryan Wu
2014-07-25  7:21     ` [PATCH] nf: xt_LED: fix too short led-always-blink Jiří Prchal
2014-07-25 12:22       ` Pablo Neira Ayuso
2014-07-25 12:40         ` [PATCH v2] " Jiří Prchal
2014-07-25 12:59           ` Pablo Neira Ayuso

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).