Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1565945 > unrolled thread
| Started by | Geert Uytterhoeven <geert+renesas@glider.be> |
|---|---|
| First post | 2017-01-24 16:50 +0100 |
| Last post | 2017-01-25 11:00 +0100 |
| Articles | 3 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH 2/2] net: phy: leds: Fix truncated LED trigger names Geert Uytterhoeven <geert+renesas@glider.be> - 2017-01-24 16:50 +0100
Re: [PATCH 2/2] net: phy: leds: Fix truncated LED trigger names Andrew Lunn <andrew@lunn.ch> - 2017-01-24 21:10 +0100
Re: [PATCH 2/2] net: phy: leds: Fix truncated LED trigger names Geert Uytterhoeven <geert@linux-m68k.org> - 2017-01-25 11:00 +0100
| From | Geert Uytterhoeven <geert+renesas@glider.be> |
|---|---|
| Date | 2017-01-24 16:50 +0100 |
| Subject | [PATCH 2/2] net: phy: leds: Fix truncated LED trigger names |
| Message-ID | <t3bw5-66h-1@gated-at.bofh.it> |
Commit 4567d686f5c6d955 ("phy: increase size of MII_BUS_ID_SIZE and
bus_id") increased the size of MII bus IDs, but forgot to update the
private definition in <linux/phy_led_triggers.h>.
This may cause:
1. Truncation of LED trigger names,
2. Duplicate LED trigger names,
3. Failures registering LED triggers,
4. Crashes due to bad error handling in the LED trigger failure path.
To fix this, and prevent the definitions going out of sync again in the
future, let the PHY LED trigger code use the existing MII_BUS_ID_SIZE
definition. Note that this requires moving the #include down, after the
definition of MII_BUS_ID_SIZE.
Example:
- Before I had triggers "ee700000.etherne:01:100Mbps" and
"ee700000.etherne:01:10Mbps",
- After the increase of MII_BUS_ID_SIZE, both became
"ee700000.ethernet-ffffffff:01:" => FAIL,
- Now, the triggers are "ee700000.ethernet-ffffffff:01:100Mbps" and
"ee700000.ethernet-ffffffff:01:10Mbps", which are unique again.
Fixes: 4567d686f5c6d955 ("phy: increase size of MII_BUS_ID_SIZE and bus_id")
Fixes: 2e0bc452f4721520 ("net: phy: leds: add support for led triggers on phy link state change")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
include/linux/phy.h | 3 ++-
include/linux/phy_led_triggers.h | 3 +--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 5c9d2529685fe215..f6ab919528ab3627 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -25,7 +25,6 @@
#include <linux/timer.h>
#include <linux/workqueue.h>
#include <linux/mod_devicetable.h>
-#include <linux/phy_led_triggers.h>
#include <linux/atomic.h>
@@ -339,6 +338,8 @@ struct phy_c45_device_ids {
u32 device_ids[8];
};
+#include <linux/phy_led_triggers.h>
+
/* phy_device: An instance of a PHY
*
* drv: Pointer to the driver for this PHY instance
diff --git a/include/linux/phy_led_triggers.h b/include/linux/phy_led_triggers.h
index a2daea0a37d2ae14..69dffb4fc5a294e9 100644
--- a/include/linux/phy_led_triggers.h
+++ b/include/linux/phy_led_triggers.h
@@ -20,9 +20,8 @@
#include <linux/leds.h>
#define PHY_LED_TRIGGER_SPEED_SUFFIX_SIZE 10
-#define PHY_MII_BUS_ID_SIZE (20 - 3)
-#define PHY_LINK_LED_TRIGGER_NAME_SIZE (PHY_MII_BUS_ID_SIZE + \
+#define PHY_LINK_LED_TRIGGER_NAME_SIZE (MII_BUS_ID_SIZE + \
FIELD_SIZEOF(struct mdio_device, addr)+\
PHY_LED_TRIGGER_SPEED_SUFFIX_SIZE)
--
1.9.1
[toc] | [next] | [standalone]
| From | Andrew Lunn <andrew@lunn.ch> |
|---|---|
| Date | 2017-01-24 21:10 +0100 |
| Message-ID | <t3fzJ-kX-49@gated-at.bofh.it> |
| In reply to | #1565945 |
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 5c9d2529685fe215..f6ab919528ab3627 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -25,7 +25,6 @@
> #include <linux/timer.h>
> #include <linux/workqueue.h>
> #include <linux/mod_devicetable.h>
> -#include <linux/phy_led_triggers.h>
>
> #include <linux/atomic.h>
>
> @@ -339,6 +338,8 @@ struct phy_c45_device_ids {
> u32 device_ids[8];
> };
>
> +#include <linux/phy_led_triggers.h>
> +
> /* phy_device: An instance of a PHY
> *
> * drv: Pointer to the driver for this PHY instance
> diff --git a/include/linux/phy_led_triggers.h b/include/linux/phy_led_triggers.h
> index a2daea0a37d2ae14..69dffb4fc5a294e9 100644
> --- a/include/linux/phy_led_triggers.h
> +++ b/include/linux/phy_led_triggers.h
> @@ -20,9 +20,8 @@
> #include <linux/leds.h>
>
> #define PHY_LED_TRIGGER_SPEED_SUFFIX_SIZE 10
> -#define PHY_MII_BUS_ID_SIZE (20 - 3)
>
> -#define PHY_LINK_LED_TRIGGER_NAME_SIZE (PHY_MII_BUS_ID_SIZE + \
> +#define PHY_LINK_LED_TRIGGER_NAME_SIZE (MII_BUS_ID_SIZE + \
> FIELD_SIZEOF(struct mdio_device, addr)+\
> PHY_LED_TRIGGER_SPEED_SUFFIX_SIZE)
Hi Geert
Using the macro is great, but it does seem a bit ugly having the
include in the middle of the file.
As far as i can see, phy.h only uses a pointer to a struct
phy_led_trigger, not struct phy_led_trigger itself. Could you try
removing the header file all together and just have a forward
declaration of phy_led_trigger?
Thanks
Andrew
[toc] | [prev] | [next] | [standalone]
| From | Geert Uytterhoeven <geert@linux-m68k.org> |
|---|---|
| Date | 2017-01-25 11:00 +0100 |
| Message-ID | <t3swV-6x-5@gated-at.bofh.it> |
| In reply to | #1566113 |
Hi Andrew,
On Tue, Jan 24, 2017 at 9:03 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>> index 5c9d2529685fe215..f6ab919528ab3627 100644
>> --- a/include/linux/phy.h
>> +++ b/include/linux/phy.h
>> @@ -25,7 +25,6 @@
>> #include <linux/timer.h>
>> #include <linux/workqueue.h>
>> #include <linux/mod_devicetable.h>
>> -#include <linux/phy_led_triggers.h>
>>
>> #include <linux/atomic.h>
>>
>> @@ -339,6 +338,8 @@ struct phy_c45_device_ids {
>> u32 device_ids[8];
>> };
>>
>> +#include <linux/phy_led_triggers.h>
>> +
>> /* phy_device: An instance of a PHY
>> *
>> * drv: Pointer to the driver for this PHY instance
>> diff --git a/include/linux/phy_led_triggers.h b/include/linux/phy_led_triggers.h
>> index a2daea0a37d2ae14..69dffb4fc5a294e9 100644
>> --- a/include/linux/phy_led_triggers.h
>> +++ b/include/linux/phy_led_triggers.h
>> @@ -20,9 +20,8 @@
>> #include <linux/leds.h>
>>
>> #define PHY_LED_TRIGGER_SPEED_SUFFIX_SIZE 10
>> -#define PHY_MII_BUS_ID_SIZE (20 - 3)
>>
>> -#define PHY_LINK_LED_TRIGGER_NAME_SIZE (PHY_MII_BUS_ID_SIZE + \
>> +#define PHY_LINK_LED_TRIGGER_NAME_SIZE (MII_BUS_ID_SIZE + \
>> FIELD_SIZEOF(struct mdio_device, addr)+\
>> PHY_LED_TRIGGER_SPEED_SUFFIX_SIZE)
>
> Hi Geert
>
> Using the macro is great, but it does seem a bit ugly having the
> include in the middle of the file.
>
> As far as i can see, phy.h only uses a pointer to a struct
> phy_led_trigger, not struct phy_led_trigger itself. Could you try
> removing the header file all together and just have a forward
> declaration of phy_led_trigger?
Thanks for the suggestion!
Yes, the include can be removed. A forward declaration isn't even needed.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web