openbmc.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
From: "Winiarska, Iwona" <iwona.winiarska@intel.com>
To: "zweiss@equinix.com" <zweiss@equinix.com>
Cc: "linux-aspeed@lists.ozlabs.org" <linux-aspeed@lists.ozlabs.org>,
	"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
	"jae.hyun.yoo@linux.intel.com" <jae.hyun.yoo@linux.intel.com>,
	"mchehab@kernel.org" <mchehab@kernel.org>,
	"corbet@lwn.net" <corbet@lwn.net>,
	"openbmc@lists.ozlabs.org" <openbmc@lists.ozlabs.org>,
	"x86@kernel.org" <x86@kernel.org>,
	"pierre-louis.bossart@linux.intel.com"
	<pierre-louis.bossart@linux.intel.com>,
	"mingo@redhat.com" <mingo@redhat.com>,
	"linux@roeck-us.net" <linux@roeck-us.net>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"jdelvare@suse.com" <jdelvare@suse.com>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"bp@alien8.de" <bp@alien8.de>,
	"Lutomirski, Andy" <luto@kernel.org>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"andriy.shevchenko@linux.intel.com"
	<andriy.shevchenko@linux.intel.com>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"linux-hwmon@vger.kernel.org" <linux-hwmon@vger.kernel.org>,
	"Luck,  Tony" <tony.luck@intel.com>,
	"andrew@aj.id.au" <andrew@aj.id.au>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"yazen.ghannam@amd.com" <yazen.ghannam@amd.com>
Subject: Re: [PATCH 09/14] peci: Add support for PECI device drivers
Date: Fri, 30 Jul 2021 20:13:14 +0000	[thread overview]
Message-ID: <5476aacfd0c83fbe69c9eb2af3327ff29a03674d.camel@intel.com> (raw)
In-Reply-To: <20210729232205.GX8018@packtop>

On Thu, 2021-07-29 at 23:22 +0000, Zev Weiss wrote:
> On Thu, Jul 29, 2021 at 04:17:06PM CDT, Winiarska, Iwona wrote:
> > On Tue, 2021-07-27 at 20:10 +0000, Zev Weiss wrote:
> > > On Mon, Jul 12, 2021 at 05:04:42PM CDT, Iwona Winiarska wrote:
> > > > 
> > > > 
> > > > +#define REVISION_NUM_MASK GENMASK(15, 8)
> > > > +static int peci_get_revision(struct peci_device *device, u8 *revision)
> > > > +{
> > > > +       struct peci_request *req;
> > > > +       u64 dib;
> > > > +
> > > > +       req = peci_get_dib(device);
> > > > +       if (IS_ERR(req))
> > > > +               return PTR_ERR(req);
> > > > +
> > > > +       dib = peci_request_data_dib(req);
> > > > +       if (dib == 0) {
> > > > +               peci_request_free(req);
> > > > +               return -EIO;
> > > 
> > > Any particular reason to check for zero specifically here?  It looks
> > > like that would be a case where the host CPU responds and everything's
> > > otherwise fine, but the host just happened to send back a bunch of zeros
> > > for whatever reason -- which may not be a valid PECI revision number,
> > > but if it sent back a bunch of 0xff bytes instead wouldn't that be
> > > equally invalid?
> > 
> > The response with all 0's is possible (and defined) in certain device
> > states. If
> > that happens - we don't want to continue adding the device (with "invalid"
> > revision 0), we just want to return error.
> > 
> 
> Okay, that's reasonable -- maybe worth a brief comment though.

/*
 * PECI device may be in a state where it is unable to return a proper DIB,
 * in which case it returns 0 as DIB value.
 * Let's treat this as an error to avoid carrying on with the detection using
 * invalid revision.
 */

> 
> > > 
> > > Also, given that the docs (the ones I have, at least) describe the DIB
> > > as a collection of individual bytes, dealing with it as a combined u64
> > > seems a bit confusing to me -- could we just return req->rx.buf[1]
> > > instead?
> > 
> > GetDIB returns 8-byte response, which is why we're treating it in this way
> > (similar to other commands). We're pulling out the whole response and use
> > FIELD_GET to obtain the data we need.
> > 
> 
> Sure -- but since the 8 bytes that GetDIB retrieves are a device info
> byte, a revision number byte, and six reserved bytes (at least as of the
> documentation I have access to), I'm not sure why we want to pack that
> all up into a u64 only to unpack a single byte from it a moment later
> with FIELD_GET(), when we've already got it in a convenient
> array-of-bytes form (req->rx.buf).  I could understand wanting a u64 if
> the 8 bytes were all a single value, or if it had sub-fields that
> spanned byte boundaries in awkward ways or something, but given the
> format of the response data a byte array seems like the most natural way
> of dealing with it.
> 
> I suppose it facilitates an easy zero check, but that could also be
> written as !memchr_inv(req->rx.buf, 0, 8) in the non-u64 case.

What you suggest would look like this:

static int peci_get_revision(struct peci_device *device, u8 *revision)
{
        struct peci_request *req;

        req = peci_get_dib(device);
        if (IS_ERR(req))
                return PTR_ERR(req);

        if (!memchr_inv(req->rx.buf, 0, PECI_GET_DIB_RD_LEN)) {
                peci_request_free(req);
                return -EIO;
        }
        *revision = req->rx.buf[1];

        peci_request_free(req);

        return 0;
}

Note that the caller (device.c) now needs to know read length -
PECI_GET_DIB_RD_LEN (which currently is internal to the request.c) and is
digging into rx.buf directly (rather than using helper from internal.h).

By forcing the callers to use helper functions, we can make things consistent
across various commands and avoid exporting everything to everyone using a giant
header with all definitions.

I would prefer to keep peci_get_revision() as is.

Thanks
-Iwona


  reply	other threads:[~2021-07-30 20:14 UTC|newest]

Thread overview: 72+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-12 22:04 [PATCH 00/14] Introduce PECI subsystem Iwona Winiarska
2021-07-12 22:04 ` [PATCH 01/14] x86/cpu: Move intel-family to arch-independent headers Iwona Winiarska
2021-07-14 16:54   ` Williams, Dan J
2021-07-15 16:47     ` Winiarska, Iwona
2021-07-15 18:13       ` Dan Williams
2021-07-15 18:29         ` Luck, Tony
2021-07-12 22:04 ` [PATCH 02/14] x86/cpu: Extract cpuid helpers to arch-independent Iwona Winiarska
2021-07-14 16:58   ` Williams, Dan J
2021-07-15 16:51     ` Winiarska, Iwona
2021-07-15 16:58       ` Winiarska, Iwona
2021-07-12 22:04 ` [PATCH 03/14] dt-bindings: Add generic bindings for PECI Iwona Winiarska
2021-07-12 22:04 ` [PATCH 04/14] dt-bindings: Add bindings for peci-aspeed Iwona Winiarska
2021-07-15 16:28   ` Rob Herring
2021-07-16 21:22     ` Winiarska, Iwona
2021-07-12 22:04 ` [PATCH 05/14] ARM: dts: aspeed: Add PECI controller nodes Iwona Winiarska
2021-07-12 22:04 ` [PATCH 06/14] peci: Add core infrastructure Iwona Winiarska
2021-07-14 17:19   ` Williams, Dan J
2021-07-16 21:08     ` Winiarska, Iwona
2021-07-16 21:50       ` Dan Williams
2021-07-17  6:12         ` gregkh
2021-07-17 20:54           ` Dan Williams
2021-07-12 22:04 ` [PATCH 07/14] peci: Add peci-aspeed controller driver Iwona Winiarska
2021-07-13  5:02   ` Randy Dunlap
2021-07-15 16:42     ` Winiarska, Iwona
2021-07-14 17:39   ` Williams, Dan J
2021-07-16 21:17     ` Winiarska, Iwona
2021-07-27  8:49   ` Zev Weiss
2021-07-29 14:03     ` Winiarska, Iwona
2021-07-29 18:15       ` Zev Weiss
2021-07-12 22:04 ` [PATCH 08/14] peci: Add device detection Iwona Winiarska
2021-07-14 21:05   ` Williams, Dan J
2021-07-16 21:20     ` Winiarska, Iwona
2021-07-27 17:49   ` Zev Weiss
2021-07-29 18:55     ` Winiarska, Iwona
2021-07-29 20:50       ` Zev Weiss
2021-07-30 20:10         ` Winiarska, Iwona
2021-07-12 22:04 ` [PATCH 09/14] peci: Add support for PECI device drivers Iwona Winiarska
2021-07-27 20:10   ` Zev Weiss
2021-07-27 21:23     ` Guenter Roeck
2021-07-29 21:17     ` Winiarska, Iwona
2021-07-29 23:22       ` Zev Weiss
2021-07-30 20:13         ` Winiarska, Iwona [this message]
2021-07-12 22:04 ` [PATCH 10/14] peci: Add peci-cpu driver Iwona Winiarska
2021-07-27 11:16   ` David Müller (ELSOFT AG)
2021-07-30 20:14     ` Winiarska, Iwona
2021-07-27 21:33   ` Zev Weiss
2021-07-30 21:21     ` Winiarska, Iwona
2021-07-12 22:04 ` [PATCH 11/14] hwmon: peci: Add cputemp driver Iwona Winiarska
2021-07-15 17:45   ` Guenter Roeck
2021-07-19 20:12     ` Winiarska, Iwona
2021-07-19 20:35       ` Guenter Roeck
2021-07-27  7:06   ` Zev Weiss
2021-07-30 21:51     ` Winiarska, Iwona
2021-07-30 22:04       ` Guenter Roeck
2021-07-12 22:04 ` [PATCH 12/14] hwmon: peci: Add dimmtemp driver Iwona Winiarska
2021-07-15 17:56   ` Guenter Roeck
2021-07-19 20:31     ` Winiarska, Iwona
2021-07-19 20:36       ` Guenter Roeck
2021-07-26 22:08   ` Zev Weiss
2021-07-30 22:48     ` Winiarska, Iwona
2021-07-12 22:04 ` [PATCH 13/14] docs: hwmon: Document PECI drivers Iwona Winiarska
2021-07-27 22:58   ` Zev Weiss
2021-07-28  0:49     ` Guenter Roeck
2021-08-02 11:39       ` Winiarska, Iwona
2021-08-02 11:37     ` Winiarska, Iwona
2021-08-04 17:52       ` Zev Weiss
2021-08-04 18:05         ` Guenter Roeck
2021-08-05 21:42           ` Winiarska, Iwona
2021-07-12 22:04 ` [PATCH 14/14] docs: Add PECI documentation Iwona Winiarska
2021-07-14 16:51 ` [PATCH 00/14] Introduce PECI subsystem Williams, Dan J
2021-07-15 17:33   ` Winiarska, Iwona
2021-07-15 19:34     ` Dan Williams

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5476aacfd0c83fbe69c9eb2af3327ff29a03674d.camel@intel.com \
    --to=iwona.winiarska@intel.com \
    --cc=andrew@aj.id.au \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=bp@alien8.de \
    --cc=corbet@lwn.net \
    --cc=devicetree@vger.kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=jae.hyun.yoo@linux.intel.com \
    --cc=jdelvare@suse.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-aspeed@lists.ozlabs.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-hwmon@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=luto@kernel.org \
    --cc=mchehab@kernel.org \
    --cc=mingo@redhat.com \
    --cc=openbmc@lists.ozlabs.org \
    --cc=pierre-louis.bossart@linux.intel.com \
    --cc=robh+dt@kernel.org \
    --cc=tglx@linutronix.de \
    --cc=tony.luck@intel.com \
    --cc=x86@kernel.org \
    --cc=yazen.ghannam@amd.com \
    --cc=zweiss@equinix.com \
    --subject='Re: [PATCH 09/14] peci: Add support for PECI device drivers' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).