Re: [PATCH net-next 2/2] net: phy: Add ability to debug RGMII connections
> I don't think you are following the big picture of what I am saying. I > was trying to follow Florian's intention (first make sure I understand > it) and suggest that the FCS checking code in the patch he submitted > is not doing what it was intended to. I am getting apparent FCS > mismatches reported by the program, when I know full well that the MAC > I am testing on would have dropped those frames were they really > invalid. I think this FCS check is not needed. If we feed the MAC random data, something like 1 in 65535 will have a valid FCS and get passed up. I've not seen this happen with Ethernet, but i have seen other network technologies wrong decoding noise on the line and passing up frames with around 1 in 65536 probability. But then having the correct Ethertype is another 1 in 65536. So it seem pretty improbably we do receiver a packet in this method which is bad. So i would say, any packet received here is a good packet, and indicate the RGMII mode works. If we don't receive a packet, the mode is very probably bad. Andrew
Re: [PATCH net-next 2/2] net: phy: Add ability to debug RGMII connections
On Fri, 18 Oct 2019 at 16:54, Russell King - ARM Linux admin wrote: > > On Fri, Oct 18, 2019 at 04:37:55PM +0300, Vladimir Oltean wrote: > > On Fri, 18 Oct 2019 at 16:23, Russell King - ARM Linux admin > > wrote: > > > > > > On Fri, Oct 18, 2019 at 04:09:30PM +0300, Vladimir Oltean wrote: > > > > Hi Andrew, > > > > > > > > On Fri, 18 Oct 2019 at 16:01, Andrew Lunn wrote: > > > > > > > > > > > Well, that's the tricky part. You're sending a frame out, with no > > > > > > guarantee you'll get the same frame back in. So I'm not sure that > > > > > > any > > > > > > identifiers put inside the frame will survive. > > > > > > How do the tests pan out for you? Do you actually get to trigger > > > > > > this > > > > > > check? As I mentioned, my NIC drops the frames with bad FCS. > > > > > > > > > > My experience is, the NIC drops the frame and increments some the > > > > > counter about bad FCS. I do very occasionally see a frame delivered, > > > > > but i guess that is 1/65536 where the FCS just happens to be good by > > > > > accident. So i think some other algorithm should be used which is > > > > > unlikely to be good when the FCS is accidentally good, or just check > > > > > the contents of the packet, you know what is should contain. > > > > > > > > > > Are there any NICs which don't do hardware FCS? Is that something we > > > > > realistically need to consider? > > > > > > > > > > > Yes, but remember, nobody guarantees that a frame with DMAC > > > > > > ff:ff:ff:ff:ff:ff on egress will still have it on its way back. > > > > > > Again, > > > > > > this all depends on how you plan to manage the rx-all ethtool > > > > > > feature. > > > > > > > > > > Humm. Never heard that before. Are you saying some NICs rewrite the > > > > > DMAN? > > > > > > > > > > > > > I'm just trying to understand the circumstances under which this > > > > kernel thread makes sense. > > > > Checking for FCS validity means that the intention was to enable the > > > > reception of frames with bad FCS. > > > > Bad FCS after bad RGMII setup/hold times doesn't mean there's a small > > > > guy in there who rewrites the checksum. It means that frame octets get > > > > garbled. All octets are just as likely to get garbled, including the > > > > SFD, preamble, DMAC, etc. > > > > All I'm saying is that, if the intention of the patch is to actually > > > > process the FCS of frames before and after, then it should actually > > > > put the interface in promiscuous mode, so that frames with a > > > > non-garbled SFD and preamble can still be received, even though their > > > > DMAC was the one that got garbled. > > > > > > Isn't the point of this to see which RGMII setting results in a working > > > setup? > > > > > > So, is it not true that what we're after is receiving a _correct_ frame > > > that corresponds to the frame that was sent out? > > > > > > > Only true if the MAC does not drop bad frames by itself. Then the FCS > > check in the kernel thread is superfluous. > > If a MAC driver doesn't drop bad frames, then surely it's buggy, since > there isn't (afaik) a way of marking a received skb with a FCS error. > Therefore, forwarding frames with bad FCS into the Linux networking > stack will allow the reception of bad frames as if they were good. > > All the network drivers I've looked at (and written), when encountering > a packet with an error, update the statistic counters and drop the > errored packet. > > Do you know of any that don't? > I don't think you are following the big picture of what I am saying. I was trying to follow Florian's intention (first make sure I understand it) and suggest that the FCS checking code in the patch he submitted is not doing what it was intended to. I am getting apparent FCS mismatches reported by the program, when I know full well that the MAC I am testing on would have dropped those frames were they really invalid. We aren't saying anything in contradiction. > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up > According to speedtest.net: 11.9Mbps down 500kbps up Thanks, -Vladimir
Re: [PATCH net-next 2/2] net: phy: Add ability to debug RGMII connections
On Fri, Oct 18, 2019 at 04:37:55PM +0300, Vladimir Oltean wrote: > On Fri, 18 Oct 2019 at 16:23, Russell King - ARM Linux admin > wrote: > > > > On Fri, Oct 18, 2019 at 04:09:30PM +0300, Vladimir Oltean wrote: > > > Hi Andrew, > > > > > > On Fri, 18 Oct 2019 at 16:01, Andrew Lunn wrote: > > > > > > > > > Well, that's the tricky part. You're sending a frame out, with no > > > > > guarantee you'll get the same frame back in. So I'm not sure that any > > > > > identifiers put inside the frame will survive. > > > > > How do the tests pan out for you? Do you actually get to trigger this > > > > > check? As I mentioned, my NIC drops the frames with bad FCS. > > > > > > > > My experience is, the NIC drops the frame and increments some the > > > > counter about bad FCS. I do very occasionally see a frame delivered, > > > > but i guess that is 1/65536 where the FCS just happens to be good by > > > > accident. So i think some other algorithm should be used which is > > > > unlikely to be good when the FCS is accidentally good, or just check > > > > the contents of the packet, you know what is should contain. > > > > > > > > Are there any NICs which don't do hardware FCS? Is that something we > > > > realistically need to consider? > > > > > > > > > Yes, but remember, nobody guarantees that a frame with DMAC > > > > > ff:ff:ff:ff:ff:ff on egress will still have it on its way back. Again, > > > > > this all depends on how you plan to manage the rx-all ethtool feature. > > > > > > > > Humm. Never heard that before. Are you saying some NICs rewrite the > > > > DMAN? > > > > > > > > > > I'm just trying to understand the circumstances under which this > > > kernel thread makes sense. > > > Checking for FCS validity means that the intention was to enable the > > > reception of frames with bad FCS. > > > Bad FCS after bad RGMII setup/hold times doesn't mean there's a small > > > guy in there who rewrites the checksum. It means that frame octets get > > > garbled. All octets are just as likely to get garbled, including the > > > SFD, preamble, DMAC, etc. > > > All I'm saying is that, if the intention of the patch is to actually > > > process the FCS of frames before and after, then it should actually > > > put the interface in promiscuous mode, so that frames with a > > > non-garbled SFD and preamble can still be received, even though their > > > DMAC was the one that got garbled. > > > > Isn't the point of this to see which RGMII setting results in a working > > setup? > > > > So, is it not true that what we're after is receiving a _correct_ frame > > that corresponds to the frame that was sent out? > > > > Only true if the MAC does not drop bad frames by itself. Then the FCS > check in the kernel thread is superfluous. If a MAC driver doesn't drop bad frames, then surely it's buggy, since there isn't (afaik) a way of marking a received skb with a FCS error. Therefore, forwarding frames with bad FCS into the Linux networking stack will allow the reception of bad frames as if they were good. All the network drivers I've looked at (and written), when encountering a packet with an error, update the statistic counters and drop the errored packet. Do you know of any that don't? -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
Re: [PATCH net-next 2/2] net: phy: Add ability to debug RGMII connections
On Fri, 18 Oct 2019 at 16:23, Russell King - ARM Linux admin wrote: > > On Fri, Oct 18, 2019 at 04:09:30PM +0300, Vladimir Oltean wrote: > > Hi Andrew, > > > > On Fri, 18 Oct 2019 at 16:01, Andrew Lunn wrote: > > > > > > > Well, that's the tricky part. You're sending a frame out, with no > > > > guarantee you'll get the same frame back in. So I'm not sure that any > > > > identifiers put inside the frame will survive. > > > > How do the tests pan out for you? Do you actually get to trigger this > > > > check? As I mentioned, my NIC drops the frames with bad FCS. > > > > > > My experience is, the NIC drops the frame and increments some the > > > counter about bad FCS. I do very occasionally see a frame delivered, > > > but i guess that is 1/65536 where the FCS just happens to be good by > > > accident. So i think some other algorithm should be used which is > > > unlikely to be good when the FCS is accidentally good, or just check > > > the contents of the packet, you know what is should contain. > > > > > > Are there any NICs which don't do hardware FCS? Is that something we > > > realistically need to consider? > > > > > > > Yes, but remember, nobody guarantees that a frame with DMAC > > > > ff:ff:ff:ff:ff:ff on egress will still have it on its way back. Again, > > > > this all depends on how you plan to manage the rx-all ethtool feature. > > > > > > Humm. Never heard that before. Are you saying some NICs rewrite the > > > DMAN? > > > > > > > I'm just trying to understand the circumstances under which this > > kernel thread makes sense. > > Checking for FCS validity means that the intention was to enable the > > reception of frames with bad FCS. > > Bad FCS after bad RGMII setup/hold times doesn't mean there's a small > > guy in there who rewrites the checksum. It means that frame octets get > > garbled. All octets are just as likely to get garbled, including the > > SFD, preamble, DMAC, etc. > > All I'm saying is that, if the intention of the patch is to actually > > process the FCS of frames before and after, then it should actually > > put the interface in promiscuous mode, so that frames with a > > non-garbled SFD and preamble can still be received, even though their > > DMAC was the one that got garbled. > > Isn't the point of this to see which RGMII setting results in a working > setup? > > So, is it not true that what we're after is receiving a _correct_ frame > that corresponds to the frame that was sent out? > Only true if the MAC does not drop bad frames by itself. Then the FCS check in the kernel thread is superfluous. > Hence, if the DMAC got changed, it's irrelevent whether we received the > packet or not - since "no packet" || "changed packet" = fail. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up > According to speedtest.net: 11.9Mbps down 500kbps up
Re: [PATCH net-next 2/2] net: phy: Add ability to debug RGMII connections
On Fri, Oct 18, 2019 at 04:09:30PM +0300, Vladimir Oltean wrote: > Hi Andrew, > > On Fri, 18 Oct 2019 at 16:01, Andrew Lunn wrote: > > > > > Well, that's the tricky part. You're sending a frame out, with no > > > guarantee you'll get the same frame back in. So I'm not sure that any > > > identifiers put inside the frame will survive. > > > How do the tests pan out for you? Do you actually get to trigger this > > > check? As I mentioned, my NIC drops the frames with bad FCS. > > > > My experience is, the NIC drops the frame and increments some the > > counter about bad FCS. I do very occasionally see a frame delivered, > > but i guess that is 1/65536 where the FCS just happens to be good by > > accident. So i think some other algorithm should be used which is > > unlikely to be good when the FCS is accidentally good, or just check > > the contents of the packet, you know what is should contain. > > > > Are there any NICs which don't do hardware FCS? Is that something we > > realistically need to consider? > > > > > Yes, but remember, nobody guarantees that a frame with DMAC > > > ff:ff:ff:ff:ff:ff on egress will still have it on its way back. Again, > > > this all depends on how you plan to manage the rx-all ethtool feature. > > > > Humm. Never heard that before. Are you saying some NICs rewrite the > > DMAN? > > > > I'm just trying to understand the circumstances under which this > kernel thread makes sense. > Checking for FCS validity means that the intention was to enable the > reception of frames with bad FCS. > Bad FCS after bad RGMII setup/hold times doesn't mean there's a small > guy in there who rewrites the checksum. It means that frame octets get > garbled. All octets are just as likely to get garbled, including the > SFD, preamble, DMAC, etc. > All I'm saying is that, if the intention of the patch is to actually > process the FCS of frames before and after, then it should actually > put the interface in promiscuous mode, so that frames with a > non-garbled SFD and preamble can still be received, even though their > DMAC was the one that got garbled. Isn't the point of this to see which RGMII setting results in a working setup? So, is it not true that what we're after is receiving a _correct_ frame that corresponds to the frame that was sent out? Hence, if the DMAC got changed, it's irrelevent whether we received the packet or not - since "no packet" || "changed packet" = fail. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
Re: [PATCH net-next 2/2] net: phy: Add ability to debug RGMII connections
Hi Andrew, On Fri, 18 Oct 2019 at 16:01, Andrew Lunn wrote: > > > Well, that's the tricky part. You're sending a frame out, with no > > guarantee you'll get the same frame back in. So I'm not sure that any > > identifiers put inside the frame will survive. > > How do the tests pan out for you? Do you actually get to trigger this > > check? As I mentioned, my NIC drops the frames with bad FCS. > > My experience is, the NIC drops the frame and increments some the > counter about bad FCS. I do very occasionally see a frame delivered, > but i guess that is 1/65536 where the FCS just happens to be good by > accident. So i think some other algorithm should be used which is > unlikely to be good when the FCS is accidentally good, or just check > the contents of the packet, you know what is should contain. > > Are there any NICs which don't do hardware FCS? Is that something we > realistically need to consider? > > > Yes, but remember, nobody guarantees that a frame with DMAC > > ff:ff:ff:ff:ff:ff on egress will still have it on its way back. Again, > > this all depends on how you plan to manage the rx-all ethtool feature. > > Humm. Never heard that before. Are you saying some NICs rewrite the > DMAN? > I'm just trying to understand the circumstances under which this kernel thread makes sense. Checking for FCS validity means that the intention was to enable the reception of frames with bad FCS. Bad FCS after bad RGMII setup/hold times doesn't mean there's a small guy in there who rewrites the checksum. It means that frame octets get garbled. All octets are just as likely to get garbled, including the SFD, preamble, DMAC, etc. All I'm saying is that, if the intention of the patch is to actually process the FCS of frames before and after, then it should actually put the interface in promiscuous mode, so that frames with a non-garbled SFD and preamble can still be received, even though their DMAC was the one that got garbled. > Andrew Thanks, -Vladimir
Re: [PATCH net-next 2/2] net: phy: Add ability to debug RGMII connections
> Well, that's the tricky part. You're sending a frame out, with no > guarantee you'll get the same frame back in. So I'm not sure that any > identifiers put inside the frame will survive. > How do the tests pan out for you? Do you actually get to trigger this > check? As I mentioned, my NIC drops the frames with bad FCS. My experience is, the NIC drops the frame and increments some the counter about bad FCS. I do very occasionally see a frame delivered, but i guess that is 1/65536 where the FCS just happens to be good by accident. So i think some other algorithm should be used which is unlikely to be good when the FCS is accidentally good, or just check the contents of the packet, you know what is should contain. Are there any NICs which don't do hardware FCS? Is that something we realistically need to consider? > Yes, but remember, nobody guarantees that a frame with DMAC > ff:ff:ff:ff:ff:ff on egress will still have it on its way back. Again, > this all depends on how you plan to manage the rx-all ethtool feature. Humm. Never heard that before. Are you saying some NICs rewrite the DMAN? Andrew
Re: [PATCH net-next 2/2] net: phy: Add ability to debug RGMII connections
On Fri, 18 Oct 2019 at 01:22, Florian Fainelli wrote:
>
>
>
> On 10/17/2019 3:06 PM, Vladimir Oltean wrote:
> >> +static int phy_rgmii_debug_rcv(struct sk_buff *skb, struct net_device
> >> *dev,
> >> + struct packet_type *pt, struct net_device *unused)
> >> +{
> >> +struct phy_rgmii_debug_priv *priv = pt->af_packet_priv;
> >> +u32 fcs;
> >> +
> >> +/* If we receive something, the Ethernet header was valid and so was
> >> + * the Ethernet type, so to re-calculate the FCS we need to undo
> >> what
> >> + * eth_type_trans() just did.
> >> + */
> >> +if (!__skb_push(skb, ETH_HLEN))
> >> +return 0;
> >
> > Why would this return NULL?
> I don't think it can, good point.
>
> >
> >> +
> >> +fcs = phy_rgmii_probe_skb_fcs(skb);
> >> +if (skb->len != priv->skb->len || fcs != priv->fcs) {
> >
> > I feel like this logic is broken. How do you know that this skb is that
> > skb? Everybody else can still enqueue to the netdev, right?
>
> That is true, so I could be defeated by someone sending an Ethernet
> Frame with a 0xdada ethernet type through, e.g.: raw sockets, good point.
>
Well, that's the tricky part. You're sending a frame out, with no
guarantee you'll get the same frame back in. So I'm not sure that any
identifiers put inside the frame will survive.
How do the tests pan out for you? Do you actually get to trigger this
check? As I mentioned, my NIC drops the frames with bad FCS.
> >
> > Actually if I'm right about the FCS errors resulting in drops below,
> > then any news here is good news, no need to even compare the FCS of two
> > frames which you don't know whether they're in fact one and the same.
>
> FCS is a bit overstated here, although it actually is what the HW would
> generate/verify but the point was really that if you have a RGMII issue
> you may very well end-up with two packets instead of one, because of the
> clock/data misalignment.
>
> >
> >> +print_hex_dump(KERN_INFO, "RX probe skb: ",
> >> + DUMP_PREFIX_OFFSET, 16, 1, skb->data, 32,
> >> + false);
> >> +netdev_warn(dev, "Calculated FCS: 0x%08x expected: 0x%08x\n",
> >> +fcs, priv->fcs);
> >> +} else {
> >> +priv->rcv_ok = 1;
> >> +}
> >> +
> >> +complete(&priv->compl);
> >> +
> >> +return 0;
> >> +}
> >> +
> >> +static int phy_rgmii_trigger_config(struct phy_device *phydev,
> >> +phy_interface_t interface)
> >> +{
> >> +int ret = 0;
> >> +
> >> +/* Configure the interface mode to be tested */
> >> +phydev->interface = interface;
> >> +
> >> +/* Forcibly run the fixups and config_init() */
> >> +ret = phy_init_hw(phydev);
> >> +if (ret) {
> >> +phydev_err(phydev, "phy_init_hw failed: %d\n", ret);
> >> +return ret;
> >> +}
> >> +
> >> +/* Some PHY drivers configure RGMII delays in their config_aneg()
> >> + * callback, so make sure we run through those as well.
> >> + */
> >> +ret = phy_start_aneg(phydev);
> >> +if (ret) {
> >> +phydev_err(phydev, "phy_start_aneg failed: %d\n", ret);
> >> +return ret;
> >> +}
> >> +
> >> +/* Put back in loopback mode since phy_init_hw() may have issued
> >> + * a software reset.
> >> + */
> >> +ret = phy_loopback(phydev, true);
> >> +if (ret)
> >> +phydev_err(phydev, "phy_loopback failed: %d\n", ret);
> >> +
> >> +return ret;
> >> +}
> >> +
> >> +static void phy_rgmii_probe_xmit_work(struct work_struct *work)
> >> +{
> >> +struct phy_rgmii_debug_priv *priv;
> >> +
> >> +priv = container_of(work, struct phy_rgmii_debug_priv, work);
> >> +
> >> +dev_queue_xmit(priv->skb);
> >
> > Oops, you just lost ownership of priv->skb here. Anything happening
> > further is in a race with the netdev driver. You need to hold a
> > reference to it with skb_get().
>
> Doh, yes, thanks!
>
> >
> >> +}
> >> +
> >> +static int phy_rgmii_prepare_probe(struct phy_rgmii_debug_priv *priv)
> >> +{
> >> +struct phy_device *phydev = priv->phydev;
> >> +struct net_device *ndev = phydev->attached_dev;
> >> +struct sk_buff *skb;
> >> +int ret;
> >> +
> >> +skb = netdev_alloc_skb(ndev, ndev->mtu);
> >> +if (!skb)
> >> +return -ENOMEM;
> >> +
> >> +priv->skb = skb;
> >
> > Could you assign priv->skb at the end, not here? This way you won't risk
> > leaking a freed pointer into priv->skb if eth_header below fails.
>
> Makes sense.
>
> >
> >> +skb->dev = ndev;
> >> +skb_put(skb, ndev->mtu);
> >> +memset(skb->data, 0xaa, skb->len);
> >> +
> >
> > I think you need to do something like this before skb_put:
> >
> > + skb->protocol = htons(ETH_P_EDSA);
> > + skb_reset_network_header(skb);
> > + skb_reset_transport_header(skb);
> >
> > Otherwise I get a lot of these errors on a bridged net device:
> >
> > [ 142.919783] protocol is buggy, dev swp2
> > [ 142.924436] protocol is b
Re: [PATCH net-next 2/2] net: phy: Add ability to debug RGMII connections
On 10/17/2019 3:06 PM, Vladimir Oltean wrote:
>> +static int phy_rgmii_debug_rcv(struct sk_buff *skb, struct net_device
>> *dev,
>> + struct packet_type *pt, struct net_device *unused)
>> +{
>> + struct phy_rgmii_debug_priv *priv = pt->af_packet_priv;
>> + u32 fcs;
>> +
>> + /* If we receive something, the Ethernet header was valid and so was
>> + * the Ethernet type, so to re-calculate the FCS we need to undo
>> what
>> + * eth_type_trans() just did.
>> + */
>> + if (!__skb_push(skb, ETH_HLEN))
>> + return 0;
>
> Why would this return NULL?
I don't think it can, good point.
>
>> +
>> + fcs = phy_rgmii_probe_skb_fcs(skb);
>> + if (skb->len != priv->skb->len || fcs != priv->fcs) {
>
> I feel like this logic is broken. How do you know that this skb is that
> skb? Everybody else can still enqueue to the netdev, right?
That is true, so I could be defeated by someone sending an Ethernet
Frame with a 0xdada ethernet type through, e.g.: raw sockets, good point.
>
> Actually if I'm right about the FCS errors resulting in drops below,
> then any news here is good news, no need to even compare the FCS of two
> frames which you don't know whether they're in fact one and the same.
FCS is a bit overstated here, although it actually is what the HW would
generate/verify but the point was really that if you have a RGMII issue
you may very well end-up with two packets instead of one, because of the
clock/data misalignment.
>
>> + print_hex_dump(KERN_INFO, "RX probe skb: ",
>> + DUMP_PREFIX_OFFSET, 16, 1, skb->data, 32,
>> + false);
>> + netdev_warn(dev, "Calculated FCS: 0x%08x expected: 0x%08x\n",
>> + fcs, priv->fcs);
>> + } else {
>> + priv->rcv_ok = 1;
>> + }
>> +
>> + complete(&priv->compl);
>> +
>> + return 0;
>> +}
>> +
>> +static int phy_rgmii_trigger_config(struct phy_device *phydev,
>> + phy_interface_t interface)
>> +{
>> + int ret = 0;
>> +
>> + /* Configure the interface mode to be tested */
>> + phydev->interface = interface;
>> +
>> + /* Forcibly run the fixups and config_init() */
>> + ret = phy_init_hw(phydev);
>> + if (ret) {
>> + phydev_err(phydev, "phy_init_hw failed: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + /* Some PHY drivers configure RGMII delays in their config_aneg()
>> + * callback, so make sure we run through those as well.
>> + */
>> + ret = phy_start_aneg(phydev);
>> + if (ret) {
>> + phydev_err(phydev, "phy_start_aneg failed: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + /* Put back in loopback mode since phy_init_hw() may have issued
>> + * a software reset.
>> + */
>> + ret = phy_loopback(phydev, true);
>> + if (ret)
>> + phydev_err(phydev, "phy_loopback failed: %d\n", ret);
>> +
>> + return ret;
>> +}
>> +
>> +static void phy_rgmii_probe_xmit_work(struct work_struct *work)
>> +{
>> + struct phy_rgmii_debug_priv *priv;
>> +
>> + priv = container_of(work, struct phy_rgmii_debug_priv, work);
>> +
>> + dev_queue_xmit(priv->skb);
>
> Oops, you just lost ownership of priv->skb here. Anything happening
> further is in a race with the netdev driver. You need to hold a
> reference to it with skb_get().
Doh, yes, thanks!
>
>> +}
>> +
>> +static int phy_rgmii_prepare_probe(struct phy_rgmii_debug_priv *priv)
>> +{
>> + struct phy_device *phydev = priv->phydev;
>> + struct net_device *ndev = phydev->attached_dev;
>> + struct sk_buff *skb;
>> + int ret;
>> +
>> + skb = netdev_alloc_skb(ndev, ndev->mtu);
>> + if (!skb)
>> + return -ENOMEM;
>> +
>> + priv->skb = skb;
>
> Could you assign priv->skb at the end, not here? This way you won't risk
> leaking a freed pointer into priv->skb if eth_header below fails.
Makes sense.
>
>> + skb->dev = ndev;
>> + skb_put(skb, ndev->mtu);
>> + memset(skb->data, 0xaa, skb->len);
>> +
>
> I think you need to do something like this before skb_put:
>
> + skb->protocol = htons(ETH_P_EDSA);
> + skb_reset_network_header(skb);
> + skb_reset_transport_header(skb);
>
> Otherwise I get a lot of these errors on a bridged net device:
>
> [ 142.919783] protocol is buggy, dev swp2
> [ 142.924436] protocol is buggy, dev eth2
>
>> + /* Build the header */
>> + ret = eth_header(skb, ndev, ETH_P_EDSA, ndev->dev_addr,
>> + NULL, ndev->mtu);
>
> A switch net device will complain about having SMAC == DMAC and drop the
> frame. Don't you want to send broadcast frames here?
Yes, that makes sense, if you do not have broadcast in your network
filter, your network adapter is not great use.
>
>> + if (ret != ETH_HLEN) {
>> + kfree_skb(skb);
>> + return -EINVAL;
>> + }
>> +
>> + priv->fcs = phy_rgmii_probe_skb_fcs(skb);
>> +
>
> I'm far from a checksumming expert, but if the FCS was invalid, would
Re: [PATCH net-next 2/2] net: phy: Add ability to debug RGMII connections
Hi Florian, [sorry, I started writing this before you sent out a v2] On 10/16/19 1:49 AM, Florian Fainelli wrote: RGMII connections are always troublesome because of the need to add delays between the RX or TX clocks and data lines. This can lead to a fair amount of breakage that upsets users. Introduce a new sysfs write only attribute which can be set to 1 to instruct the PHY library to attempt to probe what the correct RGMII phy_interface_t value should be. When such debugging is requested, the PHY library will do a number of checks whether this debugging is even necessary (RGMII used, Gigabit, not a Generic PHY driver etc.) and if successfull will proceed with: - putting the PHY in loopback mode - register a packet handler with an unused Ethernet type value in the kernel (ETH_P_EDSA is a well known unused value) - re-configure the PHY and MAC with the phy_interface_t value to be tried, which is one of the 4 possible interfaces, starting with the currently defined one - craft a MTU sized packet with the Ethernet interface's MAC SA and DA set to itself and send that in a process context - wait for that packet to be received through the registered packet_type handler If the packet is not received, we have a problem with the RGMII interface, if we do receive something, we check the FCS we calculated on transmit matches the FCS of the packet received, if we have a mismatch it most likely will not. If all is well, we stop iterating over all possible RGMII combinations and offer the one that is deemed suitable which is what an user should be trying by configuring the platform appropriately. This is not deemed to be 100% fool proof, but given how much support getting RGMII seems to involved, this seemed like a good tool to have users self-diagnose their connection issues. Future improvements could be made in order to extrapolate from a specific frame patter the type of skewing between RXC/TXC that is going on so as to refine the search process, and even possibly suggest delays. The function phy_rgmii_debug_probe() could also be used by an Ethernet controller during its selftests routines instead of open-coding that part. Signed-off-by: Florian Fainelli --- .../ABI/testing/sysfs-class-net-phydev| 11 + drivers/net/phy/Kconfig | 9 + drivers/net/phy/Makefile | 1 + drivers/net/phy/phy-rgmii-debug.c | 269 ++ drivers/net/phy/phy_device.c | 31 ++ include/linux/phy.h | 9 + 6 files changed, 330 insertions(+) create mode 100644 drivers/net/phy/phy-rgmii-debug.c diff --git a/Documentation/ABI/testing/sysfs-class-net-phydev b/Documentation/ABI/testing/sysfs-class-net-phydev index 206cbf538b59..989fc128ec94 100644 --- a/Documentation/ABI/testing/sysfs-class-net-phydev +++ b/Documentation/ABI/testing/sysfs-class-net-phydev @@ -49,3 +49,14 @@ Description: Boolean value indicating whether the PHY device is used in standalone mode, without a net_device associated, by PHYLINK. Attribute created only when this is the case. + +What: /sys/class/mdio_bus///phy_rgmii_debug +Date: October 2019 +KernelVersion: 5.5 +Contact: net...@vger.kernel.org +Description: + Write only attribute used to trigger the debugging of RGMII + connections. Upon writing, this will either return success + and print to kernel console the correct phy_interface value + or an error will be returned. See CONFIG_PHY_RGMII_DEBUG for + details. diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index fe602648b99f..e5b54627d426 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -248,6 +248,15 @@ config LED_TRIGGER_PHY Mbps OR Gbps OR link for any speed known to the PHY. +config PHY_RGMII_DEBUG + bool "Support debugging of RGMII connections" + ---help--- + This enables support for troubleshooting RGMII connections by + making use of the PHY devices standard loopback feature in order to + probe the correct RGMII connection. + + If unsure, say N here. + comment "MII PHY device drivers" diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile index a03437e091f3..1d9fddf83f6c 100644 --- a/drivers/net/phy/Makefile +++ b/drivers/net/phy/Makefile @@ -18,6 +18,7 @@ obj-$(CONFIG_MDIO_DEVICE) += mdio-bus.o endif libphy-$(CONFIG_SWPHY)+= swphy.o libphy-$(CONFIG_LED_TRIGGER_PHY) += phy_led_triggers.o +libphy-$(CONFIG_PHY_RGMII_DEBUG) += phy-rgmii-debug.o obj-$(CONFIG_PHYLINK) += phylink.o obj-$(CONFIG_PHYLIB) += libphy.o diff --git a/drivers/net/phy/phy-rgmii-debug.c b/drivers/net/phy/phy-rgmii-debug.c new file mode 100644 index ..f66ce8bc942c --- /dev/null +++ b/drivers/n
Re: [PATCH net-next 2/2] net: phy: Add ability to debug RGMII connections
> If all is well, we stop iterating over all possible RGMII combinations
> and offer the one that is deemed suitable which is what an user should
> be trying by configuring the platform appropriately.
Hi Florian
I like the idea, however...
I think it would be good to always iterate over all possible delay
modes, not until it works. We want to try to catch PHY drivers which
don't implement all four possible settings. If two or more delay modes
work, it suggests something is wrong in the implementation, not all
modes are supported correctly.
> +int phy_rgmii_debug_probe(struct phy_device *phydev)
> +{
> + struct net_device *ndev = phydev->attached_dev;
> + unsigned char operstate = ndev->operstate;
> + phy_interface_t rgmii_modes[4] = {
> + PHY_INTERFACE_MODE_RGMII,
> + PHY_INTERFACE_MODE_RGMII_ID,
> + PHY_INTERFACE_MODE_RGMII_RXID,
> + PHY_INTERFACE_MODE_RGMII_TXID
> + };
> + struct phy_rgmii_debug_priv *priv;
> + unsigned int i, count;
> + int ret;
> +
> + ret = phy_rgmii_can_debug(phydev);
> + if (ret <= 0)
> + return ret;
> +
> + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + if (phy_rgmii_probes_type.af_packet_priv)
> + return -EBUSY;
> +
> + phy_rgmii_probes_type.af_packet_priv = priv;
> + priv->phydev = phydev;
> + INIT_WORK(&priv->work, phy_rgmii_probe_xmit_work);
> + init_completion(&priv->compl);
> +
> + /* We are now testing this network device */
> + ndev->operstate = IF_OPER_TESTING;
I should dig out and re-submit my patch set of doing this.
> +
> + dev_add_pack(&phy_rgmii_probes_type);
> +
> + /* Determine where to start */
> + for (i = 0; i < ARRAY_SIZE(rgmii_modes); i++) {
> + if (phydev->interface == rgmii_modes[i])
> + break;
> + }
I don't think that is a good idea. What if setting the RGMII delay is
a NOP, and relying on strapping? We will never know we have the wrong
mode in DT, because it works. I would much prefer we used all four
modes, all four modes pass, and then we know the driver is broken.
Andrew
RE: [PATCH net-next 2/2] net: phy: Add ability to debug RGMII connections
From: Florian Fainelli
Date: Oct/15/2019, 23:49:53 (UTC+00:00)
> The function phy_rgmii_debug_probe() could also be used by an Ethernet
> controller during its selftests routines instead of open-coding that
> part.
I can add it to stmmac selftests then :)
> +int phy_rgmii_debug_probe(struct phy_device *phydev)
> +{
> + struct net_device *ndev = phydev->attached_dev;
> + unsigned char operstate = ndev->operstate;
> + phy_interface_t rgmii_modes[4] = {
This can be static.
> + PHY_INTERFACE_MODE_RGMII,
> + PHY_INTERFACE_MODE_RGMII_ID,
> + PHY_INTERFACE_MODE_RGMII_RXID,
> + PHY_INTERFACE_MODE_RGMII_TXID
> + };
> + struct phy_rgmii_debug_priv *priv;
> + unsigned int i, count;
> + int ret;
> +
> + ret = phy_rgmii_can_debug(phydev);
> + if (ret <= 0)
> + return ret;
> +
> + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + if (phy_rgmii_probes_type.af_packet_priv)
> + return -EBUSY;
You are leaking "priv" here.
> +
> + phy_rgmii_probes_type.af_packet_priv = priv;
> + priv->phydev = phydev;
> + INIT_WORK(&priv->work, phy_rgmii_probe_xmit_work);
> + init_completion(&priv->compl);
> +
> + /* We are now testing this network device */
> + ndev->operstate = IF_OPER_TESTING;
> +
> + dev_add_pack(&phy_rgmii_probes_type);
> +
> + /* Determine where to start */
> + for (i = 0; i < ARRAY_SIZE(rgmii_modes); i++) {
> + if (phydev->interface == rgmii_modes[i])
> + break;
> + }
> +
> + /* Now probe all modes */
> + for (count = 0; count < ARRAY_SIZE(rgmii_modes); count++) {
> + ret = phy_rgmii_probe_interface(priv, rgmii_modes[i]);
> + if (ret == 0) {
> + netdev_info(ndev, "Determined \"%s\" to be correct\n",
> + phy_modes(rgmii_modes[i]));
> + break;
> + }
> + i = (i + 1) % ARRAY_SIZE(rgmii_modes);
> + }
> +
> + dev_remove_pack(&phy_rgmii_probes_type);
> + kfree(priv);
> + phy_rgmii_probes_type.af_packet_priv = NULL;
I think you should set af_packet_priv to NULL before freeing "priv"
because of the "if ([...].af_packet_priv)" test, otherwise you can get
use-after-free.
---
Thanks,
Jose Miguel Abreu
