Re: [PATCH bpf-next 0/7] Add XDP_ATTACH bind() flag to AF_XDP sockets
> That is not how XDP works. XDP must run on the "master" physical driver > interface, as the "slave" interface is a virtual DSA interface. I did > mention DSA because I'm handling that on the EspressoBin implementation. Hi Jesper It is going to be interesting to see how you do that. As a user, i want to attach the XDP program to a slave interface. So i assume you somehow redirect that to the master, with some extra meta data to allow XDP on the master to detect packets for a specific slave? Andrew
Re: [net-next, RFC, 4/8] net: core: add recycle capabilities on skbs via page_pool API
> I got other concerns on the patchset though. Like how much memory is > it 'ok' to keep mapped keeping in mind we are using the streaming > DMA API. Are we going to affect anyone else negatively by doing so ? For mvneta, you can expect the target to have between 512Mbyte to 3G. You can take a look at the DT files to get a better feel for this. All of it should be low enough that it is DMA'able. These SoCs are often used for WiFi access points, and NAS boxes. So i guess the big memory usage on these boxes is the block cache for a NAS device. So if you want to see the impact of the driver using more memory, see if disk performance is affected. Andrew
Re: [PATCH bpf-next 0/7] Add XDP_ATTACH bind() flag to AF_XDP sockets
On Sat, Dec 08, 2018 at 04:12:12PM +0100, Jesper Dangaard Brouer wrote: > On Fri, 7 Dec 2018 13:21:08 -0800 > Alexei Starovoitov wrote: > > > for production I suspect the users would want > > an easy way to stay safe when they're playing with AF_XDP. > > So another builtin program that redirects ssh and ping traffic > > back to the kernel would be a nice addition. > > Are you saying a buildin program that need to parse different kinds of > Eth-type headers (DSA, VLAN, QinqQ) Hi Jesper Parsing DSA headers is quite hard, since most of them are not discoverable, you need to know they are there, and where they actually are. I also don't think there are any use cases for actually trying to parse them on the master interface. You are more likely to want to run the eBPF program on the slave interface, once the DSA header has been removed, and it is clear what interface the frame is actually for. Andrew
[PATCH net-next] net: dsa: Make dsa_master_set_mtu() static
Add the missing static keyword.
Signed-off-by: Andrew Lunn
---
net/dsa/master.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/dsa/master.c b/net/dsa/master.c
index a25242e71fb2..d7d5145aa235 100644
--- a/net/dsa/master.c
+++ b/net/dsa/master.c
@@ -158,7 +158,7 @@ static void dsa_master_ethtool_teardown(struct net_device
*dev)
cpu_dp->orig_ethtool_ops = NULL;
}
-void dsa_master_set_mtu(struct net_device *dev, struct dsa_port *cpu_dp)
+static void dsa_master_set_mtu(struct net_device *dev, struct dsa_port *cpu_dp)
{
unsigned int mtu = ETH_DATA_LEN + cpu_dp->tag_ops->overhead;
int err;
--
2.19.1
[PATCH net-next] net: dsa: Restore MTU on master device on unload
A previous change tries to set the MTU on the master device to take
into account the DSA overheads. This patch tries to reset the master
device back to the default MTU.
Signed-off-by: Andrew Lunn
---
net/dsa/master.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/net/dsa/master.c b/net/dsa/master.c
index 42f525bc68e2..a25242e71fb2 100644
--- a/net/dsa/master.c
+++ b/net/dsa/master.c
@@ -172,6 +172,18 @@ void dsa_master_set_mtu(struct net_device *dev, struct
dsa_port *cpu_dp)
rtnl_unlock();
}
+static void dsa_master_reset_mtu(struct net_device *dev)
+{
+ int err;
+
+ rtnl_lock();
+ err = dev_set_mtu(dev, ETH_DATA_LEN);
+ if (err)
+ netdev_dbg(dev,
+ "Unable to reset MTU to exclude DSA overheads\n");
+ rtnl_unlock();
+}
+
int dsa_master_setup(struct net_device *dev, struct dsa_port *cpu_dp)
{
dsa_master_set_mtu(dev, cpu_dp);
@@ -190,6 +202,7 @@ int dsa_master_setup(struct net_device *dev, struct
dsa_port *cpu_dp)
void dsa_master_teardown(struct net_device *dev)
{
dsa_master_ethtool_teardown(dev);
+ dsa_master_reset_mtu(dev);
dev->dsa_ptr = NULL;
--
2.19.1
[PATCH v2 net-next 0/2] platform data controls for mdio-gpio
Soon to be mainlined is an x86 platform with a Marvell switch, and a bit-banging MDIO bus. In order to make this work, the phy_mask of the MDIO bus needs to be set to prevent scanning for PHYs, and the phy_ignore_ta_mask needs to be set because the switch has broken turnaround. Add a platform_data structure with these parameters. Andrew Lunn (2): net: phy: mdio-gpio: Add platform_data support for phy_mask net: phy: mdio-gpio: Add phy_ignore_ta_mask to platform data MAINTAINERS | 1 + drivers/net/phy/mdio-gpio.c | 7 +++ include/linux/platform_data/mdio-gpio.h | 14 ++ 3 files changed, 22 insertions(+) create mode 100644 include/linux/platform_data/mdio-gpio.h -- 2.19.1
[PATCH v2 net-next 2/2] net: phy: mdio-gpio: Add phy_ignore_ta_mask to platform data
The Marvell 6390 Ethernet switch family does not perform MDIO
turnaround correctly. Many hardware MDIO bus masters don't care about
this, but the bitbangging implementation in Linux does by default. Add
phy_ignore_ta_mask to the platform data so that the bitbangging code
can be told which devices are known to get TA wrong.
v2
--
int -> u32 in platform data structure
Signed-off-by: Andrew Lunn
---
drivers/net/phy/mdio-gpio.c | 4 +++-
include/linux/platform_data/mdio-gpio.h | 1 +
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/mdio-gpio.c b/drivers/net/phy/mdio-gpio.c
index 1e296dd4067a..ea9a0e339778 100644
--- a/drivers/net/phy/mdio-gpio.c
+++ b/drivers/net/phy/mdio-gpio.c
@@ -130,8 +130,10 @@ static struct mii_bus *mdio_gpio_bus_init(struct device
*dev,
else
strncpy(new_bus->id, "gpio", MII_BUS_ID_SIZE);
- if (pdata)
+ if (pdata) {
new_bus->phy_mask = pdata->phy_mask;
+ new_bus->phy_ignore_ta_mask = pdata->phy_ignore_ta_mask;
+ }
dev_set_drvdata(dev, new_bus);
diff --git a/include/linux/platform_data/mdio-gpio.h
b/include/linux/platform_data/mdio-gpio.h
index a5d5ff5e174c..13874fa6e767 100644
--- a/include/linux/platform_data/mdio-gpio.h
+++ b/include/linux/platform_data/mdio-gpio.h
@@ -8,6 +8,7 @@
struct mdio_gpio_platform_data {
u32 phy_mask;
+ u32 phy_ignore_ta_mask;
};
#endif /* __LINUX_MDIO_GPIO_PDATA_H */
--
2.19.1
[PATCH v2 net-next 1/2] net: phy: mdio-gpio: Add platform_data support for phy_mask
It is sometimes necessary to instantiate a bit-banging MDIO bus as a
platform device, without the aid of device tree.
When device tree is being used, the bus is not scanned for devices,
only those devices which are in device tree are probed. Without device
tree, by default, all addresses on the bus are scanned. This may then
find a device which is not a PHY, e.g. a switch. And the switch may
have registers containing values which look like a PHY. So during the
scan, a PHY device is wrongly created.
After the bus has been registered, a search is made for
mdio_board_info structures which indicates devices on the bus, and the
driver which should be used for them. This is typically used to
instantiate Ethernet switches from platform drivers. However, if the
scanning of the bus has created a PHY device at the same location as
indicated into the board info for a switch, the switch device is not
created, since the address is already busy.
This can be avoided by setting the phy_mask of the mdio bus. This mask
prevents addresses on the bus being scanned.
v2
--
int -> u32 in platform data structure
Signed-off-by: Andrew Lunn
---
MAINTAINERS | 1 +
drivers/net/phy/mdio-gpio.c | 5 +
include/linux/platform_data/mdio-gpio.h | 13 +
3 files changed, 19 insertions(+)
create mode 100644 include/linux/platform_data/mdio-gpio.h
diff --git a/MAINTAINERS b/MAINTAINERS
index fb88b6863d10..9d3b899f9ba2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5610,6 +5610,7 @@ F:include/linux/of_net.h
F: include/linux/phy.h
F: include/linux/phy_fixed.h
F: include/linux/platform_data/mdio-bcm-unimac.h
+F: include/linux/platform_data/mdio-gpio.h
F: include/trace/events/mdio.h
F: include/uapi/linux/mdio.h
F: include/uapi/linux/mii.h
diff --git a/drivers/net/phy/mdio-gpio.c b/drivers/net/phy/mdio-gpio.c
index 0fbcedcdf6e2..1e296dd4067a 100644
--- a/drivers/net/phy/mdio-gpio.c
+++ b/drivers/net/phy/mdio-gpio.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -112,6 +113,7 @@ static struct mii_bus *mdio_gpio_bus_init(struct device
*dev,
struct mdio_gpio_info *bitbang,
int bus_id)
{
+ struct mdio_gpio_platform_data *pdata = dev_get_platdata(dev);
struct mii_bus *new_bus;
bitbang->ctrl.ops = _gpio_ops;
@@ -128,6 +130,9 @@ static struct mii_bus *mdio_gpio_bus_init(struct device
*dev,
else
strncpy(new_bus->id, "gpio", MII_BUS_ID_SIZE);
+ if (pdata)
+ new_bus->phy_mask = pdata->phy_mask;
+
dev_set_drvdata(dev, new_bus);
return new_bus;
diff --git a/include/linux/platform_data/mdio-gpio.h
b/include/linux/platform_data/mdio-gpio.h
new file mode 100644
index ..a5d5ff5e174c
--- /dev/null
+++ b/include/linux/platform_data/mdio-gpio.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * MDIO-GPIO bus platform data structure
+ */
+
+#ifndef __LINUX_MDIO_GPIO_PDATA_H
+#define __LINUX_MDIO_GPIO_PDATA_H
+
+struct mdio_gpio_platform_data {
+ u32 phy_mask;
+};
+
+#endif /* __LINUX_MDIO_GPIO_PDATA_H */
--
2.19.1
Re: [PATCH 5/5] net: dsa: ksz: Add Microchip KSZ8795 DSA driver
> +static int ksz8795_valid_dyn_entry(struct ksz_device *dev, u8 *data)
> +{
> + int timeout = 100;
> +
> + do {
> + ksz_read8(dev, REG_IND_DATA_CHECK, data);
> + timeout--;
> + } while ((*data & DYNAMIC_MAC_TABLE_NOT_READY) && timeout);
readx_poll_timeout()?
> +static inline void ksz8795_from_vlan(u16 vlan, u8 *fid, u8 *member, u8
> *valid)
Please don't use inline in C code, just in headers. Leave the compile
to decide if it should be inlined.
> +static void ksz8795_r_vlan_table(struct ksz_device *dev, u16 vid, u16 *vlan)
> +{
> + u64 buf;
> + u16 *data = (u16 *)
> + u16 addr;
> + int index;
The networking code uses reverse christmas tree. So you need to change
the order of these declarations, and do the assignment in the body of
the function. Please review all the functions.
> +static const u8 stp_multicast_addr[] = {
> + 0x01, 0x80, 0xC2, 0x00, 0x00, 0x00
> +};
include/linux/etherdevice.h defines eth_stp_addr.
Andrew
Re: [PATCH 3/5] net: dsa: ksz: Factor out common tag code
On Fri, Dec 07, 2018 at 07:18:43PM +0100, Marek Vasut wrote:
> From: Tristram Ha
>
> Factor out common code from the tag_ksz , so that the code can be used
> with other KSZ family switches which use differenly sized tags.
I prefer this implementation over what Tristram recently submitted. It
is also what we suggested a while back. However, since then we have
had Spectra/meltdown, and we now know a function call through a
pointer is expensive. This is the hot path, every frame comes through
here, so it is worth taking the time to optimize this. Could you try
to remove the ksz_tag_ops structure. xmit looks simple, since it is a
tail call, so you can do that in ksz9477_xmit. Receive looks a bit
more complex.
I also think for the moment we need it ignore PTP until we have the
big picture sorted out. If need be, the code can be refactored yet
again. But i don't want PTP holding up getting more switches
supported.
> Signed-off-by: Tristram Ha
> Signed-off-by: Marek Vasut
> Cc: Vivien Didelot
> Cc: Woojung Huh
> Cc: David S. Miller
> ---
> net/dsa/tag_ksz.c | 125 +-
> 1 file changed, 90 insertions(+), 35 deletions(-)
>
> diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
> index 036bc62198f28..d94bad1ab7e53 100644
> --- a/net/dsa/tag_ksz.c
> +++ b/net/dsa/tag_ksz.c
> @@ -14,34 +14,30 @@
> #include
> #include "dsa_priv.h"
>
> -/* For Ingress (Host -> KSZ), 2 bytes are added before FCS.
> - *
> ---
> - *
> DA(6bytes)|SA(6bytes)||Data(nbytes)|tag0(1byte)|tag1(1byte)|FCS(4bytes)
> - *
> ---
> - * tag0 : Prioritization (not used now)
> - * tag1 : each bit represents port (eg, 0x01=port1, 0x02=port2, 0x10=port5)
> - *
> - * For Egress (KSZ -> Host), 1 byte is added before FCS.
> - *
> ---
> - * DA(6bytes)|SA(6bytes)||Data(nbytes)|tag0(1byte)|FCS(4bytes)
> - *
> ---
> - * tag0 : zero-based value represents port
> - * (eg, 0x00=port1, 0x02=port3, 0x06=port7)
> - */
> +struct ksz_tag_ops {
> + void(*get_tag)(u8 *tag, unsigned int *port, unsigned int *len);
> + void(*set_tag)(struct sk_buff *skb, struct net_device *dev);
> +};
>
> -#define KSZ_INGRESS_TAG_LEN 2
> -#define KSZ_EGRESS_TAG_LEN 1
> +/* Typically only one byte is used for tail tag. */
> +#define KSZ_EGRESS_TAG_LEN 1
>
> -static struct sk_buff *ksz_xmit(struct sk_buff *skb, struct net_device *dev)
> +/* Frames with following addresse may need to be sent even when the port is
> + * closed.
> + */
> +static const u8 special_mult_addr[] = {
> + 0x01, 0x80, 0xC2, 0x00, 0x00, 0x00
> +};
> +
This is new functionality, not part of the refactoring the
code. Please add that as a new patch. Also, you can probably make use
of is_link_local_ether_addr().
Andrew
Re: [PATCH 1/5] net: dsa: ksz: Add MIB counter reading support
> +static void ksz9477_phy_setup(struct ksz_device *dev, int port,
> + struct phy_device *phy)
> +{
> + if (port < dev->phy_port_cnt) {
> + /* SUPPORTED_Asym_Pause and SUPPORTED_Pause can be removed to
> + * disable flow control when rate limiting is used.
> + */
> + phy->advertising = phy->supported;
> + }
> +}
> +
This has nothing to do with MIB counters.
Other than that, is this the same as Tristram's recent submission?
Maybe once the comments have been addressed, we can merge that
version?
Thanks
Andrew
Re: [PATCH V2] net: dsa: ksz: Add reset GPIO handling
> This actually is an individual patch, it doesn't depend on anything. > Or do you mean a series with the DT documentation change ? Yes, i mean together with the DT documentation change. Those two belong together, they are one functional change. Part of this is also to do with scalability. It takes less effort to merge one patchset of two patches, as two individual patches. The truth is, developer time is cheap, maintainer time is expensive, so the process is optimized towards making the maintainers life easy. So sometimes you do combine orthogonal changes together into one patchset, if there is a high purpose, eg. adding support for a new device on a new board. However, given the situation of two overlapping patchsets, it might be better to submit smaller patchsets. Andrew
Re: [PATCH] net: dsa: ksz: Fix port membership
On Sat, Dec 08, 2018 at 05:23:25AM +0100, Marek Vasut wrote: > On 12/08/2018 01:13 AM, tristram...@microchip.com wrote: > >> Do you have a git tree with all the KSZ patches based on -next > >> somewhere, so I don't have to look for them in random MLs ? > > > > I just sent it this Monday and the subject for that patch is > > "[PATCH RFC 6/6] net: dsa: microchip: Add switch offload forwarding > > support." > > Is all that collected in some git tree somewhere, so I don't have to > look for various patches in varying states of decay throughout the ML? Hi Marek, Tristram It is unfortunate that we have two patchsets being submitted at the same time, with overlapping functionality. Some degree of cooperation is needed to handle this. Could i ask you both to publish a git tree of your patches. And then figure out how to submit them. Maybe as smaller sets, with the easy, non-overlapping bits first? Andrew
Re: [PATCH V2] net: dsa: ksz: Add reset GPIO handling
On Fri, Dec 07, 2018 at 10:51:36PM +0100, Marek Vasut wrote: > Add code to handle optional reset GPIO in the KSZ switch driver. The switch > has a reset GPIO line which can be controlled by the CPU, so make sure it is > configured correctly in such setups. Hi Marek Please make this a patch series, not two individual patches. And as David has already said, include a cover letter. Otherwise, this looks O.K. Thanks Andrew
Re: [PATCH] net: dsa: ksz: Fix port membership
> I think if you do this without setting offload_fwd_mark you will > receive duplicate frame. I don't think it will, at least not in the normal case. The hardware should know the egress port, so there is no need to forward a copy to the CPU. The only time it should forward to the CPU is when the egress port is not known, so it floods. Without offload_fwd_mark set, the SW bridge will flood it back out the ports causing duplication. But that is not too bad. The Marvell driver did this for a while and nothing bad was reported. Andrew
Re: [PATCH] net: dsa: ksz: Add reset GPIO handling
> + dev->reset_gpio = -1;
> + reset_gpio = of_get_named_gpio_flags(np, "reset-gpios", 0,
> + _gpio_flags);
> + if (reset_gpio >= 0) {
> + flags = (reset_gpio_flags == OF_GPIO_ACTIVE_LOW) ?
> + GPIOF_ACTIVE_LOW : 0;
Can you use devm_gpiod_get_optional()? It makes this a lot simpler.
Take a look at mv88e6xxx/chip.c which also uses a GPIO for reset.
You also need to update the binding documentation for this new
property.
Andrew
Re: [PATCH] gianfar: Add gfar_change_carrier()
> Would you be happier if .ndo_change_carrier() only acted on Fixed PHYs? I think it makes sense to allow a fixed phy carrier to be changed from user space. However, i don't think you can easily plumb that to .ndo_change_carrier(), since that is a MAC feature. You need to change the fixed_phy_status to indicate the PHY has lost link, and then let the usual mechanisms tell the MAC it is down and change the carrier status. Andrew
Re: [PATCH] gianfar: Add gfar_change_carrier()
> Been a bit busy today but now I have played with dormant using ip link and > got some odd results: > # > ifconfig eth0 > eth0: flags=4163 mtu 1500 > inet 172.20.0.246 netmask 255.255.0.0 broadcast 172.20.255.255 > inet6 fe80::ad9c:b230:1da8:1821 prefixlen 64 scopeid 0x20 > ether 8c:16:45:89:cf:c6 txqueuelen 1000 (Ethernet) > RX packets 1848903 bytes 736764445 (702.6 MiB) > RX errors 0 dropped 0 overruns 0 frame 0 > TX packets 627462 bytes 222453345 (212.1 MiB) > TX errors 0 dropped 0 overruns 0 carrier 0 collisions 0 > device interrupt 16 memory 0xdc20-dc22 > # > ip link set mode dormant dev eth0 > ping sunet.se > PING sunet.se (192.36.171.231) 56(84) bytes of data. > 64 bytes from webc.sunet.se (192.36.171.231): icmp_seq=1 ttl=54 time=2.22 ms > 64 bytes from webc.sunet.se (192.36.171.231): icmp_seq=2 ttl=54 time=2.17 ms > > # > ifconfig eth0 > eth0: flags=4163 mtu 1500 > inet 172.20.0.246 netmask 255.255.0.0 broadcast 172.20.255.255 > inet6 fe80::ad9c:b230:1da8:1821 prefixlen 64 scopeid 0x20 > ether 8c:16:45:89:cf:c6 txqueuelen 1000 (Ethernet) > RX packets 1905479 bytes 753549572 (718.6 MiB) > RX errors 0 dropped 0 overruns 0 frame 0 > TX packets 648266 bytes 224421617 (214.0 MiB) > TX errors 0 dropped 0 overruns 0 carrier 0 collisions 0 > device interrupt 16 memory 0xdc20-dc22 > still RUNNING .. > > #> ip link show eth0 > 5: eth0: mtu 1500 qdisc fq_codel state UP > mode DORMANT group default qlen > > state is still UP ? > > # > ip link set state dormant dev eth0 > # > ip link show eth0 > 5: eth0: mtu 1500 qdisc fq_codel > state DORMANT mode DORMANT group default qlen 1000 > # > ifconfig eth0 > eth0: flags=4099 mtu 1500 > ... > > Now both state and not RUNNING :) > but ... > # ping sunet.se > PING sunet.se (192.36.171.231) 56(84) bytes of data. > 64 bytes from webc.sunet.se (192.36.171.231): icmp_seq=1 ttl=54 time=2.43 ms > 64 bytes from webc.sunet.se (192.36.171.231): icmp_seq=2 ttl=54 time=2.31 ms > > I can still ping though. Is this how it is supposed to work ? No sure how > state and mode relate to each other either. I don't actually know. I know some things are supposed to work while dormant. You should be able to perform 802.1x negotiation, etc. You might find the people on the wireless list know more. I think it is used by wpa_supplicant and hostapd during device authentication. Andrew
Re: [PATCH] dpaa_eth: Add dpaa_change_carrier()
On Fri, Dec 07, 2018 at 08:36:48AM +, Madalin-cristian Bucur wrote: > > -Original Message- > > From: Joakim Tjernlund > > Sent: Thursday, December 6, 2018 5:32 PM > > To: netdev @ vger . kernel . org ; Madalin- > > cristian Bucur > > Cc: jo...@infinera.com > > Subject: [PATCH] dpaa_eth: Add dpaa_change_carrier() > > > > This allows to control carrier from /sys/class/net/ethX/carrier > > Hi, > > can you please explain why it's needed? Hi Madelin See the patch: [PATCH] gianfar: Add gfar_change_carrier() Which is basically the same. A better approach is being discussed. So for the moment, to stop DaveM from merging this not knowing it is related to that patch: NACK Andrew
Re: [PATCH] ucc_geth: Add ucc_geth_change_carrier()
On Thu, Dec 06, 2018 at 04:33:25PM +0100, Joakim Tjernlund wrote: > This allows to control carrier from /sys/class/net/ethX/carrier > > Signed-off-by: Joakim Tjernlund See the discussion for [PATCH] gianfar: Add gfar_change_carrier(). For the moment: NACK Andrew
Re: [PATCH net-next 2/2] net: dsa: Set the master device's MTU to account for DSA overheads
On Thu, Dec 06, 2018 at 12:21:31PM -0800, Florian Fainelli wrote:
> On 12/6/18 2:36 AM, Andrew Lunn wrote:
> > DSA tagging of frames sent over the master interface to the switch
> > increases the size of the frame. Such frames can then be bigger than
> > the normal MTU of the master interface, and it may drop them. Use the
> > overhead information from the tagger to set the MTU of the master
> > device to include this overhead.
> >
> > Signed-off-by: Andrew Lunn
> > ---
> > net/dsa/master.c | 16
> > 1 file changed, 16 insertions(+)
> >
> > diff --git a/net/dsa/master.c b/net/dsa/master.c
> > index c90ee3227dea..42f525bc68e2 100644
> > --- a/net/dsa/master.c
> > +++ b/net/dsa/master.c
> > @@ -158,8 +158,24 @@ static void dsa_master_ethtool_teardown(struct
> > net_device *dev)
> > cpu_dp->orig_ethtool_ops = NULL;
> > }
> >
> > +void dsa_master_set_mtu(struct net_device *dev, struct dsa_port *cpu_dp)
> > +{
> > + unsigned int mtu = ETH_DATA_LEN + cpu_dp->tag_ops->overhead;
> > + int err;
> > +
> > + rtnl_lock();
> > + if (mtu <= dev->max_mtu) {
> > + err = dev_set_mtu(dev, mtu);
> > + if (err)
> > + netdev_dbg(dev, "Unable to set MTU to include for DSA
> > overheads\n");
> > + }
>
> Would it make sense to warn the user that there might be
> transmit/receive issues with the DSA tagging protocol if either
> dev_set_mtu() fails or mtu > dev->max_mtu?
I thought about that. But we have setups which work today with the
standard MTU. The master might not implement the set_mtu op, or might
impose the standard MTU, but be quite happy to deal with our DSA
packets. So i wanted to make this a hint to the master device, not a
strong requirement.
> Not that I think it matters too much to people because unbinding the
> switch driver and expecting the CPU port to continue operating is
> wishful thinking, but we should probably unwind that operation in
> dsa_master_teardown(), right?
That would make sense.
David has already accepted the patchset, so i will add a followup
patch.
Andrew
Re: [PATCH RFC 2/6] net: dsa: microchip: Add MIB counter reading support
> I wonder what is the official way to clear the counters. I don't think there is one, other than unloading the driver and loading it again. Andrew
Re: [PATCH RFC 5/6] net: dsa: microchip: Update tag_ksz.c to access switch driver
> I did try to implement this way. But the other switches do not have the same > format even though the length is the same. Then I need to change the > following > files for any new KSZ switch: include/linux/dsa.h, net/dsa/dsa.c, > net/dsa/dsa_priv.h, > and finally net/dsa/tag_ksz.c. You can always add two different tag drivers. They don't have to share code if it does not make sense. > Even then it will not work if Microchip wants to add 1588 PTP > capability to the switches. > > For KSZ9477 the length of the tail tag changes when the PTP function > is enabled. Typically this function is either enabled or disabled > all the time, but if users want to change that during normal > operation to see how the switch behaves, the transmit function > completely stops working correctly. We should figure out how to support PTP. I think that is the main issue here. > Older driver implementation is to monitor that register change and adjust the > length > dynamically. > > Another problem is the tail tag needs to include the timestamp for the 1-step > Pdelay_Resp to have accurate turnaround time when that message is sent out by > the > switch. This will require access to the main switch driver which will keep > track of those > PTP messages. > > PTP handles transmit timestamp in skb_tx_timestamp, which is typically called > after the > frame is sent, so it is too late. DSA calls dsa_skb_tx_timestamp before > sending, but it > only provides a clone to the driver that supports port_txstamp and so the > switch driver > may not be able to do anything. The current design assumes the hardware will insert the PTP timestamp into the frame using the clock inside the hardware. You then ask it what timestamp it actually used. If i understand you correctly, in your case, software was to provide the timestamp which then gets inserted into the frame. So you want to provide this timestamp as late as possible, when the frame reaches the head of the queue and is about to be sent out the master interface? > In dsa_switch_rcv() the CPU receive function is called first before > dsa_skb_defer_rx_timestamp(). That means the receive tail tag > operation has to be done first to retrieve the receive timestamp so > that it can be passed later. What i think you can do is in your tag rx function you can directly add the timestamp info to the skbuf. The dsa driver function .port_txtstamp can then always return false. Your tag function is going to need access to some driver state, but you should be able to get at that, following pointers, and placing some of the structures in global headers. Andrew
Re: [PATCH net-next 0/2] platform data controls for mdio-gpio
On Thu, Dec 06, 2018 at 09:22:27AM -0800, Florian Fainelli wrote: > Hi Andrew, > > On 12/6/18 5:58 AM, Andrew Lunn wrote: > > Soon to be mainlined is an x86 platform with a Marvell switch, and a > > bit-banging MDIO bus. In order to make this work, the phy_mask of the > > MDIO bus needs to be set to prevent scanning for PHYs, and the > > phy_ignore_ta_mask needs to be set because the switch has broken > > turnaround. > > This looks good, I would just make one/two changes which is to match the > internal phy_mask and phy_ignore_ta_mask types from the struct mii_bus > and use u32 instead of int. Yes, that makes sense. v2 to follow. Andrew
Re: [PATCH] gianfar: Add gfar_change_carrier()
> I can have a look at using dormant, but what is change_carrier > supposed to do if not this? It is intended for interfaces which are stacked, like the team driver, and for devices which don't have a phy, e.g. tun, and dummy. > I didn't find a tool for DORMANT, I guess i will have to write one > myself(using SIOCGIFFLAGS, SIOCSIFFLAGS)? ip link should be able to set it. Try ip link set mode dormant dev eth0 Andrew
Re: [PATCH] gianfar: Add gfar_change_carrier()
> I wish I had a proper DSA/Switchdev driver in place but I don't :( > Adding one is not impossible but then a lot of our user space app needs > fixing so all > in all it it a fairly big project. > Anyhow, these carrier additions should be fine I think? I'm not too sure about that. You are potentially messing up the state machine, and the MAC driver could be looking at phydev->link, which says up, but the carrier is down. https://www.kernel.org/doc/Documentation/networking/operstates.txt Could you set the interface to dormant? That seems like a better fit anyway: IF_OPER_DORMANT (5): Interface is L1 up, but waiting for an external event, f.e. for a protocol to establish. (802.1X) The interface does have L1 to the switch, but you are waiting for the external interface to go up. You can set this from user space without needing any kernel changes. Andrew
Re: [PATCH] gianfar: Add gfar_change_carrier()
> > Hi Joakim > > > > Please could you explain the use case for this. > > I have an eth I/F connected to an internal (on board) switch which has an > external port to a mgmt network. > Whenever the external link is broken I want inform linux IP stack that the > link is down on the internal eth > I/F as well. The two interfaces are isolated from the rest of the switch > ports using VLANs. Hi Jockim What type of switch is it? What i would expect is you use a switch driver, and that driver would allow access to the switches PHYs. When the external port goes down, the Linux interface for that port would go down. There is no need to change the carrier on the master interface. Andrew
Re: [PATCH] gianfar: Add gfar_change_carrier()
On Thu, Dec 06, 2018 at 04:31:25PM +0100, Joakim Tjernlund wrote: > This allows to control carrier from /sys/class/net/ethX/carrier Hi Joakim Please could you explain the use case for this. Andrew
[PATCH net-next 0/2] platform data controls for mdio-gpio
Soon to be mainlined is an x86 platform with a Marvell switch, and a bit-banging MDIO bus. In order to make this work, the phy_mask of the MDIO bus needs to be set to prevent scanning for PHYs, and the phy_ignore_ta_mask needs to be set because the switch has broken turnaround. Add a platform_data structure with these parameters. Andrew Lunn (2): net: phy: mdio-gpio: Add platform_data support for phy_mask net: phy: mdio-gpio: Add phy_ignore_ta_mask to platform data MAINTAINERS | 1 + drivers/net/phy/mdio-gpio.c | 7 +++ include/linux/platform_data/mdio-gpio.h | 14 ++ 3 files changed, 22 insertions(+) create mode 100644 include/linux/platform_data/mdio-gpio.h -- 2.19.1
[PATCH net-next 1/2] net: phy: mdio-gpio: Add platform_data support for phy_mask
It is sometimes necessary to instantiate a bit-banging MDIO bus as a
platform device, without the aid of device tree.
When device tree is being used, the bus is not scanned for devices,
only those devices which are in device tree are probed. Without device
tree, by default, all addresses on the bus are scanned. This may then
find a device which is not a PHY, e.g. a switch. And the switch may
have registers containing values which look like a PHY. So during the
scan, a PHY device is wrongly created.
After the bus has been registered, a search is made for
mdio_board_info structures which indicates devices on the bus, and the
driver which should be used for them. This is typically used to
instantiate Ethernet switches from platform drivers. However, if the
scanning of the bus has created a PHY device at the same location as
indicated into the board info for a switch, the switch device is not
created, since the address is already busy.
This can be avoided by setting the phy_mask of the mdio bus. This mask
prevents addresses on the bus being scanned.
Signed-off-by: Andrew Lunn
---
MAINTAINERS | 1 +
drivers/net/phy/mdio-gpio.c | 5 +
include/linux/platform_data/mdio-gpio.h | 13 +
3 files changed, 19 insertions(+)
create mode 100644 include/linux/platform_data/mdio-gpio.h
diff --git a/MAINTAINERS b/MAINTAINERS
index fb88b6863d10..9d3b899f9ba2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5610,6 +5610,7 @@ F:include/linux/of_net.h
F: include/linux/phy.h
F: include/linux/phy_fixed.h
F: include/linux/platform_data/mdio-bcm-unimac.h
+F: include/linux/platform_data/mdio-gpio.h
F: include/trace/events/mdio.h
F: include/uapi/linux/mdio.h
F: include/uapi/linux/mii.h
diff --git a/drivers/net/phy/mdio-gpio.c b/drivers/net/phy/mdio-gpio.c
index 0fbcedcdf6e2..1e296dd4067a 100644
--- a/drivers/net/phy/mdio-gpio.c
+++ b/drivers/net/phy/mdio-gpio.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -112,6 +113,7 @@ static struct mii_bus *mdio_gpio_bus_init(struct device
*dev,
struct mdio_gpio_info *bitbang,
int bus_id)
{
+ struct mdio_gpio_platform_data *pdata = dev_get_platdata(dev);
struct mii_bus *new_bus;
bitbang->ctrl.ops = _gpio_ops;
@@ -128,6 +130,9 @@ static struct mii_bus *mdio_gpio_bus_init(struct device
*dev,
else
strncpy(new_bus->id, "gpio", MII_BUS_ID_SIZE);
+ if (pdata)
+ new_bus->phy_mask = pdata->phy_mask;
+
dev_set_drvdata(dev, new_bus);
return new_bus;
diff --git a/include/linux/platform_data/mdio-gpio.h
b/include/linux/platform_data/mdio-gpio.h
new file mode 100644
index ..0417913aac3d
--- /dev/null
+++ b/include/linux/platform_data/mdio-gpio.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * MDIO-GPIO bus platform data structure
+ */
+
+#ifndef __LINUX_MDIO_GPIO_PDATA_H
+#define __LINUX_MDIO_GPIO_PDATA_H
+
+struct mdio_gpio_platform_data {
+ int phy_mask;
+};
+
+#endif /* __LINUX_MDIO_GPIO_PDATA_H */
--
2.19.1
[PATCH net-next 2/2] net: phy: mdio-gpio: Add phy_ignore_ta_mask to platform data
The Marvell 6390 Ethernet switch family does not perform MDIO
turnaround correctly. Many hardware MDIO bus masters don't care about
this, but the bitbangging implementation in Linux does by default. Add
phy_ignore_ta_mask to the platform data so that the bitbangging code
can be told which devices are known to get TA wrong.
Signed-off-by: Andrew Lunn
---
drivers/net/phy/mdio-gpio.c | 4 +++-
include/linux/platform_data/mdio-gpio.h | 1 +
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/mdio-gpio.c b/drivers/net/phy/mdio-gpio.c
index 1e296dd4067a..ea9a0e339778 100644
--- a/drivers/net/phy/mdio-gpio.c
+++ b/drivers/net/phy/mdio-gpio.c
@@ -130,8 +130,10 @@ static struct mii_bus *mdio_gpio_bus_init(struct device
*dev,
else
strncpy(new_bus->id, "gpio", MII_BUS_ID_SIZE);
- if (pdata)
+ if (pdata) {
new_bus->phy_mask = pdata->phy_mask;
+ new_bus->phy_ignore_ta_mask = pdata->phy_ignore_ta_mask;
+ }
dev_set_drvdata(dev, new_bus);
diff --git a/include/linux/platform_data/mdio-gpio.h
b/include/linux/platform_data/mdio-gpio.h
index 0417913aac3d..2d5a0dc29a0d 100644
--- a/include/linux/platform_data/mdio-gpio.h
+++ b/include/linux/platform_data/mdio-gpio.h
@@ -8,6 +8,7 @@
struct mdio_gpio_platform_data {
int phy_mask;
+ int phy_ignore_ta_mask;
};
#endif /* __LINUX_MDIO_GPIO_PDATA_H */
--
2.19.1
[PATCH net-next 1/2] net: dsa: Add overhead to tag protocol ops.
Each DSA tag protocol needs to add additional headers to the Ethernet
frame in order to direct it towards a specific switch egress port. It
must also remove the head from a frame received from a
switch. Indicate the maximum size of these headers in the tag protocol
ops structure, so the core can take these overheads into account.
Signed-off-by: Andrew Lunn
---
include/net/dsa.h | 1 +
net/dsa/tag_brcm.c| 2 ++
net/dsa/tag_dsa.c | 1 +
net/dsa/tag_edsa.c| 1 +
net/dsa/tag_gswip.c | 1 +
net/dsa/tag_ksz.c | 1 +
net/dsa/tag_lan9303.c | 1 +
net/dsa/tag_mtk.c | 1 +
net/dsa/tag_qca.c | 1 +
net/dsa/tag_trailer.c | 1 +
10 files changed, 11 insertions(+)
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 23690c44e167..6ee2e24e0a6e 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -113,6 +113,7 @@ struct dsa_device_ops {
struct packet_type *pt);
int (*flow_dissect)(const struct sk_buff *skb, __be16 *proto,
int *offset);
+ unsigned int overhead;
};
struct dsa_switch_tree {
diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index 2b06bb91318b..4aa1d368a5ae 100644
--- a/net/dsa/tag_brcm.c
+++ b/net/dsa/tag_brcm.c
@@ -174,6 +174,7 @@ static struct sk_buff *brcm_tag_rcv(struct sk_buff *skb,
struct net_device *dev,
const struct dsa_device_ops brcm_netdev_ops = {
.xmit = brcm_tag_xmit,
.rcv= brcm_tag_rcv,
+ .overhead = BRCM_TAG_LEN,
};
#endif
@@ -196,5 +197,6 @@ static struct sk_buff *brcm_tag_rcv_prepend(struct sk_buff
*skb,
const struct dsa_device_ops brcm_prepend_netdev_ops = {
.xmit = brcm_tag_xmit_prepend,
.rcv= brcm_tag_rcv_prepend,
+ .overhead = BRCM_TAG_LEN,
};
#endif
diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
index cd13cfc542ce..8b2f92e3f3a2 100644
--- a/net/dsa/tag_dsa.c
+++ b/net/dsa/tag_dsa.c
@@ -149,4 +149,5 @@ static struct sk_buff *dsa_rcv(struct sk_buff *skb, struct
net_device *dev,
const struct dsa_device_ops dsa_netdev_ops = {
.xmit = dsa_xmit,
.rcv= dsa_rcv,
+ .overhead = DSA_HLEN,
};
diff --git a/net/dsa/tag_edsa.c b/net/dsa/tag_edsa.c
index 4083326b806e..f5b87ee5c94e 100644
--- a/net/dsa/tag_edsa.c
+++ b/net/dsa/tag_edsa.c
@@ -168,4 +168,5 @@ static struct sk_buff *edsa_rcv(struct sk_buff *skb, struct
net_device *dev,
const struct dsa_device_ops edsa_netdev_ops = {
.xmit = edsa_xmit,
.rcv= edsa_rcv,
+ .overhead = EDSA_HLEN,
};
diff --git a/net/dsa/tag_gswip.c b/net/dsa/tag_gswip.c
index 49e9b73f1be3..cb6f82ffe5eb 100644
--- a/net/dsa/tag_gswip.c
+++ b/net/dsa/tag_gswip.c
@@ -106,4 +106,5 @@ static struct sk_buff *gswip_tag_rcv(struct sk_buff *skb,
const struct dsa_device_ops gswip_netdev_ops = {
.xmit = gswip_tag_xmit,
.rcv = gswip_tag_rcv,
+ .overhead = GSWIP_RX_HEADER_LEN,
};
diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
index 0f62effad88f..96411f70ab9f 100644
--- a/net/dsa/tag_ksz.c
+++ b/net/dsa/tag_ksz.c
@@ -99,4 +99,5 @@ static struct sk_buff *ksz_rcv(struct sk_buff *skb, struct
net_device *dev,
const struct dsa_device_ops ksz_netdev_ops = {
.xmit = ksz_xmit,
.rcv= ksz_rcv,
+ .overhead = KSZ_INGRESS_TAG_LEN,
};
diff --git a/net/dsa/tag_lan9303.c b/net/dsa/tag_lan9303.c
index 548c00254c07..f48889e46ff7 100644
--- a/net/dsa/tag_lan9303.c
+++ b/net/dsa/tag_lan9303.c
@@ -140,4 +140,5 @@ static struct sk_buff *lan9303_rcv(struct sk_buff *skb,
struct net_device *dev,
const struct dsa_device_ops lan9303_netdev_ops = {
.xmit = lan9303_xmit,
.rcv = lan9303_rcv,
+ .overhead = LAN9303_TAG_LEN,
};
diff --git a/net/dsa/tag_mtk.c b/net/dsa/tag_mtk.c
index 11535bc70743..f39f4dfeda34 100644
--- a/net/dsa/tag_mtk.c
+++ b/net/dsa/tag_mtk.c
@@ -109,4 +109,5 @@ const struct dsa_device_ops mtk_netdev_ops = {
.xmit = mtk_tag_xmit,
.rcv= mtk_tag_rcv,
.flow_dissect = mtk_tag_flow_dissect,
+ .overhead = MTK_HDR_LEN,
};
diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
index 613f4ee97771..ed4f6dc26365 100644
--- a/net/dsa/tag_qca.c
+++ b/net/dsa/tag_qca.c
@@ -101,4 +101,5 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb,
struct net_device *dev,
const struct dsa_device_ops qca_netdev_ops = {
.xmit = qca_tag_xmit,
.rcv= qca_tag_rcv,
+ .overhead = QCA_HDR_LEN,
};
diff --git a/net/dsa/tag_trailer.c b/net/dsa/tag_trailer.c
index 56197f0d9608..b40756ed6e57 100644
--- a/net/dsa/tag_trailer.c
+++ b/net/dsa/tag_trailer.c
@@ -84,4 +84,5 @@ static struct sk_buff *trailer_rcv(struct sk_buff *skb,
struct net_device *dev,
const struct dsa_device_ops trailer_netdev_ops = {
.xmit = trailer_xmit,
.rcv= trailer_rcv,
+ .overhead = 4,
};
--
2.19.1
[PATCH net-next 0/2] Adjust MTU of DSA master interface
DSA makes use of additional headers to direct a frame in/out of a specific port of the switch. When the slave interfaces uses an MTU of 1500, the master interface can be asked to handle frames with an MTU of 1504, or 1508 bytes. Some Ethernet interfaces won't transmit/receive frames which are bigger than their MTU. Automate the increasing of the MTU on the master interface, by adding to each tagging driver how much overhead they need, and then calling dev_set_mtu() of the master interface to increase its MTU as needed. Andrew Lunn (2): net: dsa: Add overhead to tag protocol ops. net: dsa: Set the master device's MTU to account for DSA overheads include/net/dsa.h | 1 + net/dsa/master.c | 16 net/dsa/tag_brcm.c| 2 ++ net/dsa/tag_dsa.c | 1 + net/dsa/tag_edsa.c| 1 + net/dsa/tag_gswip.c | 1 + net/dsa/tag_ksz.c | 1 + net/dsa/tag_lan9303.c | 1 + net/dsa/tag_mtk.c | 1 + net/dsa/tag_qca.c | 1 + net/dsa/tag_trailer.c | 1 + 11 files changed, 27 insertions(+) -- 2.19.1
[PATCH net-next 2/2] net: dsa: Set the master device's MTU to account for DSA overheads
DSA tagging of frames sent over the master interface to the switch
increases the size of the frame. Such frames can then be bigger than
the normal MTU of the master interface, and it may drop them. Use the
overhead information from the tagger to set the MTU of the master
device to include this overhead.
Signed-off-by: Andrew Lunn
---
net/dsa/master.c | 16
1 file changed, 16 insertions(+)
diff --git a/net/dsa/master.c b/net/dsa/master.c
index c90ee3227dea..42f525bc68e2 100644
--- a/net/dsa/master.c
+++ b/net/dsa/master.c
@@ -158,8 +158,24 @@ static void dsa_master_ethtool_teardown(struct net_device
*dev)
cpu_dp->orig_ethtool_ops = NULL;
}
+void dsa_master_set_mtu(struct net_device *dev, struct dsa_port *cpu_dp)
+{
+ unsigned int mtu = ETH_DATA_LEN + cpu_dp->tag_ops->overhead;
+ int err;
+
+ rtnl_lock();
+ if (mtu <= dev->max_mtu) {
+ err = dev_set_mtu(dev, mtu);
+ if (err)
+ netdev_dbg(dev, "Unable to set MTU to include for DSA
overheads\n");
+ }
+ rtnl_unlock();
+}
+
int dsa_master_setup(struct net_device *dev, struct dsa_port *cpu_dp)
{
+ dsa_master_set_mtu(dev, cpu_dp);
+
/* If we use a tagging format that doesn't have an ethertype
* field, make sure that all packets from this point on get
* sent to the tag format's receive function.
--
2.19.1
[PATCH net-next 4/6] net: mii: Add mii_lpa_mod_linkmode_lpa_t
Add a _mod_ variant of mii_lpa_to_linkmode_lpa_t. Use this to fix the
genphy_read_status() where the 1G link partner features are getting
lost.
Fixes: c0ec3c273677 ("net: phy: Convert u32 phydev->lp_advertising to linkmode")
Reported-by: Heiner Kallweit
Signed-off-by: Andrew Lunn
---
drivers/net/phy/phy_device.c | 2 +-
include/linux/mii.h | 68 +++-
2 files changed, 53 insertions(+), 17 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index c20b5ecc0f4b..7d5d698604aa 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1748,7 +1748,7 @@ int genphy_read_status(struct phy_device *phydev)
if (lpa < 0)
return lpa;
- mii_lpa_to_linkmode_lpa_t(phydev->lp_advertising, lpa);
+ mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, lpa);
adv = phy_read(phydev, MII_ADVERTISE);
if (adv < 0)
diff --git a/include/linux/mii.h b/include/linux/mii.h
index b915ef6c3692..e72447778a08 100644
--- a/include/linux/mii.h
+++ b/include/linux/mii.h
@@ -372,6 +372,36 @@ static inline u32 mii_lpa_to_ethtool_lpa_x(u32 lpa)
return result | mii_adv_to_ethtool_adv_x(lpa);
}
+/**
+ * mii_adv_mod_linkmode_adv_t
+ * @advertising:pointer to destination link mode.
+ * @adv: value of the MII_ADVERTISE register
+ *
+ * A small helper function that translates MII_ADVERTISE bits to
+ * linkmode advertisement settings. Leaves other bits unchanged.
+ */
+static inline void mii_adv_mod_linkmode_adv_t(unsigned long *advertising,
+ u32 adv)
+{
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
+advertising, adv & ADVERTISE_10HALF);
+
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT,
+advertising, adv & ADVERTISE_10FULL);
+
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT,
+advertising, adv & ADVERTISE_100HALF);
+
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
+advertising, adv & ADVERTISE_100FULL);
+
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_Pause_BIT, advertising,
+adv & ADVERTISE_PAUSE_CAP);
+
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+advertising, adv & ADVERTISE_PAUSE_ASYM);
+}
+
/**
* mii_adv_to_linkmode_adv_t
* @advertising:pointer to destination link mode.
@@ -386,22 +416,7 @@ static inline void mii_adv_to_linkmode_adv_t(unsigned long
*advertising,
{
linkmode_zero(advertising);
- if (adv & ADVERTISE_10HALF)
- linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
-advertising);
- if (adv & ADVERTISE_10FULL)
- linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT,
-advertising);
- if (adv & ADVERTISE_100HALF)
- linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT,
-advertising);
- if (adv & ADVERTISE_100FULL)
- linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
-advertising);
- if (adv & ADVERTISE_PAUSE_CAP)
- linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, advertising);
- if (adv & ADVERTISE_PAUSE_ASYM)
- linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, advertising);
+ mii_adv_mod_linkmode_adv_t(advertising, adv);
}
/**
@@ -423,6 +438,27 @@ static inline void mii_lpa_to_linkmode_lpa_t(unsigned long
*lp_advertising,
}
+/**
+ * mii_lpa_mod_linkmode_lpa_t
+ * @adv: value of the MII_LPA register
+ *
+ * A small helper function that translates MII_LPA bits, when in
+ * 1000Base-T mode, to linkmode LP advertisement settings. Leaves
+ * other bits unchanged.
+ */
+static inline void mii_lpa_mod_linkmode_lpa_t(unsigned long *lp_advertising,
+ u32 lpa)
+{
+ mii_adv_mod_linkmode_adv_t(lp_advertising, lpa);
+
+ if (lpa & LPA_LPACK)
+ linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
+lp_advertising);
+ else
+ linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
+ lp_advertising);
+}
+
/**
* linkmode_adv_to_lcl_adv_t
* @advertising:pointer to linkmode advertising
--
2.19.1
[PATCH net-next 2/6] net: mii: Rename mii_stat1000_to_linkmode_lpa_t
Rename mii_stat1000_to_linkmode_lpa_t to
mii_stat1000_mod_linkmode_lpa_t to indicate it modifies the passed
linkmode bitmap, without clearing any other bits.
Add a helper to set/clear bits in a linkmode.
Use this helper to ensure bit are clear which the stat1000 indicates
should not be set.
Suggested-by: Heiner Kallweit
Fixes: c0ec3c273677 ("net: phy: Convert u32 phydev->lp_advertising to linkmode")
Signed-off-by: Andrew Lunn
---
drivers/net/phy/marvell.c| 2 +-
drivers/net/phy/marvell10g.c | 2 +-
drivers/net/phy/phy_device.c | 4 ++--
include/linux/linkmode.h | 9 +
include/linux/mii.h | 20 ++--
5 files changed, 23 insertions(+), 14 deletions(-)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 6a9881942e53..03dafe0e68a2 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -1138,7 +1138,7 @@ static int marvell_read_status_page_an(struct phy_device
*phydev,
if (!fiber) {
mii_lpa_to_linkmode_lpa_t(phydev->lp_advertising, lpa);
- mii_stat1000_to_linkmode_lpa_t(phydev->lp_advertising, lpagb);
+ mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising, lpagb);
if (phydev->duplex == DUPLEX_FULL) {
phydev->pause = lpa & LPA_PAUSE_CAP ? 1 : 0;
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 6f6e886fc836..82ab6ed3b74e 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -490,7 +490,7 @@ static int mv3310_read_status(struct phy_device *phydev)
if (val < 0)
return val;
- mii_stat1000_to_linkmode_lpa_t(phydev->lp_advertising, val);
+ mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising, val);
if (phydev->autoneg == AUTONEG_ENABLE)
phy_resolve_aneg_linkmode(phydev);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index e6720e2a2da6..c20b5ecc0f4b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1739,8 +1739,8 @@ int genphy_read_status(struct phy_device *phydev)
return -ENOLINK;
}
- mii_stat1000_to_linkmode_lpa_t(phydev->lp_advertising,
- lpagb);
+ mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising,
+ lpagb);
common_adv_gb = lpagb & adv << 2;
}
diff --git a/include/linux/linkmode.h b/include/linux/linkmode.h
index 22443d7fb5cd..a99c58866860 100644
--- a/include/linux/linkmode.h
+++ b/include/linux/linkmode.h
@@ -57,6 +57,15 @@ static inline void linkmode_clear_bit(int nr, volatile
unsigned long *addr)
__clear_bit(nr, addr);
}
+static inline void linkmode_mod_bit(int nr, volatile unsigned long *addr,
+ int set)
+{
+ if (set)
+ linkmode_set_bit(nr, addr);
+ else
+ linkmode_clear_bit(nr, addr);
+}
+
static inline void linkmode_change_bit(int nr, volatile unsigned long *addr)
{
__change_bit(nr, addr);
diff --git a/include/linux/mii.h b/include/linux/mii.h
index 57365224306c..b915ef6c3692 100644
--- a/include/linux/mii.h
+++ b/include/linux/mii.h
@@ -288,22 +288,22 @@ static inline u32 mii_stat1000_to_ethtool_lpa_t(u32 lpa)
}
/**
- * mii_stat1000_to_linkmode_lpa_t
+ * mii_stat1000_mod_linkmode_lpa_t
* @advertising: target the linkmode advertisement settings
* @adv: value of the MII_STAT1000 register
*
* A small helper function that translates MII_STAT1000 bits, when in
- * 1000Base-T mode, to linkmode advertisement settings.
+ * 1000Base-T mode, to linkmode advertisement settings. Other bits in
+ * advertising are not changes.
*/
-static inline void mii_stat1000_to_linkmode_lpa_t(unsigned long *advertising,
- u32 lpa)
+static inline void mii_stat1000_mod_linkmode_lpa_t(unsigned long *advertising,
+ u32 lpa)
{
- if (lpa & LPA_1000HALF)
- linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
-advertising);
- if (lpa & LPA_1000FULL)
- linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
-advertising);
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
+advertising, lpa & LPA_1000HALF);
+
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
+advertising, lpa & LPA_1000FULL);
}
/**
--
2.19.1
[PATCH net-next 5/6] net: mii: mii_lpa_mod_linkmode_lpa_t: Make use of linkmode_mod_bit helper
Replace the if else code structure with a call to the helper
linkmode_mod_bit.
Signed-off-by: Andrew Lunn
---
include/linux/mii.h | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/include/linux/mii.h b/include/linux/mii.h
index e72447778a08..6fee8b1a4400 100644
--- a/include/linux/mii.h
+++ b/include/linux/mii.h
@@ -451,12 +451,8 @@ static inline void mii_lpa_mod_linkmode_lpa_t(unsigned
long *lp_advertising,
{
mii_adv_mod_linkmode_adv_t(lp_advertising, lpa);
- if (lpa & LPA_LPACK)
- linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
-lp_advertising);
- else
- linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
- lp_advertising);
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
+lp_advertising, lpa & LPA_LPACK);
}
/**
--
2.19.1
[PATCH net-next 6/6] net: phy: Fix ioctl handler when modifing MII_ADVERTISE
When the MII_ADVERTISE register is modified by the IOCTL handler,
phydev->advertising needs recalculating. Use the _mod_ variant of
mii_adv_to_linkmode_adv_t so that bits outside of the advertise
registers are not cleared.
Fixes: c0ec3c273677 ("net: phy: Convert u32 phydev->lp_advertising to linkmode")
Reported-by: Heiner Kallweit
Signed-off-by: Andrew Lunn
---
drivers/net/phy/phy.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index e1a1e54baac2..e24708f1fc16 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -437,8 +437,8 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq
*ifr, int cmd)
}
break;
case MII_ADVERTISE:
- mii_adv_to_linkmode_adv_t(phydev->advertising,
- val);
+ mii_adv_mod_linkmode_adv_t(phydev->advertising,
+ val);
change_autoneg = true;
break;
default:
--
2.19.1
[PATCH net-next 1/6] net: mii: Fix autoneg in mii_lpa_to_linkmode_lpa_t()
mii_adv_to_linkmode_adv_t() clears all bits before setting it needs to
set. This means the freshly set Autoneg gets cleared.
Change the order, and add comments about it clearing the old content
of the bitmap.
Reported-by: Heiner Kallweit
Fixes: c0ec3c273677 ("net: phy: Convert u32 phydev->lp_advertising to linkmode")
Signed-off-by: Andrew Lunn
---
include/linux/mii.h | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/include/linux/mii.h b/include/linux/mii.h
index fb7ae4ae8ce3..57365224306c 100644
--- a/include/linux/mii.h
+++ b/include/linux/mii.h
@@ -378,7 +378,8 @@ static inline u32 mii_lpa_to_ethtool_lpa_x(u32 lpa)
* @adv: value of the MII_ADVERTISE register
*
* A small helper function that translates MII_ADVERTISE bits
- * to linkmode advertisement settings.
+ * to linkmode advertisement settings. Clears the old value
+ * of advertising.
*/
static inline void mii_adv_to_linkmode_adv_t(unsigned long *advertising,
u32 adv)
@@ -408,16 +409,18 @@ static inline void mii_adv_to_linkmode_adv_t(unsigned
long *advertising,
* @adv: value of the MII_LPA register
*
* A small helper function that translates MII_LPA bits, when in
- * 1000Base-T mode, to linkmode LP advertisement settings.
+ * 1000Base-T mode, to linkmode LP advertisement settings. Clears the
+ * old value of advertising
*/
static inline void mii_lpa_to_linkmode_lpa_t(unsigned long *lp_advertising,
u32 lpa)
{
+ mii_adv_to_linkmode_adv_t(lp_advertising, lpa);
+
if (lpa & LPA_LPACK)
linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
lp_advertising);
- mii_adv_to_linkmode_adv_t(lp_advertising, lpa);
}
/**
--
2.19.1
[PATCH net-next 3/6] phy: marvell: Rename mii_lpa_to_linkmode_lpa_t
Rename mii_lpa_to_linkmode_lpa_t to mii_lpa_mod_linkmode_lpa_t to
indicate it modifies the passed linkmode bitmap, without clearing any
other bits.
Also, ensure bit are clear which the lpa indicates should not be set.
Suggested-by: Heiner Kallweit
Fixes: c0ec3c273677 ("net: phy: Convert u32 phydev->lp_advertising to linkmode")
Signed-off-by: Andrew Lunn
---
drivers/net/phy/marvell.c | 22 +++---
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 03dafe0e68a2..a9c7c7f41b0c 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -1047,21 +1047,21 @@ static int m88e1145_config_init(struct phy_device
*phydev)
}
/**
- * fiber_lpa_to_linkmode_lpa_t
+ * fiber_lpa_mod_linkmode_lpa_t
* @advertising: the linkmode advertisement settings
* @lpa: value of the MII_LPA register for fiber link
*
- * A small helper function that translates MII_LPA
- * bits to linkmode LP advertisement settings.
+ * A small helper function that translates MII_LPA bits to linkmode LP
+ * advertisement settings. Other bits in advertising are left
+ * unchanged.
*/
-static void fiber_lpa_to_linkmode_lpa_t(unsigned long *advertising, u32 lpa)
+static void fiber_lpa_mod_linkmode_lpa_t(unsigned long *advertising, u32 lpa)
{
- if (lpa & LPA_FIBER_1000HALF)
- linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
-advertising);
- if (lpa & LPA_FIBER_1000FULL)
- linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
-advertising);
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
+advertising, lpa & LPA_FIBER_1000HALF);
+
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
+advertising, lpa & LPA_FIBER_1000FULL);
}
/**
@@ -1146,7 +1146,7 @@ static int marvell_read_status_page_an(struct phy_device
*phydev,
}
} else {
/* The fiber link is only 1000M capable */
- fiber_lpa_to_linkmode_lpa_t(phydev->lp_advertising, lpa);
+ fiber_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, lpa);
if (phydev->duplex == DUPLEX_FULL) {
if (!(lpa & LPA_PAUSE_FIBER)) {
--
2.19.1
[PATCH net-next 0/6] u32 to linkmode fixes
This patchset fixes issues found in the last patchset which converted the phydev advertise etc, from a u32 to a linux bitmap. Most of the issues are the result of clearing bits which should not of been cleared. To make the API clearer, the idea from Heiner Kallweit was used, with _mod_ to indicate the function modifies just the bits it needs to, or _to_ to clear all bits and just set bit that need to be set. Andrew Lunn (6): net: mii: Fix autoneg in mii_lpa_to_linkmode_lpa_t() net: mii: Rename mii_stat1000_to_linkmode_lpa_t phy: marvell: Rename mii_lpa_to_linkmode_lpa_t net: mii: Add mii_lpa_mod_linkmode_lpa_t net: mii: mii_lpa_mod_linkmode_lpa_t: Make use of linkmode_mod_bit helper net: phy: Fix ioctl handler when modifing MII_ADVERTISE drivers/net/phy/marvell.c| 24 +- drivers/net/phy/marvell10g.c | 2 +- drivers/net/phy/phy.c| 4 +- drivers/net/phy/phy_device.c | 6 +-- include/linux/linkmode.h | 9 include/linux/mii.h | 93 +--- 6 files changed, 91 insertions(+), 47 deletions(-) -- 2.19.1
Re: [PATCH RFC 5/6] net: dsa: microchip: Update tag_ksz.c to access switch driver
On Wed, Dec 05, 2018 at 07:00:38PM +0100, Andrew Lunn wrote: > On Mon, Dec 03, 2018 at 03:34:56PM -0800, tristram...@microchip.com wrote: > > From: Tristram Ha > > > > Update tag_ksz.c to access switch driver's tail tagging operations. > > Hi Tristram > > Humm, i'm not sure we want this, the tagging spit into two places. I > need to take a closer look at the previous patch, to see why it cannot > be done here. O.K, i think i get what is going on. I would however implement it differently. One net/dsa/tag_X.c file can export two dsa_device_ops structures, allowing you to share common code for the two taggers. You could call these DSA_TAG_PROTO_KSZ_1_BYTE, and DSA_TAG_PROTO_KSZ_2_BYTE, and the .get_tag_protocol call would then return the correct one for the switch. It might also be possible to merge in tag_trailer, or at least share some code. What i don't yet understand is how you are passing PTP information around. The commit messages need to explain that, since it is not obvious, and it is the first time we have needed PTP info in a tag driver. Andrew
Re: [PATCH RFC 5/6] net: dsa: microchip: Update tag_ksz.c to access switch driver
On Mon, Dec 03, 2018 at 03:34:56PM -0800, tristram...@microchip.com wrote: > From: Tristram Ha > > Update tag_ksz.c to access switch driver's tail tagging operations. Hi Tristram Humm, i'm not sure we want this, the tagging spit into two places. I need to take a closer look at the previous patch, to see why it cannot be done here. Andrew
Re: [PATCH RFC 2/6] net: dsa: microchip: Add MIB counter reading support
Hi Tristan
> +static void ksz9477_r_mib_cnt(struct ksz_device *dev, int port, u16 addr,
> + u64 *cnt)
> +{
> + u32 data;
> + int timeout;
> + struct ksz_port *p = >ports[port];
> +
> + /* retain the flush/freeze bit */
> + data = p->freeze ? MIB_COUNTER_FLUSH_FREEZE : 0;
> + data |= MIB_COUNTER_READ;
> + data |= (addr << MIB_COUNTER_INDEX_S);
> + ksz_pwrite32(dev, port, REG_PORT_MIB_CTRL_STAT__4, data);
> +
> + timeout = 1000;
> + do {
> + ksz_pread32(dev, port, REG_PORT_MIB_CTRL_STAT__4,
> + );
> + usleep_range(1, 10);
> + if (!(data & MIB_COUNTER_READ))
> + break;
> + } while (timeout-- > 0);
Could you use readx_poll_timeout() here?
> +void ksz_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *buf)
> +{
> + struct ksz_device *dev = ds->priv;
> + struct ksz_port_mib *mib;
> +
> + mib = >ports[port].mib;
> +
> + /* freeze MIB counters if supported */
> + if (dev->dev_ops->freeze_mib)
> + dev->dev_ops->freeze_mib(dev, port, true);
> + mutex_lock(>cnt_mutex);
> + port_r_cnt(dev, port);
> + mutex_unlock(>cnt_mutex);
> + if (dev->dev_ops->freeze_mib)
> + dev->dev_ops->freeze_mib(dev, port, false);
Should the freeze be protected by the mutex as well?
> + memcpy(buf, mib->counters, dev->mib_cnt * sizeof(u64));
I wonder if this memcpy should also be protected by the mutex. As soon
as the mutex is dropped, the scheduled work could start updating
mib->counters in non-atomic ways?
> +}
> +
> int ksz_port_bridge_join(struct dsa_switch *ds, int port,
>struct net_device *br)
> {
> @@ -255,6 +349,7 @@ int ksz_enable_port(struct dsa_switch *ds, int port,
> struct phy_device *phy)
> /* setup slave port */
> dev->dev_ops->port_setup(dev, port, false);
> dev->dev_ops->phy_setup(dev, port, phy);
> + dev->dev_ops->port_init_cnt(dev, port);
This is probably not the correct place to do this. MIB counters should
not be cleared by an ifdown/ifup cycle. They should only be cleared
when the driver is probed.
Re: [PATCH RFC 1/6] net: dsa: microchip: Prepare PHY for proper advertisement
On Mon, Dec 03, 2018 at 03:34:52PM -0800, tristram...@microchip.com wrote:
> From: Tristram Ha
>
> Prepare PHY for proper advertisement and get link status for the port.
>
> Signed-off-by: Tristram Ha
> Reviewed-by: Woojung Huh
> ---
> drivers/net/dsa/microchip/ksz9477.c| 12
> drivers/net/dsa/microchip/ksz_common.c | 17 +
> drivers/net/dsa/microchip/ksz_common.h | 2 ++
> drivers/net/dsa/microchip/ksz_priv.h | 2 ++
> 4 files changed, 33 insertions(+)
>
> diff --git a/drivers/net/dsa/microchip/ksz9477.c
> b/drivers/net/dsa/microchip/ksz9477.c
> index 0684657..98aa25e 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -969,6 +969,16 @@ static void ksz9477_port_mirror_del(struct dsa_switch
> *ds, int port,
>PORT_MIRROR_SNIFFER, false);
> }
>
> +static void ksz9477_phy_setup(struct ksz_device *dev, int port,
> + struct phy_device *phy)
> +{
> + if (port < dev->phy_port_cnt) {
> + /* SUPPORTED_Asym_Pause and SUPPORTED_Pause can be removed to
> + * disable flow control when rate limiting is used.
> + */
> + }
Hi Tristram
Is this meant to be a TODO comment?
What is supposed to happen here is that all forms of pause are disable
by default. The MAC driver needs to enable what it supports by calling
phy_support_sym_pause() or phy_support_asym_pause().
Ah, is this because there is not a real PHY driver?
Thanks
Andrew
Re: [PATCH net-next v4 0/2] Add mii_bus to ixgbe driver for dsa devs
> Yes the current solution whereby we need to get a hold on the network > device's struct device reference is not quite ideal, AFAIR, Andrew has > had the same problem. Yes, it is not nice, and there is a race with systemd renaming the interfaces using its naming rules. We need a way to lookup an interface based on a bus location, or similar, not by name. Andrew
Re: [PATCH net-next v3 2/2] ixgbe: use mii_bus to handle MII related ioctls
On Mon, Dec 03, 2018 at 06:55:26PM +, Steve Douthit wrote: > Use the mii_bus callbacks to address the entire clause 22/45 address > space. Enables userspace to poke switch registers instead of a single > PHY address. > > The ixgbe firmware may be polling PHYs in a way that is not protected by > the mii_bus lock. This isn't new behavior, but as Andrew Lunn pointed > out there are more addresses available for conflicts. > > Signed-off-by: Stephen Douthit Reviewed-by: Andrew Lunn Andrew
Re: [PATCH net-next v3 1/2] ixgbe: register a mdiobus
On Mon, Dec 03, 2018 at 06:55:22PM +, Steve Douthit wrote: > Most dsa devices expect a 'struct mii_bus' pointer to talk to switches > via the MII interface. > > While this works for dsa devices, it will not work safely with Linux > PHYs in all configurations since the firmware of the ixgbe device may > be polling some PHY addresses in the background. > > Signed-off-by: Stephen Douthit Reviewed-by: Andrew Lunn Andrew
Re: [PATCH net-next v2 1/2] ixgbe: register a mdiobus
> You can actually strap the 6390 and friends for a multi-chip mode where > they claim only a single address, instead of one per port, plus a couple > more for global registers. It vastly slows things down because of the > extra indirection, but it allows the switch to play nicely with other > MDIO devs. As you say, it slows things down a lot, so it is not used very often. In fact, i don't know of any recent board which actually uses a single address, for any DSA supported switch. If you need multiple devices, e.g. an odd PHY as well as a switch, i would use a couple of GPIO lines and do a bit-banging MDIO bus for the PHY, and let the switch have all the address of the hardware MDIO bus. This assumes you are using the kernel infrastructure, so you can connect the MAC to any arbitrary PHY. Andrew
Re: [PATCH net-next v2 1/2] ixgbe: register a mdiobus
> Agreed, but I'd argue it's the same behavior we have today with the > existing MII ioctls in this driver. That's not to say this is good, > it's just not any less broken than the current state of things. Agreed. I actually would be happy with a warning in the commit message that this code is not sufficient to make use of Linux PHY drivers, because of the hardware polling. You can then leave fixing that to whoever needs Linux PHY drivers. > AFAICT the polling hardware only pokes the device address that the > driver stores in 'hw->phy.mdio.prtad', so the PHY polling unit would > never see the switch, if it's even polling at all. Some of the MAC > configurations will store MDIO_PRTAD_NONE, in which case I wouldn't > expect the polling unit to be active. It's up to the board designer to > ensure there's no address conflicts on the bus. Well, the 6390 does use address 0-10 for its port registers, and there is something which looks like a PHYID in register 3. So for your use case of DSA, it would be good to ensure it really is disabled. > Then in the ioctl code, in addition to checking the mii_bus is > available, also check that the requested address is one that phy_mask > says mii_bus owns, otherwise we fall through to the old code. I'm not too bothered with the ioctl. It is there so you can shoot yourself in the foot. The hardware polling unit just adds more interesting weapons you can use to shoot yourself in the foot. Andrew
Re: [PATCH net-next v2 1/2] ixgbe: register a mdiobus
On Mon, Dec 03, 2018 at 05:02:40PM +, Steve Douthit wrote:
> On 12/3/18 11:54 AM, Andrew Lunn wrote:
> >> +static s32 ixgbe_x550em_a_mii_bus_read(struct mii_bus *bus, int addr,
> >> + int regnum)
> >> +{
> >> + struct ixgbe_adapter *adapter = (struct ixgbe_adapter *)bus->priv;
> >> + struct ixgbe_hw *hw = >hw;
> >> + u32 gssr = hw->phy.phy_semaphore_mask | IXGBE_GSSR_TOKEN_SM;
> >> +
> >> + if (hw->bus.lan_id)
> >> + gssr |= IXGBE_GSSR_PHY1_SM;
> >> + else
> >> + gssr |= IXGBE_GSSR_PHY0_SM;
> >
> > Hi Steve
> >
> > If you only have one bus, do you still need this? One semaphore is all
> > you need. And i'm not even sure you need that. The MDIO layer will
> > perform locking, assuming everything goes through the MDIO layer.
>
> AFAIK I still need part of it. There's a PHY polling unit in the card
> that we need to sync with independent of the locking in the MDIO layer.
> I can drop the hw->bus.lan_id check though and unconditionally OR in the
> IXGBE_GSSR_PHY0_SM since we always register on function 0 now.
Hi Steve
In general, this is not enough to make a PHY polling unit safe. At
least not with C22 devices. They often have a register which can be
used to select a different page. The software driver can do a write to
swap the page at any time, e.g. to select the page with the
temperature sensor. After it releases the semaphore for that write
changing the page, the hardware could then poll the PHY, and read a
temperature sensor register instead of the link status, and then bad
things happen.
When using Intel's PHY drivers embedded within the Intel MAC driver,
this is not a problem. They can avoid swapping to different
pages. However, you are on the path to allow the Linux PHY drivers to
be used, and some of them will change the page. And with the embedded
SOC you are working on, i would not be too surprised to see somebody
build a system using a different PHY which the Intel PHY drivers don't
support, but does have a Linux PHY driver.
You also need to watch out for your use case of Marvell
mv88e6390. Polling that makes no sense. Does the hardware actually
recognise it is not a PHY?
Andrew
Re: [PATCH net-next v2 1/2] ixgbe: register a mdiobus
> +static s32 ixgbe_x550em_a_mii_bus_read(struct mii_bus *bus, int addr,
> +int regnum)
> +{
> + struct ixgbe_adapter *adapter = (struct ixgbe_adapter *)bus->priv;
> + struct ixgbe_hw *hw = >hw;
> + u32 gssr = hw->phy.phy_semaphore_mask | IXGBE_GSSR_TOKEN_SM;
> +
> + if (hw->bus.lan_id)
> + gssr |= IXGBE_GSSR_PHY1_SM;
> + else
> + gssr |= IXGBE_GSSR_PHY0_SM;
Hi Steve
If you only have one bus, do you still need this? One semaphore is all
you need. And i'm not even sure you need that. The MDIO layer will
perform locking, assuming everything goes through the MDIO layer.
Andrew
Re: [PATCH net] net: phy: don't allow __set_phy_supported to add unsupported modes
On Mon, Dec 03, 2018 at 08:19:33AM +0100, Heiner Kallweit wrote:
> Currently __set_phy_supported allows to add modes w/o checking whether
> the PHY supports them. This is wrong, it should never add modes but
> only remove modes we don't want to support.
>
> The commit marked as fixed didn't do anything wrong, it just copied
> existing functionality to the helper which is being fixed now.
>
> Fixes: f3a6bd393c2c ("phylib: Add phy_set_max_speed helper")
> Signed-off-by: Heiner Kallweit
Reviewed-by: Andrew Lunn
Andrew
Re: [PATCH net-next] net: phy: don't allow __set_phy_supported to add unsupported modes
On Mon, Dec 03, 2018 at 08:04:57AM +0100, Heiner Kallweit wrote: > Currently __set_phy_supported allows to add modes w/o checking whether > the PHY supports them. This is wrong, it should never add modes but > only remove modes we don't want to support. > > Signed-off-by: Heiner Kallweit Reviewed-by: Andrew Lunn Andrew
Re: [PATCH 3/3] dt-bindings: net: dsa: add new bindings MT7530
> >I don't think any of these properties are necessary, if you can either > >use a compatible string, and/or infer the actual model at runtime in the > >driver's probe function, then you can assess based on that chip model as > > There is an ID register in the 7530 - though I don't know if the lower > 16 bits of it can tell us enough information about the device. For me on > the MT7621 they return "0001", I assume it is a revsion ID of some type. > Problem is we do not read that until after the regulators and some of > the clocking is setup. Hi Greg I would suggest you refactor the code, so you know the ID early on. Reading such an ID is very common in device drivers, to decide what to do. The silicon is generally more reliable than device tree when it comes to identification. The only time you need a different device tree compatibly string is when you cannot actually get to the ID, e.g. you need to turn some clock on, or the ID is in a different place. Andrew
Re: [PATCH net-next v2] net: phy: Also request modules for C45 IDs
On Sun, Dec 02, 2018 at 04:33:14PM +0100, Jose Abreu wrote: > Logic of phy_device_create() requests PHY modules according to PHY ID > but for C45 PHYs we use different field for the IDs. > > Let's also request the modules for these IDs. > > Changes from v1: > - Only request C22 modules if C45 are not present (Andrew) > > Signed-off-by: Jose Abreu > Cc: Andrew Lunn > Cc: Florian Fainelli > Cc: "David S. Miller" > Cc: Joao Pinto Reviewed-by: Andrew Lunn Andrew
Re: [PATCH net-next] net: phy: Also request modules for C45 IDs
> @@ -606,6 +606,18 @@ struct phy_device *phy_device_create(struct mii_bus
> *bus, int addr, int phy_id,
>* there's no driver _already_ loaded.
>*/
> request_module(MDIO_MODULE_PREFIX MDIO_ID_FMT, MDIO_ID_ARGS(phy_id));
This line above is for C22. If you look at phy_bus_match(), it will
perform a match on the c45_ids->device_ids[] if it is a c45 PHY, or
the phy_id if it is c22. So i think we should also have this one or
the other here, not both.
Thanks
Andrew
> + if (c45_ids) {
> + const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
> + int i;
> +
> + for (i = 1; i < num_ids; i++) {
> + if (!(c45_ids->devices_in_package & (1 << i)))
> + continue;
> +
> + request_module(MDIO_MODULE_PREFIX MDIO_ID_FMT,
> +MDIO_ID_ARGS(c45_ids->device_ids[i]));
> + }
> + }
>
> device_initialize(>dev);
>
> --
> 2.7.4
>
>
Re: [PATCH net-next 1/2] ixgbe: register a mdiobus
> 'cards_found' doesn't exist for the ixgbe driver. Agh, sorry, i was looking at ixgb, not ixgbe. Andrew
Re: [PATCH net-next 1/2] ixgbe: register a mdiobus
> Yep, registering multiple interfaces is wrong. The first board I tested > against only had a single MAC enabled (they can be disabled/hidden via > straps) so it just happened to work. Hi Steve Can you hide any/all via straps, or is 00.0 always guaranteed to exist? > The Intel C3xxx family of SoCs have up to four ixgbe MACs. These are > structured as two devices of two functions each on fixed internal root > ports. > > from lspci: > > +-16.0-[05]--+-00.0 > |\-00.1 > +-17.0-[06]--+-00.0 > |\-00.1 > Is there any other hardware resource which is shared between the MAC interfaces? I'm just wondering if the driver has already solved this once. Is there an EEPROM per interface for the MAC address, or one shared EEPROM? Ah, how about using the 'cards_found' found variable. It is not perfect, in that it is not decremented in ixgb_remove(), and i wonder about race conditions since there does not appear to be any lock when it is incremented. But if cards_found == 0, register the MDIO bus. Andrew
Re: [PATCH 0/3]: net: dsa: mt7530: support MT7530 in the MT7621 SoC
> > 1. TX packets are not getting an IP header checksum via the normal > >off-loaded checksumming when in DSA mode. I have to switch off > >NETIF_F_IP_CSUM, so the software stack generates the checksum. > >That checksum offloading works ok when not using the 7530 DSA driver. > > Hmm. How do I test this? If there are no IP checksums in the frame, the receiver will generally drop the packet. ethtool -k will show you what features the MAC has in terms of offloading. So if you see NETIF_F_IP_CSUM, you know the MAC should be doing it in hardware. Andrew
Re: [PATCH 0/3]: net: dsa: mt7530: support MT7530 in the MT7621 SoC
> 1. TX packets are not getting an IP header checksum via the normal >off-loaded checksumming when in DSA mode. I have to switch off >NETIF_F_IP_CSUM, so the software stack generates the checksum. >That checksum offloading works ok when not using the 7530 DSA driver. With some vendors MAC hardware, there is a field in the descriptor to indicate how big a VLAN tag the frame has. The hardware can then use this information to skip over the VLAN tags to find the IP header, and then perform checksuming. You might be able to re-use that, consider the DSA header as part of the VLAN header. Other vendors, there is no way i've found to get hadware offload of checksumming working. Andrew
Re: [PATCH 0/3]: net: dsa: mt7530: support MT7530 in the MT7621 SoC
> 2. Maximal sized RX packets get silently dropped. So receive side packets >that are large (perfect case is the all-but-last packets in a fragemented >larger packet) appear to be dropped at the mt7621 ethernet MAC level. >The 7530 MIB switch register counters show receive packets at the physical >switch port side and at the CPU switch port - but I get no packets >received or errors in the 7621 ethernet MAC. If I set the mtu of the >server at the other end a little smaller (a few bytes is enough) then >I get all the packets through. It seems like the DSA/VLAN tag bytes >are causing a too large packet to get silently dropped somewhere. Hi Gerg Try increasing the MTU on the master device. Some hardware will reject receiving packets bigger than the MTU. But if you increase the MTU by the DSA header size, it will then receive the frame. I have a patchset i will be posting soon to do this automatically. Andrew
Re: [PATCH net-next 1/2] ixgbe: register a mdiobus
Hi Steve
Cool to see another interface being made DSA capable.
> +/**
> + * ixgbe_msca - Write the command register and poll for completion/timeout
> + * @hw: pointer to hardware structure
> + * @cmd: command register value to write
> + **/
> +static s32 ixgbe_msca_cmd(struct ixgbe_hw *hw, u32 cmd)
> +{
> + u32 i;
> +
> + IXGBE_WRITE_REG(hw, IXGBE_MSCA, cmd);
> +
> + for (i = 0; i < IXGBE_MDIO_COMMAND_TIMEOUT; i++) {
> + udelay(10);
> + cmd = IXGBE_READ_REG(hw, IXGBE_MSCA);
> + if (!(cmd & IXGBE_MSCA_MDI_COMMAND))
> + return 0;
> + }
> +
> + return -ETIMEDOUT;
Please consider using readx_poll_timeout()
> +}
> +
> +/**
> + * ixgbe_msca - Use device_id to figure out if MDIO bus is shared between
> MACs.
> + * The embedded x550(s) in the C3000 line of SoCs only have a single mii_bus
> + * shared between all MACs, and that introduces some new mask flags that
> need
> + * to be passed to the *_swfw_sync callbacks.
> + * @hw: pointer to hardware structure
> + **/
> +static bool ixgbe_is_shared_mii(struct ixgbe_hw *hw)
> +{
> + switch (hw->device_id) {
> + case IXGBE_DEV_ID_X550EM_A_KR:
> + case IXGBE_DEV_ID_X550EM_A_KR_L:
> + case IXGBE_DEV_ID_X550EM_A_SFP_N:
> + case IXGBE_DEV_ID_X550EM_A_SGMII:
> + case IXGBE_DEV_ID_X550EM_A_SGMII_L:
> + case IXGBE_DEV_ID_X550EM_A_10G_T:
> + case IXGBE_DEV_ID_X550EM_A_SFP:
> + case IXGBE_DEV_ID_X550EM_A_1G_T:
> + case IXGBE_DEV_ID_X550EM_A_1G_T_L:
> + return true;
> + }
> +
> + return false;
> +}
> +
> +/**
> + * ixgbe_mii_bus_read - Read a clause 22/45 register
> + * @hw: pointer to hardware structure
> + * @addr: address
> + * @regnum: register number
> + **/
> +static s32 ixgbe_mii_bus_read(struct mii_bus *bus, int addr, int regnum)
> +{
> + struct ixgbe_adapter *adapter = (struct ixgbe_adapter *)bus->priv;
> + struct ixgbe_hw *hw = >hw;
> + u32 hwaddr, cmd, gssr = hw->phy.phy_semaphore_mask;
> + s32 data;
> +
> + if (ixgbe_is_shared_mii(hw)) {
> + gssr |= IXGBE_GSSR_TOKEN_SM;
> + if (hw->bus.lan_id)
> + gssr |= IXGBE_GSSR_PHY1_SM;
> + else
> + gssr |= IXGBE_GSSR_PHY0_SM;
> + }
Could you explain this shared bus a bit more. If there is only one
physical MDIO bus, you should only register one bus with Linux. So i
would not normally expect to see ixgbe_is_shared_mii() sort of
statements into the read/write functions. That tends to happen at the
point you are registering the MDIO bus, and when looking for the MACs
PHY on the bus.
Andrew
Re: [PATCH] net: phy: sfp: correct location of SFP standards
On Thu, Nov 29, 2018 at 12:00:05PM +0200, Baruch Siach wrote: > SFP standards are now available from the SNIA (Storage Networking > Industry Association) website. > > Cc: Andrew Lunn > Cc: Florian Fainelli > Signed-off-by: Baruch Siach Reviewed-by: Andrew Lunn Andrew
Re: [PATCH net-next] tun: implement carrier change
On Thu, Nov 29, 2018 at 12:06:18PM +0100, Nicolas Dichtel wrote: > Le 28/11/2018 à 22:48, Andrew Lunn a écrit : > > On Wed, Nov 28, 2018 at 07:12:56PM +0100, Nicolas Dichtel wrote: > >> The userspace may need to control the carrier state. > > > > Hi Nicolas > Hi Andrew, > > > > > Could you explain your user case a bit more. > > > > Are you running a routing daemon on top of the interface, and want it > > to reroute when the carrier goes down? > Sort of, we have a daemon that monitors the app and may re-route the traffic > to > a secondary app if needed. O.K, this sounds sensible. It is often useful to explain the use case for changes like this. People try to do crazy things sometimes. Thanks Andrew
Re: [PATCH net-next] tun: implement carrier change
On Wed, Nov 28, 2018 at 07:12:56PM +0100, Nicolas Dichtel wrote: > The userspace may need to control the carrier state. Hi Nicolas Could you explain your user case a bit more. Are you running a routing daemon on top of the interface, and want it to reroute when the carrier goes down? Thanks Andrew
Re: [PATCH net] net: phy: add workaround for issue where PHY driver doesn't bind to the device
On Fri, Nov 23, 2018 at 07:41:29PM +0100, Heiner Kallweit wrote:
> After switching the r8169 driver to use phylib some user reported that
> their network is broken. This was caused by the genphy PHY driver being
> used instead of the dedicated PHY driver for the RTL8211B. Users
> reported that loading the Realtek PHY driver module upfront fixes the
> issue. See also this mail thread:
> https://marc.info/?t=15427978183=1=2
> The issue is quite weird and the root cause seems to be somewhere in
> the base driver core. The patch works around the issue and may be
> removed once the actual issue is fixed.
>
> The Fixes tag refers to the first reported occurrence of the issue.
> The issue itself may have been existing much longer and it may affect
> users of other network chips as well. Users typically will recognize
> this issue only if their PHY stops working when being used with the
> genphy driver.
>
> Fixes: f1e911d5d0df ("r8169: add basic phylib support")
> Signed-off-by: Heiner Kallweit
> ---
> I'm not sure how long it will take to find and fix the root cause of
> the issue. With this workaround affected users have a working network
> again from 4.19.5 at least.
Hi Heiner
Lets go with this for the moment. That gives us time to try to figure
out what is going on to cause the wrong driver to be used for the
r8169.
Reviewed-by: Andrew Lunn
Andrew
Re: [PATCH net-next] net: phy: fix two issues with linkmode bitmaps
> Because function naming is the same I'm afraid they easily can be used > incorrectly (the bugs we just discuss are good examples). Maybe it > could be an option to reflect the semantics in the name like this > (better suited proposals welcome): > > case 1: mii_xxx_to_linkmode_yyy > case 2: mii_xxx_or_linkmode_yyy > case 3: mii_xxx_mod_linkmode_yyy Hi Heiner I started a patchset using this idea, and reworks your fix. Lets work on that, rather than merge this patch. Andrew
Re: [PATCH net-next] net: phy: fix two issues with linkmode bitmaps
> Eventually we'd have three types of mii_xxx_to_linkmode_yyy functions: > > 1. Function first zeroes the destination linkmode bitmap > 2. Function sets bits in the linkmode bitmap but doesn't clear bits >if condition isn't met > 3. Function clears / sets bits it's responsible for > > example case 1: mmd_eee_adv_to_linkmode > example case 2: mii_stat1000_to_linkmode_lpa_t > example case 3: what you just proposed as fix for > mii_adv_to_linkmode_adv_t > > Because function naming is the same I'm afraid they easily can be used > incorrectly (the bugs we just discuss are good examples). Maybe it > could be an option to reflect the semantics in the name like this > (better suited proposals welcome): > > case 1: mii_xxx_to_linkmode_yyy > case 2: mii_xxx_or_linkmode_yyy > case 3: mii_xxx_mod_linkmode_yyy Hi Heiner That is a good idea. We should probably do this first, it will help find the bugs. Andrew
Re: [PATCH net-next] net: phy: fix two issues with linkmode bitmaps
On Sun, Nov 25, 2018 at 03:23:42PM +0100, Heiner Kallweit wrote:
> I wondered why ethtool suddenly reports that link partner doesn't
> support aneg and GBit modes. It turned out that this is caused by two
> bugs in conversion to linkmode bitmaps.
>
> 1. In genphy_read_status the value of phydev->lp_advertising is
>overwritten, thus GBit modes aren't reported any longer.
> 2. In mii_lpa_to_linkmode_lpa_t the aneg bit was overwritten by the
>call to mii_adv_to_linkmode_adv_t.
Hi Heiner
Thanks for looking into this.
There are more bugs :-(
static inline void mii_lpa_to_linkmode_lpa_t(unsigned long *lp_advertising,
u32 lpa)
{
if (lpa & LPA_LPACK)
linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
lp_advertising);
mii_adv_to_linkmode_adv_t(lp_advertising, lpa);
}
But
static inline void mii_adv_to_linkmode_adv_t(unsigned long *advertising,
u32 adv)
{
linkmode_zero(advertising);
if (adv & ADVERTISE_10HALF)
linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
advertising);
So the Autoneg_BIT gets cleared.
I think the better fix is to take the linkmode_zero() out from here.
Then:
if (adv & ADVERTISE_10HALF)
linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
advertising);
+ else
+ linkmode_clear_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
+ advertising);
for all the bits mii_adv_to_linkmode_adv_t() looks at.
So mii_adv_to_linkmode_adv_t() only modifies bits it is responsible
for, and leaves the others alone.
Andrew
Re: [PATCH] dt-bindings: dsa: Fix typo in "probed"
On Fri, Nov 23, 2018 at 03:46:50PM -0200, Fabio Estevam wrote: > The correct form is "can be probed", so fix the typo. > > Signed-off-by: Fabio Estevam Reviewed-by: Andrew Lunn Andrew
Re: [PATCH v3 net-next 04/12] net: ethernet: Use phy_set_max_speed() to limit advertised speed
On Thu, Nov 22, 2018 at 12:40:25PM +0200, Anssi Hannula wrote: > Hi, > > On 12.9.2018 2:53, Andrew Lunn wrote: > > Many Ethernet MAC drivers want to limit the PHY to only advertise a > > maximum speed of 100Mbs or 1Gbps. Rather than using a mask, make use > > of the helper function phy_set_max_speed(). > > But what if the PHY does not support 1Gbps in the first place? Yes, you are correct. __set_phy_supported() needs modifying to take into account what the PHY can do. Thanks for pointing this out. I will take a look. Andrew
Re: [PATCH net-next 4/4] octeontx2-af: Bringup CGX LMAC links by default
On Thu, Nov 22, 2018 at 05:18:37PM +0530, Linu Cherian wrote: > From: Linu Cherian > > - Added new CGX firmware interface API for sending link up/down > commands > > - Do link up for cgx lmac ports by default at the time of CGX > driver probe. Hi Linu This is a complex driver which i don't understand... By link up, do you mean the equivalent of 'ip link set up dev ethX'? Andrew
Re: [PATCH v1 net] lan743x: Enable driver to work with LAN7431
On Wed, Nov 21, 2018 at 02:22:45PM -0500, Bryan Whitehead wrote:
> This driver was designed to work with both LAN7430 and LAN7431.
> The only difference between the two is the LAN7431 has support
> for external phy.
>
> This change adds LAN7431 to the list of recognized devices
> supported by this driver.
>
> fixes: driver won't load for LAN7431
Hi Bryan
There is a well defined format for Fixes:.
Fixes: 23f0703c125b ("lan743x: Add main source files for new lan743x driver")
Andrew
Re: [PATCH net-next 2/2] net: bridge: add no_linklocal_learn bool option
> int br_boolopt_get(const struct net_bridge *br, enum br_boolopt_id opt)
> {
> - int optval = 0;
> -
> switch (opt) {
> + case BR_BOOLOPT_NO_LL_LEARN:
> + return br_opt_get(br, BROPT_NO_LL_LEARN);
> default:
> break;
> }
>
> - return optval;
> + return 0;
> }
It seems like 1/2 of that change belongs in the previous patch.
> --- a/net/bridge/br_sysfs_br.c
> +++ b/net/bridge/br_sysfs_br.c
> @@ -328,6 +328,27 @@ static ssize_t flush_store(struct device *d,
> }
> static DEVICE_ATTR_WO(flush);
>
> +static ssize_t no_linklocal_learn_show(struct device *d,
> +struct device_attribute *attr,
> +char *buf)
> +{
> + struct net_bridge *br = to_bridge(d);
> + return sprintf(buf, "%d\n", br_boolopt_get(br, BR_BOOLOPT_NO_LL_LEARN));
> +}
> +
> +static int set_no_linklocal_learn(struct net_bridge *br, unsigned long val)
> +{
> + return br_boolopt_toggle(br, BR_BOOLOPT_NO_LL_LEARN, !!val);
> +}
> +
> +static ssize_t no_linklocal_learn_store(struct device *d,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + return store_bridge_parm(d, buf, len, set_no_linklocal_learn);
> +}
> +static DEVICE_ATTR_RW(no_linklocal_learn);
I thought we where trying to move away from sysfs? Do we need to add
new options here? It seems like forcing people to use iproute2 for
newer options is a good way to get people to convert to iproute2.
Andrew
Re: [PATCH v1 net] lan743x: fix return value for lan743x_tx_napi_poll
> Andrew, > > Admittedly, my knowledge of what the kernel is doing behind the > scenes is limited. Me too. Lets see if anybody can make sense of the information you provided. Thanks Andrew
Re: [PATCH v1 net] lan743x: fix return value for lan743x_tx_napi_poll
On Tue, Nov 20, 2018 at 01:26:43PM -0500, Bryan Whitehead wrote: > It has been noticed that under stress the lan743x driver will > sometimes hang or cause a kernel panic. It has been noticed > that returning '0' instead of 'weight' fixes this issue. > > fixes: rare kernel panic under heavy traffic load. > Signed-off-by: Bryan Whitehead Hi Bryan This sounds like a band aid over something which is broken, not a real fix. Can you show us the stack trace from the panic? Andrew
Re: [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions
On Mon, Nov 19, 2018 at 05:14:38PM +0100, Andreas Schwab wrote: > On Nov 19 2018, Andrew Lunn wrote: > > > Could you turn on lockdep and see if it reports a deadlock. > > How do I "turn on lockdep"? make menuconfig Kernel hacking Lock Debugging (spinlocks, mutexes, etc...) Lock debugging: prove locking correctness It has changes its name at some point. it used to be called CONFIG_LOCKDEP, for locking dependencies. Anybody who has been kernel hacking for a while still calls it lockdep. Andrew
Re: [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions
On Mon, Nov 19, 2018 at 04:50:14PM +0100, Andreas Schwab wrote: > On Nov 19 2018, Alexandre Belloni wrote: > > > My first intuition is that he mac driver quentin is using does > > phy_connect when the interface is opened while macb is doing it at probe > > time. I didn't investigate but maybe this can help :) > > The phy probing of macb is very unreliable, perhaps it needs to be > rewritten. Hi Andreas I still don't see why that would cause a hang. Could you turn on lockdep and see if it reports a deadlock. Thanks Andrew
Re: [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions
On Mon, Nov 19, 2018 at 04:13:10PM +0100, Andreas Schwab wrote: > On Nov 19 2018, Andrew Lunn wrote: > > > On Mon, Nov 19, 2018 at 03:57:17PM +0100, Andreas Schwab wrote: > >> On Okt 08 2018, Quentin Schulz wrote: > >> > >> > The Microsemi PHYs have multiple banks of registers (called pages). > >> > Registers can only be accessed from one page, if we need a register from > >> > another page, we need to switch the page and the registers of all other > >> > pages are not accessible anymore. > >> > > >> > Basically, to read register 5 from page 0, 1, 2, etc., you do the same > >> > phy_read(phydev, 5); but you need to set the desired page beforehand. > >> > > >> > In order to guarantee that two concurrent functions do not change the > >> > page, we need to do some locking per page. This can be achieved with the > >> > use of phy_select_page and phy_restore_page functions but phy_write/read > >> > calls in-between those two functions shall be replaced by their > >> > lock-free alternative __phy_write/read. > >> > > >> > Let's migrate this driver to those functions. > >> > >> This has some serious locking problem. > > > > Hi Andreas > > > > Could you be more specific. Are you getting a deadlock? A WARN_ON? > > See the stack trace. That's where it hangs. So you never said it hangs. The stacktrace helps, but a description of what actually happens also helps. And i expect Quentin has booted this code lots of times and not had a hang. So some hits how to reproduce it would also help. Maybe your kernel config? I'm interested because he is using the core mdio locking primitives. If those are broken, i want to know. Thanks Andrew
Re: [PATCH net-next 1/4] enetc: Introduce basic PF and VF ENETC ethernet drivers
> >> +static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff
> >> *skb);
> >> +static void enetc_unmap_tx_buff(struct enetc_bdr *tx_ring,
> >> + struct enetc_tx_swbd *tx_swbd);
> >> +static int enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int budget);
> >> +
> >> +static struct sk_buff *enetc_map_rx_buff_to_skb(struct enetc_bdr *rx_ring,
> >> + int i, u16 size);
> >> +static void enetc_add_rx_buff_to_skb(struct enetc_bdr *rx_ring, int i,
> >> + u16 size, struct sk_buff *skb);
> >> +static void enetc_process_skb(struct enetc_bdr *rx_ring, struct sk_buff
> >> *skb);
> >> +static int enetc_clean_rx_ring(struct enetc_bdr *rx_ring,
> >> + struct napi_struct *napi, int work_limit);
> >> +
> >
> >Please try to avoid forward declarations. Move the code around so you
> >don't need them.
> >
>
> Maybe I could drop some of these, but...
> For some performance critical functions on the datapath this would be an
> attempt to improve icache hit rate by having the caller function located in
> memory just before it's children (callees).
That is kind of the point of moving the functions. GCC can only inline
a function when it has already seen it, at least as far as i know. So
by having the functions in the correct order, you increase the
likelihood gcc inlines it, making the icache layout better, no
function calls, etc.
Try building the code both ways, and then disassemble it. See which
looks better.
> >> +static bool enetc_tx_csum(struct sk_buff *skb, union enetc_tx_bd *txbd)
> >> +{
> >> + int l3_start, l3_hsize;
> >> + u16 l3_flags, l4_flags;
> >> +
> >> + if (skb->ip_summed != CHECKSUM_PARTIAL)
> >> + return false;
> >> +
> >> + switch (skb->csum_offset) {
> >> + case offsetof(struct tcphdr, check):
> >> + l4_flags = ENETC_TXBD_L4_TCP;
> >> + break;
> >> + case offsetof(struct udphdr, check):
> >> + l4_flags = ENETC_TXBD_L4_UDP;
> >> + break;
> >> + default:
> >> + skb_checksum_help(skb);
> >> + return false;
> >> + }
> >> +
> >> + l3_start = skb_network_offset(skb);
> >> + l3_hsize = skb_network_header_len(skb);
> >> +
> >> + l3_flags = 0;
> >> + if (skb->protocol == htons(ETH_P_IPV6))
> >> + l3_flags = ENETC_TXBD_L3_IPV6;
> >
> >Is there no need to do anything with IPv4?
> >
>
> No, 0 for this bit means IPv4. Only UDP and TCP over IPv4 and IPv6 supported.
Ah, O.K. Some i assume there is some way to tell it this is actually
an IP packet, not an IPX packet, etc.
> This code path is also reached by the VF driver (via the open() hook which is
> common to both
> PF and VF drivers, and VFs don't manage PHYs).
> Also, the driver may be exercised in MAC loopback mode (ethtool -K loopback
> on), when the
> PHY nodes are removed. And it's always good to be able to run the driver on
> a simulator or
> an emulator without having to modify it.
O.K, that is fine.
> >> +int enetc_pci_probe(struct pci_dev *pdev, const char *name, int
> >> sizeof_priv)
> >> +{
> >> + struct enetc_si *si, *p;
> >> + struct enetc_hw *hw;
> >> + size_t alloc_size;
> >> + int err, len;
> >> +
> >> + err = pci_enable_device_mem(pdev);
> >> + if (err) {
> >> + dev_err(>dev, "device enable failed\n");
> >> + return err;
> >> + }
> >> +
> >> + /* set up for high or low dma */
> >> + err = dma_set_mask_and_coherent(>dev, DMA_BIT_MASK(64));
> >> + if (err) {
> >> + err = dma_set_mask_and_coherent(>dev,
> >DMA_BIT_MASK(32));
> >> + if (err) {
> >> + dev_err(>dev,
> >> + "DMA configuration failed: 0x%x\n", err);
> >> + goto err_dma;
> >> + }
> >> + }
> >
> >Humm, i thought drivers were not supposed to do this. The architecture
> >code should be setting masks. But i've not followed all those
> >conversations...
> >
>
> Documentation/DMA-API-HOWTO.txt still states:
> " For correct operation, you must interrogate the kernel in your device
> probe routine to see if the DMA controller on the machine can properly
> support the DMA addressing limitation your device has. It is good
> style to do this even if your device holds the default setting, [...]"
O.K, thanks for the reference.
Andrew
Re: [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions
On Mon, Nov 19, 2018 at 03:57:17PM +0100, Andreas Schwab wrote: > On Okt 08 2018, Quentin Schulz wrote: > > > The Microsemi PHYs have multiple banks of registers (called pages). > > Registers can only be accessed from one page, if we need a register from > > another page, we need to switch the page and the registers of all other > > pages are not accessible anymore. > > > > Basically, to read register 5 from page 0, 1, 2, etc., you do the same > > phy_read(phydev, 5); but you need to set the desired page beforehand. > > > > In order to guarantee that two concurrent functions do not change the > > page, we need to do some locking per page. This can be achieved with the > > use of phy_select_page and phy_restore_page functions but phy_write/read > > calls in-between those two functions shall be replaced by their > > lock-free alternative __phy_write/read. > > > > Let's migrate this driver to those functions. > > This has some serious locking problem. Hi Andreas Could you be more specific. Are you getting a deadlock? A WARN_ON? Thanks, Andrew
Re: [PATCH net-next] MAINTAINERS: Add myself as third phylib maintainer
On Mon, Nov 19, 2018 at 08:01:03AM +0100, Heiner Kallweit wrote: > Add myself as third phylib maintainer. > > Signed-off-by: Heiner Kallweit Acked-by: Andrew Lunn Andrew
Re: DSA support for Marvell 88e6065 switch
> If I wanted it to work, what do I need to do? AFAICT phy autoprobing
> should just attach it as soon as it is compiled in?
Nope. It is a switch, not a PHY. Switches are never auto-probed
because they are not guaranteed to have ID registers.
You need to use the legacy device tree binding. Look in
Documentation/devicetree/bindings/net/dsa/dsa.txt, section Deprecated
Binding. You can get more examples if you checkout old kernels. Or
kirkwood-rd88f6281.dtsi, the dsa { } node which is disabled.
Andrew
Re: DSA support for Marvell 88e6065 switch
On Sun, Nov 18, 2018 at 07:07:12PM +0100, Pavel Machek wrote: > On Thu 2018-11-15 21:26:18, Andrew Lunn wrote: > > On Thu, Nov 15, 2018 at 08:51:11PM +0100, Pavel Machek wrote: > > > Hi! > > > > > > I'm trying to create support for Marvell 88e6065 switch... and it > > > seems like drivers/net/dsa supports everything, but this model. > > > > > > Did someone work with this hardware before? Any idea if it would be > > > more suitable to support by existing 88e6060 code, or if 88e6xxx code > > > should serve as a base? > > > > Hi Pavel > > > > The 88e6xxx should be extended to support this. I think you will find > > a lot of the building blocks are already in the driver. Compare the > > various implementations of the functions in the mv88e6xxx_ops to what > > the datasheet says for the registers, and pick those that match. > > Ok, so I played a bit. > > It looks like e6065 has different register layout from those supported > by 6xxx, and is quite similar to e6060. Hi Pavel However, if you look in the mv88e6xxx, there are quite a few functions called mv88e6065_foo. Marvell keeps changing the register layout. When writing code, we try to name the functions based on which family of devices introduced those registers. But we don't have the whole history, so we probably have some names wrong. > I understand how 88e6xxx code is supposed to work... but I don't > understand how e6060 code is supposed to be probed. It does not seem > to have device tree support. It seems to be older code, but is way > simpler, and seems to be targetted at similar hardware. The e6060 code is really old, pretty much abandoned. To make it usable, you are going to have to make a lot of changes. Turn it into an mdio driver, add device tree, make it use the new dsa2.c API, etc. I still think you should be looking at the mv88e6xxx driver, but maybe taking bits of code from the 6060 driver and adding it to the mv88e6xxx driver as needed. Andrew
Re: Linux kernel hangs if using RV1108 with MSZ8863 switch with two ports connected
> The kernel starts booting normally and then hangs like this when two > Ethernet cables are connected to the KSZ8863 switch: > http://dark-code.bulix.org/3xexu5-507563 > > This has the lock detection, inside kernel hacking, enabled. Maybe turn on all the hung-task debug and magic key support. With magic key, you might be able to get a backtrace of where it is spinning. Maybe also add #define DEBUG at the top of drivers/net/phy/phy.c. Does it hang during a PHY state transition? Maybe both PHYs are interrupting at the same time, and the interrupt code is broken? Maybe look at the switch driver and see if there is any code which is executed on link up. Put some printk() in there. If you PHYs are using interrupt mode, maybe disable that and use polling. Do you know if this ever worked properly before? If you know when it did work, you can do a git bisect to narrow it down to the one patch which broke it.. Basically, at the moment, you just need to try lots of things, to narrow it down. Andrew
Re: [PATCH RFC net-next] net: SAIL based FIB lookup for XDP
> > + * The router can have upto 255 ports. This limitation > > + * allows us to represent netdev_index as an u8 > > + */ > > +#define NETDEV_COUNT_MAX 255 > > ... 255 is high for a physical port count but not a logical device > count. In the presence of VLANs 255 is nothing and VLANs are an > essential deployment feature. I agree with David here. Doing some network simulation work using namespaces, i've had more than 255 devices in the root namespace. Andrew
Re: Linux kernel hangs if using RV1108 with MSZ8863 switch with two ports connected
On Fri, Nov 16, 2018 at 04:28:29PM -0200, Otavio Salvador wrote:
> Hi,
>
> I have a custom design based on Rockchip RV1108 that uses an MSZ8863
> switch running kernel 4.19.
>
> The dts part is as follows:
>
> {
> pinctrl-names = "default";
> pinctrl-0 = <_pins>;
> snps,reset-gpio = < RK_PC1 GPIO_ACTIVE_LOW>;
> snps,reset-active-low;
> clock_in_out = "output";
> status = "okay";
> };
>
> RV1108 GMAC is connected to KSZ8863 port 3 and after kernel boots, I
> can put an Ethernet cable from my router to the uplink port of
> KSZ8863, which makes the RV1108 board and a Linux PC connected to the
> other KSZ8863 port to both get IP addresses.
>
> So in this usecase the setup is working fine.
>
> However, if the RV1108 board boots with both Ethernet cables to the
> KSZ8863 switch connected, then the kernel silently hangs.
Hi Otavio
By silently, you mean it prints nothing at all?
I would try building the kernel with all the lock debugging turned
on. That might find something even with your working case, if there is
a potential deadlock.
If the kernel dies very early, you might need to enable "kernel
low-level debugping print and EARLY_PRINTK, in order to see anything.
Andrew
Re: DSA support for Marvell 88e6065 switch
On Thu, Nov 15, 2018 at 08:51:11PM +0100, Pavel Machek wrote: > Hi! > > I'm trying to create support for Marvell 88e6065 switch... and it > seems like drivers/net/dsa supports everything, but this model. > > Did someone work with this hardware before? Any idea if it would be > more suitable to support by existing 88e6060 code, or if 88e6xxx code > should serve as a base? Hi Pavel The 88e6xxx should be extended to support this. I think you will find a lot of the building blocks are already in the driver. Compare the various implementations of the functions in the mv88e6xxx_ops to what the datasheet says for the registers, and pick those that match. Andrew
[PATCHv2 net-net] net: dsa: mv88e6xxx: Work around mv886e6161 SERDES missing MII_PHYSID2
We already have a workaround for a couple of switches whose internal
PHYs only have the Marvel OUI, but no model number. We detect such
PHYs and give them the 6390 ID as the model number. However the
mv88e6161 has two SERDES interfaces in the same address range as its
internal PHYs. These suffer from the same problem, the Marvell OUI,
but no model number. As a result, these SERDES interfaces were getting
the same PHY ID as the mv88e6390, even though they are not PHYs, and
the Marvell PHY driver was trying to drive them.
Add a special case to stop this from happen.
Reported-by: Chris Healy
Signed-off-by: Andrew Lunn
---
v2:
grammar fix in commit message.
PHYS->PHYs
---
drivers/net/dsa/mv88e6xxx/chip.c | 21 -
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index fc0f508879d4..b603f8d6ee3e 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2524,11 +2524,22 @@ static int mv88e6xxx_mdio_read(struct mii_bus *bus, int
phy, int reg)
mutex_unlock(>reg_lock);
if (reg == MII_PHYSID2) {
- /* Some internal PHYS don't have a model number. Use
-* the mv88e6390 family model number instead.
-*/
- if (!(val & 0x3f0))
- val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4;
+ /* Some internal PHYs don't have a model number. */
+ if (chip->info->family != MV88E6XXX_FAMILY_6165)
+ /* Then there is the 6165 family. It gets is
+* PHYs correct. But it can also have two
+* SERDES interfaces in the PHY address
+* space. And these don't have a model
+* number. But they are not PHYs, so we don't
+* want to give them something a PHY driver
+* will recognise.
+*
+* Use the mv88e6390 family model number
+* instead, for anything which really could be
+* a PHY,
+*/
+ if (!(val & 0x3f0))
+ val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4;
}
return err ? err : val;
--
2.19.1
Re: [PATCH net-next] net: phy: switch to lockdep_assert_held in phylib
On Sun, Nov 11, 2018 at 10:33:08PM +0100, Heiner Kallweit wrote:
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 2e59a8419..5cb06f021 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -541,7 +541,7 @@ int __mdiobus_read(struct mii_bus *bus, int addr, u32
> regnum)
> {
> int retval;
>
> - WARN_ON_ONCE(!mutex_is_locked(>mdio_lock));
> + lockdep_assert_held_once(>mdio_lock);
Hi Heiner
I don't think there is a clear right/wrong here. This is not hot path
code. The cost for checking the lock is held is very small compared to
the actual MDIO transaction. So i don't think we really need to
optimise this. I do sometimes build with lockdep on, but not
always. So it is good to know when locking is broken on normal builds.
Florian, what do you think?
Andrew
Re: [PATCH net-next] net: phy: icplus: add config_intr callback
On Sun, Nov 11, 2018 at 09:49:12PM +0100, Heiner Kallweit wrote: > Move IRQ configuration for IP101A/G from config_init to config_intr > callback. Reasons: > > 1. This allows phylib to disable interrupts if needed. > 2. Icplus was the only driver supporting interrupts w/o defining a >config_intr callback. Now we can add a phylib plausibility check >disabling interrupt mode if one of the two irq-related callbacks >isn't defined. > > I don't own hardware with this PHY, and the change is based on the > datasheet for IP101A LF (which is supposed to be register-compatible > with IP101A/G). Change is compile-tested only. Hi Heiner This looks sensible. Thanks for fixing up this driver. Reviewed-by: Andrew Lunn Andrew
Re: [RFC PATCH 1/3] acpi: Add acpi mdio support code
On Thu, Nov 08, 2018 at 03:22:16PM +0800, Wang Dongsheng wrote:
> Add support for parsing the ACPI data node for PHY devices on an MDIO bus.
> The current implementation depend on mdio bus scan.
> With _DSD device properties we can finally do this:
>
> Device (MDIO) {
> Name (_DSD, Package () {
> ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
> Package () { Package () { "ethernet-phy@0", PHY0 }, }
> })
> Name (PHY0, Package() {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () { Package () { "reg", 0x0 }, }
> })
> }
>
> Device (MACO) {
> Name (_DSD, Package () {
> ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () { Package () { "phy-handle", \_SB.MDIO,
> "ethernet-phy@0" }, }
> })
> }
>
> Documentations:
> The DT "phy-handle" binding that we reuse for ACPI is documented in
> Documentation/devicetree/bindings/phy/phy-bindings.txt
>
> Documentation/acpi/dsd/data-node-references.txt
> Documentation/acpi/dsd/graph.txt
>
> Signed-off-by: Wang Dongsheng
> ---
> drivers/acpi/Kconfig | 6 ++
> drivers/acpi/Makefile | 1 +
> drivers/acpi/acpi_mdio.c | 167 +
> drivers/net/phy/mdio_bus.c | 3 +
> include/linux/acpi_mdio.h | 82 ++
> 5 files changed, 259 insertions(+)
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 9705fc986da9..0fefa3410ce9 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -252,6 +252,12 @@ config ACPI_PROCESSOR_IDLE
> config ACPI_MCFG
> bool
>
> +config ACPI_MDIO
> + def_tristate PHYLIB
> + depends on PHYLIB
> + help
> + ACPI MDIO bus (Ethernet PHY) accessors
> +
> config ACPI_CPPC_LIB
> bool
> depends on ACPI_PROCESSOR
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 6d59aa109a91..ec7461a064fc 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -41,6 +41,7 @@ acpi-y += ec.o
> acpi-$(CONFIG_ACPI_DOCK) += dock.o
> acpi-y += pci_root.o pci_link.o pci_irq.o
> obj-$(CONFIG_ACPI_MCFG) += pci_mcfg.o
> +acpi-$(CONFIG_ACPI_MDIO) += acpi_mdio.o
> acpi-y += acpi_lpss.o acpi_apd.o
> acpi-y += acpi_platform.o
> acpi-y += acpi_pnp.o
> diff --git a/drivers/acpi/acpi_mdio.c b/drivers/acpi/acpi_mdio.c
> new file mode 100644
> index ..293bf9a63197
> --- /dev/null
> +++ b/drivers/acpi/acpi_mdio.c
> @@ -0,0 +1,167 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +// Lots of code in this file is copy from drivers/of/of_mdio.c
> +// Copyright (c) 2018 Huaxintong Semiconductor Technology Co., Ltd.
I agree with Rafael here. We should try to refactor and combine this
code. And where possible, we should try to add a uniform fwnode_ API
which underneath either uses OF or ACPI, depending on the type of the
node. We already have fwnode_get_mac_address(), fwmode_irq_get(),
fwnode_get_phy_mode(), so where possible, PHYs should be no different.
Andrew
Re: [RFC PATCH 0/3] acpi: Add acpi mdio support code
> > I'm just trying to ensure whatever is defined is flexible enough that > > we really can later support everything which DT does. We have PHYs on > > MDIO busses, inside switches, which are on MDIO busses, which are > > inside Ethernet interfaces, etc. > > I think it can be satisfied. See the table I'm using above. Hi Dongsheng Since i don't know anything better, i think i have to trust you have this correct. It would be good to document this, so that the next person who needs to add ACPI support for a PHY has some documentation to look at. Could you add something to Documentation/acpi/dsd? Andrew
Re: [RFC PATCH 2/6] phy: armada38x: add common phy support
> +static int a38x_comphy_poll(struct a38x_comphy_lane *lane,
> + unsigned int offset, u32 mask, u32 value)
> +{
> + unsigned int timeout = 10;
> + u32 val;
> +
> + while (1) {
> + val = readl_relaxed(lane->base + offset);
> + if ((val & mask) == value)
> + return 0;
> + if (!timeout--)
> + break;
> + udelay(10);
> + }
Hi Russell
Maybe use one of the readx_poll_timeout() variants?
Andrew
[PATCH net-next] net: dsa: mv88e6xxx: Work around mv886e6161 SERDES missing MII_PHYSID2
We already have a workaround for a couple of switches whose internal
PHYs only have the Marvel OUI, but no model number. We detect such
PHYs and give them the 6390 ID as the model number. However the
mv88e6161 has two SERDES interfaces in the same address range as its
internal PHYs. These suffer from the same problem, the Marvell OUI,
but no model number. As a result, these SERDES interfaces were getting
the same PHY ID as the mv88e6390, even though they are not PHYs, and
the Marvell PHY driver was trying to drive them.
Add a special case to stop this happen.
Reported-by: Chris Healy
Signed-off-by: Andrew Lunn
---
drivers/net/dsa/mv88e6xxx/chip.c | 21 -
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index fc0f508879d4..4906a4f39d62 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2524,11 +2524,22 @@ static int mv88e6xxx_mdio_read(struct mii_bus *bus, int
phy, int reg)
mutex_unlock(>reg_lock);
if (reg == MII_PHYSID2) {
- /* Some internal PHYS don't have a model number. Use
-* the mv88e6390 family model number instead.
-*/
- if (!(val & 0x3f0))
- val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4;
+ /* Some internal PHYS don't have a model number. */
+ if (chip->info->family != MV88E6XXX_FAMILY_6165)
+ /* Then there is the 6165 family. It gets is
+* PHYs correct. But it can also have two
+* SERDES interfaces in the PHY address
+* space. And these don't have a model
+* number. But they are not PHYs, so we don't
+* want to give them something a PHY driver
+* will recognise.
+*
+* Use the mv88e6390 family model number
+* instead, for anything which really could be
+* a PHY,
+*/
+ if (!(val & 0x3f0))
+ val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4;
}
return err ? err : val;
--
2.19.1
Re: Should the bridge learn from frames with link local destination MAC address?
> >Andrew, I agree with your analysis also. We have hit this problem too > >(and we have an internal bug tracking it). > >We have not acted on this so far because of the fear of breaking > >existing deployments. I am all for fixing this if there is a > >clean way. > > +1 and since this would be a new bridge boolean option I'd like to add one new > 64 bit option with mask for new boolean bridge options so we can avoid > increasing the max rtnl attr id for such options. Please let me know > if you plan to work on the new option or I can cook something. Hi Nik For the moment i made a hack, which is enough for my own personal use. I'm not too familiar with the bridge code and its netlink interface. I suspect you can implement this properly much quicker than i could. So i would prefer leaving it to you. But we can talk about this during LPC. Andrew
[patch net] net: dsa: mv88e6xxx: Fix clearing of stats counters
The mv88e6161 would sometime fail to probe with a timeout waiting for
the switch to complete an operation. This operation is supposed to
clear the statistics counters. However, due to a read/modify/write,
without the needed mask, the operation actually carried out was more
random, with invalid parameters, resulting in the switch not
responding. We need to preserve the histogram mode bits, so apply a
mask to keep them.
Reported-by: Chris Healy
Fixes: 40cff8fca9e3 ("net: dsa: mv88e6xxx: Fix stats histogram mode")
Signed-off-by: Andrew Lunn
---
drivers/net/dsa/mv88e6xxx/global1.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/dsa/mv88e6xxx/global1.c
b/drivers/net/dsa/mv88e6xxx/global1.c
index d721ccf7d8be..38e399e0f30e 100644
--- a/drivers/net/dsa/mv88e6xxx/global1.c
+++ b/drivers/net/dsa/mv88e6xxx/global1.c
@@ -567,6 +567,8 @@ int mv88e6xxx_g1_stats_clear(struct mv88e6xxx_chip *chip)
if (err)
return err;
+ /* Keep the histogram mode bits */
+ val &= MV88E6XXX_G1_STATS_OP_HIST_RX_TX;
val |= MV88E6XXX_G1_STATS_OP_BUSY | MV88E6XXX_G1_STATS_OP_FLUSH_ALL;
err = mv88e6xxx_g1_write(chip, MV88E6XXX_G1_STATS_OP, val);
--
2.19.1
[PATCH net-next 4/4] net: dsa: mv88e6xxx: Add support for SERDES on ports 2-8 for 6390X
The 6390X family has 8 SERDES interfaces. When ports 9 and 10 are not
using all their SERDES interfaces, the unused ones can be assigned to
ports 2-8. Add support for interrupts from SERDES interfaces connected
to these lower ports.
Signed-off-by: Andrew Lunn
---
drivers/net/dsa/mv88e6xxx/chip.c | 8
drivers/net/dsa/mv88e6xxx/serdes.c | 26 +++---
drivers/net/dsa/mv88e6xxx/serdes.h | 2 ++
3 files changed, 25 insertions(+), 11 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 733bb137efbf..fc0f508879d4 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3293,8 +3293,8 @@ static const struct mv88e6xxx_ops mv88e6190x_ops = {
.vtu_getnext = mv88e6390_g1_vtu_getnext,
.vtu_loadpurge = mv88e6390_g1_vtu_loadpurge,
.serdes_power = mv88e6390x_serdes_power,
- .serdes_irq_setup = mv88e6390_serdes_irq_setup,
- .serdes_irq_free = mv88e6390_serdes_irq_free,
+ .serdes_irq_setup = mv88e6390x_serdes_irq_setup,
+ .serdes_irq_free = mv88e6390x_serdes_irq_free,
.gpio_ops = _gpio_ops,
.phylink_validate = mv88e6390x_phylink_validate,
};
@@ -3780,8 +3780,8 @@ static const struct mv88e6xxx_ops mv88e6390x_ops = {
.vtu_getnext = mv88e6390_g1_vtu_getnext,
.vtu_loadpurge = mv88e6390_g1_vtu_loadpurge,
.serdes_power = mv88e6390x_serdes_power,
- .serdes_irq_setup = mv88e6390_serdes_irq_setup,
- .serdes_irq_free = mv88e6390_serdes_irq_free,
+ .serdes_irq_setup = mv88e6390x_serdes_irq_setup,
+ .serdes_irq_free = mv88e6390x_serdes_irq_free,
.gpio_ops = _gpio_ops,
.avb_ops = _avb_ops,
.ptp_ops = _ptp_ops,
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c
b/drivers/net/dsa/mv88e6xxx/serdes.c
index bb69650ff772..2caa8c8b4b55 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -619,15 +619,11 @@ static irqreturn_t mv88e6390_serdes_thread_fn(int irq,
void *dev_id)
return ret;
}
-int mv88e6390_serdes_irq_setup(struct mv88e6xxx_chip *chip, int port)
+int mv88e6390x_serdes_irq_setup(struct mv88e6xxx_chip *chip, int port)
{
int lane;
int err;
- /* Only support ports 9 and 10 at the moment */
- if (port < 9)
- return 0;
-
lane = mv88e6390x_serdes_get_lane(chip, port);
if (lane == -ENODEV)
@@ -663,11 +659,19 @@ int mv88e6390_serdes_irq_setup(struct mv88e6xxx_chip
*chip, int port)
return mv88e6390_serdes_irq_enable(chip, port, lane);
}
-void mv88e6390_serdes_irq_free(struct mv88e6xxx_chip *chip, int port)
+int mv88e6390_serdes_irq_setup(struct mv88e6xxx_chip *chip, int port)
+{
+ if (port < 9)
+ return 0;
+
+ return mv88e6390_serdes_irq_setup(chip, port);
+}
+
+void mv88e6390x_serdes_irq_free(struct mv88e6xxx_chip *chip, int port)
{
int lane = mv88e6390x_serdes_get_lane(chip, port);
- if (port < 9)
+ if (lane == -ENODEV)
return;
if (lane < 0)
@@ -685,6 +689,14 @@ void mv88e6390_serdes_irq_free(struct mv88e6xxx_chip
*chip, int port)
chip->ports[port].serdes_irq = 0;
}
+void mv88e6390_serdes_irq_free(struct mv88e6xxx_chip *chip, int port)
+{
+ if (port < 9)
+ return;
+
+ mv88e6390x_serdes_irq_free(chip, port);
+}
+
int mv88e6341_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on)
{
u8 cmode = chip->ports[port].cmode;
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.h
b/drivers/net/dsa/mv88e6xxx/serdes.h
index 7870c5a9ef12..573dce8b1eb4 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.h
+++ b/drivers/net/dsa/mv88e6xxx/serdes.h
@@ -77,6 +77,8 @@ int mv88e6390_serdes_power(struct mv88e6xxx_chip *chip, int
port, bool on);
int mv88e6390x_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on);
int mv88e6390_serdes_irq_setup(struct mv88e6xxx_chip *chip, int port);
void mv88e6390_serdes_irq_free(struct mv88e6xxx_chip *chip, int port);
+int mv88e6390x_serdes_irq_setup(struct mv88e6xxx_chip *chip, int port);
+void mv88e6390x_serdes_irq_free(struct mv88e6xxx_chip *chip, int port);
int mv88e6352_serdes_get_sset_count(struct mv88e6xxx_chip *chip, int port);
int mv88e6352_serdes_get_strings(struct mv88e6xxx_chip *chip,
int port, uint8_t *data);
--
2.19.1
[PATCH net-next 0/4] net: dsa: mv88e6xxx: Support more SERDES interfacxes
Currently the SERDES interfaces for ports 9 and 10 on the mv88e6390x are supported, allowing upto 10G. However, when unused, these SERDES interfaces can be used by some of the lower ports for 1000Base-X. The tricky bit here is ordering. The SERDES have to become free from ports 9 or 10 before they can be used with lower ports. Normally, this would happen only when these ports would be configured up, which is too late. So at probe time, defaulting ports 9 and 10 to 1000BaseX frees them for use with lower ports. If they are actually needed, they will be taken back when port 9 and 10 goes up. Andrew Lunn (4): net: dsa: mv88e6xxx: Group cmode ops together net: dsa: mv88e6xxx: Differentiate between 6390 and 6390X cmodes net: dsa: mv88e6xxx: Default ports 9/10 6390X CMODE to 1000BaseX net: dsa: mv88e6xxx: Add support for SERDES on ports 2-8 for 6390X drivers/net/dsa/mv88e6xxx/chip.c | 17 ++--- drivers/net/dsa/mv88e6xxx/port.c | 24 +--- drivers/net/dsa/mv88e6xxx/port.h | 2 ++ drivers/net/dsa/mv88e6xxx/serdes.c | 26 +++--- drivers/net/dsa/mv88e6xxx/serdes.h | 2 ++ 5 files changed, 54 insertions(+), 17 deletions(-) -- 2.19.1
