From patchwork Thu Aug 10 02:26:40 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Corey Minyard X-Patchwork-Id: 800072 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.ozlabs.org (lists.ozlabs.org [103.22.144.68]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3xSY1P72ktz9sNc for ; Thu, 10 Aug 2017 13:06:01 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="DdxLco0I"; dkim-atps=neutral Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id 3xSY1P5hcYzDqws for ; Thu, 10 Aug 2017 13:06:01 +1000 (AEST) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="DdxLco0I"; dkim-atps=neutral X-Original-To: openbmc@lists.ozlabs.org Delivered-To: openbmc@lists.ozlabs.org Received: from mail-oi0-x241.google.com (mail-oi0-x241.google.com [IPv6:2607:f8b0:4003:c06::241]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 3xSX8B4Q9hzDqrx for ; Thu, 10 Aug 2017 12:26:50 +1000 (AEST) Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="DdxLco0I"; dkim-atps=neutral Received: by mail-oi0-x241.google.com with SMTP id q70so7389892oic.2 for ; Wed, 09 Aug 2017 19:26:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=reply-to:subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language; bh=/Mmt3oc2pA70bOPKoVJOw5dLxCgZ3C3LzUrkrQ3EEKc=; b=DdxLco0IsFdPmBQkO6nLcMmVVa3BevXiL7SYYNSIlknARg9dmKkWpRI1xod9yfpRGy 5YXGUYlk/fN22iprKW/EQzS0AtE5JWg2gYhh6clxTj8doeo4KmllF29s5rryRwId/PjF y1KVAtNJoXfo6Nht9pzV1MPe4CRI9CV2ig6slaubAzI5/3mjfCJARsNv+YiEJfBb7rig +rNqYhqAwoO+R9kmHQsYQfp6tYT122KO8fRVvKl15KoemewbvL79DbArhuTBUUBc6RG0 NPfk/uL8BEmnMZ+e2nNiUjvCZI5JO5e2QEU01E8wHC+xqXD3ucrE/Kq4OAh9l62LW7Cy XzFg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:reply-to:subject:to:cc:references:from :message-id:date:user-agent:mime-version:in-reply-to :content-language; bh=/Mmt3oc2pA70bOPKoVJOw5dLxCgZ3C3LzUrkrQ3EEKc=; b=VmylB/othHRjEAwA243sC2SCl9duLp9OZRddnheAd5o/+H96nbSK0bNUFSSfWiYtSt 8exld9Y9khVhJWFfynnQuYVAVDPnBacGmXWLnhJt+vb2LajOoNE/5gjwndom1rPlPPxR 9rDdLX4szSVfwJLYyA/oTwHGJ9bDZUNDNDWEkGHj2gCvMJ8Dhv51F4oh4CfxVobEDth/ R7thiyc/PsyQB4eM4ItTZys65iWtSwMn00EFDk2vbG6hDKJSH8d3goPN3/xtCXtbhpj9 Ry1RyZlo61veRS3bd6zzsGolWDWijSn4ykphyyZodXheK8v0Q5EJw02HTS1CeNxUf4x5 VpeA== X-Gm-Message-State: AHYfb5hq0w70ZAIzCziPLBbIBOaaurocrQbXOtWYQE7WBhWoanxDLMrv ZtDhz+vQOdkNNw== X-Received: by 10.202.44.148 with SMTP id s142mr11561592ois.206.1502332008268; Wed, 09 Aug 2017 19:26:48 -0700 (PDT) Received: from [192.168.27.3] ([47.184.154.34]) by smtp.gmail.com with ESMTPSA id f204sm4414326oih.41.2017.08.09.19.26.45 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 09 Aug 2017 19:26:46 -0700 (PDT) Subject: Re: [PATCH v2 0/4] ipmi: bt-i2c: added IPMI Block Transfer over I2C To: Brendan Higgins , Corey Minyard References: <20170805011855.1027-1-brendanhiggins@google.com> <8f5579cb-2c4d-2cd0-e874-a44374e08417@acm.org> From: Corey Minyard Message-ID: <9c0aef7c-6dcc-2328-f712-d35ef81ebc41@gmail.com> Date: Wed, 9 Aug 2017 21:26:40 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: Content-Language: en-GB X-Mailman-Approved-At: Thu, 10 Aug 2017 13:05:18 +1000 X-BeenThere: openbmc@lists.ozlabs.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Development list for OpenBMC List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: minyard@acm.org Cc: Mark Rutland , devicetree , Benjamin Fair , Arnd Bergmann , Jonathan Corbet , Greg KH , OpenBMC Maillist , linux-doc@vger.kernel.org, Linux Kernel Mailing List , Rob Herring , openipmi-developer@lists.sourceforge.net Errors-To: openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org Sender: "openbmc" On 08/09/2017 08:04 PM, Brendan Higgins wrote: >> Perhaps that is some level of abuse, but it's pretty common. I'm not >> against it. >> >> There is standard IPMI firmware NetFN (though no commands defined) that if >> you use >> the driver automatically goes into "Maintenance mode" and modified the >> timeouts >> and handling to some extent to help with this. > That is a really good point, I missed that. > ... >> >> There are ways to accomplish this that aren't that complex. You can create >> an OEM >> command that can query the maximum message size and the ability to do >> sequence >> numbers in the messages. >> >> If messages larger than 32-bytes are supported, and the host I2C/SMBus >> driver >> supports it, you could use the standard SSIF SMBus commands to do this, they >> have an 8-bit length field. >> >> If sequence numbers are supported, The SSIF could use different SMBus >> commands >> to do the write and read requests. Since this is only if you get an OEM >> command, >> and if you put the sequence numbers at the end where they are easy to add on >> the send side, this is a small change to the driver. > What if we just had an OEM command that changed the message structure from > that point on? We could abuse the "maintenance mode" NetFN to get back into > normal SSIF if necessary. Actually, I wouldn't have a separate "openbmc mode". I would have OpenBMC always work with standard SSIF, and have separate SMBus commands for messages with the sequence number and messages larger than 32 bytes. I've attached a patch with what I would expect the changes to be to the host driver. It doesn't handle multiple outstanding messages, but it shows what detection and a separate SMBus command would look like. >> So I think the changes would be small and contained. I'm actually ok with a >> different driver, but I think it would be more valuable to the OpenBMC >> project >> to have a standardized interface that would work (in a not quite as >> efficient >> mode) with software that does not use the Linux IPMI driver. > I guess I see the all of my asks as hacky things which we can hopefully remove > at some point. Hopefully, most OpenBMC users won't want or need these things. > ... >>> Regardless of what we do with the "BT-I2C" stuff, I am still interested in >>> what >>> you think about this. >> >> I think you are right, it probably belongs some place else. The way that >> makes the most >> sense to me would be to have an "ipmi" directory with a "host" and "slave" >> side, and since >> ipmi is not really a char driver, to move it to the main driver directory. >> That might be >> fairly disruptive, though. > That was my thinking exactly. > >> The other option that makes sense to me would be to add a >> drivers/char/ipmi_slave directory, >> or something like that, and put the slave code there. That would be less >> disruptive. > Right that is the approach I took, except I called it drivers/char/ipmi_bmc. > > I originally thought doing the less disruptive thing is best; however, I know > there are also some OpenBMC people who are interested in implementing > IPMB. So maybe now is the time to bite the bullet and create an ipmi > directory under drivers/. I'm not sure IPMB would make much difference, there's no host side change as it's already supported. I don't think there would be any significant code sharing between the two. If there end up being a significant amount of common code, then it would definitely be worth the effort to move it. >> -corey > In summary, I think I can live with making it a mangled form of SSIF, but > I would prefer to put it in its own driver. You can look at the patch and consider it, and consider that you would need to implement flag and event handling. On an x86 host there would be SMBIOS and ACPI stuff to deal with somehow for discovery. There's probably few other things to deal with. > In any case, I think I would rather focus on the the BMC side IPMI framework > now, since it is a bigger change and would also reduce the work of > implementing a BMC side SSIF driver. > > Here is what I propose: we focus on the BMC side IPMI framework RFC that > I sent out the other day: > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1463473.html > I will add a change to the BMC side IPMI framework patchset to move all the > IPMI stuff to the new drivers/ipmi directory as discussed and then drop the > patch in that patchset that depends on this patchset. > > Let me know what you think Let's hold off on the new directory, there's probably some convincing of the "powers that be" for that. I'll look at the patch set tomorrow, unless something critical comes up. Thanks, -corey diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c index 11237e8..d467b1a 100644 --- a/drivers/char/ipmi/ipmi_ssif.c +++ b/drivers/char/ipmi/ipmi_ssif.c @@ -60,6 +60,11 @@ #define IPMI_GET_SYSTEM_INTERFACE_CAPABILITIES_CMD 0x57 +static u8 openbmc_iana[3] = { 0x10, 0x20, 0x30 }; +#define IPMI_OPENBMC_CAPABILITY_REQUEST_CMD 0x01 +#define SSIF_OPENBMC_REQUEST 0x80 +#define SSIF_OPENBMC_RESPONSE 0x81 + #define SSIF_IPMI_REQUEST 2 #define SSIF_IPMI_MULTI_PART_REQUEST_START 6 #define SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE 7 @@ -224,6 +229,12 @@ struct ssif_info { bool supports_alert; /* + * OpenBMC mode, with large message and sequence number + * support. An int because we use it as a number, too. + */ + unsigned int is_openbmc; + + /* * Used to tell what we should do with alerts. If we are * waiting on a response, read the data immediately. */ @@ -537,12 +548,13 @@ static void msg_done_handler(struct ssif_info *ssif_info, int result, static void start_get(struct ssif_info *ssif_info) { int rv; + int command = ssif_info->is_openbmc ? SSIF_OPENBMC_RESPONSE : + SSIF_IPMI_RESPONSE; ssif_info->rtc_us_timer = 0; ssif_info->multi_pos = 0; - rv = ssif_i2c_send(ssif_info, msg_done_handler, I2C_SMBUS_READ, - SSIF_IPMI_RESPONSE, + rv = ssif_i2c_send(ssif_info, msg_done_handler, I2C_SMBUS_READ, command, ssif_info->recv, I2C_SMBUS_BLOCK_DATA); if (rv < 0) { /* request failed, just return the error. */ @@ -611,7 +623,8 @@ static void msg_done_handler(struct ssif_info *ssif_info, int result, * start messing with driver states or the queues. */ - if (result < 0) { + if (result < 0 || len < 3) { + ignore_msg: ssif_info->retries_left--; if (ssif_info->retries_left > 0) { ssif_inc_stat(ssif_info, receive_retries); @@ -633,7 +646,13 @@ static void msg_done_handler(struct ssif_info *ssif_info, int result, goto continue_op; } - if ((len > 1) && (ssif_info->multi_pos == 0) + if (ssif_info->is_openbmc) { + if (len < 4) + goto ignore_msg; + /* Eventually extract sequence number from data[0]. */ + data++; + len--; + } else if ((len > 1) && (ssif_info->multi_pos == 0) && (data[0] == 0x00) && (data[1] == 0x01)) { /* Start of multi-part read. Start the next transaction. */ int i; @@ -966,7 +985,11 @@ static int start_resend(struct ssif_info *ssif_info) ssif_info->got_alert = false; - if (ssif_info->data_len > 32) { + if (ssif_info->is_openbmc) { + ssif_info->multi_data = NULL; + command = SSIF_OPENBMC_REQUEST; + ssif_info->data[0] = ssif_info->data_len; + } else if (ssif_info->data_len > 32) { command = SSIF_IPMI_MULTI_PART_REQUEST_START; ssif_info->multi_data = ssif_info->data; ssif_info->multi_len = ssif_info->data_len; @@ -1000,7 +1023,11 @@ static int start_send(struct ssif_info *ssif_info, return -E2BIG; ssif_info->retries_left = SSIF_SEND_RETRIES; - memcpy(ssif_info->data + 1, data, len); + memcpy(ssif_info->data + 1 + ssif_info->is_openbmc, data, len); + if (ssif_info->is_openbmc) { + ssif_info->data[1] = 0; /* Sequence number eventually. */ + len++; + } ssif_info->data_len = len; return start_resend(ssif_info); } @@ -1507,7 +1534,7 @@ static struct ipmi_fw_revision_range ssif_broken_alert_bmcs[] = { static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id) { - unsigned char msg[3]; + unsigned char msg[6]; unsigned char *resp; struct ssif_info *ssif_info; int rv = 0; @@ -1643,6 +1670,24 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id) ssif_info->supports_pec = 0; } + msg[0] = IPMI_NETFN_OEM_GROUP_REQUEST << 2; + msg[1] = IPMI_OPENBMC_CAPABILITY_REQUEST_CMD; + memcpy(msg, openbmc_iana, sizeof(openbmc_iana)); + rv = do_cmd(client, 5, msg, &len, resp); + if (!rv && (len >= 3) && (resp[2] == 0)) { + /* It appears we have an OpenBMC device. */ + + /* Turn of multi support. */ + ssif_info->multi_support = SSIF_NO_MULTI; + ssif_info->max_xmit_msg_size = 32; + ssif_info->max_recv_msg_size = 32; + if (len > 3) + ssif_info->max_xmit_msg_size = resp[3]; + if (len > 4) + ssif_info->max_recv_msg_size = resp[4]; + ssif_info->is_openbmc = 1; + } + /* Make sure the NMI timeout is cleared. */ msg[0] = IPMI_NETFN_APP_REQUEST << 2; msg[1] = IPMI_CLEAR_MSG_FLAGS_CMD; diff --git a/include/uapi/linux/ipmi_msgdefs.h b/include/uapi/linux/ipmi_msgdefs.h index df97e6e..bb0af2e 100644 --- a/include/uapi/linux/ipmi_msgdefs.h +++ b/include/uapi/linux/ipmi_msgdefs.h @@ -71,6 +71,9 @@ #define IPMI_NETFN_FIRMWARE_REQUEST 0x08 #define IPMI_NETFN_FIRMWARE_RESPONSE 0x09 +#define IPMI_NETFN_OEM_GROUP_REQUEST 0x2e +#define IPMI_NETFN_OEM_GROUP_RESPONSE 0x2f + /* The default slave address */ #define IPMI_BMC_SLAVE_ADDR 0x20