Message ID | 1485272470-3497-3-git-send-email-geert+renesas@glider.be |
---|---|
State | Superseded, archived |
Delegated to: | David Miller |
Headers | show |
> 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
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
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)
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(-)