Re: ACPI IRQ storm with 6.10

[Bjorn Helgaas <helgaas@kernel.org>]

[+to Tony, Przemek for e1000e questions; -cc Jesse]

On Mon, Aug 19, 2024 at 07:23:42AM +0200, Jiri Slaby wrote:
> On 19. 08. 24, 6:50, Jiri Slaby wrote:
> > CC e1000e guys + Jesse (due to 75a3f93b5383) + Bjorn (due to b2c289415b2b)
> 
> Bjorn,
> 
> I am confused by these changes:
> ==========================================
> @@ -291,16 +288,13 @@ static int e1000_set_link_ksettings(struct net_device
> *net
> dev,
>          * duplex is forced.
>          */
>         if (cmd->base.eth_tp_mdix_ctrl) {
> -               if (hw->phy.media_type != e1000_media_type_copper) {
> -                       ret_val = -EOPNOTSUPP;
> -                       goto out;
> -               }
> +               if (hw->phy.media_type != e1000_media_type_copper)
> +                       return -EOPNOTSUPP;
> 
>                 if ((cmd->base.eth_tp_mdix_ctrl != ETH_TP_MDI_AUTO) &&
>                     (cmd->base.autoneg != AUTONEG_ENABLE)) {
>                         e_err("forcing MDI/MDI-X state is not supported when
> lin
> k speed and/or duplex are forced\n");
> -                       ret_val = -EINVAL;
> -                       goto out;
> +                       return -EINVAL;
>                 }
>         }
> 
> @@ -347,7 +341,6 @@ static int e1000_set_link_ksettings(struct net_device
> *netde
> v,
>         }
> 
>  out:
> -       pm_runtime_put_sync(netdev->dev.parent);
>         clear_bit(__E1000_RESETTING, &adapter->state);
>         return ret_val;
>  }
> ==========================================
> 
> So no more clear_bit(__E1000_RESETTING in the above fail paths. Is that
> intentional?

I don't remember if it was intentional, but the use of
__E1000_RESETTING is a bit subtle and I don't know what is correct.

Here's how it was used before I changed it with b2c289415b2b, i.e., in
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/intel/e1000e/ethtool.c?id=39f59c72ad3a:

  e1000_set_link_ksettings(...)
  {
    if (hw->phy.ops.check_reset_block(hw)) {
      ret_val = -EINVAL;
      goto out;
    }
    while (test_and_set_bit(__E1000_RESETTING, &adapter->state))
      usleep_range(1000, 2000);
    if (err) {
      ret_val = -EINVAL;
      goto out;
    }
    ...
  out:
    clear_bit(__E1000_RESETTING, &adapter->state);
  }

In this case, we *always* clear __E1000_RESETTING, even if we bail out
before the test_and_set_bit(__E1000_RESETTING).

It makes sense to me that we clear __E1000_RESETTING after we've set
it via test_and_set_bit() because we know it was set *here*.

But it seems wrong to me that we clear __E1000_RESETTING even when we
haven't done the test_and_set_bit() because it may have been set by a
concurrent thread executing a different operation.

  e1000_set_ringparam(...)
  {
    if ((ring->rx_mini_pending) || (ring->rx_jumbo_pending))
      return -EINVAL;
    while (test_and_set_bit(__E1000_RESETTING, &adapter->state))
      usleep_range(1000, 2000);
    err = e1000e_setup_tx_resources(...);
    if (err)
      goto out;
    ...
  out:
    clear_bit(__E1000_RESETTING, &adapter->state);
  }

But here, we *don't* clear __E1000_RESETTING if we bail out before the
test_and_set_bit(__E1000_RESETTING).  This seems like the correct
behavior.

In the e1000 driver (not the e1000e driver),
e1000_set_link_ksettings() does *not* clear __E1000_RESETTING unless
it has already done the test_and_set_bit().

b2c289415b2b changed e1000e to work that way, too.

FWIW, 3ef672ab1862 ("e1000e: ethtool unnecessarily takes device out of
RPM suspend") changed e1000e e1000_set_link_ksettings() to clear
__E1000_RESETTING even when bailing out before the test_and_set_bit().
That part of 3ef672ab1862 looks possibly buggy to me.

Bjorn