| Message ID | 20190708192532.27420-14-snelson@pensando.io |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | Add ionic driver | expand |
> +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); > + > + 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); > + copper_seen++; > + break; > + case XCVR_PID_QSFP_40GBASE_CR4: > + ethtool_link_ksettings_add_link_mode(ks, supported, > + 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); > + 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); > + 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); > + break; > + case XCVR_PID_QSFP_100G_LR4: > + ethtool_link_ksettings_add_link_mode(ks, supported, > + 100000baseLR4_ER4_Full); > + break; > + case XCVR_PID_QSFP_100G_ER4: > + ethtool_link_ksettings_add_link_mode(ks, supported, > + 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); > + break; > + case XCVR_PID_QSFP_40GBASE_LR4: > + ethtool_link_ksettings_add_link_mode(ks, supported, > + 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); > + break; > + case XCVR_PID_SFP_10GBASE_SR: > + ethtool_link_ksettings_add_link_mode(ks, supported, > + 10000baseSR_Full); > + break; > + case XCVR_PID_SFP_10GBASE_LR: > + ethtool_link_ksettings_add_link_mode(ks, supported, > + 10000baseLR_Full); > + break; > + case XCVR_PID_SFP_10GBASE_LRM: > + ethtool_link_ksettings_add_link_mode(ks, supported, > + 10000baseLRM_Full); > + break; > + case XCVR_PID_SFP_10GBASE_ER: > + ethtool_link_ksettings_add_link_mode(ks, supported, > + 10000baseER_Full); > + break; I don't know these link modes too well. But only setting a single bit seems odd. What i do know is that an SFP which supports 2500BaseX should also be able to support 1000BaseX. So should a 100G SFP also support 40G, 25G, 10G etc? The SERDES just runs a slower bitstream over the basic bitpipe? > + 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; Why not add them? > + memcpy(ks->link_modes.advertising, ks->link_modes.supported, > + sizeof(ks->link_modes.advertising)); bitmap_copy() would be a better way to do this. You could consider adding a helper to ethtool.h. Andrew
> +static int ionic_set_pauseparam(struct net_device *netdev, > + struct ethtool_pauseparam *pause) > +{ > + struct lif *lif = netdev_priv(netdev); > + struct ionic *ionic = lif->ionic; > + struct ionic_dev *idev = &lif->ionic->idev; > + > + u32 requested_pause; > + u32 cur_autoneg; > + int err; > + > + cur_autoneg = idev->port_info->config.an_enable ? AUTONEG_ENABLE : > + AUTONEG_DISABLE; > + if (pause->autoneg != cur_autoneg) { > + netdev_info(netdev, "Please use 'ethtool -s ...' to change autoneg\n"); > + return -EOPNOTSUPP; > + } > + > + /* change both at the same time */ > + requested_pause = PORT_PAUSE_TYPE_LINK; > + if (pause->rx_pause) > + requested_pause |= IONIC_PAUSE_F_RX; > + if (pause->tx_pause) > + requested_pause |= IONIC_PAUSE_F_TX; > + > + if (requested_pause == idev->port_info->config.pause_type) > + return 0; > + > + idev->port_info->config.pause_type = requested_pause; > + > + mutex_lock(&ionic->dev_cmd_lock); > + ionic_dev_cmd_port_pause(idev, requested_pause); > + err = ionic_dev_cmd_wait(ionic, devcmd_timeout); > + mutex_unlock(&ionic->dev_cmd_lock); > + if (err) > + return err; Hi Shannon I've no idea what the firmware black box is doing, but this looks wrong. pause->autoneg is about if the results of auto-neg should be used or not. If false, just configure the MAC with the pause settings and you are done. If the interface is being forced, so autoneg in general is disabled, just configure the MAC and you are done. If pause->autoneg is true and the interface is using auto-neg as a whole, you pass the pause values to the PHY for it to advertise and trigger an auto-neg. Once autoneg has completed, and the resolved settings are available, the MAC is configured with the resolved values. Looking at this code, i don't see any difference between configuring the MAC or configuring the PHY. I would expect pause->autoneg to be part of requested_pause somehow, so the firmware knows what is should do. Andrew
> +static int ionic_get_module_eeprom(struct net_device *netdev, > + struct ethtool_eeprom *ee, > + u8 *data) > +{ > + struct lif *lif = netdev_priv(netdev); > + struct ionic_dev *idev = &lif->ionic->idev; > + struct xcvr_status *xcvr; > + u32 len; > + > + /* copy the module bytes into data */ > + xcvr = &idev->port_info->status.xcvr; > + len = min_t(u32, sizeof(xcvr->sprom), ee->len); > + memcpy(data, xcvr->sprom, len); Hi Shannon This also looks odd. Where is the call into the firmware to get the eeprom contents? Even though it is called 'eeprom', the data is not static. It contains real time diagnostic values, temperature, transmit power, receiver power, voltages etc. > + > + dev_dbg(&lif->netdev->dev, "notifyblock eid=0x%llx link_status=0x%x link_speed=0x%x link_down_cnt=0x%x\n", > + lif->info->status.eid, > + lif->info->status.link_status, > + lif->info->status.link_speed, > + lif->info->status.link_down_count); > + dev_dbg(&lif->netdev->dev, " port_status id=0x%x status=0x%x speed=0x%x\n", > + idev->port_info->status.id, > + idev->port_info->status.status, > + idev->port_info->status.speed); > + dev_dbg(&lif->netdev->dev, " xcvr status state=0x%x phy=0x%x pid=0x%x\n", > + xcvr->state, xcvr->phy, xcvr->pid); > + dev_dbg(&lif->netdev->dev, " port_config state=0x%x speed=0x%x mtu=0x%x an_enable=0x%x fec_type=0x%x pause_type=0x%x loopback_mode=0x%x\n", > + idev->port_info->config.state, > + idev->port_info->config.speed, > + idev->port_info->config.mtu, > + idev->port_info->config.an_enable, > + idev->port_info->config.fec_type, > + idev->port_info->config.pause_type, > + idev->port_info->config.loopback_mode); It is a funny place to have all the debug code. Andrew
> +static int ionic_nway_reset(struct net_device *netdev) > +{ > + struct lif *lif = netdev_priv(netdev); > + int err = 0; > + > + if (netif_running(netdev)) > + err = ionic_reset_queues(lif); What does ionic_reset_queues() do? It sounds nothing like restarting auto negotiation? Andrew
On Tue, Jul 09, 2019 at 12:04:06AM +0200, Andrew Lunn wrote: > > +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); > > + > > + 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); > > + copper_seen++; > > + break; > > + case XCVR_PID_QSFP_40GBASE_CR4: > > + ethtool_link_ksettings_add_link_mode(ks, supported, > > + 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); > > + 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); > > + 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); > > + break; > > + case XCVR_PID_QSFP_100G_LR4: > > + ethtool_link_ksettings_add_link_mode(ks, supported, > > + 100000baseLR4_ER4_Full); > > + break; > > + case XCVR_PID_QSFP_100G_ER4: > > + ethtool_link_ksettings_add_link_mode(ks, supported, > > + 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); > > + break; > > + case XCVR_PID_QSFP_40GBASE_LR4: > > + ethtool_link_ksettings_add_link_mode(ks, supported, > > + 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); > > + break; > > + case XCVR_PID_SFP_10GBASE_SR: > > + ethtool_link_ksettings_add_link_mode(ks, supported, > > + 10000baseSR_Full); > > + break; > > + case XCVR_PID_SFP_10GBASE_LR: > > + ethtool_link_ksettings_add_link_mode(ks, supported, > > + 10000baseLR_Full); > > + break; > > + case XCVR_PID_SFP_10GBASE_LRM: > > + ethtool_link_ksettings_add_link_mode(ks, supported, > > + 10000baseLRM_Full); > > + break; > > + case XCVR_PID_SFP_10GBASE_ER: > > + ethtool_link_ksettings_add_link_mode(ks, supported, > > + 10000baseER_Full); > > + break; > > I don't know these link modes too well. But only setting a single bit > seems odd. What i do know is that an SFP which supports 2500BaseX > should also be able to support 1000BaseX. So should a 100G SFP also > support 40G, 25G, 10G etc? The SERDES just runs a slower bitstream > over the basic bitpipe? > > > + 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; > > Why not add them? > > > > + memcpy(ks->link_modes.advertising, ks->link_modes.supported, > > + sizeof(ks->link_modes.advertising)); > > bitmap_copy() would be a better way to do this. You could consider > adding a helper to ethtool.h. Also, there is no need to zero initialize ks->link_modes.advertising above if it's going to be rewritten here anyway. Michal
On Mon, Jul 08, 2019 at 12:25:26PM -0700, Shannon Nelson wrote: > Add in the basic ethtool callbacks for device information > and control. > > Signed-off-by: Shannon Nelson <snelson@pensando.io> > --- > drivers/net/ethernet/pensando/ionic/Makefile | 2 +- > .../net/ethernet/pensando/ionic/ionic_dev.h | 3 + > .../ethernet/pensando/ionic/ionic_ethtool.c | 509 ++++++++++++++++++ > .../ethernet/pensando/ionic/ionic_ethtool.h | 9 + > .../net/ethernet/pensando/ionic/ionic_lif.c | 2 + > .../net/ethernet/pensando/ionic/ionic_lif.h | 8 + > 6 files changed, 532 insertions(+), 1 deletion(-) > create mode 100644 drivers/net/ethernet/pensando/ionic/ionic_ethtool.c > create mode 100644 drivers/net/ethernet/pensando/ionic/ionic_ethtool.h ... > +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_fc; > + int err = 0; > + > + /* set autoneg */ > + if (ks->base.autoneg != idev->port_info->config.an_enable) { > + 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; > + > + idev->port_info->config.an_enable = ks->base.autoneg; > + } > + > + /* set speed */ > + if (ks->base.speed != le32_to_cpu(idev->port_info->config.speed)) { > + 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; > + > + idev->port_info->config.speed = cpu_to_le32(ks->base.speed); > + } > + > + /* set FEC */ > + req_rs = ethtool_link_ksettings_test_link_mode(ks, advertising, FEC_RS); > + req_fc = ethtool_link_ksettings_test_link_mode(ks, advertising, FEC_BASER); > + if (req_rs && req_fc) { > + netdev_info(netdev, "Only select one FEC mode at a time\n"); > + return -EINVAL; > + > + } else if (req_fc && > + 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_fc) && > + idev->port_info->config.fec_type != PORT_FEC_TYPE_NONE) { > + fec_type = PORT_FEC_TYPE_NONE; > + } > + > + if (fec_type != idev->port_info->config.fec_type) { > + mutex_lock(&ionic->dev_cmd_lock); > + ionic_dev_cmd_port_fec(idev, PORT_FEC_TYPE_NONE); The second argument should be fec_type, I believe. > + err = ionic_dev_cmd_wait(ionic, devcmd_timeout); > + mutex_unlock(&ionic->dev_cmd_lock); > + if (err) > + return err; > + > + idev->port_info->config.fec_type = fec_type; > + } > + > + 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; > + } You can use is_power_of_2() here (it wouldn't allow 0 but you probably don't want to allow that either). Michal > + > + /* if nothing to do return success */ > + if (ring->tx_pending == lif->ntxq_descs && > + ring->rx_pending == lif->nrxq_descs) > + return 0; > + > + while (test_and_set_bit(LIF_QUEUE_RESET, lif->state)) > + usleep_range(200, 400); > + > + running = test_bit(LIF_UP, lif->state); > + if (running) > + ionic_stop(netdev); > + > + lif->ntxq_descs = ring->tx_pending; > + lif->nrxq_descs = ring->rx_pending; > + > + if (running) > + ionic_open(netdev); > + clear_bit(LIF_QUEUE_RESET, lif->state); > + > + return 0; > +}
On 7/8/19 10:25 PM, Michal Kubecek wrote: > On Mon, Jul 08, 2019 at 12:25:26PM -0700, Shannon Nelson wrote: >> Add in the basic ethtool callbacks for device information >> and control. >> >> + >> + if (fec_type != idev->port_info->config.fec_type) { >> + mutex_lock(&ionic->dev_cmd_lock); >> + ionic_dev_cmd_port_fec(idev, PORT_FEC_TYPE_NONE); > The second argument should be fec_type, I believe. Yep. > >> + err = ionic_dev_cmd_wait(ionic, devcmd_timeout); >> + mutex_unlock(&ionic->dev_cmd_lock); >> + if (err) >> + return err; >> + >> + idev->port_info->config.fec_type = fec_type; >> + } >> + >> + 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; >> + } > You can use is_power_of_2() here (it wouldn't allow 0 but you probably > don't want to allow that either). Sure. Thanks, sln
On 7/8/19 10:15 PM, Michal Kubecek wrote: > Also, there is no need to zero initialize ks->link_modes.advertising > above if it's going to be rewritten here anyway. > > Michal Got it. Thanks, sln
On 7/8/19 7:27 PM, Andrew Lunn wrote: >> +static int ionic_get_module_eeprom(struct net_device *netdev, >> + struct ethtool_eeprom *ee, >> + u8 *data) >> +{ >> + struct lif *lif = netdev_priv(netdev); >> + struct ionic_dev *idev = &lif->ionic->idev; >> + struct xcvr_status *xcvr; >> + u32 len; >> + >> + /* copy the module bytes into data */ >> + xcvr = &idev->port_info->status.xcvr; >> + len = min_t(u32, sizeof(xcvr->sprom), ee->len); >> + memcpy(data, xcvr->sprom, len); > Hi Shannon > > This also looks odd. Where is the call into the firmware to get the > eeprom contents? Even though it is called 'eeprom', the data is not > static. It contains real time diagnostic values, temperature, transmit > power, receiver power, voltages etc. idev->port_info is a memory mapped space that the device keeps up-to-date. > >> + >> + dev_dbg(&lif->netdev->dev, "notifyblock eid=0x%llx link_status=0x%x link_speed=0x%x link_down_cnt=0x%x\n", >> + lif->info->status.eid, >> + lif->info->status.link_status, >> + lif->info->status.link_speed, >> + lif->info->status.link_down_count); >> + dev_dbg(&lif->netdev->dev, " port_status id=0x%x status=0x%x speed=0x%x\n", >> + idev->port_info->status.id, >> + idev->port_info->status.status, >> + idev->port_info->status.speed); >> + dev_dbg(&lif->netdev->dev, " xcvr status state=0x%x phy=0x%x pid=0x%x\n", >> + xcvr->state, xcvr->phy, xcvr->pid); >> + dev_dbg(&lif->netdev->dev, " port_config state=0x%x speed=0x%x mtu=0x%x an_enable=0x%x fec_type=0x%x pause_type=0x%x loopback_mode=0x%x\n", >> + idev->port_info->config.state, >> + idev->port_info->config.speed, >> + idev->port_info->config.mtu, >> + idev->port_info->config.an_enable, >> + idev->port_info->config.fec_type, >> + idev->port_info->config.pause_type, >> + idev->port_info->config.loopback_mode); > It is a funny place to have all the debug code. Someone wanted a hook for getting some link info on the fly on request. sln > > Andrew
On 7/8/19 3:04 PM, Andrew Lunn wrote: >> + case XCVR_PID_SFP_10GBASE_ER: >> + ethtool_link_ksettings_add_link_mode(ks, supported, >> + 10000baseER_Full); >> + break; > I don't know these link modes too well. But only setting a single bit > seems odd. What i do know is that an SFP which supports 2500BaseX > should also be able to support 1000BaseX. So should a 100G SFP also > support 40G, 25G, 10G etc? The SERDES just runs a slower bitstream > over the basic bitpipe? Yes, but in this initial release we're not supporting changes to the modes yet. That flexibility will come later. > >> + 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; > Why not add them? Yes, this has been mentioned before. I might in the future, but I have my hands full at the moment. > > >> + memcpy(ks->link_modes.advertising, ks->link_modes.supported, >> + sizeof(ks->link_modes.advertising)); > bitmap_copy() would be a better way to do this. You could consider > adding a helper to ethtool.h. Sure. Thanks for your comments, and sorry I haven't responded as quickly as I'd like... I'll be going through these and your other comments over the next few days. sln
On 7/8/19 7:14 PM, Andrew Lunn wrote: >> +static int ionic_set_pauseparam(struct net_device *netdev, >> + struct ethtool_pauseparam *pause) >> +{ >> + struct lif *lif = netdev_priv(netdev); >> + struct ionic *ionic = lif->ionic; >> + struct ionic_dev *idev = &lif->ionic->idev; >> + >> + u32 requested_pause; >> + u32 cur_autoneg; >> + int err; >> + >> + cur_autoneg = idev->port_info->config.an_enable ? AUTONEG_ENABLE : >> + AUTONEG_DISABLE; >> + if (pause->autoneg != cur_autoneg) { >> + netdev_info(netdev, "Please use 'ethtool -s ...' to change autoneg\n"); >> + return -EOPNOTSUPP; >> + } >> + >> + /* change both at the same time */ >> + requested_pause = PORT_PAUSE_TYPE_LINK; >> + if (pause->rx_pause) >> + requested_pause |= IONIC_PAUSE_F_RX; >> + if (pause->tx_pause) >> + requested_pause |= IONIC_PAUSE_F_TX; >> + >> + if (requested_pause == idev->port_info->config.pause_type) >> + return 0; >> + >> + idev->port_info->config.pause_type = requested_pause; >> + >> + mutex_lock(&ionic->dev_cmd_lock); >> + ionic_dev_cmd_port_pause(idev, requested_pause); >> + err = ionic_dev_cmd_wait(ionic, devcmd_timeout); >> + mutex_unlock(&ionic->dev_cmd_lock); >> + if (err) >> + return err; > Hi Shannon > > I've no idea what the firmware black box is doing, but this looks > wrong. > > pause->autoneg is about if the results of auto-neg should be used or > not. If false, just configure the MAC with the pause settings and you > are done. If the interface is being forced, so autoneg in general is > disabled, just configure the MAC and you are done. > > If pause->autoneg is true and the interface is using auto-neg as a > whole, you pass the pause values to the PHY for it to advertise and > trigger an auto-neg. Once autoneg has completed, and the resolved > settings are available, the MAC is configured with the resolved > values. > > Looking at this code, i don't see any difference between configuring > the MAC or configuring the PHY. I would expect pause->autoneg to be > part of requested_pause somehow, so the firmware knows what is should > do. > > Andrew In this device there's actually very little the driver can do to directly configure the mac or phy besides passing through to the firmware what the user has requested - that happens here for the pause values, and in ionic_set_link_ksettings() for autoneg. The firmware is managing the port based on these requests with the help of internally configured rules defined in a customer setting. sln
On 7/8/19 7:30 PM, Andrew Lunn wrote: >> +static int ionic_nway_reset(struct net_device *netdev) >> +{ >> + struct lif *lif = netdev_priv(netdev); >> + int err = 0; >> + >> + if (netif_running(netdev)) >> + err = ionic_reset_queues(lif); > What does ionic_reset_queues() do? It sounds nothing like restarting > auto negotiation? > > Andrew Basically, it's a rip-it-all-down-and-start-over way of restarting the connection, and is also useful for fixing queues that are misbehaving. It's a little old-fashioned, taken from the ixgbe example, but is effective when there isn't an actual "restart auto-negotiation" command in the firmware. I'll try to make it a little more evident. sln
On Tue, Jul 09, 2019 at 03:42:39PM -0700, Shannon Nelson wrote: > On 7/8/19 7:27 PM, Andrew Lunn wrote: > >>+static int ionic_get_module_eeprom(struct net_device *netdev, > >>+ struct ethtool_eeprom *ee, > >>+ u8 *data) > >>+{ > >>+ struct lif *lif = netdev_priv(netdev); > >>+ struct ionic_dev *idev = &lif->ionic->idev; > >>+ struct xcvr_status *xcvr; > >>+ u32 len; > >>+ > >>+ /* copy the module bytes into data */ > >>+ xcvr = &idev->port_info->status.xcvr; > >>+ len = min_t(u32, sizeof(xcvr->sprom), ee->len); > >>+ memcpy(data, xcvr->sprom, len); > >Hi Shannon > > > >This also looks odd. Where is the call into the firmware to get the > >eeprom contents? Even though it is called 'eeprom', the data is not > >static. It contains real time diagnostic values, temperature, transmit > >power, receiver power, voltages etc. > > idev->port_info is a memory mapped space that the device keeps up-to-date. Hi Shannon It at least needs a comment. How frequently does the device update this chunk of memory? It would be good to comment about that as well. Or do MMIO reads block while i2c operations occur to update the memory? > >>+ > >>+ dev_dbg(&lif->netdev->dev, "notifyblock eid=0x%llx link_status=0x%x link_speed=0x%x link_down_cnt=0x%x\n", > >>+ lif->info->status.eid, > >>+ lif->info->status.link_status, > >>+ lif->info->status.link_speed, > >>+ lif->info->status.link_down_count); > >>+ dev_dbg(&lif->netdev->dev, " port_status id=0x%x status=0x%x speed=0x%x\n", > >>+ idev->port_info->status.id, > >>+ idev->port_info->status.status, > >>+ idev->port_info->status.speed); > >>+ dev_dbg(&lif->netdev->dev, " xcvr status state=0x%x phy=0x%x pid=0x%x\n", > >>+ xcvr->state, xcvr->phy, xcvr->pid); > >>+ dev_dbg(&lif->netdev->dev, " port_config state=0x%x speed=0x%x mtu=0x%x an_enable=0x%x fec_type=0x%x pause_type=0x%x loopback_mode=0x%x\n", > >>+ idev->port_info->config.state, > >>+ idev->port_info->config.speed, > >>+ idev->port_info->config.mtu, > >>+ idev->port_info->config.an_enable, > >>+ idev->port_info->config.fec_type, > >>+ idev->port_info->config.pause_type, > >>+ idev->port_info->config.loopback_mode); > >It is a funny place to have all the debug code. > > Someone wanted a hook for getting some link info on the fly on request. I would suggest deleting it all. Most of it is already available via ethtool. Andrew
On Fri, Jul 12, 2019 at 10:16:31PM -0700, Shannon Nelson wrote: > On 7/8/19 7:14 PM, Andrew Lunn wrote: > >>+static int ionic_set_pauseparam(struct net_device *netdev, > >>+ struct ethtool_pauseparam *pause) > >>+{ > >>+ struct lif *lif = netdev_priv(netdev); > >>+ struct ionic *ionic = lif->ionic; > >>+ struct ionic_dev *idev = &lif->ionic->idev; > >>+ > >>+ u32 requested_pause; > >>+ u32 cur_autoneg; > >>+ int err; > >>+ > >>+ cur_autoneg = idev->port_info->config.an_enable ? AUTONEG_ENABLE : > >>+ AUTONEG_DISABLE; > >>+ if (pause->autoneg != cur_autoneg) { > >>+ netdev_info(netdev, "Please use 'ethtool -s ...' to change autoneg\n"); > >>+ return -EOPNOTSUPP; > >>+ } > >>+ > >>+ /* change both at the same time */ > >>+ requested_pause = PORT_PAUSE_TYPE_LINK; > >>+ if (pause->rx_pause) > >>+ requested_pause |= IONIC_PAUSE_F_RX; > >>+ if (pause->tx_pause) > >>+ requested_pause |= IONIC_PAUSE_F_TX; > >>+ > >>+ if (requested_pause == idev->port_info->config.pause_type) > >>+ return 0; > >>+ > >>+ idev->port_info->config.pause_type = requested_pause; > >>+ > >>+ mutex_lock(&ionic->dev_cmd_lock); > >>+ ionic_dev_cmd_port_pause(idev, requested_pause); > >>+ err = ionic_dev_cmd_wait(ionic, devcmd_timeout); > >>+ mutex_unlock(&ionic->dev_cmd_lock); > >>+ if (err) > >>+ return err; > >Hi Shannon > > > >I've no idea what the firmware black box is doing, but this looks > >wrong. > > > >pause->autoneg is about if the results of auto-neg should be used or > >not. If false, just configure the MAC with the pause settings and you > >are done. If the interface is being forced, so autoneg in general is > >disabled, just configure the MAC and you are done. > > > >If pause->autoneg is true and the interface is using auto-neg as a > >whole, you pass the pause values to the PHY for it to advertise and > >trigger an auto-neg. Once autoneg has completed, and the resolved > >settings are available, the MAC is configured with the resolved > >values. > > > >Looking at this code, i don't see any difference between configuring > >the MAC or configuring the PHY. I would expect pause->autoneg to be > >part of requested_pause somehow, so the firmware knows what is should > >do. > > > > Andrew > > In this device there's actually very little the driver can do to directly > configure the mac or phy besides passing through to the firmware what the > user has requested - that happens here for the pause values, and in > ionic_set_link_ksettings() for autoneg. The firmware is managing the port > based on these requests with the help of internally configured rules defined > in a customer setting. I get that. But the firmware needs to conform to what Linux expects. And what i see here does not conform. That is why i gave a bit of detail in my reply. What exactly does the firmware do? Once we know that, we can figure out when the driver should return -EOPNOTSUPP because of firmware limitations, and what it can configure and should return 0. Andrew
On Fri, Jul 12, 2019 at 10:32:38PM -0700, Shannon Nelson wrote: > On 7/8/19 7:30 PM, Andrew Lunn wrote: > >>+static int ionic_nway_reset(struct net_device *netdev) > >>+{ > >>+ struct lif *lif = netdev_priv(netdev); > >>+ int err = 0; > >>+ > >>+ if (netif_running(netdev)) > >>+ err = ionic_reset_queues(lif); > >What does ionic_reset_queues() do? It sounds nothing like restarting > >auto negotiation? > > > > Andrew > Basically, it's a rip-it-all-down-and-start-over way of restarting the > connection, and is also useful for fixing queues that are misbehaving. It's > a little old-fashioned, taken from the ixgbe example, but is effective when > there isn't an actual "restart auto-negotiation" command in the firmware. O.K. More comments please. Did you consider throwing the firmware away and just letting Linux control the hardware? It would make this all much more transparent and debuggable. Andrew
On 7/17/19 8:21 PM, Andrew Lunn wrote: > On Tue, Jul 09, 2019 at 03:42:39PM -0700, Shannon Nelson wrote: >> On 7/8/19 7:27 PM, Andrew Lunn wrote: >>>> +static int ionic_get_module_eeprom(struct net_device *netdev, >>>> + struct ethtool_eeprom *ee, >>>> + u8 *data) >>>> +{ >>>> + struct lif *lif = netdev_priv(netdev); >>>> + struct ionic_dev *idev = &lif->ionic->idev; >>>> + struct xcvr_status *xcvr; >>>> + u32 len; >>>> + >>>> + /* copy the module bytes into data */ >>>> + xcvr = &idev->port_info->status.xcvr; >>>> + len = min_t(u32, sizeof(xcvr->sprom), ee->len); >>>> + memcpy(data, xcvr->sprom, len); >>> Hi Shannon >>> >>> This also looks odd. Where is the call into the firmware to get the >>> eeprom contents? Even though it is called 'eeprom', the data is not >>> static. It contains real time diagnostic values, temperature, transmit >>> power, receiver power, voltages etc. >> idev->port_info is a memory mapped space that the device keeps up-to-date. > Hi Shannon > > It at least needs a comment. How frequently does the device update > this chunk of memory? It would be good to comment about that as > well. Or do MMIO reads block while i2c operations occur to update the > memory? The device keeps this updated when changes happen internally so that there is no need to block on MMIO read. Sure, I'll add a little more commentary here and perhaps in a couple other similar places. > >>>> + >>>> + dev_dbg(&lif->netdev->dev, "notifyblock eid=0x%llx link_status=0x%x link_speed=0x%x link_down_cnt=0x%x\n", >>>> + lif->info->status.eid, >>>> + lif->info->status.link_status, >>>> + lif->info->status.link_speed, >>>> + lif->info->status.link_down_count); >>>> + dev_dbg(&lif->netdev->dev, " port_status id=0x%x status=0x%x speed=0x%x\n", >>>> + idev->port_info->status.id, >>>> + idev->port_info->status.status, >>>> + idev->port_info->status.speed); >>>> + dev_dbg(&lif->netdev->dev, " xcvr status state=0x%x phy=0x%x pid=0x%x\n", >>>> + xcvr->state, xcvr->phy, xcvr->pid); >>>> + dev_dbg(&lif->netdev->dev, " port_config state=0x%x speed=0x%x mtu=0x%x an_enable=0x%x fec_type=0x%x pause_type=0x%x loopback_mode=0x%x\n", >>>> + idev->port_info->config.state, >>>> + idev->port_info->config.speed, >>>> + idev->port_info->config.mtu, >>>> + idev->port_info->config.an_enable, >>>> + idev->port_info->config.fec_type, >>>> + idev->port_info->config.pause_type, >>>> + idev->port_info->config.loopback_mode); >>> It is a funny place to have all the debug code. >> Someone wanted a hook for getting some link info on the fly on request. > I would suggest deleting it all. Most of it is already available via > ethtool. > Sure. sln
On 7/17/19 8:31 PM, Andrew Lunn wrote: > On Fri, Jul 12, 2019 at 10:32:38PM -0700, Shannon Nelson wrote: >> On 7/8/19 7:30 PM, Andrew Lunn wrote: >>>> +static int ionic_nway_reset(struct net_device *netdev) >>>> +{ >>>> + struct lif *lif = netdev_priv(netdev); >>>> + int err = 0; >>>> + >>>> + if (netif_running(netdev)) >>>> + err = ionic_reset_queues(lif); >>> What does ionic_reset_queues() do? It sounds nothing like restarting >>> auto negotiation? >>> >>> Andrew >> Basically, it's a rip-it-all-down-and-start-over way of restarting the >> connection, and is also useful for fixing queues that are misbehaving. It's >> a little old-fashioned, taken from the ixgbe example, but is effective when >> there isn't an actual "restart auto-negotiation" command in the firmware. > O.K. More comments please. After a little more discussion with the nic connection folks, they've said to just flap the link to force a new auto-negotiation, so that's what I'm adding - set state to off, then back on. And yes, another comment here. > > Did you consider throwing the firmware away and just letting Linux > control the hardware? It would make this all much more transparent and > debuggable. > Yes, the tension between direct driver bit twiddling and smartnic offloading continues. sln
On Thu, Jul 18, 2019 at 10:05:23AM -0700, Shannon Nelson wrote: > On 7/17/19 8:21 PM, Andrew Lunn wrote: > >On Tue, Jul 09, 2019 at 03:42:39PM -0700, Shannon Nelson wrote: > >>On 7/8/19 7:27 PM, Andrew Lunn wrote: > >>>>+static int ionic_get_module_eeprom(struct net_device *netdev, > >>>>+ struct ethtool_eeprom *ee, > >>>>+ u8 *data) > >>>>+{ > >>>>+ struct lif *lif = netdev_priv(netdev); > >>>>+ struct ionic_dev *idev = &lif->ionic->idev; > >>>>+ struct xcvr_status *xcvr; > >>>>+ u32 len; > >>>>+ > >>>>+ /* copy the module bytes into data */ > >>>>+ xcvr = &idev->port_info->status.xcvr; > >>>>+ len = min_t(u32, sizeof(xcvr->sprom), ee->len); > >>>>+ memcpy(data, xcvr->sprom, len); > >>>Hi Shannon > >>> > >>>This also looks odd. Where is the call into the firmware to get the > >>>eeprom contents? Even though it is called 'eeprom', the data is not > >>>static. It contains real time diagnostic values, temperature, transmit > >>>power, receiver power, voltages etc. > >>idev->port_info is a memory mapped space that the device keeps up-to-date. > >Hi Shannon > > > >It at least needs a comment. How frequently does the device update > >this chunk of memory? It would be good to comment about that as > >well. Or do MMIO reads block while i2c operations occur to update the > >memory? > > The device keeps this updated when changes happen internally so that there > is no need to block on MMIO read. Hi Shannon I'm thinking about the diagnostic page. RX and TX power, temperature, alarms etc. These are real time values, so you should read them on demand, or at last only cache them for a short time. Andrew
On 7/18/19 10:28 AM, Andrew Lunn wrote: > On Thu, Jul 18, 2019 at 10:05:23AM -0700, Shannon Nelson wrote: >> On 7/17/19 8:21 PM, Andrew Lunn wrote: >>> On Tue, Jul 09, 2019 at 03:42:39PM -0700, Shannon Nelson wrote: >>>> On 7/8/19 7:27 PM, Andrew Lunn wrote: >>>>>> +static int ionic_get_module_eeprom(struct net_device *netdev, >>>>>> + struct ethtool_eeprom *ee, >>>>>> + u8 *data) >>>>>> +{ >>>>>> + struct lif *lif = netdev_priv(netdev); >>>>>> + struct ionic_dev *idev = &lif->ionic->idev; >>>>>> + struct xcvr_status *xcvr; >>>>>> + u32 len; >>>>>> + >>>>>> + /* copy the module bytes into data */ >>>>>> + xcvr = &idev->port_info->status.xcvr; >>>>>> + len = min_t(u32, sizeof(xcvr->sprom), ee->len); >>>>>> + memcpy(data, xcvr->sprom, len); >>>>> Hi Shannon >>>>> >>>>> This also looks odd. Where is the call into the firmware to get the >>>>> eeprom contents? Even though it is called 'eeprom', the data is not >>>>> static. It contains real time diagnostic values, temperature, transmit >>>>> power, receiver power, voltages etc. >>>> idev->port_info is a memory mapped space that the device keeps up-to-date. >>> Hi Shannon >>> >>> It at least needs a comment. How frequently does the device update >>> this chunk of memory? It would be good to comment about that as >>> well. Or do MMIO reads block while i2c operations occur to update the >>> memory? >> The device keeps this updated when changes happen internally so that there >> is no need to block on MMIO read. > Hi Shannon > > I'm thinking about the diagnostic page. RX and TX power, temperature, > alarms etc. These are real time values, so you should read them on > demand, or at last only cache them for a short time. > > They *are* read on demand. The port_info and lif_info structs are dma mapped spaces that the device keeps up-to-date with PCI writes in the background so that the driver can do a quick memory read for current data. sln
On 7/17/19 8:28 PM, Andrew Lunn wrote: > On Fri, Jul 12, 2019 at 10:16:31PM -0700, Shannon Nelson wrote: >> On 7/8/19 7:14 PM, Andrew Lunn wrote: >>>> +static int ionic_set_pauseparam(struct net_device *netdev, >>>> + struct ethtool_pauseparam *pause) >>>> +{ >>>> + struct lif *lif = netdev_priv(netdev); >>>> + struct ionic *ionic = lif->ionic; >>>> + struct ionic_dev *idev = &lif->ionic->idev; >>>> + >>>> + u32 requested_pause; >>>> + u32 cur_autoneg; >>>> + int err; >>>> + >>>> + cur_autoneg = idev->port_info->config.an_enable ? AUTONEG_ENABLE : >>>> + AUTONEG_DISABLE; >>>> + if (pause->autoneg != cur_autoneg) { >>>> + netdev_info(netdev, "Please use 'ethtool -s ...' to change autoneg\n"); >>>> + return -EOPNOTSUPP; >>>> + } >>>> + >>>> + /* change both at the same time */ >>>> + requested_pause = PORT_PAUSE_TYPE_LINK; >>>> + if (pause->rx_pause) >>>> + requested_pause |= IONIC_PAUSE_F_RX; >>>> + if (pause->tx_pause) >>>> + requested_pause |= IONIC_PAUSE_F_TX; >>>> + >>>> + if (requested_pause == idev->port_info->config.pause_type) >>>> + return 0; >>>> + >>>> + idev->port_info->config.pause_type = requested_pause; >>>> + >>>> + mutex_lock(&ionic->dev_cmd_lock); >>>> + ionic_dev_cmd_port_pause(idev, requested_pause); >>>> + err = ionic_dev_cmd_wait(ionic, devcmd_timeout); >>>> + mutex_unlock(&ionic->dev_cmd_lock); >>>> + if (err) >>>> + return err; >>> Hi Shannon >>> >>> I've no idea what the firmware black box is doing, but this looks >>> wrong. >>> >>> pause->autoneg is about if the results of auto-neg should be used or >>> not. If false, just configure the MAC with the pause settings and you >>> are done. If the interface is being forced, so autoneg in general is >>> disabled, just configure the MAC and you are done. >>> >>> If pause->autoneg is true and the interface is using auto-neg as a >>> whole, you pass the pause values to the PHY for it to advertise and >>> trigger an auto-neg. Once autoneg has completed, and the resolved >>> settings are available, the MAC is configured with the resolved >>> values. >>> >>> Looking at this code, i don't see any difference between configuring >>> the MAC or configuring the PHY. I would expect pause->autoneg to be >>> part of requested_pause somehow, so the firmware knows what is should >>> do. >>> >>> Andrew >> In this device there's actually very little the driver can do to directly >> configure the mac or phy besides passing through to the firmware what the >> user has requested - that happens here for the pause values, and in >> ionic_set_link_ksettings() for autoneg. The firmware is managing the port >> based on these requests with the help of internally configured rules defined >> in a customer setting. > I get that. But the firmware needs to conform to what Linux > expects. And what i see here does not conform. That is why i gave a > bit of detail in my reply. > > What exactly does the firmware do? Once we know that, we can figure > out when the driver should return -EOPNOTSUPP because of firmware > limitations, and what it can configure and should return 0. Because this is fairly smart FW, it handles this as expected. I can add this as another comment in the code. sln
On Thu, Jul 18, 2019 at 05:12:07PM -0700, Shannon Nelson wrote: > On 7/17/19 8:28 PM, Andrew Lunn wrote: > >On Fri, Jul 12, 2019 at 10:16:31PM -0700, Shannon Nelson wrote: > >>On 7/8/19 7:14 PM, Andrew Lunn wrote: > >>>>+static int ionic_set_pauseparam(struct net_device *netdev, > >>>>+ struct ethtool_pauseparam *pause) > >>>>+{ > >>>>+ struct lif *lif = netdev_priv(netdev); > >>>>+ struct ionic *ionic = lif->ionic; > >>>>+ struct ionic_dev *idev = &lif->ionic->idev; > >>>>+ > >>>>+ u32 requested_pause; > >>>>+ u32 cur_autoneg; > >>>>+ int err; > >>>>+ > >>>>+ cur_autoneg = idev->port_info->config.an_enable ? AUTONEG_ENABLE : > >>>>+ AUTONEG_DISABLE; > >>>>+ if (pause->autoneg != cur_autoneg) { > >>>>+ netdev_info(netdev, "Please use 'ethtool -s ...' to change autoneg\n"); > >>>>+ return -EOPNOTSUPP; > >>>>+ } > >>>>+ > >>>>+ /* change both at the same time */ > >>>>+ requested_pause = PORT_PAUSE_TYPE_LINK; > >>>>+ if (pause->rx_pause) > >>>>+ requested_pause |= IONIC_PAUSE_F_RX; > >>>>+ if (pause->tx_pause) > >>>>+ requested_pause |= IONIC_PAUSE_F_TX; > >>>>+ > >>>>+ if (requested_pause == idev->port_info->config.pause_type) > >>>>+ return 0; > >>>>+ > >>>>+ idev->port_info->config.pause_type = requested_pause; > >>>>+ > >>>>+ mutex_lock(&ionic->dev_cmd_lock); > >>>>+ ionic_dev_cmd_port_pause(idev, requested_pause); > >>>>+ err = ionic_dev_cmd_wait(ionic, devcmd_timeout); > >>>>+ mutex_unlock(&ionic->dev_cmd_lock); > >>>>+ if (err) > >>>>+ return err; > >>>Hi Shannon > >>> > >>>I've no idea what the firmware black box is doing, but this looks > >>>wrong. > >>> > >>>pause->autoneg is about if the results of auto-neg should be used or > >>>not. If false, just configure the MAC with the pause settings and you > >>>are done. If the interface is being forced, so autoneg in general is > >>>disabled, just configure the MAC and you are done. > >>> > >>>If pause->autoneg is true and the interface is using auto-neg as a > >>>whole, you pass the pause values to the PHY for it to advertise and > >>>trigger an auto-neg. Once autoneg has completed, and the resolved > >>>settings are available, the MAC is configured with the resolved > >>>values. > >>> > >>>Looking at this code, i don't see any difference between configuring > >>>the MAC or configuring the PHY. I would expect pause->autoneg to be > >>>part of requested_pause somehow, so the firmware knows what is should > >>>do. > >>> > >>> Andrew > >>In this device there's actually very little the driver can do to directly > >>configure the mac or phy besides passing through to the firmware what the > >>user has requested - that happens here for the pause values, and in > >>ionic_set_link_ksettings() for autoneg. The firmware is managing the port > >>based on these requests with the help of internally configured rules defined > >>in a customer setting. > >I get that. But the firmware needs to conform to what Linux > >expects. And what i see here does not conform. That is why i gave a > >bit of detail in my reply. > > > >What exactly does the firmware do? Once we know that, we can figure > >out when the driver should return -EOPNOTSUPP because of firmware > >limitations, and what it can configure and should return 0. > > Because this is fairly smart FW, it handles this as expected. I can add > this as another comment in the code. Hi Shannon Looking at the code, i don't see how it can handle this correctly. Please look at my comments, particularly the meaning of pause->autoneg and describe how the firmware does the right thing, given what information it is passed. Andrew
On 7/18/19 7:40 PM, Andrew Lunn wrote: > On Thu, Jul 18, 2019 at 05:12:07PM -0700, Shannon Nelson wrote: >> On 7/17/19 8:28 PM, Andrew Lunn wrote: >>> On Fri, Jul 12, 2019 at 10:16:31PM -0700, Shannon Nelson wrote: >>>> On 7/8/19 7:14 PM, Andrew Lunn wrote: >>>>>> +static int ionic_set_pauseparam(struct net_device *netdev, >>>>>> + struct ethtool_pauseparam *pause) >>>>>> +{ >>>>>> + struct lif *lif = netdev_priv(netdev); >>>>>> + struct ionic *ionic = lif->ionic; >>>>>> + struct ionic_dev *idev = &lif->ionic->idev; >>>>>> + >>>>>> + u32 requested_pause; >>>>>> + u32 cur_autoneg; >>>>>> + int err; >>>>>> + >>>>>> + cur_autoneg = idev->port_info->config.an_enable ? AUTONEG_ENABLE : >>>>>> + AUTONEG_DISABLE; >>>>>> + if (pause->autoneg != cur_autoneg) { >>>>>> + netdev_info(netdev, "Please use 'ethtool -s ...' to change autoneg\n"); >>>>>> + return -EOPNOTSUPP; >>>>>> + } >>>>>> + >>>>>> + /* change both at the same time */ >>>>>> + requested_pause = PORT_PAUSE_TYPE_LINK; >>>>>> + if (pause->rx_pause) >>>>>> + requested_pause |= IONIC_PAUSE_F_RX; >>>>>> + if (pause->tx_pause) >>>>>> + requested_pause |= IONIC_PAUSE_F_TX; >>>>>> + >>>>>> + if (requested_pause == idev->port_info->config.pause_type) >>>>>> + return 0; >>>>>> + >>>>>> + idev->port_info->config.pause_type = requested_pause; >>>>>> + >>>>>> + mutex_lock(&ionic->dev_cmd_lock); >>>>>> + ionic_dev_cmd_port_pause(idev, requested_pause); >>>>>> + err = ionic_dev_cmd_wait(ionic, devcmd_timeout); >>>>>> + mutex_unlock(&ionic->dev_cmd_lock); >>>>>> + if (err) >>>>>> + return err; >>>>> Hi Shannon >>>>> >>>>> I've no idea what the firmware black box is doing, but this looks >>>>> wrong. >>>>> >>>>> pause->autoneg is about if the results of auto-neg should be used or >>>>> not. If false, just configure the MAC with the pause settings and you >>>>> are done. If the interface is being forced, so autoneg in general is >>>>> disabled, just configure the MAC and you are done. >>>>> >>>>> If pause->autoneg is true and the interface is using auto-neg as a >>>>> whole, you pass the pause values to the PHY for it to advertise and >>>>> trigger an auto-neg. Once autoneg has completed, and the resolved >>>>> settings are available, the MAC is configured with the resolved >>>>> values. >>>>> >>>>> Looking at this code, i don't see any difference between configuring >>>>> the MAC or configuring the PHY. I would expect pause->autoneg to be >>>>> part of requested_pause somehow, so the firmware knows what is should >>>>> do. >>>>> >>>>> Andrew >>>> In this device there's actually very little the driver can do to directly >>>> configure the mac or phy besides passing through to the firmware what the >>>> user has requested - that happens here for the pause values, and in >>>> ionic_set_link_ksettings() for autoneg. The firmware is managing the port >>>> based on these requests with the help of internally configured rules defined >>>> in a customer setting. >>> I get that. But the firmware needs to conform to what Linux >>> expects. And what i see here does not conform. That is why i gave a >>> bit of detail in my reply. >>> >>> What exactly does the firmware do? Once we know that, we can figure >>> out when the driver should return -EOPNOTSUPP because of firmware >>> limitations, and what it can configure and should return 0. >> Because this is fairly smart FW, it handles this as expected. I can add >> this as another comment in the code. > Hi Shannon > > Looking at the code, i don't see how it can handle this > correctly. Please look at my comments, particularly the meaning of > pause->autoneg and describe how the firmware does the right thing, > given what information it is passed. > I guess I'm not sure how much better I can answer your question. Like several other devices, we don't support selecting pause->autoneg. The firmware manages the PHY itself, the driver doesn't have direct access to the PHY. The driver passes the tx and rx pause info down to the firmware in a command request. The NIC firmware does an autoneg if autoneg is enabled on the port. sln
On Fri, Jul 19, 2019 at 11:41:28AM -0700, Shannon Nelson wrote: > On 7/18/19 7:40 PM, Andrew Lunn wrote: > >On Thu, Jul 18, 2019 at 05:12:07PM -0700, Shannon Nelson wrote: > >>On 7/17/19 8:28 PM, Andrew Lunn wrote: > >>>On Fri, Jul 12, 2019 at 10:16:31PM -0700, Shannon Nelson wrote: > >>>>On 7/8/19 7:14 PM, Andrew Lunn wrote: > >>>>>>+static int ionic_set_pauseparam(struct net_device *netdev, > >>>>>>+ struct ethtool_pauseparam *pause) > >>>>>>+{ > >>>>>>+ struct lif *lif = netdev_priv(netdev); > >>>>>>+ struct ionic *ionic = lif->ionic; > >>>>>>+ struct ionic_dev *idev = &lif->ionic->idev; > >>>>>>+ > >>>>>>+ u32 requested_pause; > >>>>>>+ u32 cur_autoneg; > >>>>>>+ int err; > >>>>>>+ > >>>>>>+ cur_autoneg = idev->port_info->config.an_enable ? AUTONEG_ENABLE : > >>>>>>+ AUTONEG_DISABLE; > >>>>>>+ if (pause->autoneg != cur_autoneg) { > >>>>>>+ netdev_info(netdev, "Please use 'ethtool -s ...' to change autoneg\n"); > >>>>>>+ return -EOPNOTSUPP; > >>>>>>+ } > >>>>>>+ > >>>>>>+ /* change both at the same time */ > >>>>>>+ requested_pause = PORT_PAUSE_TYPE_LINK; > >>>>>>+ if (pause->rx_pause) > >>>>>>+ requested_pause |= IONIC_PAUSE_F_RX; > >>>>>>+ if (pause->tx_pause) > >>>>>>+ requested_pause |= IONIC_PAUSE_F_TX; > >>>>>>+ > >>>>>>+ if (requested_pause == idev->port_info->config.pause_type) > >>>>>>+ return 0; > >>>>>>+ > >>>>>>+ idev->port_info->config.pause_type = requested_pause; > >>>>>>+ > >>>>>>+ mutex_lock(&ionic->dev_cmd_lock); > >>>>>>+ ionic_dev_cmd_port_pause(idev, requested_pause); > >>>>>>+ err = ionic_dev_cmd_wait(ionic, devcmd_timeout); > >>>>>>+ mutex_unlock(&ionic->dev_cmd_lock); > >>>>>>+ if (err) > >>>>>>+ return err; > >>>>>Hi Shannon > >>>>> > >>>>>I've no idea what the firmware black box is doing, but this looks > >>>>>wrong. > >>>>> > >>>>>pause->autoneg is about if the results of auto-neg should be used or > >>>>>not. If false, just configure the MAC with the pause settings and you > >>>>>are done. If the interface is being forced, so autoneg in general is > >>>>>disabled, just configure the MAC and you are done. > >>>>> > >>>>>If pause->autoneg is true and the interface is using auto-neg as a > >>>>>whole, you pass the pause values to the PHY for it to advertise and > >>>>>trigger an auto-neg. Once autoneg has completed, and the resolved > >>>>>settings are available, the MAC is configured with the resolved > >>>>>values. > >>>>> > >>>>>Looking at this code, i don't see any difference between configuring > >>>>>the MAC or configuring the PHY. I would expect pause->autoneg to be > >>>>>part of requested_pause somehow, so the firmware knows what is should > >>>>>do. > >>>>> > >>>>> Andrew > >>>>In this device there's actually very little the driver can do to directly > >>>>configure the mac or phy besides passing through to the firmware what the > >>>>user has requested - that happens here for the pause values, and in > >>>>ionic_set_link_ksettings() for autoneg. The firmware is managing the port > >>>>based on these requests with the help of internally configured rules defined > >>>>in a customer setting. > >>>I get that. But the firmware needs to conform to what Linux > >>>expects. And what i see here does not conform. That is why i gave a > >>>bit of detail in my reply. > >>> > >>>What exactly does the firmware do? Once we know that, we can figure > >>>out when the driver should return -EOPNOTSUPP because of firmware > >>>limitations, and what it can configure and should return 0. > >>Because this is fairly smart FW, it handles this as expected. I can add > >>this as another comment in the code. > >Hi Shannon > > > >Looking at the code, i don't see how it can handle this > >correctly. Please look at my comments, particularly the meaning of > >pause->autoneg and describe how the firmware does the right thing, > >given what information it is passed. > > > > I guess I'm not sure how much better I can answer your question. Like > several other devices, we don't support selecting pause->autoneg. The > firmware manages the PHY itself, the driver doesn't have direct access to > the PHY. The driver passes the tx and rx pause info down to the firmware in > a command request. The NIC firmware does an autoneg if autoneg is enabled > on the port. Hi Shannon Thanks. That was the information i was looking for. Please return -EOPNOTSUPP if pause->autoneg indicates autoneg results should not be used. Your firmware does not support it, so the driver should error out. Also the get function should use a hard coded value for pause->autoneg. If you ever fix your firmware, you can add full support for pause configuration. Thanks Andrew
On 7/19/19 12:07 PM, Andrew Lunn wrote: > On Fri, Jul 19, 2019 at 11:41:28AM -0700, Shannon Nelson wrote: >> On 7/18/19 7:40 PM, Andrew Lunn wrote: >>> On Thu, Jul 18, 2019 at 05:12:07PM -0700, Shannon Nelson wrote: >>>> On 7/17/19 8:28 PM, Andrew Lunn wrote: >>>>> On Fri, Jul 12, 2019 at 10:16:31PM -0700, Shannon Nelson wrote: >>>>>> On 7/8/19 7:14 PM, Andrew Lunn wrote: >>>>>>>> +static int ionic_set_pauseparam(struct net_device *netdev, >>>>>>>> + struct ethtool_pauseparam *pause) >>>>>>>> +{ >>>>>>>> + struct lif *lif = netdev_priv(netdev); >>>>>>>> + struct ionic *ionic = lif->ionic; >>>>>>>> + struct ionic_dev *idev = &lif->ionic->idev; >>>>>>>> + >>>>>>>> + u32 requested_pause; >>>>>>>> + u32 cur_autoneg; >>>>>>>> + int err; >>>>>>>> + >>>>>>>> + cur_autoneg = idev->port_info->config.an_enable ? AUTONEG_ENABLE : >>>>>>>> + AUTONEG_DISABLE; >>>>>>>> + if (pause->autoneg != cur_autoneg) { >>>>>>>> + netdev_info(netdev, "Please use 'ethtool -s ...' to change autoneg\n"); >>>>>>>> + return -EOPNOTSUPP; >>>>>>>> + } >>>>>>>> + >>>>>>>> + /* change both at the same time */ >>>>>>>> + requested_pause = PORT_PAUSE_TYPE_LINK; >>>>>>>> + if (pause->rx_pause) >>>>>>>> + requested_pause |= IONIC_PAUSE_F_RX; >>>>>>>> + if (pause->tx_pause) >>>>>>>> + requested_pause |= IONIC_PAUSE_F_TX; >>>>>>>> + >>>>>>>> + if (requested_pause == idev->port_info->config.pause_type) >>>>>>>> + return 0; >>>>>>>> + >>>>>>>> + idev->port_info->config.pause_type = requested_pause; >>>>>>>> + >>>>>>>> + mutex_lock(&ionic->dev_cmd_lock); >>>>>>>> + ionic_dev_cmd_port_pause(idev, requested_pause); >>>>>>>> + err = ionic_dev_cmd_wait(ionic, devcmd_timeout); >>>>>>>> + mutex_unlock(&ionic->dev_cmd_lock); >>>>>>>> + if (err) >>>>>>>> + return err; >>>>>>> Hi Shannon >>>>>>> >>>>>>> I've no idea what the firmware black box is doing, but this looks >>>>>>> wrong. >>>>>>> >>>>>>> pause->autoneg is about if the results of auto-neg should be used or >>>>>>> not. If false, just configure the MAC with the pause settings and you >>>>>>> are done. If the interface is being forced, so autoneg in general is >>>>>>> disabled, just configure the MAC and you are done. >>>>>>> >>>>>>> If pause->autoneg is true and the interface is using auto-neg as a >>>>>>> whole, you pass the pause values to the PHY for it to advertise and >>>>>>> trigger an auto-neg. Once autoneg has completed, and the resolved >>>>>>> settings are available, the MAC is configured with the resolved >>>>>>> values. >>>>>>> >>>>>>> Looking at this code, i don't see any difference between configuring >>>>>>> the MAC or configuring the PHY. I would expect pause->autoneg to be >>>>>>> part of requested_pause somehow, so the firmware knows what is should >>>>>>> do. >>>>>>> >>>>>>> Andrew >>>>>> In this device there's actually very little the driver can do to directly >>>>>> configure the mac or phy besides passing through to the firmware what the >>>>>> user has requested - that happens here for the pause values, and in >>>>>> ionic_set_link_ksettings() for autoneg. The firmware is managing the port >>>>>> based on these requests with the help of internally configured rules defined >>>>>> in a customer setting. >>>>> I get that. But the firmware needs to conform to what Linux >>>>> expects. And what i see here does not conform. That is why i gave a >>>>> bit of detail in my reply. >>>>> >>>>> What exactly does the firmware do? Once we know that, we can figure >>>>> out when the driver should return -EOPNOTSUPP because of firmware >>>>> limitations, and what it can configure and should return 0. >>>> Because this is fairly smart FW, it handles this as expected. I can add >>>> this as another comment in the code. >>> Hi Shannon >>> >>> Looking at the code, i don't see how it can handle this >>> correctly. Please look at my comments, particularly the meaning of >>> pause->autoneg and describe how the firmware does the right thing, >>> given what information it is passed. >>> >> I guess I'm not sure how much better I can answer your question. Like >> several other devices, we don't support selecting pause->autoneg. The >> firmware manages the PHY itself, the driver doesn't have direct access to >> the PHY. The driver passes the tx and rx pause info down to the firmware in >> a command request. The NIC firmware does an autoneg if autoneg is enabled >> on the port. > Hi Shannon > > Thanks. That was the information i was looking for. > > Please return -EOPNOTSUPP if pause->autoneg indicates autoneg results > should not be used. Your firmware does not support it, so the driver > should error out. Also the get function should use a hard coded value > for pause->autoneg. > > If you ever fix your firmware, you can add full support for pause > configuration. > > Thanks for the help, sln
diff --git a/drivers/net/ethernet/pensando/ionic/Makefile b/drivers/net/ethernet/pensando/ionic/Makefile index 7d9cdc5f02a1..9b19bf57a489 100644 --- a/drivers/net/ethernet/pensando/ionic/Makefile +++ b/drivers/net/ethernet/pensando/ionic/Makefile @@ -3,5 +3,5 @@ obj-$(CONFIG_IONIC) := ionic.o -ionic-y := ionic_main.o ionic_bus_pci.o ionic_dev.o \ +ionic-y := ionic_main.o ionic_bus_pci.o ionic_dev.o ionic_ethtool.o \ ionic_lif.o ionic_rx_filter.o ionic_debugfs.o diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.h b/drivers/net/ethernet/pensando/ionic/ionic_dev.h index d44220c1d430..a95af4c4dbf0 100644 --- a/drivers/net/ethernet/pensando/ionic/ionic_dev.h +++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.h @@ -12,6 +12,9 @@ #define IONIC_MIN_MTU ETH_MIN_MTU #define IONIC_MAX_MTU 9194 +#define IONIC_MAX_TXRX_DESC 16384 +#define IONIC_MIN_TXRX_DESC 16 +#define IONIC_DEF_TXRX_DESC 4096 #define IONIC_LIFS_MAX 1024 struct ionic_dev_bar { diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c new file mode 100644 index 000000000000..2bbe5819387b --- /dev/null +++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c @@ -0,0 +1,509 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright(c) 2017 - 2019 Pensando Systems, Inc */ + +#include <linux/module.h> +#include <linux/netdevice.h> + +#include "ionic.h" +#include "ionic_bus.h" +#include "ionic_lif.h" +#include "ionic_ethtool.h" + +static void ionic_get_drvinfo(struct net_device *netdev, + struct ethtool_drvinfo *drvinfo) +{ + struct lif *lif = netdev_priv(netdev); + struct ionic *ionic = lif->ionic; + struct ionic_dev *idev = &ionic->idev; + + strlcpy(drvinfo->driver, DRV_NAME, sizeof(drvinfo->driver)); + strlcpy(drvinfo->version, DRV_VERSION, sizeof(drvinfo->version)); + strlcpy(drvinfo->fw_version, idev->dev_info.fw_version, + sizeof(drvinfo->fw_version)); + strlcpy(drvinfo->bus_info, ionic_bus_info(ionic), + sizeof(drvinfo->bus_info)); +} + +#define DEV_CMD_REG_VERSION 1 +#define DEV_INFO_REG_COUNT 32 +#define DEV_CMD_REG_COUNT 32 +static int ionic_get_regs_len(struct net_device *netdev) +{ + return (DEV_INFO_REG_COUNT + DEV_CMD_REG_COUNT) * sizeof(u32); +} + +static void ionic_get_regs(struct net_device *netdev, struct ethtool_regs *regs, + void *p) +{ + struct lif *lif = netdev_priv(netdev); + unsigned int size; + + regs->version = DEV_CMD_REG_VERSION; + + size = DEV_INFO_REG_COUNT * sizeof(u32); + memcpy_fromio(p, lif->ionic->idev.dev_info_regs->words, size); + + size = DEV_CMD_REG_COUNT * sizeof(u32); + memcpy_fromio(p, lif->ionic->idev.dev_cmd_regs->words, size); +} + +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); + + 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); + copper_seen++; + break; + case XCVR_PID_QSFP_40GBASE_CR4: + ethtool_link_ksettings_add_link_mode(ks, supported, + 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); + 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); + 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); + break; + case XCVR_PID_QSFP_100G_LR4: + ethtool_link_ksettings_add_link_mode(ks, supported, + 100000baseLR4_ER4_Full); + break; + case XCVR_PID_QSFP_100G_ER4: + ethtool_link_ksettings_add_link_mode(ks, supported, + 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); + break; + case XCVR_PID_QSFP_40GBASE_LR4: + ethtool_link_ksettings_add_link_mode(ks, supported, + 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); + break; + case XCVR_PID_SFP_10GBASE_SR: + ethtool_link_ksettings_add_link_mode(ks, supported, + 10000baseSR_Full); + break; + case XCVR_PID_SFP_10GBASE_LR: + ethtool_link_ksettings_add_link_mode(ks, supported, + 10000baseLR_Full); + break; + case XCVR_PID_SFP_10GBASE_LRM: + ethtool_link_ksettings_add_link_mode(ks, supported, + 10000baseLRM_Full); + break; + case XCVR_PID_SFP_10GBASE_ER: + ethtool_link_ksettings_add_link_mode(ks, supported, + 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; + 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; + } + ethtool_link_ksettings_add_link_mode(ks, supported, FEC_NONE); + ethtool_link_ksettings_add_link_mode(ks, supported, FEC_RS); + ethtool_link_ksettings_add_link_mode(ks, supported, FEC_BASER); + + memcpy(ks->link_modes.advertising, ks->link_modes.supported, + sizeof(ks->link_modes.advertising)); + + ethtool_link_ksettings_add_link_mode(ks, supported, FIBRE); + + if (ionic_is_pf(lif->ionic)) + ethtool_link_ksettings_add_link_mode(ks, supported, Autoneg); + + 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->status.xcvr.phy == PHY_TYPE_COPPER || + copper_seen) { + ks->base.port = PORT_DA; + } else if (idev->port_info->status.xcvr.phy == PHY_TYPE_FIBER) { + ks->base.port = PORT_FIBRE; + } else { + ks->base.port = PORT_OTHER; + } + + ks->base.speed = le32_to_cpu(lif->info->status.link_speed); + + if (idev->port_info->config.an_enable) + ks->base.autoneg = AUTONEG_ENABLE; + + if (le16_to_cpu(lif->info->status.link_status)) + ks->base.duplex = DUPLEX_FULL; + else + ks->base.duplex = DUPLEX_UNKNOWN; + + return 0; +} + +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_fc; + int err = 0; + + /* set autoneg */ + if (ks->base.autoneg != idev->port_info->config.an_enable) { + 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; + + idev->port_info->config.an_enable = ks->base.autoneg; + } + + /* set speed */ + if (ks->base.speed != le32_to_cpu(idev->port_info->config.speed)) { + 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; + + idev->port_info->config.speed = cpu_to_le32(ks->base.speed); + } + + /* set FEC */ + req_rs = ethtool_link_ksettings_test_link_mode(ks, advertising, FEC_RS); + req_fc = ethtool_link_ksettings_test_link_mode(ks, advertising, FEC_BASER); + if (req_rs && req_fc) { + netdev_info(netdev, "Only select one FEC mode at a time\n"); + return -EINVAL; + + } else if (req_fc && + 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_fc) && + idev->port_info->config.fec_type != PORT_FEC_TYPE_NONE) { + fec_type = PORT_FEC_TYPE_NONE; + } + + if (fec_type != idev->port_info->config.fec_type) { + mutex_lock(&ionic->dev_cmd_lock); + ionic_dev_cmd_port_fec(idev, PORT_FEC_TYPE_NONE); + err = ionic_dev_cmd_wait(ionic, devcmd_timeout); + mutex_unlock(&ionic->dev_cmd_lock); + if (err) + return err; + + idev->port_info->config.fec_type = fec_type; + } + + return 0; +} + +static void ionic_get_pauseparam(struct net_device *netdev, + struct ethtool_pauseparam *pause) +{ + struct lif *lif = netdev_priv(netdev); + struct ionic_dev *idev = &lif->ionic->idev; + uint8_t pause_type = idev->port_info->config.pause_type; + + pause->autoneg = idev->port_info->config.an_enable; + + if (pause_type) { + pause->rx_pause = pause_type & IONIC_PAUSE_F_RX ? 1 : 0; + pause->tx_pause = pause_type & IONIC_PAUSE_F_TX ? 1 : 0; + } +} + +static int ionic_set_pauseparam(struct net_device *netdev, + struct ethtool_pauseparam *pause) +{ + struct lif *lif = netdev_priv(netdev); + struct ionic *ionic = lif->ionic; + struct ionic_dev *idev = &lif->ionic->idev; + + u32 requested_pause; + u32 cur_autoneg; + int err; + + cur_autoneg = idev->port_info->config.an_enable ? AUTONEG_ENABLE : + AUTONEG_DISABLE; + if (pause->autoneg != cur_autoneg) { + netdev_info(netdev, "Please use 'ethtool -s ...' to change autoneg\n"); + return -EOPNOTSUPP; + } + + /* change both at the same time */ + requested_pause = PORT_PAUSE_TYPE_LINK; + if (pause->rx_pause) + requested_pause |= IONIC_PAUSE_F_RX; + if (pause->tx_pause) + requested_pause |= IONIC_PAUSE_F_TX; + + if (requested_pause == idev->port_info->config.pause_type) + return 0; + + idev->port_info->config.pause_type = requested_pause; + + mutex_lock(&ionic->dev_cmd_lock); + ionic_dev_cmd_port_pause(idev, requested_pause); + err = ionic_dev_cmd_wait(ionic, devcmd_timeout); + mutex_unlock(&ionic->dev_cmd_lock); + if (err) + return err; + + return 0; +} + +static int ionic_get_coalesce(struct net_device *netdev, + struct ethtool_coalesce *coalesce) +{ + struct lif *lif = netdev_priv(netdev); + + coalesce->tx_coalesce_usecs = lif->tx_coalesce_usecs; + coalesce->rx_coalesce_usecs = lif->rx_coalesce_usecs; + + return 0; +} + +static void ionic_get_ringparam(struct net_device *netdev, + struct ethtool_ringparam *ring) +{ + struct lif *lif = netdev_priv(netdev); + + ring->tx_max_pending = IONIC_MAX_TXRX_DESC; + ring->tx_pending = lif->ntxq_descs; + ring->rx_max_pending = IONIC_MAX_TXRX_DESC; + ring->rx_pending = lif->nrxq_descs; +} + +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 nothing to do return success */ + if (ring->tx_pending == lif->ntxq_descs && + ring->rx_pending == lif->nrxq_descs) + return 0; + + while (test_and_set_bit(LIF_QUEUE_RESET, lif->state)) + usleep_range(200, 400); + + running = test_bit(LIF_UP, lif->state); + if (running) + ionic_stop(netdev); + + lif->ntxq_descs = ring->tx_pending; + lif->nrxq_descs = ring->rx_pending; + + if (running) + ionic_open(netdev); + clear_bit(LIF_QUEUE_RESET, lif->state); + + return 0; +} + +static void ionic_get_channels(struct net_device *netdev, + struct ethtool_channels *ch) +{ + struct lif *lif = netdev_priv(netdev); + + /* report maximum channels */ + ch->max_combined = lif->ionic->ntxqs_per_lif; + + /* report current channels */ + ch->combined_count = lif->nxqs; +} + +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->nxqs) + return 0; + + while (test_and_set_bit(LIF_QUEUE_RESET, lif->state)) + usleep_range(200, 400); + + running = test_bit(LIF_UP, lif->state); + if (running) + ionic_stop(netdev); + + lif->nxqs = ch->combined_count; + + if (running) + ionic_open(netdev); + clear_bit(LIF_QUEUE_RESET, lif->state); + + return 0; +} + +static int ionic_get_module_info(struct net_device *netdev, + struct ethtool_modinfo *modinfo) + +{ + struct lif *lif = netdev_priv(netdev); + struct ionic_dev *idev = &lif->ionic->idev; + struct xcvr_status *xcvr; + + xcvr = &idev->port_info->status.xcvr; + + /* report the module data type and length */ + switch (xcvr->sprom[0]) { + case 0x03: /* SFP */ + modinfo->type = ETH_MODULE_SFF_8079; + modinfo->eeprom_len = ETH_MODULE_SFF_8079_LEN; + break; + case 0x0D: /* QSFP */ + case 0x11: /* QSFP28 */ + modinfo->type = ETH_MODULE_SFF_8436; + modinfo->eeprom_len = ETH_MODULE_SFF_8436_LEN; + break; + default: + netdev_info(netdev, "unknown xcvr type 0x%02x\n", + xcvr->sprom[0]); + break; + } + + return 0; +} + +static int ionic_get_module_eeprom(struct net_device *netdev, + struct ethtool_eeprom *ee, + u8 *data) +{ + struct lif *lif = netdev_priv(netdev); + struct ionic_dev *idev = &lif->ionic->idev; + struct xcvr_status *xcvr; + u32 len; + + /* copy the module bytes into data */ + xcvr = &idev->port_info->status.xcvr; + len = min_t(u32, sizeof(xcvr->sprom), ee->len); + memcpy(data, xcvr->sprom, len); + + dev_dbg(&lif->netdev->dev, "notifyblock eid=0x%llx link_status=0x%x link_speed=0x%x link_down_cnt=0x%x\n", + lif->info->status.eid, + lif->info->status.link_status, + lif->info->status.link_speed, + lif->info->status.link_down_count); + dev_dbg(&lif->netdev->dev, " port_status id=0x%x status=0x%x speed=0x%x\n", + idev->port_info->status.id, + idev->port_info->status.status, + idev->port_info->status.speed); + dev_dbg(&lif->netdev->dev, " xcvr status state=0x%x phy=0x%x pid=0x%x\n", + xcvr->state, xcvr->phy, xcvr->pid); + dev_dbg(&lif->netdev->dev, " port_config state=0x%x speed=0x%x mtu=0x%x an_enable=0x%x fec_type=0x%x pause_type=0x%x loopback_mode=0x%x\n", + idev->port_info->config.state, + idev->port_info->config.speed, + idev->port_info->config.mtu, + idev->port_info->config.an_enable, + idev->port_info->config.fec_type, + idev->port_info->config.pause_type, + idev->port_info->config.loopback_mode); + + return 0; +} + +static int ionic_nway_reset(struct net_device *netdev) +{ + struct lif *lif = netdev_priv(netdev); + int err = 0; + + if (netif_running(netdev)) + err = ionic_reset_queues(lif); + + return err; +} + +static const struct ethtool_ops ionic_ethtool_ops = { + .get_drvinfo = ionic_get_drvinfo, + .get_regs_len = ionic_get_regs_len, + .get_regs = ionic_get_regs, + .get_link = ethtool_op_get_link, + .get_link_ksettings = ionic_get_link_ksettings, + .get_coalesce = ionic_get_coalesce, + .get_ringparam = ionic_get_ringparam, + .set_ringparam = ionic_set_ringparam, + .get_channels = ionic_get_channels, + .set_channels = ionic_set_channels, + .get_module_info = ionic_get_module_info, + .get_module_eeprom = ionic_get_module_eeprom, + .get_pauseparam = ionic_get_pauseparam, + .set_pauseparam = ionic_set_pauseparam, + .set_link_ksettings = ionic_set_link_ksettings, + .nway_reset = ionic_nway_reset, +}; + +void ionic_ethtool_set_ops(struct net_device *netdev) +{ + netdev->ethtool_ops = &ionic_ethtool_ops; +} diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.h b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.h new file mode 100644 index 000000000000..38b91b1d70ae --- /dev/null +++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.h @@ -0,0 +1,9 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* Copyright(c) 2017 - 2019 Pensando Systems, Inc */ + +#ifndef _IONIC_ETHTOOL_H_ +#define _IONIC_ETHTOOL_H_ + +void ionic_ethtool_set_ops(struct net_device *netdev); + +#endif /* _IONIC_ETHTOOL_H_ */ diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c index f52af9cb6264..2bd8ce61c4a0 100644 --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c @@ -10,6 +10,7 @@ #include "ionic.h" #include "ionic_bus.h" #include "ionic_lif.h" +#include "ionic_ethtool.h" #include "ionic_debugfs.h" static void ionic_lif_rx_mode(struct lif *lif, unsigned int rx_mode); @@ -980,6 +981,7 @@ static struct lif *ionic_lif_alloc(struct ionic *ionic, unsigned int index) lif->netdev = netdev; ionic->master_lif = lif; netdev->netdev_ops = &ionic_netdev_ops; + ionic_ethtool_set_ops(netdev); netdev->watchdog_timeo = 2 * HZ; netdev->min_mtu = IONIC_MIN_MTU; diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.h b/drivers/net/ethernet/pensando/ionic/ionic_lif.h index 9930b9390c8a..d8589a306aa5 100644 --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.h +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.h @@ -111,6 +111,8 @@ struct lif { u64 last_eid; unsigned int neqs; unsigned int nxqs; + unsigned int ntxq_descs; + unsigned int nrxq_descs; unsigned int rx_mode; u64 hw_features; bool mc_overflow; @@ -124,6 +126,8 @@ struct lif { struct rx_filters rx_filters; struct ionic_deferred deferred; + u32 tx_coalesce_usecs; + u32 rx_coalesce_usecs; unsigned long *dbid_inuse; unsigned int dbid_count; struct dentry *dentry; @@ -165,6 +169,10 @@ int ionic_lif_identify(struct ionic *ionic, u8 lif_type, union lif_identity *lif_ident); int ionic_lifs_size(struct ionic *ionic); +int ionic_open(struct net_device *netdev); +int ionic_stop(struct net_device *netdev); +int ionic_reset_queues(struct lif *lif); + static inline void debug_stats_napi_poll(struct qcq *qcq, unsigned int work_done) {
Add in the basic ethtool callbacks for device information and control. Signed-off-by: Shannon Nelson <snelson@pensando.io> --- drivers/net/ethernet/pensando/ionic/Makefile | 2 +- .../net/ethernet/pensando/ionic/ionic_dev.h | 3 + .../ethernet/pensando/ionic/ionic_ethtool.c | 509 ++++++++++++++++++ .../ethernet/pensando/ionic/ionic_ethtool.h | 9 + .../net/ethernet/pensando/ionic/ionic_lif.c | 2 + .../net/ethernet/pensando/ionic/ionic_lif.h | 8 + 6 files changed, 532 insertions(+), 1 deletion(-) create mode 100644 drivers/net/ethernet/pensando/ionic/ionic_ethtool.c create mode 100644 drivers/net/ethernet/pensando/ionic/ionic_ethtool.h