linux-leds.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Andrew Lunn <andrew@lunn.ch>
To: "Marek Behún" <kabel@kernel.org>
Cc: Ansuel Smith <ansuelsmth@gmail.com>,
	Vivien Didelot <vivien.didelot@gmail.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Vladimir Oltean <olteanv@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Jonathan Corbet <corbet@lwn.net>, Pavel Machek <pavel@ucw.cz>,
	John Crispin <john@phrozen.org>,
	netdev@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
	linux-leds@vger.kernel.org
Subject: Re: [RFC PATCH v2 1/5] leds: trigger: add API for HW offloading of triggers
Date: Mon, 8 Nov 2021 20:53:36 +0100	[thread overview]
Message-ID: <YYmAQDIBGxPXCNff@lunn.ch> (raw)
In-Reply-To: <20211108185637.21b63d40@thinkpad>

> I guess I will have to work on this again ASAP or we will end up with
> solution that I don't like.
> 
> Nonetheless, what is your opinion about offloading netdev trigger vs
> introducing another trigger?

It is a solution that fits the general pattern, do it in software, and
offload it if possible.

However, i'm not sure the software solution actually works very well.
At least for switches. The two DSA drivers which implement
get_stats64() simply copy the cached statistics. The XRS700X updates
its cached values every 3000ms. The ar9331 is the same. Those are the
only two switch drivers which implement get_stats64 and none implement
get_stats. There was also was an issue that get_stats64() cannot
perform blocking calls. I don't remember if that was fixed, but if
not, get_stats64() is going to be pretty useless on switches.

We also need to handle drivers which don't actually implement
dev_get_stats(). That probably means only supporting offloads, all
modes which cannot be offloaded need to be rejected. This is pretty
much the same case of software control of the LEDs is not possible.
Unfortunately, dev_get_stats() does not return -EOPNOTSUPP, you need
to look at dev->netdev_ops->ndo_get_stats64 and
dev->netdev_ops->ndo_get_stats.

Are you working on Marvell switches? Have you implemented
get_stats64() for mv88e6xxx? How often do you poll the hardware for
the stats?

Given this, i think we need to bias the API so that it very likely
ends up offloading, if offloading is available.

     Andrew


  reply	other threads:[~2021-11-08 19:53 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-08  0:24 [RFC PATCH v2 0/5] Adds support for PHY LEDs with offload triggers Ansuel Smith
2021-11-08  0:24 ` [RFC PATCH v2 1/5] leds: trigger: add API for HW offloading of triggers Ansuel Smith
2021-11-08  2:29   ` Randy Dunlap
2021-11-08 14:04   ` Andrew Lunn
2021-11-08 15:16     ` Ansuel Smith
2021-11-08 16:13       ` Marek Behún
2021-11-08 16:46         ` Ansuel Smith
2021-11-08 17:35           ` Marek Behún
2021-11-08 17:58             ` Ansuel Smith
2021-11-08 18:41               ` Marek Behún
2021-11-08 19:08                 ` Ansuel Smith
2021-11-08 19:17                   ` Marek Behún
2021-11-08 17:46         ` Andrew Lunn
2021-11-08 17:56           ` Marek Behún
2021-11-08 19:53             ` Andrew Lunn [this message]
2021-11-08 20:03               ` Vladimir Oltean
2021-11-08 20:11               ` Marek Behún
2021-11-08 20:15                 ` Ansuel Smith
2021-11-08 17:37       ` Andrew Lunn
2021-11-08  0:24 ` [RFC PATCH v2 2/5] leds: add function to configure offload leds Ansuel Smith
2021-11-08  2:22   ` Randy Dunlap
2021-11-08  0:24 ` [RFC PATCH v2 3/5] leds: trigger: add offload-phy-activity trigger Ansuel Smith
2021-11-08  2:24   ` Randy Dunlap
2021-11-08 14:17   ` Andrew Lunn
2021-11-08 15:19     ` Ansuel Smith
2021-11-08  0:24 ` [RFC PATCH v2 4/5] net: dsa: qca8k: add LEDs support Ansuel Smith
2021-11-08  2:26   ` Randy Dunlap
2021-11-08  0:25 ` [RFC PATCH v2 5/5] dt-bindings: net: dsa: qca8k: add LEDs definition example Ansuel Smith

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=YYmAQDIBGxPXCNff@lunn.ch \
    --to=andrew@lunn.ch \
    --cc=ansuelsmth@gmail.com \
    --cc=corbet@lwn.net \
    --cc=davem@davemloft.net \
    --cc=devicetree@vger.kernel.org \
    --cc=f.fainelli@gmail.com \
    --cc=john@phrozen.org \
    --cc=kabel@kernel.org \
    --cc=kuba@kernel.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=olteanv@gmail.com \
    --cc=pavel@ucw.cz \
    --cc=robh+dt@kernel.org \
    --cc=vivien.didelot@gmail.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).