Re: [PATCH v6 2/2] s390x/pci: Reset a device in error state

[Matthew Rosato <mjrosato@linux.ibm.com>]

> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -1505,6 +1505,8 @@ static void s390_pci_device_reset(DeviceState *dev)
>          return;
>      case ZPCI_FS_STANDBY:
>          break;
> +    case ZPCI_FS_ERROR:
> +        break;
>      default:
>          pbdev->fh &= ~FH_MASK_ENABLE;
>          pbdev->state = ZPCI_FS_DISABLED;
> @@ -1517,6 +1519,11 @@ static void s390_pci_device_reset(DeviceState *dev)
>      } else if (pbdev->summary_ind) {
>          pci_dereg_irqs(pbdev);
>      }
> +
> +    if (pbdev->state == ZPCI_FS_ERROR) {
> +        s390_pci_reset(pbdev);
> +    }

I know it was mentioned in the prior version, so apologies for not
chiming in til now.  But you really need a comment block explaining why
it's OK to reset the device yet leave it in the ZPCI_FS_ERROR state
after the fact.  That's confusing.

You mentioned in the last version it's because this happens on the
disable path, and there yes we will set the device to ZPCI_FS_DISABLED
right after this -- but what about any other case where we drive this
reset path (e.g. subsystem reset, reboot, and the ISM-specific extra paths)

Note that the non-error cases are ensuring we either leave this function
with the device listed as disabled or standby/reserved.

Would there be harm in, after performing the reset, doing
pbdev->fh &= ~FH_MASK_ENABLE;
pbdev->state = ZPCI_FS_DISABLED;
for the ZPCI_FS_ERROR case now?

Consider that:
1) For a guest-initiated disable, we already checked those were valid
before reaching this point.
2) For all other reset cases, if the device were not in
error/reserved/standby we were leaving here with the device disabled
before this patch.  If the device happened to be in error, now you fix
it up but isn't the proper fix state now 'disabled' to match what an
enabled device would have been?

Thanks,
Matt