devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Andrew Lunn <andrew@lunn.ch>
To: Bastien Curutchet <bastien.curutchet@bootlin.com>
Cc: "David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>, Pavel Machek <pavel@ucw.cz>,
	Lee Jones <lee@kernel.org>,
	Richard Cochran <richardcochran@gmail.com>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	Russell King <linux@armlinux.org.uk>,
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
	devicetree@vger.kernel.org, linux-leds@vger.kernel.org,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	herve.codina@bootlin.com, maxime.chevallier@bootlin.com,
	christophercordahi@nanometrics.ca
Subject: Re: [PATCH v2 2/6] leds: trigger: Create a new LED netdev trigger for collision
Date: Thu, 29 Feb 2024 16:17:47 +0100	[thread overview]
Message-ID: <9dd1b2d0-4ba5-4d34-a892-a6cc8c01df28@lunn.ch> (raw)
In-Reply-To: <e1936774-14bf-4ae5-9754-e21f5a0c59b3@bootlin.com>

> > How useful is collision? How did you test this? How did you cause
> > collisions to see if the LED actually worked?
> Indeed I am not able to generate collision on my setup so I did not test
> this
> collision part.
> My use case is that the hardware strap configuration that selects the LED
> output mode
> can not be trusted so I have to force configuration with software. I added
> this collision
> part because I wanted to cover all the LED configuration modes offered by
> the PHY.

There are a few things i want to avoid here:

1) Vendor SDK mentality. The hardware can do this, lets add a knob to
make use of it. We end up with 100 of configuration knobs which nobody
ever uses. Do you actually have a board where the strapping is wrong?
Are you going to submit a .dts file making use of this option?

2) LEDs are the wild west, because it is not part of 802.3. Every
vendor does it differently, and has their own special blinking
patterns. My preference is to keep it simple to what people actually
use. You cannot actually generate a collision, the developer who wants
to add support for collision. I have to ask, is collision actually
useful?

> > As far as i can see, this is just a normal 100Base-T PHY. Everybody
> > uses that point-to-point nowadays. If it was an 100Base-T1, with a
> > shared medium, good old CSMA/CD then collision might actually be
> > useful.
> > 
> > I also disagree with not having software fallback:
> > 
> > ip -s link show eth0
> > 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UP mode DEFAULT group default qlen 1000
> >      link/ether 80:ee:73:83:60:27 brd ff:ff:ff:ff:ff:ff
> >      RX:     bytes    packets errors dropped  missed   mcast
> >      4382213540983 2947876747      0       0       0  154890
> >      TX:     bytes    packets errors dropped carrier collsns
> >        18742773651  197507119      0       0       0       0
> > 
> > collsns = 0. The information is there in a standard format. However,
> > when did you last see it not 0?
> 
> Ok, I could add the software callback but I will not be able to test it ...

My personal experience is, anything not tested is broken...

Think about what Russell actually said. That should give you a clue
how to cause collisions. If not, go study history books about CSMA/CD.

   Andrew

  reply	other threads:[~2024-02-29 15:17 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-27  9:39 [PATCH v2 0/6] net: phy: Add TI's DP83640 device tree binding Bastien Curutchet
2024-02-27  9:39 ` [PATCH v2 1/6] dt-bindings: net: Add bindings for PHY DP83640 Bastien Curutchet
2024-02-28 11:37   ` Conor Dooley
2024-02-27  9:39 ` [PATCH v2 2/6] leds: trigger: Create a new LED netdev trigger for collision Bastien Curutchet
2024-02-27 16:03   ` Andrew Lunn
2024-02-27 16:26     ` Russell King (Oracle)
2024-02-29  7:24     ` Bastien Curutchet
2024-02-29 15:17       ` Andrew Lunn [this message]
2024-02-29 16:58         ` Russell King (Oracle)
2024-02-27  9:39 ` [PATCH v2 3/6] net: phy: DP83640: Add LED handling Bastien Curutchet
2024-02-27  9:58   ` Maxime Chevallier
2024-02-27 10:50     ` Russell King (Oracle)
2024-02-28 15:04   ` Andrew Lunn
2024-02-29  7:28     ` Bastien Curutchet
2024-02-27  9:39 ` [PATCH v2 4/6] net: phy: DP83640: Add EDPD management Bastien Curutchet
2024-02-27 10:02   ` Maxime Chevallier
2024-02-27  9:39 ` [PATCH v2 5/6] net: phy: DP83640: Explicitly disabling PHY Control Frames Bastien Curutchet
2024-02-27 10:08   ` Maxime Chevallier
2024-02-27  9:39 ` [PATCH v2 6/6] net: phy: DP83640: Add fiber mode enabling/disabling from device tree Bastien Curutchet
2024-02-27 11:01   ` Russell King (Oracle)
2024-02-27 16:18   ` Andrew Lunn
2024-02-29  7:31     ` Bastien Curutchet
2024-02-29 15:23       ` Andrew Lunn
2024-03-01 10:37         ` Maxime Chevallier
2024-03-01 14:00           ` Andrew Lunn

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=9dd1b2d0-4ba5-4d34-a892-a6cc8c01df28@lunn.ch \
    --to=andrew@lunn.ch \
    --cc=bastien.curutchet@bootlin.com \
    --cc=christophercordahi@nanometrics.ca \
    --cc=conor+dt@kernel.org \
    --cc=davem@davemloft.net \
    --cc=devicetree@vger.kernel.org \
    --cc=edumazet@google.com \
    --cc=herve.codina@bootlin.com \
    --cc=hkallweit1@gmail.com \
    --cc=krzysztof.kozlowski+dt@linaro.org \
    --cc=kuba@kernel.org \
    --cc=lee@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=maxime.chevallier@bootlin.com \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=pavel@ucw.cz \
    --cc=richardcochran@gmail.com \
    --cc=robh+dt@kernel.org \
    --cc=thomas.petazzoni@bootlin.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).