From: Michal Kubecek <mkubecek@suse.cz>
To: netdev@vger.kernel.org
Cc: Shannon Nelson <snelson@pensando.io>
Subject: Re: [PATCH net-next 13/18] ionic: Add initial ethtool support
Date: Fri, 21 Jun 2019 04:32:05 +0200 [thread overview]
Message-ID: <20190621023205.GD21796@unicorn.suse.cz> (raw)
In-Reply-To: <20190620202424.23215-14-snelson@pensando.io>
On Thu, Jun 20, 2019 at 01:24:19PM -0700, Shannon Nelson wrote:
> Add in the basic ethtool callbacks for device information
> and control.
>
> Signed-off-by: Shannon Nelson <snelson@pensando.io>
> ---
...
> +static int ionic_get_link_ksettings(struct net_device *netdev,
> + struct ethtool_link_ksettings *ks)
> +{
> + struct lif *lif = netdev_priv(netdev);
> + struct ionic_dev *idev = &lif->ionic->idev;
> + int copper_seen = 0;
> +
> + ethtool_link_ksettings_zero_link_mode(ks, supported);
> + ethtool_link_ksettings_zero_link_mode(ks, advertising);
> +
> + if (ionic_is_mnic(lif->ionic)) {
> + ethtool_link_ksettings_add_link_mode(ks, supported, Backplane);
> + ethtool_link_ksettings_add_link_mode(ks, advertising, Backplane);
> + } else {
> + ethtool_link_ksettings_add_link_mode(ks, supported, FIBRE);
> + ethtool_link_ksettings_add_link_mode(ks, advertising, FIBRE);
> +
> + if (ionic_is_pf(lif->ionic)) {
> + ethtool_link_ksettings_add_link_mode(ks, supported,
> + Autoneg);
> + ethtool_link_ksettings_add_link_mode(ks, advertising,
> + Autoneg);
> + }
> + }
> +
> + switch (le16_to_cpu(idev->port_info->status.xcvr.pid)) {
> + /* Copper */
> + case XCVR_PID_QSFP_100G_CR4:
> + ethtool_link_ksettings_add_link_mode(ks, supported,
> + 100000baseCR4_Full);
> + ethtool_link_ksettings_add_link_mode(ks, advertising,
> + 100000baseCR4_Full);
> + copper_seen++;
> + break;
> + case XCVR_PID_QSFP_40GBASE_CR4:
> + ethtool_link_ksettings_add_link_mode(ks, supported,
> + 40000baseCR4_Full);
> + ethtool_link_ksettings_add_link_mode(ks, advertising,
> + 40000baseCR4_Full);
> + copper_seen++;
> + break;
> + case XCVR_PID_SFP_25GBASE_CR_S:
> + case XCVR_PID_SFP_25GBASE_CR_L:
> + case XCVR_PID_SFP_25GBASE_CR_N:
> + ethtool_link_ksettings_add_link_mode(ks, supported,
> + 25000baseCR_Full);
> + ethtool_link_ksettings_add_link_mode(ks, advertising,
> + 25000baseCR_Full);
> + copper_seen++;
> + break;
> + case XCVR_PID_SFP_10GBASE_AOC:
> + case XCVR_PID_SFP_10GBASE_CU:
> + ethtool_link_ksettings_add_link_mode(ks, supported,
> + 10000baseCR_Full);
> + ethtool_link_ksettings_add_link_mode(ks, advertising,
> + 10000baseCR_Full);
> + copper_seen++;
> + break;
> +
> + /* Fibre */
> + case XCVR_PID_QSFP_100G_SR4:
> + case XCVR_PID_QSFP_100G_AOC:
> + ethtool_link_ksettings_add_link_mode(ks, supported,
> + 100000baseSR4_Full);
> + ethtool_link_ksettings_add_link_mode(ks, advertising,
> + 100000baseSR4_Full);
> + break;
> + case XCVR_PID_QSFP_100G_LR4:
> + ethtool_link_ksettings_add_link_mode(ks, supported,
> + 100000baseLR4_ER4_Full);
> + ethtool_link_ksettings_add_link_mode(ks, advertising,
> + 100000baseLR4_ER4_Full);
> + break;
> + case XCVR_PID_QSFP_100G_ER4:
> + ethtool_link_ksettings_add_link_mode(ks, supported,
> + 100000baseLR4_ER4_Full);
> + ethtool_link_ksettings_add_link_mode(ks, advertising,
> + 100000baseLR4_ER4_Full);
> + break;
> + case XCVR_PID_QSFP_40GBASE_SR4:
> + case XCVR_PID_QSFP_40GBASE_AOC:
> + ethtool_link_ksettings_add_link_mode(ks, supported,
> + 40000baseSR4_Full);
> + ethtool_link_ksettings_add_link_mode(ks, advertising,
> + 40000baseSR4_Full);
> + break;
> + case XCVR_PID_QSFP_40GBASE_LR4:
> + ethtool_link_ksettings_add_link_mode(ks, supported,
> + 40000baseLR4_Full);
> + ethtool_link_ksettings_add_link_mode(ks, advertising,
> + 40000baseLR4_Full);
> + break;
> + case XCVR_PID_SFP_25GBASE_SR:
> + case XCVR_PID_SFP_25GBASE_AOC:
> + ethtool_link_ksettings_add_link_mode(ks, supported,
> + 25000baseSR_Full);
> + ethtool_link_ksettings_add_link_mode(ks, advertising,
> + 25000baseSR_Full);
> + break;
> + case XCVR_PID_SFP_10GBASE_SR:
> + ethtool_link_ksettings_add_link_mode(ks, supported,
> + 10000baseSR_Full);
> + ethtool_link_ksettings_add_link_mode(ks, advertising,
> + 10000baseSR_Full);
> + break;
> + case XCVR_PID_SFP_10GBASE_LR:
> + ethtool_link_ksettings_add_link_mode(ks, supported,
> + 10000baseLR_Full);
> + ethtool_link_ksettings_add_link_mode(ks, advertising,
> + 10000baseLR_Full);
> + break;
> + case XCVR_PID_SFP_10GBASE_LRM:
> + ethtool_link_ksettings_add_link_mode(ks, supported,
> + 10000baseLRM_Full);
> + ethtool_link_ksettings_add_link_mode(ks, advertising,
> + 10000baseLRM_Full);
> + break;
> + case XCVR_PID_SFP_10GBASE_ER:
> + ethtool_link_ksettings_add_link_mode(ks, supported,
> + 10000baseER_Full);
> + ethtool_link_ksettings_add_link_mode(ks, advertising,
> + 10000baseER_Full);
> + break;
> + case XCVR_PID_QSFP_100G_ACC:
> + case XCVR_PID_QSFP_40GBASE_ER4:
> + case XCVR_PID_SFP_25GBASE_LR:
> + case XCVR_PID_SFP_25GBASE_ER:
> + dev_info(lif->ionic->dev, "no decode bits for xcvr type pid=%d / 0x%x\n",
> + idev->port_info->status.xcvr.pid,
> + idev->port_info->status.xcvr.pid);
> + break;
Maybe you should rather add these modes so that they can be shown and
set.
> + case XCVR_PID_UNKNOWN:
> + break;
> + default:
> + dev_info(lif->ionic->dev, "unknown xcvr type pid=%d / 0x%x\n",
> + idev->port_info->status.xcvr.pid,
> + idev->port_info->status.xcvr.pid);
> + break;
> + }
Up to this point, you always set each bit in both supported and
advertised modes. Thus you could set the modes in only one of the
bitmaps and copy it to the other here.
> +
> + ethtool_link_ksettings_add_link_mode(ks, supported, Pause);
> + if (idev->port_info->config.pause_type)
> + ethtool_link_ksettings_add_link_mode(ks, advertising, Pause);
> +
> + if (idev->port_info->config.fec_type == PORT_FEC_TYPE_FC)
> + ethtool_link_ksettings_add_link_mode(ks, advertising, FEC_BASER);
> + else if (idev->port_info->config.fec_type == PORT_FEC_TYPE_RS)
> + ethtool_link_ksettings_add_link_mode(ks, advertising, FEC_RS);
> + else
> + ethtool_link_ksettings_add_link_mode(ks, advertising, FEC_NONE);
Is it correct to set these FEC bits only in advertising and not in
supported?
> +static int ionic_set_link_ksettings(struct net_device *netdev,
> + const struct ethtool_link_ksettings *ks)
> +{
> + struct lif *lif = netdev_priv(netdev);
> + struct ionic *ionic = lif->ionic;
> + struct ionic_dev *idev = &lif->ionic->idev;
> + u8 fec_type = PORT_FEC_TYPE_NONE;
> + u32 req_rs, req_b;
> + int err = 0;
> +
> + /* set autoneg */
> + if (ks->base.autoneg != idev->port_info->config.an_enable) {
> + idev->port_info->config.an_enable = ks->base.autoneg;
IMHO you should only save the value if the command below succeeds,
otherwise next time you will be comparing against the value which wasn't
actually set.
> + mutex_lock(&ionic->dev_cmd_lock);
> + ionic_dev_cmd_port_autoneg(idev, ks->base.autoneg);
> + err = ionic_dev_cmd_wait(ionic, devcmd_timeout);
> + mutex_unlock(&ionic->dev_cmd_lock);
> + if (err)
> + return err;
> + }
> +
> + /* set speed */
> + if (ks->base.speed != le32_to_cpu(idev->port_info->config.speed)) {
> + idev->port_info->config.speed = cpu_to_le32(ks->base.speed);
Same here.
> + mutex_lock(&ionic->dev_cmd_lock);
> + ionic_dev_cmd_port_speed(idev, ks->base.speed);
> + err = ionic_dev_cmd_wait(ionic, devcmd_timeout);
> + mutex_unlock(&ionic->dev_cmd_lock);
> + if (err)
> + return err;
> + }
> +
> + /* set FEC */
> + req_rs = ethtool_link_ksettings_test_link_mode(ks, advertising, FEC_RS);
> + req_b = ethtool_link_ksettings_test_link_mode(ks, advertising, FEC_BASER);
> + if (req_rs && req_b) {
> + netdev_info(netdev, "Only select one FEC mode at a time\n");
> + return -EINVAL;
> +
> + } else if (req_b &&
> + idev->port_info->config.fec_type != PORT_FEC_TYPE_FC) {
> + fec_type = PORT_FEC_TYPE_FC;
> + } else if (req_rs &&
> + idev->port_info->config.fec_type != PORT_FEC_TYPE_RS) {
> + fec_type = PORT_FEC_TYPE_RS;
> + } else if (!(req_rs | req_b) &&
> + idev->port_info->config.fec_type != PORT_FEC_TYPE_NONE) {
> + fec_type = PORT_FEC_TYPE_NONE;
> + }
AFAICS if userspace requests a mode which is already set, you end up
with fec_type = PORT_FEC_TYPE_NONE here. This doesn't seem right.
I assume you would rather want to skip the setting below in such case.
> +
> + idev->port_info->config.fec_type = fec_type;
> + mutex_lock(&ionic->dev_cmd_lock);
> + ionic_dev_cmd_port_fec(idev, PORT_FEC_TYPE_NONE);
Shouldn't the argument be fec_type here?
> + err = ionic_dev_cmd_wait(ionic, devcmd_timeout);
> + mutex_unlock(&ionic->dev_cmd_lock);
> + if (err)
> + return err;
> +
> + return 0;
> +}
...
> +static int ionic_set_ringparam(struct net_device *netdev,
> + struct ethtool_ringparam *ring)
> +{
> + struct lif *lif = netdev_priv(netdev);
> + bool running;
> + int i, j;
> +
> + if (ring->rx_mini_pending || ring->rx_jumbo_pending) {
> + netdev_info(netdev, "Changing jumbo or mini descriptors not supported\n");
> + return -EINVAL;
> + }
> +
> + i = ring->tx_pending & (ring->tx_pending - 1);
> + j = ring->rx_pending & (ring->rx_pending - 1);
> + if (i || j) {
> + netdev_info(netdev, "Descriptor count must be a power of 2\n");
> + return -EINVAL;
> + }
> +
> + if (ring->tx_pending > IONIC_MAX_TXRX_DESC ||
> + ring->tx_pending < IONIC_MIN_TXRX_DESC ||
> + ring->rx_pending > IONIC_MAX_TXRX_DESC ||
> + ring->rx_pending < IONIC_MIN_TXRX_DESC) {
> + netdev_info(netdev, "Descriptors count must be in the range [%d-%d]\n",
> + IONIC_MIN_TXRX_DESC, IONIC_MAX_TXRX_DESC);
> + return -EINVAL;
> + }
The upper bounds have been already checked in ethtool_set_ringparam() so
that the two conditions can never be satisfied here.
...
> +static int ionic_set_channels(struct net_device *netdev,
> + struct ethtool_channels *ch)
> +{
> + struct lif *lif = netdev_priv(netdev);
> + bool running;
> +
> + if (!ch->combined_count || ch->other_count ||
> + ch->rx_count || ch->tx_count)
> + return -EINVAL;
> +
> + if (ch->combined_count > lif->ionic->ntxqs_per_lif)
> + return -EINVAL;
This has been already checked in ethtool_set_channels().
Michal Kubecek
next prev parent reply other threads:[~2019-06-21 2:32 UTC|newest]
Thread overview: 63+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-20 20:24 [PATCH net-next 00/18] Add ionic driver Shannon Nelson
2019-06-20 20:24 ` [PATCH net-next 01/18] ionic: Add basic framework for IONIC Network device driver Shannon Nelson
2019-06-20 21:24 ` Andrew Lunn
2019-06-21 22:13 ` Shannon Nelson
2019-06-24 20:07 ` Jakub Kicinski
2019-06-24 21:54 ` Shannon Nelson
2019-06-24 20:03 ` Jakub Kicinski
2019-06-24 21:46 ` Shannon Nelson
2019-06-20 20:24 ` [PATCH net-next 02/18] ionic: Add hardware init and device commands Shannon Nelson
2019-06-20 21:54 ` Andrew Lunn
2019-06-21 22:22 ` Shannon Nelson
2019-06-24 20:13 ` Jakub Kicinski
2019-06-24 21:50 ` Shannon Nelson
2019-06-21 9:27 ` kbuild test robot
2019-06-21 9:27 ` [PATCH] ionic: fix simple_open.cocci warnings kbuild test robot
2019-06-21 15:42 ` Shannon Nelson
2019-06-21 13:03 ` [PATCH net-next 02/18] ionic: Add hardware init and device commands kbuild test robot
2019-06-24 20:53 ` Jakub Kicinski
2019-06-24 22:29 ` Shannon Nelson
2019-06-20 20:24 ` [PATCH net-next 03/18] ionic: Add port management commands Shannon Nelson
2019-06-20 20:24 ` [PATCH net-next 04/18] ionic: Add basic lif support Shannon Nelson
2019-06-20 20:24 ` [PATCH net-next 05/18] ionic: Add interrupts and doorbells Shannon Nelson
2019-06-20 20:24 ` [PATCH net-next 06/18] ionic: Add basic adminq support Shannon Nelson
2019-06-21 6:03 ` kbuild test robot
2019-06-20 20:24 ` [PATCH net-next 07/18] ionic: Add adminq action Shannon Nelson
2019-06-20 20:24 ` [PATCH net-next 08/18] ionic: Add notifyq support Shannon Nelson
2019-06-25 23:21 ` Jakub Kicinski
2019-06-26 15:26 ` Shannon Nelson
2019-06-20 20:24 ` [PATCH net-next 09/18] ionic: Add the basic NDO callbacks for netdev support Shannon Nelson
2019-06-25 23:27 ` Jakub Kicinski
2019-06-26 15:41 ` Shannon Nelson
2019-06-20 20:24 ` [PATCH net-next 10/18] ionic: Add management of rx filters Shannon Nelson
2019-06-25 23:37 ` Jakub Kicinski
2019-06-26 15:52 ` Shannon Nelson
2019-06-27 15:59 ` Shannon Nelson
2019-06-20 20:24 ` [PATCH net-next 11/18] ionic: Add Rx filter and rx_mode nod support Shannon Nelson
2019-06-21 10:30 ` kbuild test robot
2019-06-21 10:30 ` [PATCH] ionic: fix semicolon.cocci warnings kbuild test robot
2019-06-21 15:43 ` Shannon Nelson
2019-06-25 23:44 ` [PATCH net-next 11/18] ionic: Add Rx filter and rx_mode nod support Jakub Kicinski
2019-06-26 15:53 ` Shannon Nelson
2019-06-20 20:24 ` [PATCH net-next 12/18] ionic: Add async link status check and basic stats Shannon Nelson
2019-06-25 23:47 ` Jakub Kicinski
2019-06-26 15:54 ` Shannon Nelson
2019-06-20 20:24 ` [PATCH net-next 13/18] ionic: Add initial ethtool support Shannon Nelson
2019-06-21 2:32 ` Michal Kubecek [this message]
2019-06-21 22:30 ` Shannon Nelson
2019-06-24 7:26 ` Michal Kubecek
2019-06-24 21:44 ` Shannon Nelson
2019-06-25 23:54 ` Jakub Kicinski
2019-06-26 16:07 ` Shannon Nelson
2019-06-26 16:18 ` Jakub Kicinski
2019-06-20 20:24 ` [PATCH net-next 14/18] ionic: Add Tx and Rx handling Shannon Nelson
2019-06-26 0:08 ` Jakub Kicinski
2019-06-26 16:49 ` Shannon Nelson
2019-06-20 20:24 ` [PATCH net-next 15/18] ionic: Add netdev-event handling Shannon Nelson
2019-06-20 20:24 ` [PATCH net-next 16/18] ionic: Add driver stats Shannon Nelson
2019-06-20 20:24 ` [PATCH net-next 17/18] ionic: Add RSS support Shannon Nelson
2019-06-26 0:20 ` Jakub Kicinski
2019-06-26 17:04 ` Shannon Nelson
2019-06-20 20:24 ` [PATCH net-next 18/18] ionic: Add coalesce and other features Shannon Nelson
2019-06-24 20:19 ` [PATCH net-next 00/18] Add ionic driver Jakub Kicinski
2019-06-24 21:53 ` David Miller
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=20190621023205.GD21796@unicorn.suse.cz \
--to=mkubecek@suse.cz \
--cc=netdev@vger.kernel.org \
--cc=snelson@pensando.io \
/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).