Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 29 Apr 2022 11:25:45 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Nathan Lynch <nathanl@linux.ibm.com>
Cc:     Tyrel Datwyler <tyreld@linux.ibm.com>,
        linux-pci <linux-pci@vger.kernel.org>,
        Linux Kernel <linux-kernel@vger.kernel.org>,
        Mahesh Salgaonkar <mahesh@linux.ibm.com>,
        linuxppc-dev <linuxppc-dev@ozlabs.org>,
        Oliver O'Halloran <oohall@gmail.com>,
        Bjorn Helgaas <bhelgaas@google.com>
Subject: Re: [PATCH v6] PCI hotplug: rpaphp: Error out on busy status from
 get-sensor-state
Message-ID: <20220429162545.GA79541@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87k0b8q1px.fsf@linux.ibm.com>
Precedence: bulk

On Thu, Apr 28, 2022 at 05:31:38PM -0500, Nathan Lynch wrote:
> Bjorn Helgaas <helgaas@kernel.org> writes:
> > On Tue, Apr 26, 2022 at 11:07:39PM +0530, Mahesh Salgaonkar wrote:
> >> +/*
> >> + * RTAS call get-sensor-state(DR_ENTITY_SENSE) return values as per PAPR:
> >> + *    -1: Hardware Error
> >> + *    -2: RTAS_BUSY
> >> + *    -3: Invalid sensor. RTAS Parameter Error.
> >> + * -9000: Need DR entity to be powered up and unisolated before RTAS call
> >> + * -9001: Need DR entity to be powered up, but not unisolated, before RTAS call
> >> + * -9002: DR entity unusable
> >> + *  990x: Extended delay - where x is a number in the range of 0-5
> >> + */
> >> +#define RTAS_HARDWARE_ERROR	(-1)
> >> +#define RTAS_INVALID_SENSOR	(-3)
> >> +#define SLOT_UNISOLATED		(-9000)
> >> +#define SLOT_NOT_UNISOLATED	(-9001)

> >> +static int rtas_to_errno(int rtas_rc)
> >> +{
> >> +	int rc;
> >> +
> >> +	switch (rtas_rc) {
> >> +	case RTAS_HARDWARE_ERROR:
> >> +		rc = -EIO;
> >> +		break;
> >> +	case RTAS_INVALID_SENSOR:
> >> +		rc = -EINVAL;
> >> +		break;
> >> +	case SLOT_UNISOLATED:
> >> +	case SLOT_NOT_UNISOLATED:
> >> +		rc = -EFAULT;
> >> +		break;
> >> +	case SLOT_NOT_USABLE:
> >> +		rc = -ENODEV;
> >> +		break;
> >> +	case RTAS_BUSY:
> >> +	case RTAS_EXTENDED_DELAY_MIN...RTAS_EXTENDED_DELAY_MAX:
> >> +		rc = -EBUSY;
> >> +		break;
> >> +	default:
> >> +		err("%s: unexpected RTAS error %d\n", __func__, rtas_rc);
> >> +		rc = -ERANGE;
> >> +		break;
> >> +	}
> >> +	return rc;
> >
> > This basically duplicates rtas_error_rc().  Why do we need two copies?
> 
> It treats RTAS_BUSY, RTAS_EXTENDED_DELAY_MIN...RTAS_EXTENDED_DELAY_MAX
> differently, which is part of the point of this change.

I think it would reduce confusion overall to:

  - add RTAS_HARDWARE_ERROR, RTAS_INVALID_SENSOR to rtas.h

  - rename and move SLOT_UNISOLATED, etc to rtas.h; they look
    analogous to function-specific things like RTAS_SUSPEND_ABORTED

  - change rtas_error_rc() to use the RTAS_HARDWARE_ERROR, etc
    constants

  - use the constants (SLOT_NOT_USABLE) instead of "9902" in the
    commit log and code comments

> Aside: rtas_error_rc() (from powerpc's rtas.c) is badly named. Its
> conversions make sense for only a handful of RTAS calls. RTAS error
> codes have function-specific interpretations.

Maybe there's a case for factoring out the generic error codes and
have rtas_to_errno() (which sounds like maybe it should be renamed as
well) handle the function-specific part and fall back to the generic
one otherwise:

  int rtas_to_errno(int rtas_rc)
  {
    switch (rtas_rc) {
    case SLOT_UNISOLATED:
    case SLOT_NOT_UNISOLATED:
      return -EINVAL;
    case SLOT_NOT_USABLE:
      return -ENODEV;
    ...
    default:
      return rtas_error_rc(rtas_rc);
    }
  }