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
