nf: xt_LED: led-always-blink invisible

BUG Superseded Jiří Prchal 3 participants 7 messages Jun 19, 2014 – Jul 25, 2014

Subsystems (3)

Networking netfilter LED

lore

Share & tools

lore

Jiří Prchal

author

2014-06-19 09:17 UTC #0 lore raw

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.

net/netfilter/xt_LED.c +5 -3

index 993de2b..430584b 100644
@@ -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;
 }

Pablo Neira Ayuso

maintainer

2014-06-30 10:54 UTC #1 lore raw

Cc'ing LED subsystem developers.

Quoted 6 lines · Jiří Prchal

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.

Quoted 30 lines

net/netfilter/xt_LED.c +5 -3

index 993de2b..430584b 100644
@@ -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));

net/netfilter/xt_LED.c static unsigned int · line 85

Quoted 10 lines

net/netfilter/xt_LED.c +0 -0

 	/* 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Bryan Wu

2014-06-30 23:36 UTC #2 lore raw

Quoted 30 lines · Pablo Neira Ayuso

Cc'ing LED subsystem developers.
Quoted 6 lines · Jiří Prchal
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.
Quoted 11 lines
net/netfilter/xt_LED.c +1 -0
index 993de2b..430584b 100644
@@ -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;
 

Quoted 11 lines

net/netfilter/xt_LED.c static unsigned int · line 67

Quoted 11 lines

net/netfilter/xt_LED.c +2 -3

      /*
       * 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

Quoted 13 lines

net/netfilter/xt_LED.c +2 -0

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

Quoted 5 lines

net/netfilter/xt_LED.c static unsigned int · line 85
Quoted 5 lines
net/netfilter/xt_LED.c +0 -0
      /* 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jiří Prchal

author

2014-07-25 07:21 UTC #3 lore raw

PATCH 1/1 nf: xt_LED: fix too short led-always-blink

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 <[email protected]>

---
 net/netfilter/xt_LED.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

net/netfilter/xt_LED.c +5 -3

index 993de2b..29b5af4 100644
@@ -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) {

Signature · 1 line

1.9.1

Pablo Neira Ayuso

maintainer

2014-07-25 12:22 UTC #4 lore raw

Re: [PATCH] nf: xt_LED: fix too short led-always-blink

Hi,

Quoted 14 lines · Jiří Prchal

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 <[email protected]>
---
 net/netfilter/xt_LED.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
net/netfilter/xt_LED.c +0 -0
index 993de2b..29b5af4 100644

net/netfilter/xt_LED.c led_tg(struct sk_buff *skb, const struct xt_action_param *par) · line 58

Quoted 4 lines

net/netfilter/xt_LED.c +1 -0

@@ -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

net/netfilter/xt_LED.c led_tg(struct sk_buff *skb, const struct xt_action_param *par) · line 68

Quoted 12 lines

net/netfilter/xt_LED.c +3 -3

 	/*
 	 * 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jiří Prchal

author

2014-07-25 12:40 UTC #5 lore raw

PATCH v2 1/1 nf: xt_LED: fix too short led-always-blink

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 <[email protected]>

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(-)

net/netfilter/xt_LED.c +7 -3

index 993de2b..9c12f16 100644
@@ -28,6 +28,8 @@
 
 #include <linux/netfilter/xt_LED.h>
 
+#define XT_LED_BLINK_DELAY 50 /* ms */
+
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Adam Nielsen <[email protected]>");
 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) {

Signature · 1 line

1.9.1

Pablo Neira Ayuso

maintainer

2014-07-25 12:59 UTC #6 lore raw

Re: [PATCH v2] nf: xt_LED: fix too short led-always-blink

Quoted 8 lines · Jiří Prchal

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 <[email protected]>
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