Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH v2 0/3] net/phy: Improvements to Cavium Thunder MDIO code.
From: David Daney @ 2016-03-12  0:08 UTC (permalink / raw)
  To: David Daney
  Cc: David S. Miller, netdev, linux-arm-kernel, Florian Fainelli,
	Robert Richter, Sunil Goutham, Kumar Gala, Ian Campbell,
	Mark Rutland, Pawel Moll, Rob Herring, linux-kernel,
	Radha Mohan Chintakuntla, David Daney
In-Reply-To: <1457718791-6505-1-git-send-email-ddaney.cavm@gmail.com>

I am going to send a new version of this set.

David Daney


On 03/11/2016 09:53 AM, David Daney wrote:
> From: David Daney <david.daney@cavium.com>
>
> Changes from v1:
>
>   - In 1/3 Add back check for non-OF objects in bgx_init_of_phy().  It
>     is probably not necessary, but better safe than sorry...
>
> The firmware on many Cavium Thunder systems configures the MDIO bus
> hardware to be probed as a PCI device.  In order to use the MDIO bus
> drivers in this configuration, we must add PCI probing to the driver.
>
> There are two parts to this set of three patches:
>
>   1) Cleanup the PHY probing code in thunder_bgx.c to handle the case
>      where there is no PHY attached to a port, as well as being more
>      robust in the face of driver loading order by use of
>      -EPROBE_DEFER.
>
>   2) Split mdio-octeon.c into two drivers, one with platform probing,
>   and the other with PCI probing.  Common code is shared between the
>   two.
>
> Tested on several different Thunder and OCTEON systems, also compile
> tested on x86_64.
>
> David Daney (3):
>    net: thunderx: Cleanup PHY probing code.
>    phy: mdio-octeon: Refactor into two files/modules
>    phy: mdio-thunder:  Add driver for Cavium Thunder SoC MDIO buses.
>
>   .../devicetree/bindings/net/cavium-mdio.txt        |  61 ++++-
>   drivers/net/ethernet/cavium/thunder/thunder_bgx.c  |  29 ++-
>   drivers/net/phy/Kconfig                            |  22 +-
>   drivers/net/phy/Makefile                           |   2 +
>   drivers/net/phy/mdio-cavium.c                      | 149 +++++++++++
>   drivers/net/phy/mdio-cavium.h                      | 119 +++++++++
>   drivers/net/phy/mdio-octeon.c                      | 280 ++-------------------
>   drivers/net/phy/mdio-thunder.c                     | 154 ++++++++++++
>   8 files changed, 535 insertions(+), 281 deletions(-)
>   create mode 100644 drivers/net/phy/mdio-cavium.c
>   create mode 100644 drivers/net/phy/mdio-cavium.h
>   create mode 100644 drivers/net/phy/mdio-thunder.c
>

^ permalink raw reply

* Re: [RFC PATCH net-next 2/2] phy: fixed-phy: Allow DT description of an MDIO bus and PHYs.
From: Andrew Lunn @ 2016-03-12  0:05 UTC (permalink / raw)
  To: Florian Fainelli; +Cc: netdev
In-Reply-To: <56E35530.6000009@gmail.com>

> > +Child nodes represent PHYs on this mdio bus. Standard properties for
> > +fixed links, 'speed', 'full-duplex', 'pause', 'asym-pause',
> > +'link-gpios', as defined above are used. Additionally a 'reg' property
> > +is required for the address of the PHY on the bus. This should be of
> > +value 0 to 31.
> 
> This is a virtual bus, the only limitation is because we re-use to the
> maximum permission extent the real MDIO bus code, and this is putting a
> SW constraint on something that does not have one here.

True. I was too lazy to audit the phy and mdio code to see if it
actually allows phy_addr >= 32. And it is easy to instantiate another
bus if you need it.
 
> > +
> > +Example
> > +-------
> > +
> > +mdio {
> > +	compatible = "linux,mdio-fixed-phy";
> > +	#address-cells = <1>;
> > +	#size-cells = <0>;
> > +
> > +	phy0: phy@0 {
> > +		reg = <0>;
> > +		speed = <1000>;
> > +		pause;
> > +		link-gpios = <&gpio0 12 GPIO_ACTIVE_HIGH>;
> > +	};
> > +
> > +	phy1: phy@8 {
> > +		reg = <8>;
> > +		speed = <100>;
> > +		full-duplex;
> > +	};
> 
> The 'fixed-link' property, although it suffers from one basic issue
> which is that it is placed on the consumer of it (e.g: an Ethernet MAC
> Device Tree node) while being a two-headed snake (you actually describe
> a data-pipe with a fixed-link) is not quite perfect, but this does not
> seem to look any better.

It is actually just the same. Not worse, not better.

I see the real benefit in the drivers. They get fixed-phy for
free. Cleanup on release just works, which most drivers don't even do
now, so leak a fixed phy every time they are unloaded. DSA has some
messy code, which has had a number of bugs, handling fixed phys. By
adding this extra code, which i hope is reasonably simple to
understand and review, we make the MAC drivers a lot simpler and less
error prone.
 
> One thing that Thomas solved nicely was avoid the need for allocating an
> address on the virtual MDIO bus, but this is coming back here, which
> does not seem needed except for correctness wrt. how real MDIO buses.

Actually Thomas's code is broken. Fixed phy addresses are allocated
incrementally. When a fixed phy is released, its address is not
re-used. So as soon as you have gone through 32 alloc/free cycles, all
further allocs fail. This is however fixable.

	Andrew

^ permalink raw reply

* Re: [PATCH RFC v2 08/32] net: dsa: dsa.c: Refactor to increase symmetry
From: Florian Fainelli @ 2016-03-11 23:54 UTC (permalink / raw)
  To: Andrew Lunn, Vivien Didelot, netdev
In-Reply-To: <1456677700-23027-9-git-send-email-andrew@lunn.ch>

On 28/02/16 08:41, Andrew Lunn wrote:
> Create a dsa_switch_finish() which does the opposite of dsa_switch_setup().
> Create a dsa_finish_dst() which does the opposite of dsa_setup_dst().
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

^ permalink raw reply

* Re: [PATCH RFC v2 03/32] dsa: Make setup and finish more symmetrical
From: Florian Fainelli @ 2016-03-11 23:54 UTC (permalink / raw)
  To: Andrew Lunn, Vivien Didelot, netdev
In-Reply-To: <1456677700-23027-4-git-send-email-andrew@lunn.ch>

On 28/02/16 08:41, Andrew Lunn wrote:
> Rename and reposition dsa_switch_destroy() to dsa_switch_finish_one()
> to make it clear it is the opposite of dsa_switch_setup_one().
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

^ permalink raw reply

* Re: [PATCH RFC v2 00/32] Make DSA switches linux devices.
From: Florian Fainelli @ 2016-03-11 23:41 UTC (permalink / raw)
  To: Andrew Lunn; +Cc: Vivien Didelot, netdev
In-Reply-To: <20160303202716.GQ15541@lunn.ch>

On 03/03/16 12:27, Andrew Lunn wrote:
>> - first of all, the original design around the special platform device
>> did not allow multiple switch trees within the same system to coexist
>> (dsa platform device were not numbered (id = -1)), but such a thing
>> could exist and is desirable, you could have a single switch hanging off
>> eth0, and more switches hanging off eth1 for instance, and not be part
>> of the same tree
> 
> I have hardware i can test such a setup on. 
> 
>>
>> - the direction we want to move people to is to make them use DSA for
>> their switch needs and get the proven benefits from having a consistent
>> per-port slave network device model along with a good binding for
>> representing ports within a switch (and all thedetails associated with
>> that), the next step is to make this available to not just MDIO drivers,
>> which you are addressing here, but then, being able to call
>> dsa_switch_register() just becomes a service from the network stack with
>> DSA support included, if we need the special dsa platform device again,
>> we are not way better than where we were before
> 
> We are a bit better. dsa_switch_register() does not care about the
> communication channel to the switch. An SPI based switch should now be
> possible, as well as a cleaner way to do MMIO switches.

Right, thanks to your patch series and recent efforts, and that is
exactly the direction we want.

[snip]

> 
> The third switch is as you would expect, dsa,member = <0 2>;

I like that representation.

> 
> The probe order does not actually matter. You allocate the dst when
> the first switch arrives, and plug that switch in. You then evaluate
> the dst. Are all dsa links fulfilled. If yes, you have the full tree,
> and you can set it up and running. If no, wait until more switches are
> registered.

So does that mean you agree we do not need the DSA platform device
anymore :)? If not, why, and how much help from the Device Tree do we
need to instantiate that special "dsa" platform device?

> 
> This is quite a big change, so why not make it bigger...
> 
> One thing i don't like is the complexity we have in matching phys to
> ports, and fixed-link phys. Maybe we should consolidate this:

Very true, we support a wide variety of setups, and that creates a lot
of complexity that could probably be absorbed by a more generic helper
function?

> 
> 1) The switch device should use mdiobus_alloc()/mdiobus_register() for
> its own MDIO bus.

Agreed, possibly with the help of the DSA slave code, since some of that
is already doing its magic for most drivers here.

> 2) ports use phy-handle to point to phys on their own mdio bus.

I would refrain from needing that unless the mapping between Port and
built-in PHY is design/board-level configurable, or we connect to PHYs
external to the switch which are not located on its own internal MDIO
bus controller.
-- 
Florian

^ permalink raw reply

* Re: [RFC PATCH net-next 0/2] DT MDIO bus of fixed phys
From: Florian Fainelli @ 2016-03-11 23:38 UTC (permalink / raw)
  To: Andrew Lunn; +Cc: netdev
In-Reply-To: <20160311233610.GB23969@lunn.ch>

On 11/03/16 15:36, Andrew Lunn wrote:
> On Fri, Mar 11, 2016 at 03:26:42PM -0800, Florian Fainelli wrote:
>> On 11/03/16 15:08, Andrew Lunn wrote:
>>> Currently, supporting a fixed-phy in a MAC driver is a bit messy. It
>>> needs to be explicit supported, since a fixed phy is somewhat
>>> different from a normal phy.
>>>
>>> These two patches solve this by making fixed-phys appear as normal
>>> PHYs within device tree, allowing them to be referenced by a phandle.
>>> Any MAC driver that supports phy-handle can then automatically support
>>> a fixed-phy without any code change.
>>
>> Humm, if that's the problem we want to solve, we could introduce a
>> helper function which tries to locate the phy using a 'phy-handle'
>> property
> 
> I don't follow you. Where do you get a phandle from to use with
> phy-handle?

>From the caller of the function: the consumer of that phy-handle and/or
fixed-link property which is either an Ethernet MAC driver or a DSA's
switch port node.
-- 
Florian

^ permalink raw reply

* Re: [RFC PATCH net-next 0/2] DT MDIO bus of fixed phys
From: Andrew Lunn @ 2016-03-11 23:36 UTC (permalink / raw)
  To: Florian Fainelli; +Cc: netdev
In-Reply-To: <56E35432.2010300@gmail.com>

On Fri, Mar 11, 2016 at 03:26:42PM -0800, Florian Fainelli wrote:
> On 11/03/16 15:08, Andrew Lunn wrote:
> > Currently, supporting a fixed-phy in a MAC driver is a bit messy. It
> > needs to be explicit supported, since a fixed phy is somewhat
> > different from a normal phy.
> > 
> > These two patches solve this by making fixed-phys appear as normal
> > PHYs within device tree, allowing them to be referenced by a phandle.
> > Any MAC driver that supports phy-handle can then automatically support
> > a fixed-phy without any code change.
> 
> Humm, if that's the problem we want to solve, we could introduce a
> helper function which tries to locate the phy using a 'phy-handle'
> property

I don't follow you. Where do you get a phandle from to use with
phy-handle?

	Andrew

^ permalink raw reply

* Re: [RFC PATCH net-next 2/2] phy: fixed-phy: Allow DT description of an MDIO bus and PHYs.
From: Florian Fainelli @ 2016-03-11 23:30 UTC (permalink / raw)
  To: Andrew Lunn, netdev
In-Reply-To: <1457737726-23907-3-git-send-email-andrew@lunn.ch>

On 11/03/16 15:08, Andrew Lunn wrote:
> Not all MACs are connected to PHYs. They can for example be connected
> to a switch. Using the fixed PHY properties with the MAC is possible,
> but requires support in the MAC. It is however simpler to make use of
> the phy-handle property to point to a PHY. To achieve this, the PHY
> must be in the device tree.
> 
> Allow virtual MDIO busses to be represented in device tree, which
> contains virtual fixed-phys.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  .../devicetree/bindings/net/fixed-link.txt         |  39 +++++++
>  drivers/net/phy/fixed_phy.c                        | 122 ++++++++++++++++++++-
>  2 files changed, 157 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/fixed-link.txt b/Documentation/devicetree/bindings/net/fixed-link.txt
> index ec5d889fe3d8..2a22b92007fa 100644
> --- a/Documentation/devicetree/bindings/net/fixed-link.txt
> +++ b/Documentation/devicetree/bindings/net/fixed-link.txt
> @@ -52,3 +52,42 @@ ethernet@1 {
>  	};
>  	...
>  };
> +
> +Fixed link PHYs on an MDIO bus
> +------------------------------
> +
> +An alternative to using the fixed link properties in the MAC is to
> +define an MDIO bus with a number of fixed link phys on it.
> +
> +Required properties:
> +- compatible = ""linux,mdio-fixed-phy";

One too many " here.

> +- #address-cells = <1>;
> +- #size-cells = <0>;
> +
> +Child nodes represent PHYs on this mdio bus. Standard properties for
> +fixed links, 'speed', 'full-duplex', 'pause', 'asym-pause',
> +'link-gpios', as defined above are used. Additionally a 'reg' property
> +is required for the address of the PHY on the bus. This should be of
> +value 0 to 31.

This is a virtual bus, the only limitation is because we re-use to the
maximum permission extent the real MDIO bus code, and this is putting a
SW constraint on something that does not have one here.

> +
> +Example
> +-------
> +
> +mdio {
> +	compatible = "linux,mdio-fixed-phy";
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	phy0: phy@0 {
> +		reg = <0>;
> +		speed = <1000>;
> +		pause;
> +		link-gpios = <&gpio0 12 GPIO_ACTIVE_HIGH>;
> +	};
> +
> +	phy1: phy@8 {
> +		reg = <8>;
> +		speed = <100>;
> +		full-duplex;
> +	};

The 'fixed-link' property, although it suffers from one basic issue
which is that it is placed on the consumer of it (e.g: an Ethernet MAC
Device Tree node) while being a two-headed snake (you actually describe
a data-pipe with a fixed-link) is not quite perfect, but this does not
seem to look any better.

One thing that Thomas solved nicely was avoid the need for allocating an
address on the virtual MDIO bus, but this is coming back here, which
does not seem needed except for correctness wrt. how real MDIO buses.

-- 
Florian

^ permalink raw reply

* Re: [RFC PATCH net-next 0/2] DT MDIO bus of fixed phys
From: Florian Fainelli @ 2016-03-11 23:26 UTC (permalink / raw)
  To: Andrew Lunn, netdev
In-Reply-To: <1457737726-23907-1-git-send-email-andrew@lunn.ch>

On 11/03/16 15:08, Andrew Lunn wrote:
> Currently, supporting a fixed-phy in a MAC driver is a bit messy. It
> needs to be explicit supported, since a fixed phy is somewhat
> different from a normal phy.
> 
> These two patches solve this by making fixed-phys appear as normal
> PHYs within device tree, allowing them to be referenced by a phandle.
> Any MAC driver that supports phy-handle can then automatically support
> a fixed-phy without any code change.

Humm, if that's the problem we want to solve, we could introduce a
helper function which tries to locate the phy using a 'phy-handle'
property, and if it fails falls back to looking for a fixed-link
property and use that if desired. That behavior could either be
automatic, or controlled via boolean flag for instance which indicates
whether fallback is appropriate?

I am not sure the virtual MDIO bus is looking much better in that
regard, it does provide a good model for how the fixed PHYs are
implemented: as a virtual MDIO bus, but that seems quite a bit of a
stretch here...
-- 
Florian

^ permalink raw reply

* Re: [PATCH net-next 5/5] phy: fixed: Fix removal of phys.
From: Florian Fainelli @ 2016-03-11 23:18 UTC (permalink / raw)
  To: Andrew Lunn, David Miller; +Cc: netdev, Vivien Didelot
In-Reply-To: <1457737300-23660-6-git-send-email-andrew@lunn.ch>

On 11/03/16 15:01, Andrew Lunn wrote:
> The fixed phys delete function simply removed the fixed phy from the
> internal linked list and freed the memory. It however did not
> unregister the associated phy device. This meant it was still possible
> to find the phy device on the mdio bus.
> 
> Make fixed_phy_del() an internal function and add a
> fixed_phy_unregister() to unregisters the phy device and then uses
> fixed_phy_del() to free resources.
> 
> Modify DSA to use this new API function, so we don't leak phys.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/net/phy/fixed_phy.c | 11 +++++++++--
>  include/linux/phy_fixed.h   |  5 ++---
>  net/dsa/dsa.c               |  4 +---
>  3 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
> index ab9c473d75ea..fc07a8866020 100644
> --- a/drivers/net/phy/fixed_phy.c
> +++ b/drivers/net/phy/fixed_phy.c
> @@ -285,7 +285,7 @@ err_regs:
>  }
>  EXPORT_SYMBOL_GPL(fixed_phy_add);
>  
> -void fixed_phy_del(int phy_addr)
> +static void fixed_phy_del(int phy_addr)
>  {
>  	struct fixed_mdio_bus *fmb = &platform_fmb;
>  	struct fixed_phy *fp, *tmp;
> @@ -300,7 +300,6 @@ void fixed_phy_del(int phy_addr)
>  		}
>  	}
>  }
> -EXPORT_SYMBOL_GPL(fixed_phy_del);
>  
>  static int phy_fixed_addr;
>  static DEFINE_SPINLOCK(phy_fixed_addr_lock);
> @@ -371,6 +370,14 @@ struct phy_device *fixed_phy_register(unsigned int irq,
>  }
>  EXPORT_SYMBOL_GPL(fixed_phy_register);
>  
> +void fixed_phy_unregister(struct phy_device *phy)
> +{
> +	phy_device_remove(phy);
> +
> +	fixed_phy_del(phy->mdio.addr);

fixed_phy_del() should also make sure that there is no dangling
link_update callback registered, even though this is not fatal, as it
checks whether the phydev is NULL, we should automatically unregister
one by doing something like: fixed_phy_set_link_update(phydev, NULL) for
robustness.

LGTM to me otherwise, thanks!
-- 
Florian

^ permalink raw reply

* Re: [PATCH] vmxnet3: avoid calling pskb_may_pull with interrupts disabled
From: Neil Horman @ 2016-03-11 23:09 UTC (permalink / raw)
  To: Shrikrishna Khare; +Cc: Tetsuo Handa, davem, netdev, pv-drivers
In-Reply-To: <alpine.DEB.2.02.1603111141020.7592@shri-linux.eng.vmware.com>

On Fri, Mar 11, 2016 at 11:41:42AM -0800, Shrikrishna Khare wrote:
> 
> 
> On Fri, 11 Mar 2016, Tetsuo Handa wrote:
> 
> > Neil Horman wrote:
> > > On Mon, Mar 07, 2016 at 03:16:14PM -0500, David Miller wrote:
> > > > From: Neil Horman <nhorman@tuxdriver.com>
> > > > Date: Fri,  4 Mar 2016 13:40:48 -0500
> > 
> > This patch is calling spin_unlock_irqrestore() without spin_lock_irqsave().
> > 
> > In file included from include/linux/seqlock.h:35:0,
> >                  from include/linux/time.h:5,
> >                  from include/linux/stat.h:18,
> >                  from include/linux/module.h:10,
> >                  from drivers/net/vmxnet3/vmxnet3_drv.c:27:
> > drivers/net/vmxnet3/vmxnet3_drv.c: In function 'vmxnet3_xmit_frame':
> > include/linux/spinlock.h:246:30: warning: 'flags' may be used uninitialized in this function [-Wmaybe-uninitialized]
> >    _raw_spin_unlock_irqrestore(lock, flags); \
> >                               ^
> > drivers/net/vmxnet3/vmxnet3_drv.c:977:16: note: 'flags' was declared here
> >   unsigned long flags;
> >                 ^
> > 
> > vmxnet3_tq_xmit(struct sk_buff *skb, struct vmxnet3_tx_queue *tq,
> >                 struct vmxnet3_adapter *adapter, struct net_device *netdev)
> > {
> > (...snipped...)
> > 		goto hdr_too_big;
> > (...snipped...)
> > 	spin_lock_irqsave(&tq->tx_lock, flags);
> > (...snipped...)
> > 	spin_unlock_irqrestore(&tq->tx_lock, flags);
> > (...snipped...)
> > 	return NETDEV_TX_OK;
> > (...snipped...)
> > hdr_too_big:
> >         tq->stats.drop_oversized_hdr++;
> > unlock_drop_pkt:
> >         spin_unlock_irqrestore(&tq->tx_lock, flags);
> > (...snipped...)
> > 	return NETDEV_TX_OK;
> > }
> > 
> Thank you for reporting this. Will send out a fix later today.
> 
Shoot, sorry about that, I'm traveling today, but the fix looks easy.  If you
can send it out please Shrikrishna, I'd appreciate it.
Neil

^ permalink raw reply

* [RFC PATCH net-next 0/2] DT MDIO bus of fixed phys
From: Andrew Lunn @ 2016-03-11 23:08 UTC (permalink / raw)
  To: netdev, Florian Fainelli; +Cc: Andrew Lunn

Currently, supporting a fixed-phy in a MAC driver is a bit messy. It
needs to be explicit supported, since a fixed phy is somewhat
different from a normal phy.

These two patches solve this by making fixed-phys appear as normal
PHYs within device tree, allowing them to be referenced by a phandle.
Any MAC driver that supports phy-handle can then automatically support
a fixed-phy without any code change.

Andrew Lunn (2):
  of: of_mdio: Factor out fixed-link parsing
  phy: fixed-phy: Allow DT description of an MDIO bus and PHYs.

 .../devicetree/bindings/net/fixed-link.txt         |  39 +++++++
 drivers/net/phy/fixed_phy.c                        | 122 ++++++++++++++++++++-
 drivers/of/of_mdio.c                               |  34 +++---
 include/linux/of_mdio.h                            |  11 +-
 4 files changed, 188 insertions(+), 18 deletions(-)

-- 
2.7.0

^ permalink raw reply

* [RFC PATCH net-next 1/2] of: of_mdio: Factor out fixed-link parsing
From: Andrew Lunn @ 2016-03-11 23:08 UTC (permalink / raw)
  To: netdev, Florian Fainelli; +Cc: Andrew Lunn
In-Reply-To: <1457737726-23907-1-git-send-email-andrew@lunn.ch>

Turn the parsing of the fixed link properties into a helper function,
and export it for others to use.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/of/of_mdio.c    | 34 +++++++++++++++++++++-------------
 include/linux/of_mdio.h | 11 ++++++++++-
 2 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 5e7838290998..e9ad328174ff 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -397,6 +397,23 @@ bool of_phy_is_fixed_link(struct device_node *np)
 }
 EXPORT_SYMBOL(of_phy_is_fixed_link);
 
+int of_phy_parse_fixed_link(struct device_node *np,
+			    struct fixed_phy_status *status,
+			    int *link_gpio)
+{
+	status->link = 1;
+	status->duplex = of_property_read_bool(np, "full-duplex");
+	if (of_property_read_u32(np, "speed", &status->speed))
+		return -EINVAL;
+	status->pause = of_property_read_bool(np, "pause");
+	status->asym_pause = of_property_read_bool(np, "asym-pause");
+	*link_gpio = of_get_named_gpio_flags(np, "link-gpios", 0, NULL);
+	if (*link_gpio == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+	return 0;
+}
+EXPORT_SYMBOL(of_phy_parse_fixed_link);
+
 int of_phy_register_fixed_link(struct device_node *np)
 {
 	struct fixed_phy_status status = {};
@@ -419,20 +436,11 @@ int of_phy_register_fixed_link(struct device_node *np)
 	/* New binding */
 	fixed_link_node = of_get_child_by_name(np, "fixed-link");
 	if (fixed_link_node) {
-		status.link = 1;
-		status.duplex = of_property_read_bool(fixed_link_node,
-						      "full-duplex");
-		if (of_property_read_u32(fixed_link_node, "speed", &status.speed))
-			return -EINVAL;
-		status.pause = of_property_read_bool(fixed_link_node, "pause");
-		status.asym_pause = of_property_read_bool(fixed_link_node,
-							  "asym-pause");
-		link_gpio = of_get_named_gpio_flags(fixed_link_node,
-						    "link-gpios", 0, NULL);
+		err = of_phy_parse_fixed_link(fixed_link_node, &status,
+					      &link_gpio);
 		of_node_put(fixed_link_node);
-		if (link_gpio == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
-
+		if (err)
+			return err;
 		phy = fixed_phy_register(PHY_POLL, &status, link_gpio, np);
 		return IS_ERR(phy) ? PTR_ERR(phy) : 0;
 	}
diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
index 8f2237eb3485..1286a76dbcf0 100644
--- a/include/linux/of_mdio.h
+++ b/include/linux/of_mdio.h
@@ -10,6 +10,7 @@
 #define __LINUX_OF_MDIO_H
 
 #include <linux/phy.h>
+#include <linux/phy_fixed.h>
 #include <linux/of.h>
 
 #ifdef CONFIG_OF
@@ -25,7 +26,6 @@ struct phy_device *of_phy_attach(struct net_device *dev,
 
 extern struct mii_bus *of_mdio_find_bus(struct device_node *mdio_np);
 extern int of_mdio_parse_addr(struct device *dev, const struct device_node *np);
-
 #else /* CONFIG_OF */
 static inline int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 {
@@ -72,6 +72,9 @@ static inline int of_mdio_parse_addr(struct device *dev,
 #if defined(CONFIG_OF) && defined(CONFIG_FIXED_PHY)
 extern int of_phy_register_fixed_link(struct device_node *np);
 extern bool of_phy_is_fixed_link(struct device_node *np);
+extern int of_phy_parse_fixed_link(struct device_node *np,
+				   struct fixed_phy_status *status,
+				   int *link_gpio);
 #else
 static inline int of_phy_register_fixed_link(struct device_node *np)
 {
@@ -81,6 +84,12 @@ static inline bool of_phy_is_fixed_link(struct device_node *np)
 {
 	return false;
 }
+static inline int of_phy_parse_fixed_link(struct device_node *np,
+				   struct fixed_phy_status *status,
+				   int *link_gpio)
+{
+	return -ENOSYS;
+}
 #endif
 
 
-- 
2.7.0

^ permalink raw reply related

* [RFC PATCH net-next 2/2] phy: fixed-phy: Allow DT description of an MDIO bus and PHYs.
From: Andrew Lunn @ 2016-03-11 23:08 UTC (permalink / raw)
  To: netdev, Florian Fainelli; +Cc: Andrew Lunn
In-Reply-To: <1457737726-23907-1-git-send-email-andrew@lunn.ch>

Not all MACs are connected to PHYs. They can for example be connected
to a switch. Using the fixed PHY properties with the MAC is possible,
but requires support in the MAC. It is however simpler to make use of
the phy-handle property to point to a PHY. To achieve this, the PHY
must be in the device tree.

Allow virtual MDIO busses to be represented in device tree, which
contains virtual fixed-phys.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 .../devicetree/bindings/net/fixed-link.txt         |  39 +++++++
 drivers/net/phy/fixed_phy.c                        | 122 ++++++++++++++++++++-
 2 files changed, 157 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/fixed-link.txt b/Documentation/devicetree/bindings/net/fixed-link.txt
index ec5d889fe3d8..2a22b92007fa 100644
--- a/Documentation/devicetree/bindings/net/fixed-link.txt
+++ b/Documentation/devicetree/bindings/net/fixed-link.txt
@@ -52,3 +52,42 @@ ethernet@1 {
 	};
 	...
 };
+
+Fixed link PHYs on an MDIO bus
+------------------------------
+
+An alternative to using the fixed link properties in the MAC is to
+define an MDIO bus with a number of fixed link phys on it.
+
+Required properties:
+- compatible = ""linux,mdio-fixed-phy";
+- #address-cells = <1>;
+- #size-cells = <0>;
+
+Child nodes represent PHYs on this mdio bus. Standard properties for
+fixed links, 'speed', 'full-duplex', 'pause', 'asym-pause',
+'link-gpios', as defined above are used. Additionally a 'reg' property
+is required for the address of the PHY on the bus. This should be of
+value 0 to 31.
+
+Example
+-------
+
+mdio {
+	compatible = "linux,mdio-fixed-phy";
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	phy0: phy@0 {
+		reg = <0>;
+		speed = <1000>;
+		pause;
+		link-gpios = <&gpio0 12 GPIO_ACTIVE_HIGH>;
+	};
+
+	phy1: phy@8 {
+		reg = <8>;
+		speed = <100>;
+		full-duplex;
+	};
+};
diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
index fc07a8866020..4df2fcdcee68 100644
--- a/drivers/net/phy/fixed_phy.c
+++ b/drivers/net/phy/fixed_phy.c
@@ -22,6 +22,8 @@
 #include <linux/err.h>
 #include <linux/slab.h>
 #include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_mdio.h>
 #include <linux/gpio.h>
 
 #define MII_REGS_NUM 29
@@ -241,12 +243,12 @@ int fixed_phy_update_state(struct phy_device *phydev,
 }
 EXPORT_SYMBOL(fixed_phy_update_state);
 
-int fixed_phy_add(unsigned int irq, int phy_addr,
-		  struct fixed_phy_status *status,
-		  int link_gpio)
+int fixed_phy_add_fmb(struct fixed_mdio_bus *fmb,
+		      unsigned int irq, int phy_addr,
+		      struct fixed_phy_status *status,
+		      int link_gpio)
 {
 	int ret;
-	struct fixed_mdio_bus *fmb = &platform_fmb;
 	struct fixed_phy *fp;
 
 	fp = kzalloc(sizeof(*fp), GFP_KERNEL);
@@ -283,6 +285,14 @@ err_regs:
 	kfree(fp);
 	return ret;
 }
+
+int fixed_phy_add(unsigned int irq, int phy_addr,
+		  struct fixed_phy_status *status,
+		  int link_gpio)
+{
+	return fixed_phy_add_fmb(&platform_fmb, irq, phy_addr, status,
+				 link_gpio);
+}
 EXPORT_SYMBOL_GPL(fixed_phy_add);
 
 static void fixed_phy_del(int phy_addr)
@@ -378,6 +388,103 @@ void fixed_phy_unregister(struct phy_device *phy)
 }
 EXPORT_SYMBOL_GPL(fixed_phy_unregister);
 
+static int mdio_fixed_phy_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct fixed_phy_status status;
+	struct fixed_mdio_bus *fmb;
+	struct device_node *child;
+	struct mii_bus *bus;
+	int phy_addr, irq;
+	int link_gpio;
+
+	int ret;
+
+	if (!np)
+		return -EINVAL;
+
+	bus = mdiobus_alloc_size(sizeof(*fmb));
+	if (!bus)
+		return -ENOMEM;
+
+	if (strlen(pdev->name) >= MII_BUS_ID_SIZE)
+		return -EINVAL;
+
+	strncpy(bus->id, pdev->name, MII_BUS_ID_SIZE - 1);
+	bus->name = bus->id;
+	bus->parent = &pdev->dev;
+	bus->read = fixed_mdio_read;
+	bus->write = fixed_mdio_write;
+	fmb = bus->priv;
+	fmb->mii_bus = bus;
+	INIT_LIST_HEAD(&fmb->phys);
+
+	for_each_available_child_of_node(np, child) {
+		phy_addr = of_mdio_parse_addr(&pdev->dev, child);
+		if (phy_addr < 0) {
+			ret = phy_addr;
+			goto err_out_free_phys;
+		}
+		irq = irq_of_parse_and_map(child, 0);
+
+		ret = of_phy_parse_fixed_link(child, &status, &link_gpio);
+		if (ret < 0)
+			goto err_out_free_phys;
+
+		ret = fixed_phy_add_fmb(fmb, irq, phy_addr,
+					&status, link_gpio);
+		if (ret < 0)
+			goto err_out_free_phys;
+	}
+
+	ret = of_mdiobus_register(bus, np);
+	if (ret < 0)
+		goto err_out_free_mdiobus;
+
+	platform_set_drvdata(pdev, bus);
+
+	return 0;
+
+err_out_free_phys:
+	for (phy_addr = 0; phy_addr < PHY_MAX_ADDR; phy_addr++)
+		fixed_phy_del(phy_addr);
+
+err_out_free_mdiobus:
+	mdiobus_free(bus);
+	return ret;
+}
+
+static int mdio_fixed_phy_remove(struct platform_device *pdev)
+{
+	struct mii_bus *bus = platform_get_drvdata(pdev);
+	int phy_addr;
+
+	mdiobus_unregister(bus);
+
+	for (phy_addr = 0; phy_addr < PHY_MAX_ADDR; phy_addr++)
+		fixed_phy_del(phy_addr);
+
+	mdiobus_free(bus);
+
+	return 0;
+}
+
+static const struct of_device_id mdio_fixed_phy_dt_ids[] = {
+	{ .compatible = "linux,mdio-fixed-phy" },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(of, mdio_fixed_phy_dt_ids);
+
+static struct platform_driver mdio_fixed_phy_driver = {
+	.probe = mdio_fixed_phy_probe,
+	.remove = mdio_fixed_phy_remove,
+	.driver = {
+		.name = "mdio-fixed-phy",
+		.of_match_table = mdio_fixed_phy_dt_ids,
+	},
+};
+
 static int __init fixed_mdio_bus_init(void)
 {
 	struct fixed_mdio_bus *fmb = &platform_fmb;
@@ -406,8 +513,14 @@ static int __init fixed_mdio_bus_init(void)
 	if (ret)
 		goto err_mdiobus_alloc;
 
+	ret = platform_driver_register(&mdio_fixed_phy_driver);
+	if (ret)
+		goto err_mdiobus_register;
+
 	return 0;
 
+err_mdiobus_register:
+	mdiobus_unregister(fmb->mii_bus);
 err_mdiobus_alloc:
 	mdiobus_free(fmb->mii_bus);
 err_mdiobus_reg:
@@ -422,6 +535,7 @@ static void __exit fixed_mdio_bus_exit(void)
 	struct fixed_mdio_bus *fmb = &platform_fmb;
 	struct fixed_phy *fp, *tmp;
 
+	platform_driver_unregister(&mdio_fixed_phy_driver);
 	mdiobus_unregister(fmb->mii_bus);
 	mdiobus_free(fmb->mii_bus);
 	platform_device_unregister(pdev);
-- 
2.7.0

^ permalink raw reply related

* Re: [PATCH 1/2] of_mdio: use IS_ERR_OR_NULL()
From: Florian Fainelli @ 2016-03-11 23:03 UTC (permalink / raw)
  To: Vladimir Zapolskiy, Sergei Shtylyov, grant.likely, robh+dt,
	devicetree, netdev, frowand.list
In-Reply-To: <56E34C56.10300@mleia.com>

On 11/03/16 14:53, Vladimir Zapolskiy wrote:
> Hello Sergei,
> 
> On 12.03.2016 00:12, Sergei Shtylyov wrote:
>> IS_ERR_OR_NULL() is open coded in of_mdiobus_register_{phy|device}(), so
>> just call it directly...
>>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>
>> ---
>>  drivers/of/of_mdio.c |    4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> Index: net-next/drivers/of/of_mdio.c
>> ===================================================================
>> --- net-next.orig/drivers/of/of_mdio.c
>> +++ net-next/drivers/of/of_mdio.c
>> @@ -56,7 +56,7 @@ static int of_mdiobus_register_phy(struc
>>  		phy = phy_device_create(mdio, addr, phy_id, 0, NULL);
>>  	else
>>  		phy = get_phy_device(mdio, addr, is_c45);
>> -	if (!phy || IS_ERR(phy))
>> +	if (IS_ERR_OR_NULL(phy))
>>  		return 1;
>>  
>>  	rc = irq_of_parse_and_map(child, 0);
>> @@ -98,7 +98,7 @@ static int of_mdiobus_register_device(st
>>  	int rc;
>>  
>>  	mdiodev = mdio_device_create(mdio, addr);
>> -	if (!mdiodev || IS_ERR(mdiodev))
>> +	if (IS_ERR_OR_NULL(mdiodev))
>>  		return 1;
>>  
> 
> I don't see how it is possible for mdio_device_create() to return NULL,
> instead of this change consider to remove if (!mdiodev) check.

Good point, worst case we return PTR_ERR(-ENOMEM).
-- 
Florian

^ permalink raw reply

* [PATCH net-next 0/5] DSA cleanup and fixes
From: Andrew Lunn @ 2016-03-11 23:01 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, Florian Fainelli, Vivien Didelot, Andrew Lunn

The RFC patchset for re-architecturing DSA probing contains a few
standalone patches, either clean up or fixes. This pulls them out for
submission.

Andrew Lunn (5):
  dsa: Rename mv88e6123_61_65 to mv88e6123 to be consistent
  dsa: slave: Don't reference NULL pointer during phy_disconnect
  dsa: Destroy fixed link phys after the phy has been disconnected
  dsa: dsa: Fix freeing of fixed-phys from user ports.
  phy: fixed: Fix removal of phys.

 arch/arm/configs/multi_v5_defconfig |   2 +-
 arch/arm/configs/mvebu_v5_defconfig |   2 +-
 arch/arm/configs/orion5x_defconfig  |   2 +-
 arch/tile/configs/tilegx_defconfig  |   2 +-
 arch/tile/configs/tilepro_defconfig |   2 +-
 drivers/net/dsa/Kconfig             |   2 +-
 drivers/net/dsa/Makefile            |   4 +-
 drivers/net/dsa/mv88e6123.c         | 124 ++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6123_61_65.c   | 124 ------------------------------------
 drivers/net/dsa/mv88e6xxx.c         |   8 +--
 drivers/net/dsa/mv88e6xxx.h         |   2 +-
 drivers/net/phy/fixed_phy.c         |  11 +++-
 include/linux/phy_fixed.h           |   5 +-
 net/dsa/dsa.c                       |  27 ++++----
 net/dsa/slave.c                     |  12 ++--
 15 files changed, 167 insertions(+), 162 deletions(-)
 create mode 100644 drivers/net/dsa/mv88e6123.c
 delete mode 100644 drivers/net/dsa/mv88e6123_61_65.c

-- 
2.7.0

^ permalink raw reply

* [PATCH net-next 1/5] dsa: Rename mv88e6123_61_65 to mv88e6123 to be consistent
From: Andrew Lunn @ 2016-03-11 23:01 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, Florian Fainelli, Vivien Didelot, Andrew Lunn
In-Reply-To: <1457737300-23660-1-git-send-email-andrew@lunn.ch>

All the drivers support multiple chips, but mv88e6123_61_65 is the
only one that reflects this in its naming. Change it to be consistent
with the other drivers.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 arch/arm/configs/multi_v5_defconfig |   2 +-
 arch/arm/configs/mvebu_v5_defconfig |   2 +-
 arch/arm/configs/orion5x_defconfig  |   2 +-
 arch/tile/configs/tilegx_defconfig  |   2 +-
 arch/tile/configs/tilepro_defconfig |   2 +-
 drivers/net/dsa/Kconfig             |   2 +-
 drivers/net/dsa/Makefile            |   4 +-
 drivers/net/dsa/mv88e6123.c         | 124 ++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6123_61_65.c   | 124 ------------------------------------
 drivers/net/dsa/mv88e6xxx.c         |   8 +--
 drivers/net/dsa/mv88e6xxx.h         |   2 +-
 11 files changed, 137 insertions(+), 137 deletions(-)
 create mode 100644 drivers/net/dsa/mv88e6123.c
 delete mode 100644 drivers/net/dsa/mv88e6123_61_65.c

diff --git a/arch/arm/configs/multi_v5_defconfig b/arch/arm/configs/multi_v5_defconfig
index 1f9ca4737ef6..afb1f62fb05e 100644
--- a/arch/arm/configs/multi_v5_defconfig
+++ b/arch/arm/configs/multi_v5_defconfig
@@ -91,7 +91,7 @@ CONFIG_SATA_MV=y
 CONFIG_NETDEVICES=y
 CONFIG_NET_DSA_MV88E6060=y
 CONFIG_NET_DSA_MV88E6131=y
-CONFIG_NET_DSA_MV88E6123_61_65=y
+CONFIG_NET_DSA_MV88E6123=y
 CONFIG_NET_DSA_MV88E6171=y
 CONFIG_NET_DSA_MV88E6352=y
 CONFIG_MV643XX_ETH=y
diff --git a/arch/arm/configs/mvebu_v5_defconfig b/arch/arm/configs/mvebu_v5_defconfig
index af29780accdc..6c4c54037bc4 100644
--- a/arch/arm/configs/mvebu_v5_defconfig
+++ b/arch/arm/configs/mvebu_v5_defconfig
@@ -92,7 +92,7 @@ CONFIG_SATA_MV=y
 CONFIG_NETDEVICES=y
 CONFIG_NET_DSA_MV88E6060=y
 CONFIG_NET_DSA_MV88E6131=y
-CONFIG_NET_DSA_MV88E6123_61_65=y
+CONFIG_NET_DSA_MV88E6123=y
 CONFIG_NET_DSA_MV88E6171=y
 CONFIG_NET_DSA_MV88E6352=y
 CONFIG_MV643XX_ETH=y
diff --git a/arch/arm/configs/orion5x_defconfig b/arch/arm/configs/orion5x_defconfig
index 5876ce7af130..6a5bc27538f1 100644
--- a/arch/arm/configs/orion5x_defconfig
+++ b/arch/arm/configs/orion5x_defconfig
@@ -86,7 +86,7 @@ CONFIG_SATA_MV=y
 CONFIG_NETDEVICES=y
 CONFIG_MII=y
 CONFIG_NET_DSA_MV88E6131=y
-CONFIG_NET_DSA_MV88E6123_61_65=y
+CONFIG_NET_DSA_MV88E6123=y
 CONFIG_MV643XX_ETH=y
 CONFIG_MARVELL_PHY=y
 # CONFIG_INPUT_MOUSEDEV is not set
diff --git a/arch/tile/configs/tilegx_defconfig b/arch/tile/configs/tilegx_defconfig
index 37dc9364c4a1..984fa00a8c25 100644
--- a/arch/tile/configs/tilegx_defconfig
+++ b/arch/tile/configs/tilegx_defconfig
@@ -222,7 +222,7 @@ CONFIG_TUN=y
 CONFIG_VETH=m
 CONFIG_NET_DSA_MV88E6060=y
 CONFIG_NET_DSA_MV88E6131=y
-CONFIG_NET_DSA_MV88E6123_61_65=y
+CONFIG_NET_DSA_MV88E6123=y
 CONFIG_SKY2=y
 CONFIG_PTP_1588_CLOCK_TILEGX=y
 # CONFIG_WLAN is not set
diff --git a/arch/tile/configs/tilepro_defconfig b/arch/tile/configs/tilepro_defconfig
index 76a2781dec2c..71ad9f7e40c9 100644
--- a/arch/tile/configs/tilepro_defconfig
+++ b/arch/tile/configs/tilepro_defconfig
@@ -341,7 +341,7 @@ CONFIG_TUN=y
 CONFIG_VETH=m
 CONFIG_NET_DSA_MV88E6060=y
 CONFIG_NET_DSA_MV88E6131=y
-CONFIG_NET_DSA_MV88E6123_61_65=y
+CONFIG_NET_DSA_MV88E6123=y
 # CONFIG_NET_VENDOR_3COM is not set
 CONFIG_E1000E=y
 # CONFIG_WLAN is not set
diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
index 4c483d937481..90ba003d8fdf 100644
--- a/drivers/net/dsa/Kconfig
+++ b/drivers/net/dsa/Kconfig
@@ -27,7 +27,7 @@ config NET_DSA_MV88E6131
 	  This enables support for the Marvell 88E6085/6095/6095F/6131
 	  ethernet switch chips.
 
-config NET_DSA_MV88E6123_61_65
+config NET_DSA_MV88E6123
 	tristate "Marvell 88E6123/6161/6165 ethernet switch chip support"
 	depends on NET_DSA
 	select NET_DSA_MV88E6XXX
diff --git a/drivers/net/dsa/Makefile b/drivers/net/dsa/Makefile
index e2d51c4b9382..a6e09939be65 100644
--- a/drivers/net/dsa/Makefile
+++ b/drivers/net/dsa/Makefile
@@ -1,8 +1,8 @@
 obj-$(CONFIG_NET_DSA_MV88E6060) += mv88e6060.o
 obj-$(CONFIG_NET_DSA_MV88E6XXX) += mv88e6xxx_drv.o
 mv88e6xxx_drv-y += mv88e6xxx.o
-ifdef CONFIG_NET_DSA_MV88E6123_61_65
-mv88e6xxx_drv-y += mv88e6123_61_65.o
+ifdef CONFIG_NET_DSA_MV88E6123
+mv88e6xxx_drv-y += mv88e6123.o
 endif
 ifdef CONFIG_NET_DSA_MV88E6131
 mv88e6xxx_drv-y += mv88e6131.o
diff --git a/drivers/net/dsa/mv88e6123.c b/drivers/net/dsa/mv88e6123.c
new file mode 100644
index 000000000000..69a6f79dcb10
--- /dev/null
+++ b/drivers/net/dsa/mv88e6123.c
@@ -0,0 +1,124 @@
+/*
+ * net/dsa/mv88e6123_61_65.c - Marvell 88e6123/6161/6165 switch chip support
+ * Copyright (c) 2008-2009 Marvell Semiconductor
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/delay.h>
+#include <linux/jiffies.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/phy.h>
+#include <net/dsa.h>
+#include "mv88e6xxx.h"
+
+static const struct mv88e6xxx_switch_id mv88e6123_table[] = {
+	{ PORT_SWITCH_ID_6123, "Marvell 88E6123" },
+	{ PORT_SWITCH_ID_6123_A1, "Marvell 88E6123 (A1)" },
+	{ PORT_SWITCH_ID_6123_A2, "Marvell 88E6123 (A2)" },
+	{ PORT_SWITCH_ID_6161, "Marvell 88E6161" },
+	{ PORT_SWITCH_ID_6161_A1, "Marvell 88E6161 (A1)" },
+	{ PORT_SWITCH_ID_6161_A2, "Marvell 88E6161 (A2)" },
+	{ PORT_SWITCH_ID_6165, "Marvell 88E6165" },
+	{ PORT_SWITCH_ID_6165_A1, "Marvell 88E6165 (A1)" },
+	{ PORT_SWITCH_ID_6165_A2, "Marvell 88e6165 (A2)" },
+};
+
+static char *mv88e6123_probe(struct device *host_dev, int sw_addr)
+{
+	return mv88e6xxx_lookup_name(host_dev, sw_addr, mv88e6123_table,
+				     ARRAY_SIZE(mv88e6123_table));
+}
+
+static int mv88e6123_setup_global(struct dsa_switch *ds)
+{
+	u32 upstream_port = dsa_upstream_port(ds);
+	int ret;
+	u32 reg;
+
+	ret = mv88e6xxx_setup_global(ds);
+	if (ret)
+		return ret;
+
+	/* Disable the PHY polling unit (since there won't be any
+	 * external PHYs to poll), don't discard packets with
+	 * excessive collisions, and mask all interrupt sources.
+	 */
+	REG_WRITE(REG_GLOBAL, GLOBAL_CONTROL, 0x0000);
+
+	/* Configure the upstream port, and configure the upstream
+	 * port as the port to which ingress and egress monitor frames
+	 * are to be sent.
+	 */
+	reg = upstream_port << GLOBAL_MONITOR_CONTROL_INGRESS_SHIFT |
+		upstream_port << GLOBAL_MONITOR_CONTROL_EGRESS_SHIFT |
+		upstream_port << GLOBAL_MONITOR_CONTROL_ARP_SHIFT;
+	REG_WRITE(REG_GLOBAL, GLOBAL_MONITOR_CONTROL, reg);
+
+	/* Disable remote management for now, and set the switch's
+	 * DSA device number.
+	 */
+	REG_WRITE(REG_GLOBAL, GLOBAL_CONTROL_2, ds->index & 0x1f);
+
+	return 0;
+}
+
+static int mv88e6123_setup(struct dsa_switch *ds)
+{
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	int ret;
+
+	ret = mv88e6xxx_setup_common(ds);
+	if (ret < 0)
+		return ret;
+
+	switch (ps->id) {
+	case PORT_SWITCH_ID_6123:
+		ps->num_ports = 3;
+		break;
+	case PORT_SWITCH_ID_6161:
+	case PORT_SWITCH_ID_6165:
+		ps->num_ports = 6;
+		break;
+	default:
+		return -ENODEV;
+	}
+
+	ret = mv88e6xxx_switch_reset(ds, false);
+	if (ret < 0)
+		return ret;
+
+	ret = mv88e6123_setup_global(ds);
+	if (ret < 0)
+		return ret;
+
+	return mv88e6xxx_setup_ports(ds);
+}
+
+struct dsa_switch_driver mv88e6123_switch_driver = {
+	.tag_protocol		= DSA_TAG_PROTO_EDSA,
+	.priv_size		= sizeof(struct mv88e6xxx_priv_state),
+	.probe			= mv88e6123_probe,
+	.setup			= mv88e6123_setup,
+	.set_addr		= mv88e6xxx_set_addr_indirect,
+	.phy_read		= mv88e6xxx_phy_read,
+	.phy_write		= mv88e6xxx_phy_write,
+	.get_strings		= mv88e6xxx_get_strings,
+	.get_ethtool_stats	= mv88e6xxx_get_ethtool_stats,
+	.get_sset_count		= mv88e6xxx_get_sset_count,
+	.adjust_link		= mv88e6xxx_adjust_link,
+#ifdef CONFIG_NET_DSA_HWMON
+	.get_temp		= mv88e6xxx_get_temp,
+#endif
+	.get_regs_len		= mv88e6xxx_get_regs_len,
+	.get_regs		= mv88e6xxx_get_regs,
+};
+
+MODULE_ALIAS("platform:mv88e6123");
+MODULE_ALIAS("platform:mv88e6161");
+MODULE_ALIAS("platform:mv88e6165");
diff --git a/drivers/net/dsa/mv88e6123_61_65.c b/drivers/net/dsa/mv88e6123_61_65.c
deleted file mode 100644
index d4fcf4570d95..000000000000
--- a/drivers/net/dsa/mv88e6123_61_65.c
+++ /dev/null
@@ -1,124 +0,0 @@
-/*
- * net/dsa/mv88e6123_61_65.c - Marvell 88e6123/6161/6165 switch chip support
- * Copyright (c) 2008-2009 Marvell Semiconductor
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- */
-
-#include <linux/delay.h>
-#include <linux/jiffies.h>
-#include <linux/list.h>
-#include <linux/module.h>
-#include <linux/netdevice.h>
-#include <linux/phy.h>
-#include <net/dsa.h>
-#include "mv88e6xxx.h"
-
-static const struct mv88e6xxx_switch_id mv88e6123_61_65_table[] = {
-	{ PORT_SWITCH_ID_6123, "Marvell 88E6123" },
-	{ PORT_SWITCH_ID_6123_A1, "Marvell 88E6123 (A1)" },
-	{ PORT_SWITCH_ID_6123_A2, "Marvell 88E6123 (A2)" },
-	{ PORT_SWITCH_ID_6161, "Marvell 88E6161" },
-	{ PORT_SWITCH_ID_6161_A1, "Marvell 88E6161 (A1)" },
-	{ PORT_SWITCH_ID_6161_A2, "Marvell 88E6161 (A2)" },
-	{ PORT_SWITCH_ID_6165, "Marvell 88E6165" },
-	{ PORT_SWITCH_ID_6165_A1, "Marvell 88E6165 (A1)" },
-	{ PORT_SWITCH_ID_6165_A2, "Marvell 88e6165 (A2)" },
-};
-
-static char *mv88e6123_61_65_probe(struct device *host_dev, int sw_addr)
-{
-	return mv88e6xxx_lookup_name(host_dev, sw_addr, mv88e6123_61_65_table,
-				     ARRAY_SIZE(mv88e6123_61_65_table));
-}
-
-static int mv88e6123_61_65_setup_global(struct dsa_switch *ds)
-{
-	u32 upstream_port = dsa_upstream_port(ds);
-	int ret;
-	u32 reg;
-
-	ret = mv88e6xxx_setup_global(ds);
-	if (ret)
-		return ret;
-
-	/* Disable the PHY polling unit (since there won't be any
-	 * external PHYs to poll), don't discard packets with
-	 * excessive collisions, and mask all interrupt sources.
-	 */
-	REG_WRITE(REG_GLOBAL, GLOBAL_CONTROL, 0x0000);
-
-	/* Configure the upstream port, and configure the upstream
-	 * port as the port to which ingress and egress monitor frames
-	 * are to be sent.
-	 */
-	reg = upstream_port << GLOBAL_MONITOR_CONTROL_INGRESS_SHIFT |
-		upstream_port << GLOBAL_MONITOR_CONTROL_EGRESS_SHIFT |
-		upstream_port << GLOBAL_MONITOR_CONTROL_ARP_SHIFT;
-	REG_WRITE(REG_GLOBAL, GLOBAL_MONITOR_CONTROL, reg);
-
-	/* Disable remote management for now, and set the switch's
-	 * DSA device number.
-	 */
-	REG_WRITE(REG_GLOBAL, GLOBAL_CONTROL_2, ds->index & 0x1f);
-
-	return 0;
-}
-
-static int mv88e6123_61_65_setup(struct dsa_switch *ds)
-{
-	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
-	int ret;
-
-	ret = mv88e6xxx_setup_common(ds);
-	if (ret < 0)
-		return ret;
-
-	switch (ps->id) {
-	case PORT_SWITCH_ID_6123:
-		ps->num_ports = 3;
-		break;
-	case PORT_SWITCH_ID_6161:
-	case PORT_SWITCH_ID_6165:
-		ps->num_ports = 6;
-		break;
-	default:
-		return -ENODEV;
-	}
-
-	ret = mv88e6xxx_switch_reset(ds, false);
-	if (ret < 0)
-		return ret;
-
-	ret = mv88e6123_61_65_setup_global(ds);
-	if (ret < 0)
-		return ret;
-
-	return mv88e6xxx_setup_ports(ds);
-}
-
-struct dsa_switch_driver mv88e6123_61_65_switch_driver = {
-	.tag_protocol		= DSA_TAG_PROTO_EDSA,
-	.priv_size		= sizeof(struct mv88e6xxx_priv_state),
-	.probe			= mv88e6123_61_65_probe,
-	.setup			= mv88e6123_61_65_setup,
-	.set_addr		= mv88e6xxx_set_addr_indirect,
-	.phy_read		= mv88e6xxx_phy_read,
-	.phy_write		= mv88e6xxx_phy_write,
-	.get_strings		= mv88e6xxx_get_strings,
-	.get_ethtool_stats	= mv88e6xxx_get_ethtool_stats,
-	.get_sset_count		= mv88e6xxx_get_sset_count,
-	.adjust_link		= mv88e6xxx_adjust_link,
-#ifdef CONFIG_NET_DSA_HWMON
-	.get_temp		= mv88e6xxx_get_temp,
-#endif
-	.get_regs_len		= mv88e6xxx_get_regs_len,
-	.get_regs		= mv88e6xxx_get_regs,
-};
-
-MODULE_ALIAS("platform:mv88e6123");
-MODULE_ALIAS("platform:mv88e6161");
-MODULE_ALIAS("platform:mv88e6165");
diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 5f07524083c3..5309c738ff00 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -2989,8 +2989,8 @@ static int __init mv88e6xxx_init(void)
 #if IS_ENABLED(CONFIG_NET_DSA_MV88E6131)
 	register_switch_driver(&mv88e6131_switch_driver);
 #endif
-#if IS_ENABLED(CONFIG_NET_DSA_MV88E6123_61_65)
-	register_switch_driver(&mv88e6123_61_65_switch_driver);
+#if IS_ENABLED(CONFIG_NET_DSA_MV88E6123)
+	register_switch_driver(&mv88e6123_switch_driver);
 #endif
 #if IS_ENABLED(CONFIG_NET_DSA_MV88E6352)
 	register_switch_driver(&mv88e6352_switch_driver);
@@ -3010,8 +3010,8 @@ static void __exit mv88e6xxx_cleanup(void)
 #if IS_ENABLED(CONFIG_NET_DSA_MV88E6352)
 	unregister_switch_driver(&mv88e6352_switch_driver);
 #endif
-#if IS_ENABLED(CONFIG_NET_DSA_MV88E6123_61_65)
-	unregister_switch_driver(&mv88e6123_61_65_switch_driver);
+#if IS_ENABLED(CONFIG_NET_DSA_MV88E6123)
+	unregister_switch_driver(&mv88e6123_switch_driver);
 #endif
 #if IS_ENABLED(CONFIG_NET_DSA_MV88E6131)
 	unregister_switch_driver(&mv88e6131_switch_driver);
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 3425616987ed..281cefe86afd 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -519,7 +519,7 @@ int mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
 			     int reg, int val);
 
 extern struct dsa_switch_driver mv88e6131_switch_driver;
-extern struct dsa_switch_driver mv88e6123_61_65_switch_driver;
+extern struct dsa_switch_driver mv88e6123_switch_driver;
 extern struct dsa_switch_driver mv88e6352_switch_driver;
 extern struct dsa_switch_driver mv88e6171_switch_driver;
 
-- 
2.7.0

^ permalink raw reply related

* [PATCH net-next 3/5] dsa: Destroy fixed link phys after the phy has been disconnected
From: Andrew Lunn @ 2016-03-11 23:01 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, Florian Fainelli, Vivien Didelot, Andrew Lunn
In-Reply-To: <1457737300-23660-1-git-send-email-andrew@lunn.ch>

The phy is disconnected from the slave in dsa_slave_destroy(). Don't
destroy fixed link phys until after this, since there can be fixed
linked phys connected to ports.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 net/dsa/dsa.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index d8fb47fcad05..1018e7dcfcc9 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -430,7 +430,18 @@ static void dsa_switch_destroy(struct dsa_switch *ds)
 		hwmon_device_unregister(ds->hwmon_dev);
 #endif
 
-	/* Disable configuration of the CPU and DSA ports */
+	/* Destroy network devices for physical switch ports. */
+	for (port = 0; port < DSA_MAX_PORTS; port++) {
+		if (!(ds->phys_port_mask & (1 << port)))
+			continue;
+
+		if (!ds->ports[port])
+			continue;
+
+		dsa_slave_destroy(ds->ports[port]);
+	}
+
+	/* Remove any fixed link PHYs */
 	for (port = 0; port < DSA_MAX_PORTS; port++) {
 		if (!(dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port)))
 			continue;
@@ -448,17 +459,6 @@ static void dsa_switch_destroy(struct dsa_switch *ds)
 		}
 	}
 
-	/* Destroy network devices for physical switch ports. */
-	for (port = 0; port < DSA_MAX_PORTS; port++) {
-		if (!(ds->phys_port_mask & (1 << port)))
-			continue;
-
-		if (!ds->ports[port])
-			continue;
-
-		dsa_slave_destroy(ds->ports[port]);
-	}
-
 	mdiobus_unregister(ds->slave_mii_bus);
 }
 
-- 
2.7.0

^ permalink raw reply related

* [PATCH net-next 4/5] dsa: dsa: Fix freeing of fixed-phys from user ports.
From: Andrew Lunn @ 2016-03-11 23:01 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, Florian Fainelli, Vivien Didelot, Andrew Lunn
In-Reply-To: <1457737300-23660-1-git-send-email-andrew@lunn.ch>

All ports types can have a fixed PHY associated with it. Remove the
check which limits removal to only CPU and DSA ports.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 net/dsa/dsa.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 1018e7dcfcc9..f100f340d93f 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -443,9 +443,6 @@ static void dsa_switch_destroy(struct dsa_switch *ds)
 
 	/* Remove any fixed link PHYs */
 	for (port = 0; port < DSA_MAX_PORTS; port++) {
-		if (!(dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port)))
-			continue;
-
 		port_dn = cd->port_dn[port];
 		if (of_phy_is_fixed_link(port_dn)) {
 			phydev = of_phy_find_device(port_dn);
-- 
2.7.0

^ permalink raw reply related

* Re: [PATCH 2/2] of_mdio: use PTR_ERR_OR_ZERO()
From: Vladimir Zapolskiy @ 2016-03-11 23:02 UTC (permalink / raw)
  To: Sergei Shtylyov, grant.likely, robh+dt, devicetree, f.fainelli,
	netdev, frowand.list
In-Reply-To: <11956015.EKV4zhDQJ5@wasted.cogentembedded.com>

Hello Sergei,

On 12.03.2016 00:13, Sergei Shtylyov wrote:
> PTR_ERR_OR_ZERO() is open coded in of_phy_register_fixed_link(), so just
> call it directly...
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> ---
>  drivers/of/of_mdio.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> Index: net-next/drivers/of/of_mdio.c
> ===================================================================
> --- net-next.orig/drivers/of/of_mdio.c
> +++ net-next/drivers/of/of_mdio.c
> @@ -412,7 +412,7 @@ int of_phy_register_fixed_link(struct de
>  		if (strcmp(managed, "in-band-status") == 0) {
>  			/* status is zeroed, namely its .link member */
>  			phy = fixed_phy_register(PHY_POLL, &status, -1, np);
> -			return IS_ERR(phy) ? PTR_ERR(phy) : 0;
> +			return PTR_ERR_OR_ZERO(phy);
>  		}
>  	}
>  
> @@ -434,7 +434,7 @@ int of_phy_register_fixed_link(struct de
>  			return -EPROBE_DEFER;
>  
>  		phy = fixed_phy_register(PHY_POLL, &status, link_gpio, np);
> -		return IS_ERR(phy) ? PTR_ERR(phy) : 0;
> +		return PTR_ERR_OR_ZERO(phy);
>  	}
>  
>  	/* Old binding */
> @@ -446,7 +446,7 @@ int of_phy_register_fixed_link(struct de
>  		status.pause = be32_to_cpu(fixed_link_prop[3]);
>  		status.asym_pause = be32_to_cpu(fixed_link_prop[4]);
>  		phy = fixed_phy_register(PHY_POLL, &status, -1, np);
> -		return IS_ERR(phy) ? PTR_ERR(phy) : 0;
> +		return PTR_ERR_OR_ZERO(phy);
>  	}
>  
>  	return -ENODEV;
> 

Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>

^ permalink raw reply

* [PATCH net-next 2/5] dsa: slave: Don't reference NULL pointer during phy_disconnect
From: Andrew Lunn @ 2016-03-11 23:01 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, Florian Fainelli, Vivien Didelot, Andrew Lunn
In-Reply-To: <1457737300-23660-1-git-send-email-andrew@lunn.ch>

When the phy is disconnected, the parent pointer to the netdev it was
attached to is set to NULL. The code then tries to suspend the phy,
but dsa_slave_fixed_link_update needs the parent pointer to determine
which switch the phy is connected to. So it dereferenced a NULL
pointer. Check for this condition.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 net/dsa/slave.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 27bf03d11670..49056d90b179 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -896,11 +896,15 @@ static void dsa_slave_adjust_link(struct net_device *dev)
 static int dsa_slave_fixed_link_update(struct net_device *dev,
 				       struct fixed_phy_status *status)
 {
-	struct dsa_slave_priv *p = netdev_priv(dev);
-	struct dsa_switch *ds = p->parent;
+	struct dsa_slave_priv *p;
+	struct dsa_switch *ds;
 
-	if (ds->drv->fixed_link_update)
-		ds->drv->fixed_link_update(ds, p->port, status);
+	if (dev) {
+		p = netdev_priv(dev);
+		ds = p->parent;
+		if (ds->drv->fixed_link_update)
+			ds->drv->fixed_link_update(ds, p->port, status);
+	}
 
 	return 0;
 }
-- 
2.7.0

^ permalink raw reply related

* [PATCH net-next 5/5] phy: fixed: Fix removal of phys.
From: Andrew Lunn @ 2016-03-11 23:01 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, Florian Fainelli, Vivien Didelot, Andrew Lunn
In-Reply-To: <1457737300-23660-1-git-send-email-andrew@lunn.ch>

The fixed phys delete function simply removed the fixed phy from the
internal linked list and freed the memory. It however did not
unregister the associated phy device. This meant it was still possible
to find the phy device on the mdio bus.

Make fixed_phy_del() an internal function and add a
fixed_phy_unregister() to unregisters the phy device and then uses
fixed_phy_del() to free resources.

Modify DSA to use this new API function, so we don't leak phys.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/fixed_phy.c | 11 +++++++++--
 include/linux/phy_fixed.h   |  5 ++---
 net/dsa/dsa.c               |  4 +---
 3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
index ab9c473d75ea..fc07a8866020 100644
--- a/drivers/net/phy/fixed_phy.c
+++ b/drivers/net/phy/fixed_phy.c
@@ -285,7 +285,7 @@ err_regs:
 }
 EXPORT_SYMBOL_GPL(fixed_phy_add);
 
-void fixed_phy_del(int phy_addr)
+static void fixed_phy_del(int phy_addr)
 {
 	struct fixed_mdio_bus *fmb = &platform_fmb;
 	struct fixed_phy *fp, *tmp;
@@ -300,7 +300,6 @@ void fixed_phy_del(int phy_addr)
 		}
 	}
 }
-EXPORT_SYMBOL_GPL(fixed_phy_del);
 
 static int phy_fixed_addr;
 static DEFINE_SPINLOCK(phy_fixed_addr_lock);
@@ -371,6 +370,14 @@ struct phy_device *fixed_phy_register(unsigned int irq,
 }
 EXPORT_SYMBOL_GPL(fixed_phy_register);
 
+void fixed_phy_unregister(struct phy_device *phy)
+{
+	phy_device_remove(phy);
+
+	fixed_phy_del(phy->mdio.addr);
+}
+EXPORT_SYMBOL_GPL(fixed_phy_unregister);
+
 static int __init fixed_mdio_bus_init(void)
 {
 	struct fixed_mdio_bus *fmb = &platform_fmb;
diff --git a/include/linux/phy_fixed.h b/include/linux/phy_fixed.h
index 2400d2ea4f34..1d41ec44e39d 100644
--- a/include/linux/phy_fixed.h
+++ b/include/linux/phy_fixed.h
@@ -19,7 +19,7 @@ extern struct phy_device *fixed_phy_register(unsigned int irq,
 					     struct fixed_phy_status *status,
 					     int link_gpio,
 					     struct device_node *np);
-extern void fixed_phy_del(int phy_addr);
+extern void fixed_phy_unregister(struct phy_device *phydev);
 extern int fixed_phy_set_link_update(struct phy_device *phydev,
 			int (*link_update)(struct net_device *,
 					   struct fixed_phy_status *));
@@ -40,9 +40,8 @@ static inline struct phy_device *fixed_phy_register(unsigned int irq,
 {
 	return ERR_PTR(-ENODEV);
 }
-static inline int fixed_phy_del(int phy_addr)
+static inline void fixed_phy_unregister(struct phy_device *phydev)
 {
-	return -ENODEV;
 }
 static inline int fixed_phy_set_link_update(struct phy_device *phydev,
 			int (*link_update)(struct net_device *,
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index f100f340d93f..c28c47463b7e 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -447,11 +447,9 @@ static void dsa_switch_destroy(struct dsa_switch *ds)
 		if (of_phy_is_fixed_link(port_dn)) {
 			phydev = of_phy_find_device(port_dn);
 			if (phydev) {
-				int addr = phydev->mdio.addr;
-
 				phy_device_free(phydev);
 				of_node_put(port_dn);
-				fixed_phy_del(addr);
+				fixed_phy_unregister(phydev);
 			}
 		}
 	}
-- 
2.7.0

^ permalink raw reply related

* Re: Generic TSO (was Re: [net-next PATCH 0/2] GENEVE/VXLAN: Enable outer Tx checksum by default)
From: Tom Herbert @ 2016-03-11 22:55 UTC (permalink / raw)
  To: Alexander Duyck; +Cc: Edward Cree, Linux Kernel Network Developers
In-Reply-To: <CAKgT0UeBx8NepNM2oGkYOnPV2Niy9GPd7RDZdgDMY3EcRMxNmw@mail.gmail.com>

On Fri, Mar 11, 2016 at 2:31 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Fri, Mar 11, 2016 at 1:29 PM, Edward Cree <ecree@solarflare.com> wrote:
>> On 11/03/16 21:09, Alexander Duyck wrote:
>>> The only real issue with the "generic" TSO is that it isn't going to
>>> be so generic.  We have different devices that will support different
>>> levels of stuff.  For example the ixgbe drivers will need to treat the
>>> outer tunnel header as one giant L2 header.  As a result we will need
>>> to populate all the fields in the outer header including the outer IP
>>> ID, checksum, udp->len, and UDP or GRE checksum if requested.  For
>>> i40e I think this gets a bit simpler as they already handle the outer
>>> IPv4 ID and checksum.  I think there we may need to only populate the
>>> checksum for it to work out correctly.  As such I may look at coming
>>> up with a number of functions so that we can mix and match based on
>>> what is needed in order to assemble a partially segmented frame.
>> AIUI, the point of the design is that we _can_ populate everything,
>> because we're keeping lengths and outer IP ID fixed, so outer checksums
>> stay the same and the outer tunnel header _is_ just one giant L2 header
>> which is bit-for-bit identical for each generated segment.  So every
>> devicegets to just be dumb and treat it as opaque.
>
> This works so long as the device isn't trying to do anything like
> insert VLAN tags.  Then I think we might have an issue since we don't
> want to confuse the device and have it trying to insert the tag on the
> inner frame's Ethernet header.
>
In Edward's giant L2 header mode, couldn't VLAN tags just be part of that?

> I suspect we may have differing levels of "dumb" that we have to deal
> with.  That is all I am saying.  By default we could just populate all
> of the length and checksum fields in the outer header, we would just
> have to be consistent about what is provided then.  In addition there
> will be the matter of sorting out the IP ID bits.  For example some of
> the i40e parts support tunnel offloads, but not tunnel offloads with
> checksums enabled.  I suspect those parts will end up wanting to
> handle the outer IP header and UDP length values.  As a result there
> trying to do a "dumb" send may result in us really messing up the IP
> ID values if we don't take steps to make it a bit smarter.
>
>>> The other issue I am working on at the moment to enable all this is to
>>> fix the differents between csum_tcpudp_magic and csum_ipv6_magic in
>>> terms of handling packet lengths greater than 65535.  Currently we are
>>> messing up the checksum in relation to IPv6 since we are using the
>>> truncated uh->len value.  I'll be submitting some patches later today
>>> that will hopefully get that fixed and that in turn should make the
>>> rest of the segmentation work easier.
>> Again, in the superpacket we want to calculate the checksum based on the
>> subsegment length, rather than the length of the superpacket.  The idea
>> is to create the header for an MSS-sized segment, then follow it with an
>> inner IP & TCP header, and n*MSS bytes of payload.  (This of course
>> produces a superpacket that's not what you'd send over a link with a 64k
>> MTU, unlike how we do it today.)
>
> The question is at what point do we do the chopping.  Should we be
> doing this in the drivers or somewhere higher in the stack like we do
> for standard GSO segmentation.  I would think we would need to add
> another bit that says we can do GSO with custom outer headers since I
> can see VLANs being a possible issue otherwise.
>
>> Then hw just chops up the payload, fixes up the inner headers, and glues
>> the "L2" header on each packet.
>
> Yea, it sounds really straight forward and easy.  It isn't till you
> start digging into the actual code that it gets a bit hairy.
>
> What this effectively is is another form of TSO where each driver will
> want to do things a little differently.  Alot of it has to do with the
> fact that this is kind of a nasty hack that we are trying to add since
> many devices won't like the fact that we are lying about the size of
> our actual L2 header so things like VLAN tag insertion are going to
> end up blowing back on us.
>
Right, the point is that we're trying to get out of the model where
every driver/device implements TSO differently, supports ad hoc
protocols, etc. Do you see any other common invasive technique that we
need to deal with other than VLAN insertion and IP ID?

> Really my preference in the case of ixgbe would have been to let the
> hardware update the outer IP header and the inner TCP header and then
> do the UDP and inner IP header as the static headers.  That way we
> could still theoretically support fragmentation on the outer headers
> which last I knew is a very real possibility since the DF bit is not
> set on the outer headers for VXLAN I believe.
>
> - Alex

^ permalink raw reply

* Re: [PATCH 1/2] of_mdio: use IS_ERR_OR_NULL()
From: Vladimir Zapolskiy @ 2016-03-11 22:53 UTC (permalink / raw)
  To: Sergei Shtylyov, grant.likely-QSEj5FYQhm4dnm+yROfE0A,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, devicetree-u79uwXL29TY76Z2rM5mHXA,
	f.fainelli-Re5JQEeQqe8AvxtiuMwx3w, netdev-u79uwXL29TY76Z2rM5mHXA,
	frowand.list-Re5JQEeQqe8AvxtiuMwx3w
In-Reply-To: <3305414.rAFdOb57tB-gHKXc3Y1Z8zGSmamagVegGFoWSdPRAKMAL8bYrjMMd8@public.gmane.org>

Hello Sergei,

On 12.03.2016 00:12, Sergei Shtylyov wrote:
> IS_ERR_OR_NULL() is open coded in of_mdiobus_register_{phy|device}(), so
> just call it directly...
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org>
> 
> ---
>  drivers/of/of_mdio.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Index: net-next/drivers/of/of_mdio.c
> ===================================================================
> --- net-next.orig/drivers/of/of_mdio.c
> +++ net-next/drivers/of/of_mdio.c
> @@ -56,7 +56,7 @@ static int of_mdiobus_register_phy(struc
>  		phy = phy_device_create(mdio, addr, phy_id, 0, NULL);
>  	else
>  		phy = get_phy_device(mdio, addr, is_c45);
> -	if (!phy || IS_ERR(phy))
> +	if (IS_ERR_OR_NULL(phy))
>  		return 1;
>  
>  	rc = irq_of_parse_and_map(child, 0);
> @@ -98,7 +98,7 @@ static int of_mdiobus_register_device(st
>  	int rc;
>  
>  	mdiodev = mdio_device_create(mdio, addr);
> -	if (!mdiodev || IS_ERR(mdiodev))
> +	if (IS_ERR_OR_NULL(mdiodev))
>  		return 1;
>  

I don't see how it is possible for mdio_device_create() to return NULL,
instead of this change consider to remove if (!mdiodev) check.

>  	/* Associate the OF node with the device structure so it
> 

--
With best wishes,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 2/2] of_mdio: use PTR_ERR_OR_ZERO()
From: Florian Fainelli @ 2016-03-11 22:49 UTC (permalink / raw)
  To: Sergei Shtylyov, grant.likely, robh+dt, devicetree, netdev,
	frowand.list
In-Reply-To: <11956015.EKV4zhDQJ5@wasted.cogentembedded.com>

On 11/03/16 14:13, Sergei Shtylyov wrote:
> PTR_ERR_OR_ZERO() is open coded in of_phy_register_fixed_link(), so just
> call it directly...
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox