Re: [PATCH v4 06/12] ARM: dove: add gigabit ethernet and mvmdio device tree nodes
On Tue, May 21, 2013 at 06:41:44PM +0200, Sebastian Hesselbarth wrote:
> This patch adds orion-eth and mvmdio device tree nodes for DT enabled
> Dove boards. As there is only one ethernet controller on Dove, a default
> phy node is also added with a note to set its reg property on a per-board
> basis.
>
> Signed-off-by: Sebastian Hesselbarth
> ---
> Changelog:
> v3->v4:
> - convert to new device tree binding
>
> Cc: David Miller
> Cc: Lennert Buytenhek
> Cc: Jason Cooper
> Cc: Andrew Lunn
> Cc: Benjamin Herrenschmidt
> Cc: net...@vger.kernel.org
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-ker...@vger.kernel.org
> ---
> arch/arm/boot/dts/dove-cubox.dts |7 +++
> arch/arm/boot/dts/dove.dtsi | 35 +++
> 2 files changed, 42 insertions(+)
>
> diff --git a/arch/arm/boot/dts/dove-cubox.dts
> b/arch/arm/boot/dts/dove-cubox.dts
> index 7e3065a..02618fa 100644
> --- a/arch/arm/boot/dts/dove-cubox.dts
> +++ b/arch/arm/boot/dts/dove-cubox.dts
> @@ -49,6 +49,13 @@
> &uart0 { status = "okay"; };
> &sata0 { status = "okay"; };
> &i2c0 { status = "okay"; };
> +&mdio { status = "okay"; };
> +ð { status = "okay"; };
> +
> +ðphy {
> + compatible = "marvell,88e1310";
> + reg = <1>;
> +};
>
> &sdio0 {
> status = "okay";
> diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
> index 6cab468..8612658 100644
> --- a/arch/arm/boot/dts/dove.dtsi
> +++ b/arch/arm/boot/dts/dove.dtsi
> @@ -258,5 +258,40 @@
> dmacap,xor;
> };
> };
> +
> + mdio: mdio-bus@72004 {
> + compatible = "marvell,orion-mdio";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0x72004 0x84>;
> + interrupts = <30>;
> + clocks = <&gate_clk 2>;
> + status = "disabled";
> +
> + ethphy: ethernet-phy {
> + device-type = "ethernet-phy";
> + /* set phy address in board file */
> + };
> + };
> +
> + eth: ethernet-controller@72000 {
> + compatible = "marvell,orion-eth";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0x72000 0x4000>;
> + clocks = <&gate_clk 2>;
> + marvell,tx-checksum-limit = <1600>;
> + status = "disabled";
> +
> + ethernet-port@0 {
> + device_type = "network";
> + compatible = "marvell,orion-eth-port";
> + reg = <0>;
> + interrupts = <29>;
> + /* overwrite MAC address in bootloader */
> + local-mac-address = [00 00 00 00 00 00];
Hi Sebastian
Its probably a good idea to set the local administration bit in this
MAC address. i.e. first byte is 02.
Andrew
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v4 00/12] net: mv643xx_eth DT support and fixes
On Tue, May 21, 2013 at 06:41:38PM +0200, Sebastian Hesselbarth wrote: > This patch set picks up work by Florian Fainelli bringing full DT > support to mv643xx_eth and Marvell SoCs using it. Hi Sebastian I tested on my QNAP and topkick. Works great. Tested-by: Andrew Lunn Andrew ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] net: mv643xx_eth: proper initialization for Kirkwood SoCs
> > Why are you not keen on this? It seems like normal device driver > > practice, that is what the data field of of_device_id is typically > > used for.. > > I'm not keen on it because we don't have a document saying "All kirkwood > SoCs need PSC1 set to X after reset." We know it, but have we tested > the 6282? 6282 looses its MAC address, that much i know. I've no idea about PSC1, but if its MAC address behaviour is the same as 6281, is expect PSC1 is the same. Andrew ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 21/44] power/reset: gpio-poweroff: Register with kernel poweroff handler
On Mon, Oct 06, 2014 at 10:28:23PM -0700, Guenter Roeck wrote:
> Register with kernel poweroff handler instead of setting pm_power_off
> directly. Register with a low priority value of 64 to reflect that
> the original code only sets pm_power_off if it was not already set.
>
> Other changes:
>
> Drop note that there can not be an additional instance of this driver.
> The original reason no longer applies, it should be obvious that there
> can only be one instance of the driver if static variables are used to
> reflect its state, and support for multiple instances can now be added
> easily if needed by avoiding static variables.
>
> Do not create an error message if another poweroff handler has already been
> registered. This is perfectly normal and acceptable.
>
> Do not display a warning traceback if the poweroff handler fails to
> power off the system. There may be other poweroff handlers.
I would prefer to keep the warning traceback. We found on some
hardware the GPIO transitions were too fast and it failed to power
off. Seeing the traceback gives an idea where to go look for the
problem.
Other than that,
Acked-by: Andrew Lunn
>
> Cc: Sebastian Reichel
> Cc: Dmitry Eremin-Solenikov
> Cc: David Woodhouse
> Signed-off-by: Guenter Roeck
> ---
> drivers/power/reset/gpio-poweroff.c | 36 ++--
> 1 file changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/power/reset/gpio-poweroff.c
> b/drivers/power/reset/gpio-poweroff.c
> index ce849bc..e95a7a1 100644
> --- a/drivers/power/reset/gpio-poweroff.c
> +++ b/drivers/power/reset/gpio-poweroff.c
> @@ -14,18 +14,18 @@
> #include
> #include
> #include
> +#include
> +#include
> #include
> #include
> #include
> #include
>
> -/*
> - * Hold configuration here, cannot be more than one instance of the driver
> - * since pm_power_off itself is global.
> - */
> static struct gpio_desc *reset_gpio;
>
> -static void gpio_poweroff_do_poweroff(void)
> +static int gpio_poweroff_do_poweroff(struct notifier_block *this,
> + unsigned long unused1, void *unused2)
> +
> {
> BUG_ON(!reset_gpio);
>
> @@ -42,20 +42,18 @@ static void gpio_poweroff_do_poweroff(void)
> /* give it some time */
> mdelay(3000);
>
> - WARN_ON(1);
> + return NOTIFY_DONE;
> }
>
> +static struct notifier_block gpio_poweroff_nb = {
> + .notifier_call = gpio_poweroff_do_poweroff,
> + .priority = 64,
> +};
> +
> static int gpio_poweroff_probe(struct platform_device *pdev)
> {
> bool input = false;
> -
> - /* If a pm_power_off function has already been added, leave it alone */
> - if (pm_power_off != NULL) {
> - dev_err(&pdev->dev,
> - "%s: pm_power_off function already registered",
> -__func__);
> - return -EBUSY;
> - }
> + int err;
>
> reset_gpio = devm_gpiod_get(&pdev->dev, NULL);
> if (IS_ERR(reset_gpio))
> @@ -77,14 +75,16 @@ static int gpio_poweroff_probe(struct platform_device
> *pdev)
> }
> }
>
> - pm_power_off = &gpio_poweroff_do_poweroff;
> - return 0;
> + err = register_poweroff_handler(&gpio_poweroff_nb);
> + if (err)
> + dev_err(&pdev->dev, "Failed to register poweroff handler\n");
> +
> + return err;
> }
>
> static int gpio_poweroff_remove(struct platform_device *pdev)
> {
> - if (pm_power_off == &gpio_poweroff_do_poweroff)
> - pm_power_off = NULL;
> + unregister_poweroff_handler(&gpio_poweroff_nb);
>
> return 0;
> }
> --
> 1.9.1
>
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 23/44] power/reset: qnap-poweroff: Register with kernel poweroff handler
On Mon, Oct 06, 2014 at 10:28:25PM -0700, Guenter Roeck wrote:
> Register with kernel poweroff handler instead of setting pm_power_off
> directly. Register with default priority value of 128 to reflect that
> the original code generates an error if another poweroff handler has
> already been registered when the driver is loaded.
>
> Cc: Sebastian Reichel
> Cc: Dmitry Eremin-Solenikov
> Cc: David Woodhouse
> Signed-off-by: Guenter Roeck
Acked-by: Andrew Lunn
Thanks
Andrew
> ---
> drivers/power/reset/qnap-poweroff.c | 28 ++--
> 1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/power/reset/qnap-poweroff.c
> b/drivers/power/reset/qnap-poweroff.c
> index a75db7f..c474980 100644
> --- a/drivers/power/reset/qnap-poweroff.c
> +++ b/drivers/power/reset/qnap-poweroff.c
> @@ -16,7 +16,9 @@
>
> #include
> #include
> +#include
> #include
> +#include
> #include
> #include
> #include
> @@ -55,7 +57,8 @@ static void __iomem *base;
> static unsigned long tclk;
> static const struct power_off_cfg *cfg;
>
> -static void qnap_power_off(void)
> +static int qnap_power_off(struct notifier_block *this, unsigned long unused1,
> + void *unused2)
> {
> const unsigned divisor = ((tclk + (8 * cfg->baud)) / (16 * cfg->baud));
>
> @@ -72,14 +75,20 @@ static void qnap_power_off(void)
>
> /* send the power-off command to PIC */
> writel(cfg->cmd, UART1_REG(TX));
> +
> + return NOTIFY_DONE;
> }
>
> +static struct notifier_block qnap_poweroff_nb = {
> + .notifier_call = qnap_power_off,
> + .priority = 128,
> +};
> +
> static int qnap_power_off_probe(struct platform_device *pdev)
> {
> struct device_node *np = pdev->dev.of_node;
> struct resource *res;
> struct clk *clk;
> - char symname[KSYM_NAME_LEN];
>
> const struct of_device_id *match =
> of_match_node(qnap_power_off_of_match_table, np);
> @@ -106,22 +115,13 @@ static int qnap_power_off_probe(struct platform_device
> *pdev)
>
> tclk = clk_get_rate(clk);
>
> - /* Check that nothing else has already setup a handler */
> - if (pm_power_off) {
> - lookup_symbol_name((ulong)pm_power_off, symname);
> - dev_err(&pdev->dev,
> - "pm_power_off already claimed %p %s",
> - pm_power_off, symname);
> - return -EBUSY;
> - }
> - pm_power_off = qnap_power_off;
> -
> - return 0;
> + return register_poweroff_handler(&qnap_poweroff_nb);
> }
>
> static int qnap_power_off_remove(struct platform_device *pdev)
> {
> - pm_power_off = NULL;
> + unregister_poweroff_handler(&qnap_poweroff_nb);
> +
> return 0;
> }
>
> --
> 1.9.1
>
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 20/44] power/reset: restart-poweroff: Register with kernel poweroff handler
On Mon, Oct 06, 2014 at 10:28:22PM -0700, Guenter Roeck wrote:
> Register with kernel poweroff handler instead of seting pm_power_off
> directly. Register as poweroff handler of last resort since the driver
> does not really power off the system but executes a restart.
I would not say last resort, this is how it is designed to work. There
is no way to turn the power off from with linux, it is designed that
u-boot will put the hardware into minimal power consumption until the
"power" button is pressed.
Other than that,
Acked-by: Andrew Lunn
Thanks
Andrew
>
> Cc: Sebastian Reichel
> Cc: Dmitry Eremin-Solenikov
> Cc: David Woodhouse
> Signed-off-by: Guenter Roeck
> ---
> drivers/power/reset/restart-poweroff.c | 25 -
> 1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/power/reset/restart-poweroff.c
> b/drivers/power/reset/restart-poweroff.c
> index edd707e..5437697 100644
> --- a/drivers/power/reset/restart-poweroff.c
> +++ b/drivers/power/reset/restart-poweroff.c
> @@ -12,35 +12,34 @@
> */
> #include
> #include
> +#include
> #include
> #include
> #include
> +#include
> #include
> -#include
>
> -static void restart_poweroff_do_poweroff(void)
> +static int restart_poweroff_do_poweroff(struct notifier_block *this,
> + unsigned long unused1, void *unused2)
> {
> reboot_mode = REBOOT_HARD;
> machine_restart(NULL);
> +
> + return NOTIFY_DONE;
> }
>
> +static struct notifier_block restart_poweroff_handler = {
> + .notifier_call = restart_poweroff_do_poweroff,
> +};
> +
> static int restart_poweroff_probe(struct platform_device *pdev)
> {
> - /* If a pm_power_off function has already been added, leave it alone */
> - if (pm_power_off != NULL) {
> - dev_err(&pdev->dev,
> - "pm_power_off function already registered");
> - return -EBUSY;
> - }
> -
> - pm_power_off = &restart_poweroff_do_poweroff;
> - return 0;
> + return register_poweroff_handler(&restart_poweroff_handler);
> }
>
> static int restart_poweroff_remove(struct platform_device *pdev)
> {
> - if (pm_power_off == &restart_poweroff_do_poweroff)
> - pm_power_off = NULL;
> + unregister_poweroff_handler(&restart_poweroff_handler);
>
> return 0;
> }
> --
> 1.9.1
>
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 06/44] gpio-poweroff: Drop reference to pm_power_off from devicetree bindings
On Mon, Oct 06, 2014 at 10:28:08PM -0700, Guenter Roeck wrote: > pm_power_off is an implementation detail. Replace it with a more generic > description of the driver's functionality. > > Cc: Rob Herring > Cc: Pawel Moll > Cc: Mark Rutland > Signed-off-by: Guenter Roeck Acked-by: Andrew Lunn Thanks Andrew > --- > Documentation/devicetree/bindings/gpio/gpio-poweroff.txt | 10 +- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/gpio/gpio-poweroff.txt > b/Documentation/devicetree/bindings/gpio/gpio-poweroff.txt > index d4eab92..c95a1a6 100644 > --- a/Documentation/devicetree/bindings/gpio/gpio-poweroff.txt > +++ b/Documentation/devicetree/bindings/gpio/gpio-poweroff.txt > @@ -2,12 +2,12 @@ Driver a GPIO line that can be used to turn the power off. > > The driver supports both level triggered and edge triggered power off. > At driver load time, the driver will request the given gpio line and > -install a pm_power_off handler. If the optional properties 'input' is > -not found, the GPIO line will be driven in the inactive > +install a handler to power off the system. If the optional properties > +'input' is not found, the GPIO line will be driven in the inactive > state. Otherwise its configured as an input. > > -When the pm_power_off is called, the gpio is configured as an output, > -and drive active, so triggering a level triggered power off > +When the the poweroff handler is called, the gpio is configured as an > +output, and drive active, so triggering a level triggered power off > condition. This will also cause an inactive->active edge condition, so > triggering positive edge triggered power off. After a delay of 100ms, > the GPIO is set to inactive, thus causing an active->inactive edge, > @@ -24,7 +24,7 @@ Required properties: > > Optional properties: > - input : Initially configure the GPIO line as an input. Only reconfigure > - it to an output when the pm_power_off function is called. If this optional > + it to an output when the poweroff handler is called. If this optional >property is not specified, the GPIO is initialized as an output in its >inactive state. > > -- > 1.9.1 > > > ___ > linux-arm-kernel mailing list > linux-arm-ker...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 07/44] qnap-poweroff: Drop reference to pm_power_off from devicetree bindings
On Mon, Oct 06, 2014 at 10:28:09PM -0700, Guenter Roeck wrote: > Replace reference to pm_power_off (which is an implementation detail) > and replace it with a more generic description of the driver's functionality. Acked-by: Andrew Lunn Thanks Andrew > > Cc: Rob Herring > Cc: Pawel Moll > Cc: Mark Rutland > Signed-off-by: Guenter Roeck > --- > Documentation/devicetree/bindings/power_supply/qnap-poweroff.txt | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/power_supply/qnap-poweroff.txt > b/Documentation/devicetree/bindings/power_supply/qnap-poweroff.txt > index af25e77..1e2260a 100644 > --- a/Documentation/devicetree/bindings/power_supply/qnap-poweroff.txt > +++ b/Documentation/devicetree/bindings/power_supply/qnap-poweroff.txt > @@ -3,8 +3,8 @@ > QNAP NAS devices have a microcontroller controlling the main power > supply. This microcontroller is connected to UART1 of the Kirkwood and > Orion5x SoCs. Sending the character 'A', at 19200 baud, tells the > -microcontroller to turn the power off. This driver adds a handler to > -pm_power_off which is called to turn the power off. > +microcontroller to turn the power off. This driver installs a handler > +to power off the system. > > Synology NAS devices use a similar scheme, but a different baud rate, > 9600, and a different character, '1'. > -- > 1.9.1 > > > ___ > linux-arm-kernel mailing list > linux-arm-ker...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes
... > While looking at the DSA code, I noticed we have a > of_find_net_device_by_node(), and it looks like users of that are > similarly buggy - it looks like net/dsa/dsa.c is the only user. Fix > that too. ... > The mdiobus code also suffered from the same kind of leak, but thankfully > this only happened in one place - the mdio-mux code. Hi Russell I tested both of these with my board. It is a Freescale Vybrid, using the FEC ethernet driver, and i have three switches attached, using mdio-mux to give three mdio busses. No obvious regressions, my board boots, the switches are all present and correct. I built the FEC driver as a module, and it won't unload: kernel:unregister_netdevice: waiting for eth1 to become free. Usage count = 1 unregister_netdevice: waiting for eth1 to become free. Usage count = 1 i assume because DSA holds a reference. I've not tried a fully module build, DSA has issues with that. Tested-by: Andrew Lunn Thanks Andrew ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes
On Thu, Sep 24, 2015 at 03:15:54PM -0700, David Miller wrote: > From: Andrew Lunn > Date: Thu, 24 Sep 2015 23:57:31 +0200 > > > I built the FEC driver as a module, and it won't unload: > > > > kernel:unregister_netdevice: waiting for eth1 to become free. Usage count > > = 1 > > unregister_netdevice: waiting for eth1 to become free. Usage count = 1 > > > > i assume because DSA holds a reference. I've not tried a fully module > > build, DSA has issues with that. > > > > Tested-by: Andrew Lunn > > So, is this a regression? Sorry, worded that badly. Since DSA is still active, it should not be possible to unload the FEC driver. DSA should have a reference to it, and mdio-mux also should have a reference to the mdio bus of the FEC driver. As Russell requested, i will re-test without his patches, just to make sure. Andrew ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes
> Thanks for testing. Please could you confirm whether the same behaviour > is observed without the patches, just to make absolutely sure that isn't > a regression. So i tested this now. I have two FEC interfaces. One i my main access interface, and the second is used by DSA to access switches. With your patches, the module Used by count is equal to the number of interfaces which are up. Without your patches, the count is always 0. When i try to remove the fec module, without your patches, but DSA still using the interface, i get the same kernel:unregister_netdevice: waiting for eth1 to become free. Usage count = 1 as with your patch. So this is not a regression. Andrew ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] sbc8641: drop bogus PHY IRQ entries from DTS file
On Tue, Dec 08, 2015 at 05:44:02PM -0500, Paul Gortmaker wrote:
> This file was originally cloned off of the MPC8641D-HPCN reference
> platform, which actually had a PHY IRQ line connected. However
> this board does not. The bogus entry was largely inert and went
> undetected until commit 321beec5047af83db90c88114b7e664b156f49fe
> ("net: phy: Use interrupts when available in NOLINK state") was
> added to the tree.
>
> With the above commit, the board fails to NFS boot since it sits
> waiting for a PHY IRQ event that of course never arrives. Removing
> the bogus entries from the DTS file fixes the issue.
Hi Paul
Originally the interrupt is used for detecting the link has gone
down. That would of also been bogus before. Have you tried this? If
that is also broken, maybe you need to add a fixes: tag so that it
gets back ported?
Andrew
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] sbc8641: drop bogus PHY IRQ entries from DTS file
> > Originally the interrupt is used for detecting the link has gone > > down. That would of also been bogus before. Have you tried this? If > > Haven't tried it, but chances are you are right. > > > that is also broken, maybe you need to add a fixes: tag so that it > > gets back ported? > > Nobody noticed in the past ~7 years or so, but I guess I can point Greg > at it once it is present in mainline. The main reason I'd Cc'd netdev > is just to possibly save anyone else the investigation if they ran into > the same issue on a different board -which seems highly probable IMHO. Yes, you are probably right about other boards. So this change might be considered as causing a regression. But if they are already broken, because link down does not work, there might be less demand to get the change reverted Andrew ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/3][v2] net: phy: introduce 1000BASE-KX and 10GBASE-KR
On Thu, Jan 14, 2016 at 04:23:59PM +0800, shh@gmail.com wrote: > From: Shaohui Xie > > This commit adds necessary definitions for the PHY layer to recognize > backplane Ethernet 1000BASE-KX and 10GBASE-KR as valid PHY interfaces, > "1000base-kx" for 1000BASE-KX, "10gbase-kr" for 10GBASE-KR. > > Signed-off-by: Shaohui Xie > --- > changes in v2: > new patch. > > Documentation/devicetree/bindings/net/ethernet.txt | 4 ++-- > include/linux/phy.h| 6 ++ > 2 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/net/ethernet.txt > b/Documentation/devicetree/bindings/net/ethernet.txt > index 5d88f37..1166a5c 100644 > --- a/Documentation/devicetree/bindings/net/ethernet.txt > +++ b/Documentation/devicetree/bindings/net/ethernet.txt > @@ -11,8 +11,8 @@ The following properties are common to the Ethernet > controllers: >the maximum frame size (there's contradiction in ePAPR). > - phy-mode: string, operation mode of the PHY interface; supported values are >"mii", "gmii", "sgmii", "qsgmii", "tbi", "rev-mii", "rmii", "rgmii", > "rgmii-id", > - "rgmii-rxid", "rgmii-txid", "rtbi", "smii", "xgmii"; this is now a de-facto > - standard property; > + "rgmii-rxid", "rgmii-txid", "rtbi", "smii", "xgmii", "1000base-kx", > "10gbase-kr"; > + this is now a de-facto standard property; I know very little about this, so i'm just asking a question. None of the other interface modes contain a bit rate. So is the bit rate needed for your two new modes? With a bit of googling, K means copper backplane, X means 4B/5B and R means 64B/66B. Could there be a 10Gbps KX? a 1GBps KR? Do we actually need the speed here, or is kx and kr sufficient? Thanks Andrew ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/3][v2] net: phy: introduce 1000BASE-KX and 10GBASE-KR
> [S.H] the fsl backplane, e.g. 10GBASE-KR, needs software to handle link > training, > It's to train link partner, and trained by link partner parallel. > > But if media type is not copper, e.g. optical module, we won't need this. So what we actually need to know is copper vs fibre? Andrew ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/3][v2] net: phy: introduce 1000BASE-KX and 10GBASE-KR
On Tue, Jan 19, 2016 at 05:00:35AM +, Shaohui Xie wrote: > > -Original Message- > > From: Andrew Lunn [mailto:and...@lunn.ch] > > Sent: Monday, January 18, 2016 11:15 PM > > To: Shaohui Xie > > Cc: Sebastian Hesselbarth; Florian Fainelli; shh@gmail.com; > > devicet...@vger.kernel.org; net...@vger.kernel.org; linuxppc- > > d...@lists.ozlabs.org; da...@davemloft.net; Shaohui Xie > > Subject: Re: [PATCH 1/3][v2] net: phy: introduce 1000BASE-KX and 10GBASE-KR > > > > > [S.H] the fsl backplane, e.g. 10GBASE-KR, needs software to handle > > > link training, It's to train link partner, and trained by link partner > > parallel. > > > > > > But if media type is not copper, e.g. optical module, we won't need this. > > > > So what we actually need to know is copper vs fibre? > Copper is not enough to indicate backplane, since backplane is > always copper, but copper is not always backplane. O.K, lets try again If it is copper backplane you need to perform training. Looking at the driver probe function, it is either 1000BASE-KX, no training needed, or else it is 10GBASE-RK and training is needed. Looking at fsl_backplane_config_aneg() you expect phydev->speed to be set, and from the speed you then kick of either KR autoneg or KX autoneg. Could you also start the training at this point? Use the speed to indicate if training is needed? Andrew ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/3][v2] net: phy: introduce 1000BASE-KX and 10GBASE-KR
> The reason I mentioned maybe I should put the backplane property in > fsl's binding is because the backplane implementation is really > vendor specific, it's heavily relay how hardware implements the > feature, maybe another vendor's hardware only needs a boolean > property for driver to tell it should work in backplane, hardware > can deal with different modes, or even no any special property > needed if the hardware is strong enough to handle any connections, I > cannot assume. But I know what fsl's hardware needs to support > backplane. This is the key point really. We don't really care about the Freescale PCS. We want a generic solution for 1000BASE-KX and 10GBASE-KR, independent of who makes it, Marvell, Micrel, Broadcom, or Acme. So, what generic properties are needed for 1000BASE-KX and 10GBASE-KR? Properties that most/all manufactures are likely to need? Andrew ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: ethernet "bus" number in DTS ?
On Tue, Oct 23, 2018 at 04:49:59PM +, Joakim Tjernlund wrote:
> SPI (and others) has a way to define bus number in a aliases:
> aliases {
> ethernet4 = &enet4;
> ethernet0 = &enet0;
> ethernet1 = &enet1;
> ethernet2 = &enet2;
> ethernet3 = &enet3;
> spi0 = &spi0
> };
> The 0 in the spi0 alias will translate to bus num 0 so one can control the
> /dev nodes, like /dev/spidev0
> I am looking for the same for ethernet devices:
> ethernet4 = &enet4; /* should become eth4 */
> ethernet0 = &enet0; /* should become eth0 */
> but I cannot find something like that for eth devices.
>
> Could such functionality be added?
Hi Jocke
This has been discussed before. Take a look at
arch/arm/boot/dts/armada-38x.dtsi
/*
* As a special exception to the "order by
* register address" rule, the eth0 node is
* placed here to ensure that it gets
* registered as the first interface, since
* the network subsystem doesn't allow naming
* interfaces using DT aliases. Without this,
* the ordering of interfaces is different
* from the one used in U-Boot and the
* labeling of interfaces on the boards, which
* is very confusing for users.
*/
You should be able to find the discuss about this, and why aliases
cannot be used.
Andrew
Re: DPAA Ethernet traffice troubles with Linux kernel
> When i use mii-tool too Kick the tranceiver... it comes alive.. i can > ping the eth0 itself > > root@X5000LNX:/home/skateman# mii-tool -R eth0 > resetting the transceiver... > root@X5000LNX:/home/skateman# ping 192.168.22.44 > PING 192.168.22.44 (192.168.22.44) 56(84) bytes of data. > 64 bytes from 192.168.22.44: icmp_seq=1 ttl=64 time=0.045 ms > 64 bytes from 192.168.22.44: icmp_seq=2 ttl=64 time=0.046 ms > 64 bytes from 192.168.22.44: icmp_seq=3 ttl=64 time=0.047 ms > 64 bytes from 192.168.22.44: icmp_seq=4 ttl=64 time=0.048 ms What PHY driver are you using? This smells a bit like an RGMII-ID problem. Andrew
Re: DPAA Ethernet traffice troubles with Linux kernel
> Hi, just saw this and thought of a small patch I just wrote for mdio bus, o > idea > if it is relevant but here goes: > > From fe0b98d54a79779482700676331b4d10a0f3cada Mon Sep 17 00:00:00 2001 > From: Joakim Tjernlund > Date: Sun, 14 Jan 2018 21:27:20 +0100 > Subject: [PATCH] of_mdiobus_register: Continue after error > > of_mdiobus_register unregister itself if one phy fails to register > which is bad for system having all its PHYs on the same MDIO bus. > Just log the error and continue with the remaining PHYs instead. > > Signed-off-by: Joakim Tjernlund Hi Joakim You appear to be using an old kernel. Take a look at: commit 95f566de0269a0c59fd6a737a147731302136429 Author: Madalin Bucur Date: Tue Jan 9 14:43:34 2018 +0200 of_mdio: avoid MDIO bus removal when a PHY is missing If one of the child devices is missing the of_mdiobus_register_phy() call will return -ENODEV. When a missing device is encountered the registration of the remaining PHYs is stopped and the MDIO bus will fail to register. Propagate all errors except ENODEV to avoid it. Signed-off-by: Madalin Bucur Reviewed-by: Andrew Lunn Signed-off-by: David S. Miller Andrew
Re: DPAA Ethernet traffice troubles with Linux kernel
> > You appear to be using an old kernel. Take a look at: > > Not really, I am using 4.14.x and I don't think that is old. Development for 4.14 stopped somewhere around the beginning of September. So there has been over 4 months of work since then. We are clearly interested in fixing bugs in that kernel, since it is the current stable version. But when reporting bugs, please let is know if the latest version of the network kernel, it://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git has the issue. > Seems like this patch hasn't been sent to 4.14.x. If it fixes a bug for you, please request the fix is added to stable. Andrew
Re: DPAA Ethernet traffice troubles with Linux kernel
> *Given this if we disable that bit, we get the matching "Universally > Administered Address" 0100 (Binary), 04 (Hex) -> "04:00:00", hence my > question:"* > > This has something to do with the MAC adresses being locally administered > .. and since whe can use Uboot and choose any Mac addr we want, this could > make sense.. You cannot just flip this bit. Universally administered addresses are allocated by the IEEE. You need to purchase them from the IEEE. Locally administered MAC addresses you can use, and any sane DHCP server should work with them. This also does not fit with mii-tool -R observation. Andrew
Re: DPAA Ethernet traffice troubles with Linux kernel
> That doesn't work really, having users to hit the bug, debug it, fix it and > then > find it fixed already in upstream, then specifically request it to be > backported to stable. > I don't need this fix to be backported, already got it. Someone else might > though. The "someone else might though" is a big point of asking for it to added to stable. The other reason is it means one less patch you need to maintain in your build. > I would be interested in bug fixes upstream which fixes: Did you try upstream? Does it give the same errors? Andrew
Re: DPAA Ethernet traffice troubles with Linux kernel
> > commit 4d8ee1935bcd666360311dfdadeee235d682d69a > > Author: Florian Fainelli > > Date: Tue Aug 22 15:24:47 2017 -0700 > > fsl/man: Inherit parent device and of_node > > > > and was later addressed by this patch set: > > > > http://patchwork.ozlabs.org/project/netdev/list/?series=8462&state=* > > > > Even with these errors printed, all is working fine, it's just the > > second probing that fails. Adding the latter patches or reverting > > the one above makes the errors prints dissapear. > > Looking at the above patch seriers I see it is in state Accepted and has been > there > since 2017-10-16 > That seems like a awful long to wait in before getting into Linux, is there > something > holding these patches back ? They are in Linux, have been since October 16th. But at the moment, they are only in v4.15, not v4.14. These patches probably don't fit the stable rules, for getting them added to v4.14. https://github.com/torvalds/linux/blob/master/Documentation/process/stable-kernel-rules.rst What is needed is a minimal fix. Or just wait until Sunday, when there is a good chance v4.15 will be released. Andrew
Re: PA Semi PWRficient Gigabit Ethernet doesn't work anymore since the first networking updates for the kernel 4.16
On Sun, Feb 04, 2018 at 05:47:03PM +0100, Christian Zigotzky wrote:
> Hello,
>
> The PA Semi PWRficient Gigabit Ethernet doesn't work anymore since the first
> networking updates [1] for the kernel 4.16.
>
> Error messages:
>
> [ 0.634241] libphy: pasemi gpio mdio bus: probed
> [ 0.634749] pasemi gpio mdio bus: Cannot register as MDIO bus, err -38
-38 is ENOSYS.
> --- a/drivers/net/phy/mdio_bus.c 2018-02-03 17:34:46.973045321 +0100
> +++ b/drivers/net/phy/mdio_bus.c 2018-02-04 11:03:14.909093360 +0100
> @@ -47,41 +47,11 @@
>
> #include "mdio-boardinfo.h"
>
> -static int mdiobus_register_gpiod(struct mdio_device *mdiodev)
> -{
> - struct gpio_desc *gpiod = NULL;
> -
> - /* Deassert the optional reset signal */
> - if (mdiodev->dev.of_node)
> - gpiod = fwnode_get_named_gpiod(&mdiodev->dev.of_node->fwnode,
> -"reset-gpios", 0, GPIOD_OUT_LOW,
> -"PHY reset");
So i think you don't have GPIOLIB enabled. Hence you are hitting
http://elixir.free-electrons.com/linux/latest/source/include/linux/gpio/consumer.h#L470
static inline
struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
const char *propname, int index,
enum gpiod_flags dflags,
const char *label)
{
return ERR_PTR(-ENOSYS);
}
So rather than just deleting all this code, breaking other platforms
that need this gpio, lets try a real fix. Please try this. If it
works, i will formally submit it.
Andrew
>From a4210ba306948497d7360927c1e532eb903c58b2 Mon Sep 17 00:00:00 2001
From: Andrew Lunn
Date: Sun, 4 Feb 2018 11:09:20 -0600
Subject: [PATCH] net: phy: Handle not having GPIO enabled in the kernel
If CONFIG_GPIOLIB is disabled, fwnode_get_named_gpiod() becomes a stub
function, which return -ENOSYS. Handle this in the same way as
-ENOENT, i.e. assume there is no GPIO used to reset the PHYs.
Reported-by: Christian Zigotzky
Signed-off-by: Andrew Lunn
---
drivers/net/phy/mdio_bus.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 88272b3ac2e2..24b5511222c8 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -56,7 +56,8 @@ static int mdiobus_register_gpiod(struct mdio_device *mdiodev)
gpiod = fwnode_get_named_gpiod(&mdiodev->dev.of_node->fwnode,
"reset-gpios", 0, GPIOD_OUT_LOW,
"PHY reset");
- if (PTR_ERR(gpiod) == -ENOENT)
+ if (PTR_ERR(gpiod) == -ENOENT ||
+ PTR_ERR(gpiod) == -ENOSYS)
gpiod = NULL;
else if (IS_ERR(gpiod))
return PTR_ERR(gpiod);
--
2.15.1
Re: PA Semi PWRficient Gigabit Ethernet doesn't work anymore since the first networking updates for the kernel 4.16
On Mon, Feb 05, 2018 at 10:38:34AM +0100, Christian Zigotzky wrote: > Hello Andrew, > > Many thanks for your patch. I compiled the latest git kernel today and the > PA Semi PWRficient Gigabit Ethernet works with your patch. Great. Can i add a tested-by: Thanks Andrew
Re: DPAA Ethernet traffice troubles with Linux kernel
On Wed, Feb 07, 2018 at 10:00:45PM +0100, mad skateman wrote: > Hi, > > I just found out that something goes wrong within the ARP table (well thats > what i think). I hope someone has a clue.. This looks like 'normal' behaviour. It has been reported that the device runs out of buffers. That means it will fail to receive ARP replies. So the ARP entry will time out and be removed, as it should. Fix the buffer problem, and ARP will likely work properly. Andrew
Re: [PATCH FEAT] ibmvnic: Feature implementation of Vital Product Data (VPD) for the ibmvnic driver
> > diff --git a/drivers/net/ethernet/ibm/ibmvnic.c > > b/drivers/net/ethernet/ibm/ibmvnic.c > > index d0cff28..120f3c0 100644 > > --- a/drivers/net/ethernet/ibm/ibmvnic.c > > +++ b/drivers/net/ethernet/ibm/ibmvnic.c > > @@ -107,6 +107,9 @@ static union sub_crq *ibmvnic_next_scrq(struct > > ibmvnic_adapter *, > > struct ibmvnic_sub_crq_queue *); > > static int ibmvnic_poll(struct napi_struct *napi, int data); > > static void send_map_query(struct ibmvnic_adapter *adapter); > > +static int ibmvnic_get_vpd(struct ibmvnic_adapter *); > > +static void handle_vpd_size_rsp(union ibmvnic_crq *, struct > > ibmvnic_adapter *); > > +static void handle_vpd_rsp(union ibmvnic_crq *,struct ibmvnic_adapter *); > > There should be a space after the comma. And you should try to avoid forward declarations. Move the code around so they are not needed. Andrew
Re: [PATCH] net: ethernet: remove fs_mii_disconnect and fs_mii_connect declarations
On Fri, Sep 09, 2022 at 02:29:59PM +0800, Gaosheng Cui wrote:
> fs_mii_disconnect and fs_mii_connect have been removed since
> commit 5b4b8454344a ("[PATCH] FS_ENET: use PAL for mii management"),
> so remove them.
>
> Signed-off-by: Gaosheng Cui
Reviewed-by: Andrew Lunn
Andrew
Re: [PATCH net-next v2 00/11] net: pcs: Add support for devices probed in the "usual" manner
On Thu, Nov 10, 2022 at 03:29:26PM +, Vladimir Oltean wrote: > On Thu, Nov 10, 2022 at 09:55:32AM -0500, Sean Anderson wrote: > > On 11/9/22 17:41, Vladimir Oltean wrote: > > > On Thu, Nov 03, 2022 at 05:06:39PM -0400, Sean Anderson wrote: > > >> Several (later) patches in this series cannot be applied until a stable > > >> release has occured containing the dts updates. > > > > > > New kernels must remain compatible with old device trees. > > > > Well, this binding is not present in older device trees, so it needs to > > be added before these patches can be applied. It also could be possible > > to manually bind the driver using e.g. a helper function (like what is > > done with lynx_pcs_create_on_bus). Of course this would be tricky, > > because we would need to unbind any generic phy driver attached, but > > avoid unbinding an existing Lynx PCS driver. > > If you know the value of the MII_PHYSID1 and MII_PHYSID2 registers for > these PCS devices, would it be possible to probe them in a generic way > as MDIO devices, if they lack a compatible string? That is not how it currently works. If a device on an MDIO bus has a compatible, it is assumed to be an mdio device, and it is probed in a generic way as an sort of mdio device. It could be an Ethernet switch, or Broadcom has some generic PHYs which are mdio devices, etc. If there is no compatible, the ID registers are read and it is assumed to be a PHY. It will be probed as a PHY. The probe() function will be given a phydev etc. It will need some core changes to allow an mdio device to be probed via the ID registers. Andrew
Re: [PATCH 0/5] remove label = "cpu" from DSA dt-binding
On Wed, Nov 30, 2022 at 05:10:35PM +0300, Arınç ÜNAL wrote: > Hello folks, > > With this patch series, we're completely getting rid of 'label = "cpu";' > which is not used by the DSA dt-binding at all. > > Information for taking the patches for maintainers: > Patch 1: netdev maintainers (based off netdev/net-next.git main) > Patch 2-3: SoC maintainers (based off soc/soc.git soc/dt) > Patch 4: MIPS maintainers (based off mips/linux.git mips-next) > Patch 5: PowerPC maintainers (based off powerpc/linux.git next-test) Hi Arınç So your plan is that each architecture maintainer merges one patch? That is fine, but it is good to be explicit, otherwise patches will fall through the cracks because nobody picks them up. I generally use To: to indicate who i expect to merge a patch, and everybody else in the Cc: Reviewed-by: Andrew Lunn Andrew
Re: [RFC PATCH dpss_eth] Don't initialise ports with no PHY
On Fri, Apr 24, 2020 at 11:29:38PM +0100, Darren Stevens wrote:
> Since cbb961ca271e ("Use random MAC address when none is given")
> Varisys Cyrus P5020 boards have been listing 5 ethernet ports instead of
> the 2 the board has.This is because we were preventing the adding of the
> unused ports by not suppling them a MAC address, which this patch now
> supplies.
>
> Prevent them from appearing in the net devices list by checking for a
> 'status="disabled"' entry during probe and skipping the port if we find
> it.
Hi Darren
I'm surprised the core is probing a device which has status disabled.
Are you sure this is the correct explanation?
Thanks
Andrew
Re: [RFC PATCH dpss_eth] Don't initialise ports with no PHY
> Maybe we have to modify the dtb file. Hi Christian Could you point me at the DT file. Thanks Andrew
Re: [RFC PATCH dpss_eth] Don't initialise ports with no PHY
On Wed, Apr 29, 2020 at 03:55:28PM +0200, Christian Zigotzky wrote: > Hi Andrew, > > You can find some dtb and source files in our kernel package. > > Download: http://www.xenosoft.de/linux-image-5.7-rc3-X1000_X5000.tar.gz I have the tarball. Are we talking about linux-image-5.7-rc3-X1000_X5000/X5000_and_QEMU_e5500/dtbs/X5000_20/cyrus.eth.dtb I don't see any status = "disabled"; in the blob. So i would expect the driver to probe. Andrew
Re: [PATCH 2/4] dt-bindings: doc: Reflect new NVMEM of_get_mac_address behaviour
> diff --git a/Documentation/devicetree/bindings/net/macb.txt > b/Documentation/devicetree/bindings/net/macb.txt > index 8b80515..92c5642 100644 > --- a/Documentation/devicetree/bindings/net/macb.txt > +++ b/Documentation/devicetree/bindings/net/macb.txt > @@ -26,15 +26,15 @@ Required properties: > Optional elements: 'tsu_clk' > - clocks: Phandles to input clocks. > > -Optional properties: > -- nvmem-cells: phandle, reference to an nvmem node for the MAC address > -- nvmem-cell-names: string, should be "mac-address" if nvmem is to be used > - > Optional properties for PHY child node: > - reset-gpios : Should specify the gpio for phy reset > - magic-packet : If present, indicates that the hardware supports waking >up via magic packet. > - phy-handle : see ethernet.txt file in the same directory > +- mac-address: See ethernet.txt in the same directory. > +- local-mac-address: See ethernet.txt in the same directory. > +- nvmem-cells: See ethernet.txt in the same directory. > +- nvmem-cell-names: See ethernet.txt in the same directory. This looks wrong. The MAC address is not a PHY property, so should not be inside the PHY child node. phy-handle is in the wrong place, but that is a separate problem. Andrew
Re: [PATCH 2/4] dt-bindings: doc: Reflect new NVMEM of_get_mac_address behaviour
> diff --git a/Documentation/devicetree/bindings/net/ethernet.txt > b/Documentation/devicetree/bindings/net/ethernet.txt > index 2974e63..1e2bc9a 100644 > --- a/Documentation/devicetree/bindings/net/ethernet.txt > +++ b/Documentation/devicetree/bindings/net/ethernet.txt > @@ -10,6 +10,8 @@ Documentation/devicetree/bindings/phy/phy-bindings.txt. >the boot program; should be used in cases where the MAC address assigned to >the device by the boot program is different from the "local-mac-address" >property; > +- nvmem-cells: phandle, reference to an nvmem node for the MAC address > +- nvmem-cell-names: string, should be "mac-address" if nvmem is to be used You put the new values after local-mac-address and mac-address. That suggests they are of lower priority. That conflicts with the current patch. If you think NVMEM should take priority, please put the properties first. Andrew
Re: [PATCH v3 7/7] [not for merge] netstats: example use of stats_fs API
On Tue, May 26, 2020 at 01:03:17PM +0200, Emanuele Giuseppe Esposito wrote:
> Apply stats_fs on the networking statistics subsystem.
>
> Currently it only works with disabled network namespace
> (CONFIG_NET_NS=n), because multiple namespaces will have the same
> device name under the same root source that will cause a conflict in
> stats_fs.
Hi Emanuele
How do you atomically get and display a group of statistics?
If you look at how the netlink socket works, you will see code like:
do {
start = u64_stats_fetch_begin_irq(&cpu_stats->syncp);
rx_packets = cpu_stats->rx_packets;
rx_bytes = cpu_stats->rx_bytes;
} while (u64_stats_fetch_retry_irq(&cpu_stats->syncp, start));
It will ensure that rx_packets and rx_bytes are consistent with each
other. If the value of the sequence counter changes while inside the
loop, the loop so repeated until it does not change.
In general, hardware counters in NICs are the same. You tell it to
take a snapshot of the statistics counters, and then read them all
back, to give a consistent view across all the statistics.
I've not looked at this new code in detail, but it looks like you have
one file per statistic, and assume each statistic is independent of
every other statistic. This independence can limit how you use the
values, particularly when debugging. The netlink interface we use does
not have this limitation.
Andrew
Re: [PATCH v3 0/7] Statsfs: a new ram-based file system for Linux kernel statistics
> I don't really know a lot about the networking subsystem, and as it was > pointed out in another email on patch 7 by Andrew, networking needs to > atomically gather and display statistics in order to make them consistent, > and currently this is not supported by stats_fs but could be added in > future. Hi Emanuele Do you have any idea how you will support atomic access? It does not seem easy to implement in a filesystem based model. Andrew
Re: [PATCH v3 0/7] Statsfs: a new ram-based file system for Linux kernel statistics
On Tue, May 26, 2020 at 01:03:10PM +0200, Emanuele Giuseppe Esposito wrote: > There is currently no common way for Linux kernel subsystems to expose > statistics to userspace shared throughout the Linux kernel; subsystems have > to take care of gathering and displaying statistics by themselves, for > example in the form of files in debugfs. For example KVM has its own code > section that takes care of this in virt/kvm/kvm_main.c, where it sets up > debugfs handlers for displaying values and aggregating them from various > subfolders to obtain information about the system state (i.e. displaying > the total number of exits, calculated by summing all exits of all cpus of > all running virtual machines). > > Allowing each section of the kernel to do so has two disadvantages. First, > it will introduce redundant code. Second, debugfs is anyway not the right > place for statistics (for example it is affected by lockdown) > > In this patch series I introduce statsfs, a synthetic ram-based virtual > filesystem that takes care of gathering and displaying statistics for the > Linux kernel subsystems. > > The file system is mounted on /sys/kernel/stats and would be already used > by kvm. Statsfs was initially introduced by Paolo Bonzini [1]. > > Statsfs offers a generic and stable API, allowing any kind of > directory/file organization and supporting multiple kind of aggregations > (not only sum, but also average, max, min and count_zero) and data types > (boolean, unsigned/signed and custom types). The implementation, which is > a generalization of KVM’s debugfs statistics code, takes care of gathering > and displaying information at run time; users only need to specify the > values to be included in each source. > > Statsfs would also be a different mountpoint from debugfs, and would not > suffer from limited access due to the security lock down patches. Its main > function is to display each statistics as a file in the desired folder > hierarchy defined through the API. Statsfs files can be read, and possibly > cleared if their file mode allows it. > > Statsfs has two main components: the public API defined by > include/linux/statsfs.h, and the virtual file system which should end up in > /sys/kernel/stats. > Hi Emanuele > The API has two main elements, values and sources. Kernel subsystems like > KVM can use the API to create a source, add child sources/values/aggregates > and register it to the root source (that on the virtual fs would be > /sys/kernel/statsfs). Another issue i see with networking is that statistic counters can be dynamic. They can come and go. One of the drivers i work on has extra statistics available when a fibre interface is used, compared to a copper interface. And this happens at run time. The netlink API has no problems with this. It is a snapshot of what counters are currently available. There is no state in the API. In my humble opinion, networking is unlikely to adopt your approach. You probably want to look around for other subsystems which have statistics, and see if you can cover their requirements, and get them on board. Andrew
Re: [PATCH net] net/ibmvnic: Report last valid speed and duplex values to ethtool
On Thu, Jun 27, 2019 at 12:09:13PM -0500, Thomas Falcon wrote: > This patch resolves an issue with sensitive bonding modes > that require valid speed and duplex settings to function > properly. Currently, the adapter will report that device > speed and duplex is unknown if the communication link > with firmware is unavailable. Dumb question. If you cannot communicate with the firmware, isn't the device FUBAR? So setting the LACP port to disabled is the correct things to do. Andrew
Re: [PATCH 1/2] of: net: pass the dst buffer to of_get_mac_address()
On Tue, Apr 06, 2021 at 03:19:11AM +0800, kernel test robot wrote: > Hi Michael, > > I love your patch! Yet something to improve: Looks correct. You missed the #else case for #ifdef CONFIG_OF in stmmac_platform.c Lets see what else 0-day finds before i start reviewing. Andrew
Re: [PATCH net-next v4 1/2] of: net: pass the dst buffer to of_get_mac_address()
On Mon, Apr 12, 2021 at 07:47:17PM +0200, Michael Walle wrote:
> of_get_mac_address() returns a "const void*" pointer to a MAC address.
> Lately, support to fetch the MAC address by an NVMEM provider was added.
> But this will only work with platform devices. It will not work with
> PCI devices (e.g. of an integrated root complex) and esp. not with DSA
> ports.
>
> There is an of_* variant of the nvmem binding which works without
> devices. The returned data of a nvmem_cell_read() has to be freed after
> use. On the other hand the return of_get_mac_address() points to some
> static data without a lifetime. The trick for now, was to allocate a
> device resource managed buffer which is then returned. This will only
> work if we have an actual device.
>
> Change it, so that the caller of of_get_mac_address() has to supply a
> buffer where the MAC address is written to. Unfortunately, this will
> touch all drivers which use the of_get_mac_address().
>
> Usually the code looks like:
>
> const char *addr;
> addr = of_get_mac_address(np);
> if (!IS_ERR(addr))
> ether_addr_copy(ndev->dev_addr, addr);
>
> This can then be simply rewritten as:
>
> of_get_mac_address(np, ndev->dev_addr);
>
> Sometimes is_valid_ether_addr() is used to test the MAC address.
> of_get_mac_address() already makes sure, it just returns a valid MAC
> address. Thus we can just test its return code. But we have to be
> careful if there are still other sources for the MAC address before the
> of_get_mac_address(). In this case we have to keep the
> is_valid_ether_addr() call.
>
> The following coccinelle patch was used to convert common cases to the
> new style. Afterwards, I've manually gone over the drivers and fixed the
> return code variable: either used a new one or if one was already
> available use that. Mansour Moufid, thanks for that coccinelle patch!
>
>
> @a@
> identifier x;
> expression y, z;
> @@
> - x = of_get_mac_address(y);
> + x = of_get_mac_address(y, z);
> <...
> - ether_addr_copy(z, x);
> ...>
>
> @@
> identifier a.x;
> @@
> - if (<+... x ...+>) {}
>
> @@
> identifier a.x;
> @@
> if (<+... x ...+>) {
> ...
> }
> - else {}
>
> @@
> identifier a.x;
> expression e;
> @@
> - if (<+... x ...+>@e)
> - {}
> - else
> + if (!(e))
> {...}
>
> @@
> expression x, y, z;
> @@
> - x = of_get_mac_address(y, z);
> + of_get_mac_address(y, z);
> ... when != x
>
>
> All drivers, except drivers/net/ethernet/aeroflex/greth.c, were
> compile-time tested.
>
> Suggested-by: Andrew Lunn
> Signed-off-by: Michael Walle
I cannot say i looked at all the changes, but the ones i did exam
seemed O.K.
Reviewed-by: Andrew Lunn
Andrew
Re: [PATCH net-next v4 2/2] of: net: fix of_get_mac_addr_nvmem() for non-platform devices
On Mon, Apr 12, 2021 at 07:47:18PM +0200, Michael Walle wrote: > of_get_mac_address() already supports fetching the MAC address by an > nvmem provider. But until now, it was just working for platform devices. > Esp. it was not working for DSA ports and PCI devices. It gets more > common that PCI devices have a device tree binding since SoCs contain > integrated root complexes. > > Use the nvmem of_* binding to fetch the nvmem cells by a struct > device_node. We still have to try to read the cell by device first > because there might be a nvmem_cell_lookup associated with that device. > > Signed-off-by: Michael Walle Reviewed-by: Andrew Lunn Andrew
Re: [PATCH 5/8] net: ethernet: ibm: ibmvnic: Fix some kernel-doc misdemeanours
Hi Lee
> /**
> * build_hdr_data - creates L2/L3/L4 header data buffer
> - * @hdr_field - bitfield determining needed headers
> - * @skb - socket buffer
> - * @hdr_len - array of header lengths
> - * @tot_len - total length of data
> + * @hdr_field: bitfield determining needed headers
> + * @skb: socket buffer
> + * @hdr_len: array of header lengths
> + * @tot_len: total length of data
> *
> * Reads hdr_field to determine which headers are needed by firmware.
> * Builds a buffer containing these headers. Saves individual header
The code is:
static int build_hdr_data(u8 hdr_field, struct sk_buff *skb,
int *hdr_len, u8 *hdr_data)
{
What about hdr_data?
> /**
> * create_hdr_descs - create header and header extension descriptors
> - * @hdr_field - bitfield determining needed headers
> - * @data - buffer containing header data
> - * @len - length of data buffer
> - * @hdr_len - array of individual header lengths
> - * @scrq_arr - descriptor array
> + * @hdr_field: bitfield determining needed headers
> + * @data: buffer containing header data
> + * @len: length of data buffer
> + * @hdr_len: array of individual header lengths
> + * @scrq_arr: descriptor array
static int create_hdr_descs(u8 hdr_field, u8 *hdr_data, int len, int *hdr_len,
union sub_crq *scrq_arr)
There is no data parameter.
It looks like you just changes - to :, but did not validate the
parameters are actually correct.
Andrew
Re: [PATCH 6/8] net: ethernet: toshiba: ps3_gelic_net: Fix some kernel-doc misdemeanours
On Thu, Nov 26, 2020 at 01:38:51PM +, Lee Jones wrote: > Fixes the following W=1 kernel build warning(s): > > drivers/net/ethernet/toshiba/ps3_gelic_net.c:1107: warning: Function > parameter or member 'irq' not described in 'gelic_card_interrupt' > drivers/net/ethernet/toshiba/ps3_gelic_net.c:1107: warning: Function > parameter or member 'ptr' not described in 'gelic_card_interrupt' > drivers/net/ethernet/toshiba/ps3_gelic_net.c:1407: warning: Function > parameter or member 'txqueue' not described in 'gelic_net_tx_timeout' > drivers/net/ethernet/toshiba/ps3_gelic_net.c:1439: warning: Function > parameter or member 'napi' not described in 'gelic_ether_setup_netdev_ops' > drivers/net/ethernet/toshiba/ps3_gelic_net.c:1639: warning: Function > parameter or member 'dev' not described in 'ps3_gelic_driver_probe' > drivers/net/ethernet/toshiba/ps3_gelic_net.c:1795: warning: Function > parameter or member 'dev' not described in 'ps3_gelic_driver_remove' > > Cc: Geoff Levand > Cc: "David S. Miller" > Cc: Jakub Kicinski > Cc: Michael Ellerman > Cc: Benjamin Herrenschmidt > Cc: Paul Mackerras > Cc: Utz Bacher > Cc: Jens Osterkamp > Cc: net...@vger.kernel.org > Cc: linuxppc-dev@lists.ozlabs.org > Signed-off-by: Lee Jones Reviewed-by: Andrew Lunn Andrew
Re: [PATCH 8/8] net: ethernet: ibm: ibmvnic: Fix some kernel-doc issues
On Thu, Nov 26, 2020 at 01:38:53PM +, Lee Jones wrote: > Fixes the following W=1 kernel build warning(s): > > from drivers/net/ethernet/ibm/ibmvnic.c:35: > inlined from ‘handle_vpd_rsp’ at drivers/net/ethernet/ibm/ibmvnic.c:4124:3: > drivers/net/ethernet/ibm/ibmvnic.c:1362: warning: Function parameter or > member 'hdr_data' not described in 'build_hdr_data' > drivers/net/ethernet/ibm/ibmvnic.c:1362: warning: Excess function parameter > 'tot_len' description in 'build_hdr_data' > drivers/net/ethernet/ibm/ibmvnic.c:1423: warning: Function parameter or > member 'hdr_data' not described in 'create_hdr_descs' > drivers/net/ethernet/ibm/ibmvnic.c:1423: warning: Excess function parameter > 'data' description in 'create_hdr_descs' > drivers/net/ethernet/ibm/ibmvnic.c:1474: warning: Function parameter or > member 'txbuff' not described in 'build_hdr_descs_arr' > drivers/net/ethernet/ibm/ibmvnic.c:1474: warning: Excess function parameter > 'skb' description in 'build_hdr_descs_arr' > drivers/net/ethernet/ibm/ibmvnic.c:1474: warning: Excess function parameter > 'subcrq' description in 'build_hdr_descs_arr' Hi Lee It looks like this should be squashed into the previous patch to this file. Andrew
Re: [PATCH 01/20] ethernet: ucc_geth: set dev->max_mtu to 1518
On Sat, Dec 05, 2020 at 08:17:24PM +0100, Rasmus Villemoes wrote:
> All the buffers and registers are already set up appropriately for an
> MTU slightly above 1500, so we just need to expose this to the
> networking stack. AFAICT, there's no need to implement .ndo_change_mtu
> when the receive buffers are always set up to support the max_mtu.
>
> This fixes several warnings during boot on our mpc8309-board with an
> embedded mv88e6250 switch:
>
> mv88e6085 mdio@e0102120:10: nonfatal error -34 setting MTU 1500 on port 0
> ...
> mv88e6085 mdio@e0102120:10: nonfatal error -34 setting MTU 1500 on port 4
> ucc_geth e0102000.ethernet eth1: error -22 setting MTU to 1504 to include DSA
> overhead
>
> The last line explains what the DSA stack tries to do: achieving an MTU
> of 1500 on-the-wire requires that the master netdevice connected to
> the CPU port supports an MTU of 1500+the tagging overhead.
>
> Fixes: bfcb813203e6 ("net: dsa: configure the MTU for switch ports")
> Cc: Vladimir Oltean
> Signed-off-by: Rasmus Villemoes
Reviewed-by: Andrew Lunn
Andrew
Re: [PATCH 07/20] ethernet: ucc_geth: use qe_muram_free_addr()
On Sat, Dec 05, 2020 at 08:17:30PM +0100, Rasmus Villemoes wrote: > This removes the explicit NULL checks, and allows us to stop storing > at least some of the _offset values separately. > > Signed-off-by: Rasmus Villemoes This seems to rely on one of the missing patches. Please don't split patches like this, it makes review very difficult. Andrew
Re: [PATCH] net: ethernet: fs_enet: Add missing MODULE_LICENSE
On Tue, Jan 05, 2021 at 01:22:29PM +1100, Michael Ellerman wrote:
> Since commit 1d6cd3929360 ("modpost: turn missing MODULE_LICENSE()
> into error") the ppc32_allmodconfig build fails with:
>
> ERROR: modpost: missing MODULE_LICENSE() in
> drivers/net/ethernet/freescale/fs_enet/mii-fec.o
> ERROR: modpost: missing MODULE_LICENSE() in
> drivers/net/ethernet/freescale/fs_enet/mii-bitbang.o
>
> Add the missing MODULE_LICENSEs to fix the build. Both files include a
> copyright header indicating they are GPL v2.
>
> Signed-off-by: Michael Ellerman
> ---
> drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c | 1 +
> drivers/net/ethernet/freescale/fs_enet/mii-fec.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c
> b/drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c
> index c8e5d889bd81..76ac1a9eab58 100644
> --- a/drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c
> +++ b/drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c
> @@ -223,3 +223,4 @@ static struct platform_driver fs_enet_bb_mdio_driver = {
> };
>
> module_platform_driver(fs_enet_bb_mdio_driver);
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/net/ethernet/freescale/fs_enet/mii-fec.c
> b/drivers/net/ethernet/freescale/fs_enet/mii-fec.c
> index 8b51ee142fa3..407c330b432f 100644
> --- a/drivers/net/ethernet/freescale/fs_enet/mii-fec.c
> +++ b/drivers/net/ethernet/freescale/fs_enet/mii-fec.c
> @@ -224,3 +224,4 @@ static struct platform_driver fs_enet_fec_mdio_driver = {
> };
>
> module_platform_driver(fs_enet_fec_mdio_driver);
> +MODULE_LICENSE("GPL v2");
Hi Michael
The use of "GPL v2" has been deprecated. Please use just "GPL". There
is a discussion about this here:
https://lore.kernel.org/patchwork/patch/1036331/
https://www.kernel.org/doc/html/latest/process/license-rules.html#id1
Andrew
Re: [PATCH 01/20] ethernet: ucc_geth: set dev->max_mtu to 1518
On Tue, Jan 05, 2021 at 02:17:42PM +, Joakim Tjernlund wrote:
> On Thu, 2020-12-10 at 02:25 +0100, Andrew Lunn wrote:
> > On Sat, Dec 05, 2020 at 08:17:24PM +0100, Rasmus Villemoes wrote:
> > > All the buffers and registers are already set up appropriately for an
> > > MTU slightly above 1500, so we just need to expose this to the
> > > networking stack. AFAICT, there's no need to implement .ndo_change_mtu
> > > when the receive buffers are always set up to support the max_mtu.
> > >
> > > This fixes several warnings during boot on our mpc8309-board with an
> > > embedded mv88e6250 switch:
> > >
> > > mv88e6085 mdio@e0102120:10: nonfatal error -34 setting MTU 1500 on port 0
> > > ...
> > > mv88e6085 mdio@e0102120:10: nonfatal error -34 setting MTU 1500 on port 4
> > > ucc_geth e0102000.ethernet eth1: error -22 setting MTU to 1504 to include
> > > DSA overhead
> > >
> > > The last line explains what the DSA stack tries to do: achieving an MTU
> > > of 1500 on-the-wire requires that the master netdevice connected to
> > > the CPU port supports an MTU of 1500+the tagging overhead.
> > >
> > > Fixes: bfcb813203e6 ("net: dsa: configure the MTU for switch ports")
> > > Cc: Vladimir Oltean
> > > Signed-off-by: Rasmus Villemoes
> >
> > Reviewed-by: Andrew Lunn
> >
> > Andrew
>
> I don't see this in any kernel, seems stuck? Maybe because the series as a
> whole is not approved?
Hi Joakim
When was it posted? If it was while netdev was closed during the merge
window, you need to repost.
You should be able to see the status in the netdev patchwork instance
https://patchwork.kernel.org/project/netdevbpf/list/
Andrew
Re: [PATCH 01/20] ethernet: ucc_geth: set dev->max_mtu to 1518
> Hi Andrew > > I found here: > https://patchwork.kernel.org/project/netdevbpf/patch/20201218105538.30563-2-rasmus.villem...@prevas.dk/ > > Seem to be underway but stable isn't included, I think it should be. Stable should happen, since this was posted with [net,v2,3/3]. David or Jakub handles stable for netdev. Give it a few more days. If not, ask Jakub what is happening. Andrew
Re: [PATCH v2] net: ethernet: fs_enet: Add missing MODULE_LICENSE
On Tue, Jan 05, 2021 at 08:15:15PM +1100, Michael Ellerman wrote:
> Since commit 1d6cd3929360 ("modpost: turn missing MODULE_LICENSE()
> into error") the ppc32_allmodconfig build fails with:
>
> ERROR: modpost: missing MODULE_LICENSE() in
> drivers/net/ethernet/freescale/fs_enet/mii-fec.o
> ERROR: modpost: missing MODULE_LICENSE() in
> drivers/net/ethernet/freescale/fs_enet/mii-bitbang.o
>
> Add the missing MODULE_LICENSEs to fix the build. Both files include a
> copyright header indicating they are GPL v2.
>
> Signed-off-by: Michael Ellerman
Reviewed-by: Andrew Lunn
Andrew
Re: [PATCH net-next] ethernet: ucc_geth: Use kmemdup() rather than kmalloc+memcpy
On Sun, May 23, 2021 at 03:29:37PM +0200, Christophe Leroy wrote: > YueHaibing a écrit : > > > Issue identified with Coccinelle. > > > > Signed-off-by: YueHaibing > > --- > > drivers/net/ethernet/freescale/ucc_geth.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/freescale/ucc_geth.c > > b/drivers/net/ethernet/freescale/ucc_geth.c > > index e0936510fa34..51206272cc25 100644 > > --- a/drivers/net/ethernet/freescale/ucc_geth.c > > +++ b/drivers/net/ethernet/freescale/ucc_geth.c > > @@ -3590,10 +3590,10 @@ static int ucc_geth_probe(struct > > platform_device* ofdev) > > if ((ucc_num < 0) || (ucc_num > 7)) > > return -ENODEV; > > > > - ug_info = kmalloc(sizeof(*ug_info), GFP_KERNEL); > > + ug_info = kmemdup(&ugeth_primary_info, sizeof(*ug_info), > > + GFP_KERNEL); > > Can you keep that as a single line ? The tolerance is 100 chars per line now. Networking prefers 80. If it fits a single 80 char line, please use a single line. Otherwise please leave it as it is. Andrew
Re: [PATCH v2 net-next] ethernet: ucc_geth: Use kmemdup() rather than kmalloc+memcpy
On Mon, May 24, 2021 at 09:07:01AM +0800, YueHaibing wrote: > Issue identified with Coccinelle. > > Signed-off-by: YueHaibing Reviewed-by: Andrew Lunn Andrew
Re: [PATCH net 2/4] dt-bindings: net: Document fsl,erratum-a009885
On Sun, Jan 16, 2022 at 10:15:27PM +0100, Tobias Waldekranz wrote: > Update FMan binding documentation with the newly added workaround for > erratum A-009885. > > Signed-off-by: Tobias Waldekranz Reviewed-by: Andrew Lunn Andrew
Re: [PATCH net 4/4] net/fsl: xgmac_mdio: Fix incorrect iounmap when removing module
On Sun, Jan 16, 2022 at 10:15:29PM +0100, Tobias Waldekranz wrote: > As reported by sparse: In the remove path, the driver would attempt to > unmap its own priv pointer - instead of the io memory that it mapped > in probe. Hi Tobias The change itself is O.K. Reviewed-by: Andrew Lunn But you could also change to devm_ so the core will handle the unmap, it is very unlikely to unmap the wrong thing. Andrew
Re: [PATCH net 1/4] net/fsl: xgmac_mdio: Add workaround for erratum A-009885
On Sun, Jan 16, 2022 at 10:15:26PM +0100, Tobias Waldekranz wrote: > Once an MDIO read transaction is initiated, we must read back the data > register within 16 MDC cycles after the transaction completes. Outside > of this window, reads may return corrupt data. > > Therefore, disable local interrupts in the critical section, to > maximize the probability that we can satisfy this requirement. Since this is for net, a Fixes: tag would be nice. Maybe that would be for the commit which added this driver, or maybe when the DTSI files for the SOCs which have this errata we added? Reviewed-by: Andrew Lunn Andrew
Re: [PATCH net 1/4] net/fsl: xgmac_mdio: Add workaround for erratum A-009885
On Mon, Jan 17, 2022 at 08:24:22AM +0100, Tobias Waldekranz wrote: > On Sun, Jan 16, 2022 at 23:02, Andrew Lunn wrote: > > On Sun, Jan 16, 2022 at 10:15:26PM +0100, Tobias Waldekranz wrote: > >> Once an MDIO read transaction is initiated, we must read back the data > >> register within 16 MDC cycles after the transaction completes. Outside > >> of this window, reads may return corrupt data. > >> > >> Therefore, disable local interrupts in the critical section, to > >> maximize the probability that we can satisfy this requirement. > > > > Since this is for net, a Fixes: tag would be nice. Maybe that would be > > for the commit which added this driver, or maybe when the DTSI files > > for the SOCs which have this errata we added? > > Alright, I wasn't sure how to tag WAs for errata since it is more about > the hardware than the driver. The tag gives the backporter an idea how far back to go. If support for this SoC has only recently been added, there is no need to backport a long way. If it is an old SoC, then maybe more effort should be put into the backport? > Should I send a v2 even if nothing else > pops up, or is this more of a if-you're-sending-a-v2-anyway type of > comment? If you reply with a Fixes: patchwork will automagically append it like it does Reviewed-by, Tested-by etc. Andrew
Re: [PATCH v2 devicetree 2/2] powerpc: dts: t1040rdb: add ports for Seville Ethernet switch
> +&seville_port0 {
> + managed = "in-band-status";
> + phy-handle = <&phy_qsgmii_0>;
> + phy-mode = "qsgmii";
> + /* ETH4 written on chassis */
> + label = "swp4";
If ETH4 is on the chassis why not use ETH4?
Andrew
Re: [PATCH v2 devicetree 2/2] powerpc: dts: t1040rdb: add ports for Seville Ethernet switch
On Tue, Sep 29, 2020 at 07:39:54PM +, Vladimir Oltean wrote:
> On Tue, Sep 29, 2020 at 09:11:53PM +0200, Andrew Lunn wrote:
> > > +&seville_port0 {
> > > + managed = "in-band-status";
> > > + phy-handle = <&phy_qsgmii_0>;
> > > + phy-mode = "qsgmii";
> > > + /* ETH4 written on chassis */
> > > + label = "swp4";
> >
> > If ETH4 is on the chassis why not use ETH4?
>
> You mean all-caps, just like that?
Yes.
DSA is often used in WiFI access point, etc. The user is not a
computer professional. If the WebGUI says ETH4, and the label on the
front says ETH4, they probably think the two are the same, and are
happy.
I have one box which does not have an labels on the front panels, but
the industrial sockets for Ethernet are colour coded. So the interface
names are red, blue, green, to match the socket colour, and the cable
set is also colour coded the same.
So long as it is unique, the kernel does not care. So make it easy for
the user.
Andrew
Re: [PATCH v3 devicetree 2/2] powerpc: dts: t1040rdb: add ports for Seville Ethernet switch
On Thu, Oct 01, 2020 at 04:20:13PM +0300, Vladimir Oltean wrote: > Define the network interface names for the switch ports and hook them up > to the 2 QSGMII PHYs that are onboard. > > A conscious decision was taken to go along with the numbers that are > written on the front panel of the board and not with the hardware > numbers of the switch chip ports. The 2 numbering schemes are > shifted by 8. > > Signed-off-by: Vladimir Oltean > Reviewed-by: Maxim Kochetkov Reviewed-by: Andrew Lunn Andrew
Re: [PATCH v3 devicetree 1/2] powerpc: dts: t1040: add bindings for Seville Ethernet switch
On Thu, Oct 01, 2020 at 04:20:12PM +0300, Vladimir Oltean wrote: > Add the description of the embedded L2 switch inside the SoC dtsi file > for NXP T1040. > > Signed-off-by: Vladimir Oltean > Reviewed-by: Maxim Kochetkov Reviewed-by: Andrew Lunn Andrew
Re: [PATCH 09/12] net: ethernet: ibm: ibmvnic: Fix some kernel-doc misdemeanours
On Wed, Nov 04, 2020 at 09:06:07AM +, Lee Jones wrote: > Fixes the following W=1 kernel build warning(s): > > from drivers/net/ethernet/ibm/ibmvnic.c:35: > inlined from ‘handle_vpd_rsp’ at drivers/net/ethernet/ibm/ibmvnic.c:4124:3: > drivers/net/ethernet/ibm/ibmvnic.c:1362: warning: Function parameter or > member 'hdr_field' not described in 'build_hdr_data' > drivers/net/ethernet/ibm/ibmvnic.c:1362: warning: Function parameter or > member 'skb' not described in 'build_hdr_data' > drivers/net/ethernet/ibm/ibmvnic.c:1362: warning: Function parameter or > member 'hdr_len' not described in 'build_hdr_data' > drivers/net/ethernet/ibm/ibmvnic.c:1362: warning: Function parameter or > member 'hdr_data' not described in 'build_hdr_data' > drivers/net/ethernet/ibm/ibmvnic.c:1423: warning: Function parameter or > member 'hdr_field' not described in 'create_hdr_descs' > drivers/net/ethernet/ibm/ibmvnic.c:1423: warning: Function parameter or > member 'hdr_data' not described in 'create_hdr_descs' > drivers/net/ethernet/ibm/ibmvnic.c:1423: warning: Function parameter or > member 'len' not described in 'create_hdr_descs' > drivers/net/ethernet/ibm/ibmvnic.c:1423: warning: Function parameter or > member 'hdr_len' not described in 'create_hdr_descs' > drivers/net/ethernet/ibm/ibmvnic.c:1423: warning: Function parameter or > member 'scrq_arr' not described in 'create_hdr_descs' > drivers/net/ethernet/ibm/ibmvnic.c:1474: warning: Function parameter or > member 'txbuff' not described in 'build_hdr_descs_arr' > drivers/net/ethernet/ibm/ibmvnic.c:1474: warning: Function parameter or > member 'num_entries' not described in 'build_hdr_descs_arr' > drivers/net/ethernet/ibm/ibmvnic.c:1474: warning: Function parameter or > member 'hdr_field' not described in 'build_hdr_descs_arr' > drivers/net/ethernet/ibm/ibmvnic.c:1832: warning: Function parameter or > member 'adapter' not described in 'do_change_param_reset' > drivers/net/ethernet/ibm/ibmvnic.c:1832: warning: Function parameter or > member 'rwi' not described in 'do_change_param_reset' > drivers/net/ethernet/ibm/ibmvnic.c:1832: warning: Function parameter or > member 'reset_state' not described in 'do_change_param_reset' > drivers/net/ethernet/ibm/ibmvnic.c:1911: warning: Function parameter or > member 'adapter' not described in 'do_reset' > drivers/net/ethernet/ibm/ibmvnic.c:1911: warning: Function parameter or > member 'rwi' not described in 'do_reset' > drivers/net/ethernet/ibm/ibmvnic.c:1911: warning: Function parameter or > member 'reset_state' not described in 'do_reset' > > Cc: Dany Madden > Cc: Lijun Pan > Cc: Sukadev Bhattiprolu > Cc: Michael Ellerman > Cc: Benjamin Herrenschmidt > Cc: Paul Mackerras > Cc: "David S. Miller" > Cc: Jakub Kicinski > Cc: Santiago Leon > Cc: Thomas Falcon > Cc: John Allen > Cc: net...@vger.kernel.org > Cc: linuxppc-dev@lists.ozlabs.org > Signed-off-by: Lee Jones Reviewed-by: Andrew Lunn Andrew
Re: [PATCH 10/12] net: ethernet: toshiba: ps3_gelic_net: Fix some kernel-doc misdemeanours
On Wed, Nov 04, 2020 at 09:06:08AM +, Lee Jones wrote: > Fixes the following W=1 kernel build warning(s): > > drivers/net/ethernet/toshiba/ps3_gelic_net.c:1107: warning: Function > parameter or member 'irq' not described in 'gelic_card_interrupt' > drivers/net/ethernet/toshiba/ps3_gelic_net.c:1107: warning: Function > parameter or member 'ptr' not described in 'gelic_card_interrupt' > drivers/net/ethernet/toshiba/ps3_gelic_net.c:1407: warning: Function > parameter or member 'txqueue' not described in 'gelic_net_tx_timeout' > drivers/net/ethernet/toshiba/ps3_gelic_net.c:1439: warning: Function > parameter or member 'napi' not described in 'gelic_ether_setup_netdev_ops' > drivers/net/ethernet/toshiba/ps3_gelic_net.c:1639: warning: Function > parameter or member 'dev' not described in 'ps3_gelic_driver_probe' > drivers/net/ethernet/toshiba/ps3_gelic_net.c:1795: warning: Function > parameter or member 'dev' not described in 'ps3_gelic_driver_remove' > > Cc: Geoff Levand > Cc: "David S. Miller" > Cc: Jakub Kicinski > Cc: Michael Ellerman > Cc: Benjamin Herrenschmidt > Cc: Paul Mackerras > Cc: Utz Bacher > Cc: Jens Osterkamp > Cc: net...@vger.kernel.org > Cc: linuxppc-dev@lists.ozlabs.org > Signed-off-by: Lee Jones Reviewed-by: Andrew Lunn Andrew
Re: [PATCH 0/2] tty: Remove obsolete drivers
On Thu, Nov 05, 2020 at 12:33:55PM +, Lee Jones wrote: > As per the vendor's request. > > Lee Jones (2): > tty: Remove redundant synclink driver > tty: Remove redundant synclinkmp driver Hi Lee What exactly do you mean by redundant? That some other driver can be used? Or that nobody uses the hardware any more? I know of one deployed system which has some form of synclink card in use, but i don't know which card in particular. Andrew
Re: [PATCH v2 06/28] net: wan: Add support for QMC HDLC
> +static inline struct qmc_hdlc *netdev_to_qmc_hdlc(struct net_device *netdev)
> +{
> + return (struct qmc_hdlc *)dev_to_hdlc(netdev)->priv;
priv is a void *, so you don't need the cast.
Andrew
Re: [PATCH v2 08/28] soc: fsl: cpm1: qmc: Introduce available timeslots masks
On Wed, Jul 26, 2023 at 05:02:04PM +0200, Herve Codina wrote:
> Available timeslots masks define timeslots available for the related
> channel. These timeslots are defined by the QMC binding.
>
> Timeslots used are initialized to available timeslots but can be a
> subset of available timeslots.
> This prepares the dynamic timeslots management (ie. changing timeslots
> at runtime).
>
> Signed-off-by: Herve Codina
> ---
> drivers/soc/fsl/qe/qmc.c | 8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/soc/fsl/qe/qmc.c b/drivers/soc/fsl/qe/qmc.c
> index 2d2a9d88ba6c..21ad7e79e7bd 100644
> --- a/drivers/soc/fsl/qe/qmc.c
> +++ b/drivers/soc/fsl/qe/qmc.c
> @@ -177,7 +177,9 @@ struct qmc_chan {
> struct qmc *qmc;
> void __iomem *s_param;
> enum qmc_mode mode;
> + u64 tx_ts_mask_avail;
> u64 tx_ts_mask;
> + u64 rx_ts_mask_avail;
> u64 rx_ts_mask;
Is this for E1? So there is a maximum of 32 slots? A u32 would be
sufficient i think?
Andrew
Re: [PATCH v2 10/28] soc: fsl: cpm1: qmc: Introduce qmc_chan_setup_tsa*
> +static inline void qmc_clrsetbits16(void __iomem *addr, u16 clr, u16 set)
> +{
> + qmc_write16(addr, (qmc_read16(addr) & ~clr) | set);
> +}
> +
Please don't use inline in .c files. Let the compiler decide.
Andrew
Re: [PATCH v2 20/28] net: wan: Add framer framework support
> +int framer_pm_runtime_get(struct framer *framer)
> +{
> + int ret;
> +
> + if (!framer)
> + return 0;
Can framer be a NULL pointer? This sort of test often covers up
bugs. So either let it dereference the NULL pointer and opps, or
return -EINVAL.
Andrew
Re: [PATCH v2 21/28] dt-bindings: net: Add the Lantiq PEF2256 E1/T1/J1 framer
> + clocks: > +items: > + - description: Master clock > + - description: Receive System Clock > + - description: Transmit System Clock > + > + clock-names: > +items: > + - const: mclk > + - const: sclkr > + - const: sclkx Nit pick, but "Receive System Clock", but "sclkr'. Maybe "System Clock Receive" so you have the same word order? Andrew
Re: [PATCH v2 23/28] net: wan: framer: Add support for the Lantiq PEF2256 framer
> +static inline u8 pef2256_read8(struct pef2256 *pef2256, int offset)
> +{
> + int val;
> +
> + regmap_read(pef2256->regmap, offset, &val);
> + return val;
> +}
> +
> +static inline void pef2256_write8(struct pef2256 *pef2256, int offset, u8
> val)
> +{
> + regmap_write(pef2256->regmap, offset, val);
> +}
More cases of inline functions in .C files. Please let the compiler
decide.
> +static void pef2256_isr_default_handler(struct pef2256 *pef2256, u8 nbr, u8
> isr)
> +{
> + dev_warn(pef2256->dev, "ISR%u: 0x%02x not handled\n", nbr, isr);
> +}
Should this be rate limited? It is going to be very noise if it gets
called once per frame time.
Andrew
Re: [PATCH v2 26/28] ASoC: codecs: Add support for the framer codec
On Wed, Jul 26, 2023 at 05:02:22PM +0200, Herve Codina wrote:
> The framer codec interracts with a framer.
> It allows to use some of the framer timeslots as audio channels to
> transport audio data over the framer E1/T1/J1 lines.
> It also reports line carrier detection events through the ALSA jack
> detection feature.
>
> Signed-off-by: Herve Codina
> ---
> sound/soc/codecs/Kconfig| 15 ++
> sound/soc/codecs/Makefile | 2 +
> sound/soc/codecs/framer-codec.c | 423
> 3 files changed, 440 insertions(+)
> create mode 100644 sound/soc/codecs/framer-codec.c
>
> diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
> index f99203ef9b03..a86cdac39b72 100644
> --- a/sound/soc/codecs/Kconfig
> +++ b/sound/soc/codecs/Kconfig
> @@ -110,6 +110,7 @@ config SND_SOC_ALL_CODECS
> imply SND_SOC_ES8328_I2C
> imply SND_SOC_ES7134
> imply SND_SOC_ES7241
> + imply SND_SOC_FRAMER
> imply SND_SOC_GTM601
> imply SND_SOC_HDAC_HDMI
> imply SND_SOC_HDAC_HDA
> @@ -1043,6 +1044,20 @@ config SND_SOC_ES8328_SPI
> depends on SPI_MASTER
> select SND_SOC_ES8328
>
> +config SND_SOC_FRAMER
> + tristate "Framer codec"
> + depends on GENERIC_FRAMER
> + help
> + Enable support for the framer codec.
> + The framer codec uses the generic framer infrastructure to transport
> + some audio data over an analog E1/T1/J1 line.
> + This codec allows to use some of the time slots available on the TDM
> + bus on which the framer is connected to transport the audio data.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called snd-soc-framer.
> +
> +
> config SND_SOC_GTM601
> tristate 'GTM601 UMTS modem audio codec'
>
> diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
> index 32dcc6de58bd..54667274a0f6 100644
> --- a/sound/soc/codecs/Makefile
> +++ b/sound/soc/codecs/Makefile
> @@ -116,6 +116,7 @@ snd-soc-es8326-objs := es8326.o
> snd-soc-es8328-objs := es8328.o
> snd-soc-es8328-i2c-objs := es8328-i2c.o
> snd-soc-es8328-spi-objs := es8328-spi.o
> +snd-soc-framer-objs := framer-codec.o
> snd-soc-gtm601-objs := gtm601.o
> snd-soc-hdac-hdmi-objs := hdac_hdmi.o
> snd-soc-hdac-hda-objs := hdac_hda.o
> @@ -499,6 +500,7 @@ obj-$(CONFIG_SND_SOC_ES8326)+= snd-soc-es8326.o
> obj-$(CONFIG_SND_SOC_ES8328) += snd-soc-es8328.o
> obj-$(CONFIG_SND_SOC_ES8328_I2C)+= snd-soc-es8328-i2c.o
> obj-$(CONFIG_SND_SOC_ES8328_SPI)+= snd-soc-es8328-spi.o
> +obj-$(CONFIG_SND_SOC_FRAMER) += snd-soc-framer.o
> obj-$(CONFIG_SND_SOC_GTM601)+= snd-soc-gtm601.o
> obj-$(CONFIG_SND_SOC_HDAC_HDMI) += snd-soc-hdac-hdmi.o
> obj-$(CONFIG_SND_SOC_HDAC_HDA) += snd-soc-hdac-hda.o
> diff --git a/sound/soc/codecs/framer-codec.c b/sound/soc/codecs/framer-codec.c
> new file mode 100644
> index ..52b4546a61ee
> --- /dev/null
> +++ b/sound/soc/codecs/framer-codec.c
> @@ -0,0 +1,423 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Framer ALSA SoC driver
> +//
> +// Copyright 2023 CS GROUP France
> +//
> +// Author: Herve Codina
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define FRAMER_NB_CHANNEL32
> +#define FRAMER_JACK_MASK (SND_JACK_LINEIN | SND_JACK_LINEOUT)
> +
> +struct framer_codec {
> + struct framer *framer;
> + struct device *dev;
> + struct snd_soc_jack jack;
> + struct notifier_block nb;
> + struct work_struct carrier_work;
> + int max_chan_playback;
> + int max_chan_capture;
> +};
> +
> +static int framer_dai_set_tdm_slot(struct snd_soc_dai *dai, unsigned int
> tx_mask,
> +unsigned int rx_mask, int slots, int width)
> +{
> + struct framer_codec *framer =
> snd_soc_component_get_drvdata(dai->component);
> +
> + switch (width) {
> + case 0:
> + /* Not set -> default 8 */
> + case 8:
> + break;
> + default:
> + dev_err(dai->dev, "tdm slot width %d not supported\n", width);
> + return -EINVAL;
> + }
> +
> + framer->max_chan_playback = hweight32(tx_mask);
> + if (framer->max_chan_playback > FRAMER_NB_CHANNEL) {
> + dev_err(dai->dev, "too much tx slots defined (mask = 0x%x)
> support max %d\n",
"many", not "much".
Also, "supported".
Andrew
Re: [PATCH v2 00/28] Add support for QMC HDLC, framer infrastruture and PEF2256 framer
> The generic framer has: > - 2 consumers (QMC HDLC drv and codec) > - 1 provider (PEF2256) > > So, the design is the following: > +--+ +-+ > | QMC | <- TDM -> | PEF2256 | <-> E1 > +-+| +-+ | | | > | CPU DAI | <-data--> | QMC channel | | | | > +-+| +-+ | | | > +--+| +-+ | | | > | QMC HDLC drv | <-data--> | QMC channel | | | | > +--+| +-+ | | | > ^ +--+ | | > | ++ +-+| | > +-> | framer | <-> | PEF2256 drv | <- local bus ->| | > || | |+-+ > +-> || | | > | ++ | +---+ | > +---> | codec | | > | +---+ | > +-+ Thanks for adding the framer framework. I did not look into all the details, but the basic design looks good. Andrew
Re: [PATCH v2 23/28] net: wan: framer: Add support for the Lantiq PEF2256 framer
> > > +static void pef2256_isr_default_handler(struct pef2256 *pef2256, u8 nbr,
> > > u8 isr)
> > > +{
> > > + dev_warn(pef2256->dev, "ISR%u: 0x%02x not handled\n", nbr, isr);
> > > +}
> >
> > Should this be rate limited? It is going to be very noise if it gets
> > called once per frame time.
>
> This function should not be called.
> It is wired on some interrupts and these interrupts should not be triggered.
> It they fired, something was wrong.
>
> I would prefer to keep this dev_warn() to keep the user informed about the
> problem.
I would definitely keep it, but rate limit it. dev_warn_ratelimited().
Andrew
Re: [PATCH v2 24/28] pinctrl: Add support for the Lantic PEF2256 pinmux
On Mon, Aug 07, 2023 at 03:06:42PM +0200, Linus Walleij wrote: > On Mon, Aug 7, 2023 at 3:05 PM Linus Walleij wrote: > > > > Signed-off-by: Herve Codina > > > > So it is a bridge chip? Please use that terminology since Linux > > DRM often talks about bridges. > > Replying to self: no it's not a bridge, it's a WAN thingy. > > So perhaps write that this is a WAN interface adapter chip. Hi Linus In the E1/T1/J1 world, framer is a well understood concept. Maybe the text needs a bit more background information to explain what this is to somebody who does not have an old school telecoms background. Andrew
Re: [PATCH net-next 00/10] Remove unnecessary (void*) conversions
On Wed, Jun 28, 2023 at 04:37:43PM +0200, Andrew Lunn wrote: > > Hi, Hao Lan, > > > > Sorry for that, I just compiled these patches in the mainline branch. > > I know now, it's also necessary to compile patches in net and net-next > > branch. > > Thanks for your reply! > > net-next is also closed at the moment due to the merge window. Please > wait two weeks before reposting, by which time net-next will be open > again. Your email threading also seems to be broken, there is no threading. That might cause pathworks an issue. Andrew
Re: [PATCH net-next 00/10] Remove unnecessary (void*) conversions
> Hi, Hao Lan, > > Sorry for that, I just compiled these patches in the mainline branch. > I know now, it's also necessary to compile patches in net and net-next > branch. > Thanks for your reply! net-next is also closed at the moment due to the merge window. Please wait two weeks before reposting, by which time net-next will be open again. Andrew
Re: [PATCH net-next v2 01/10] net: wan: Remove unnecessary (void*) conversions
On Mon, Jul 10, 2023 at 02:39:33PM +0800, Su Hui wrote:
> From: wuych
>
> Pointer variables of void * type do not require type cast.
>
> Signed-off-by: wuych
> ---
> drivers/net/wan/fsl_ucc_hdlc.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
> index 47c2ad7a3e42..73c73d8f4bb2 100644
> --- a/drivers/net/wan/fsl_ucc_hdlc.c
> +++ b/drivers/net/wan/fsl_ucc_hdlc.c
> @@ -350,11 +350,11 @@ static int uhdlc_init(struct ucc_hdlc_private *priv)
> static netdev_tx_t ucc_hdlc_tx(struct sk_buff *skb, struct net_device *dev)
> {
> hdlc_device *hdlc = dev_to_hdlc(dev);
> - struct ucc_hdlc_private *priv = (struct ucc_hdlc_private *)hdlc->priv;
> - struct qe_bd *bd;
> - u16 bd_status;
> + struct ucc_hdlc_private *priv = hdlc->priv;
> unsigned long flags;
> __be16 *proto_head;
> + struct qe_bd *bd;
> + u16 bd_status;
When dealing with existing broken reverse Christmas tree, please don't
make it worse with a change. But actually fixing it should be in a
different patch.
We want patches to be obviously correct. By removing the cast and
moving variables around, it is less obvious it is correct, than having
two patches.
Andrew
Re: [PATCH net-next v2 4/7] net: ethernet: fs_enet: drop unused phy_info and mii_if_info
On Thu, Aug 29, 2024 at 06:15:27PM +0200, Maxime Chevallier wrote: > There's no user of the struct phy_info, the 'phy' field and the > mii_if_info in the fs_enet driver, probably dating back when phylib > wasn't as widely used. Drop these from the driver code. > > Acked-by: Christophe Leroy > Reviewed-by: Christophe Leroy > Signed-off-by: Maxime Chevallier Reviewed-by: Andrew Lunn Andrew
Re: [PATCH net-next v2 4/7] net: ethernet: fs_enet: drop unused phy_info and mii_if_info
On Thu, Aug 29, 2024 at 06:15:27PM +0200, Maxime Chevallier wrote: > There's no user of the struct phy_info, the 'phy' field and the > mii_if_info in the fs_enet driver, probably dating back when phylib > wasn't as widely used. Drop these from the driver code. There might be an include of linux/mii.h you can also drop? Andrew
Re: [PATCH net-next v2 2/7] net: ethernet: fs_enet: cosmetic cleanups
> @@ -235,15 +219,15 @@ static int fs_enet_napi(struct napi_struct *napi, int
> budget)
> if (pkt_len <= fpi->rx_copybreak) {
> /* +2 to make IP header L1 cache aligned */
> skbn = netdev_alloc_skb(dev, pkt_len + 2);
> - if (skbn != NULL) {
> + if (skbn) {
> skb_reserve(skbn, 2); /* align IP
> header */
> - skb_copy_from_linear_data(skb,
> - skbn->data, pkt_len);
> + skb_copy_from_linear_data(skb,
> skbn->data,
> + pkt_len);
> swap(skb, skbn);
> dma_sync_single_for_cpu(fep->dev,
> - CBDR_BUFADDR(bdp),
> - L1_CACHE_ALIGN(pkt_len),
> - DMA_FROM_DEVICE);
> +
> CBDR_BUFADDR(bdp),
> +
> L1_CACHE_ALIGN(pkt_len),
> +
> DMA_FROM_DEVICE);
The indentation level here suggest refactoring into helpers might be
nice. But not a prerequisite for merging.
Reviewed-by: Andrew Lunn
Andrew
Re: [PATCH net-next v2 5/7] net: ethernet: fs_enet: fcc: use macros for speed and duplex values
On Thu, Aug 29, 2024 at 06:15:28PM +0200, Maxime Chevallier wrote: > The PHY speed and duplex should be manipulated using the SPEED_XXX and > DUPLEX_XXX macros available. Use it in the fcc, fec and scc MAC for fs_enet. > > Acked-by: Christophe Leroy > Reviewed-by: Christophe Leroy > Signed-off-by: Maxime Chevallier Reviewed-by: Andrew Lunn Andrew
Re: [PATCH net-next v2 3/7] net: ethernet: fs_enet: drop the .adjust_link custom fs_ops
> --- a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c > +++ b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c > @@ -649,12 +649,7 @@ static void fs_adjust_link(struct net_device *dev) > unsigned long flags; > > spin_lock_irqsave(&fep->lock, flags); > - > - if (fep->ops->adjust_link) > - fep->ops->adjust_link(dev); > - else > - generic_adjust_link(dev); > - > + generic_adjust_link(dev); > spin_unlock_irqrestore(&fep->lock, flags); Holding a spinlock is pretty unusual. We are in thread context, and the phydev mutex is held. Looking at generic_adjust_link, do any of the fep->foo variables actually need protecting, particularly from changes in interrupts context? Andrew
Re: [PATCH net-next v2 6/7] net: ethernet: fs_enet: simplify clock handling with devm accessors
On Thu, Aug 29, 2024 at 06:15:29PM +0200, Maxime Chevallier wrote: > devm_clock_get_enabled() can be used to simplify clock handling for the > PER register clock. > > Signed-off-by: Maxime Chevallier Reviewed-by: Andrew Lunn Andrew
Re: [PATCH net-next v2 1/7] net: ethernet: fs_enet: convert to SPDX
On Thu, Aug 29, 2024 at 06:15:24PM +0200, Maxime Chevallier wrote: > The ENET driver has SPDX tags in the header files, but they were missing > in the C files. Change the licence information to SPDX format. > > Acked-by: Christophe Leroy > Reviewed-by: Christophe Leroy > Signed-off-by: Maxime Chevallier Reviewed-by: Andrew Lunn Andrew
Re: [PATCH net-next v2 3/7] net: ethernet: fs_enet: drop the .adjust_link custom fs_ops
On Wed, Sep 04, 2024 at 10:27:11AM +0200, Maxime Chevallier wrote: > Hi Andrew, > > On Fri, 30 Aug 2024 23:06:08 +0200 > Andrew Lunn wrote: > > > > --- a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c > > > +++ b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c > > > @@ -649,12 +649,7 @@ static void fs_adjust_link(struct net_device *dev) > > > unsigned long flags; > > > > > > spin_lock_irqsave(&fep->lock, flags); > > > - > > > - if (fep->ops->adjust_link) > > > - fep->ops->adjust_link(dev); > > > - else > > > - generic_adjust_link(dev); > > > - > > > + generic_adjust_link(dev); > > > spin_unlock_irqrestore(&fep->lock, flags); > > > > Holding a spinlock is pretty unusual. We are in thread context, and > > the phydev mutex is held. Looking at generic_adjust_link, do any of > > the fep->foo variables actually need protecting, particularly from > > changes in interrupts context? > > Yes there are, the interrupt mask/event registers are being accessed > from the interrupt handler and the ->restart() hook. I can try to > rework this a bit for a cleaner interrupt handling, but I don't have > means to test this on all mac flavors (fec/fcc/scc) :( As far as i can see, none of the fep->old* members are accessed outside of fs_enet-main.c. There values are not important for the restart call. So the spinlock has nothing to do with adjust_link as such, but restart. So maybe narrow down the lock to just the restart call? But it is not a big issues, just unusual. Andrew
