Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] ipmi: update ssif max_xmit_msg_size limit for multi-part
 messages
To:     Kamlakant Patel <kamlakant.patel@cavium.com>
Cc:     openipmi-developer@lists.sourceforge.net,
        linux-kernel@vger.kernel.org
References: <1530784849-3376-1-git-send-email-kamlakant.patel@cavium.com>
From:   Corey Minyard <cminyard@mvista.com>
Message-ID: <c457c4c4-7ed4-e465-54b6-68151ea04f0b@mvista.com>
Date:   Fri, 6 Jul 2018 11:52:03 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.8.0
MIME-Version: 1.0
In-Reply-To: <1530784849-3376-1-git-send-email-kamlakant.patel@cavium.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-GB
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 07/05/2018 05:00 AM, Kamlakant Patel wrote:
> There is an issue with Host name Write command with payload size of 132 bytes.
> Currently ipmi driver limits max_xmit_msg_size to 63 bytes. Due to this
> all IPMI commands with request size more than 63 bytes will not work.
> This is seen with AMI OEM Host Name write command: payload 132 bytes.
> When a DNS host name set is tried from host through SSIF interface, the name
> gets truncated due to ipmi_ssif driver limiting the length to 63 bytes
>
> As per IPMI Spec v2.0 section 12.3:
> The maximum message size returned by the Get SSIF Interface Capabilities command
> is 255 bytes.
>
> This patch updates the max_xmit_msg_size to 255 in case SSIF_MULTI_n_PART.

Umm, did you read that monstrous comment just above that code?  It's 
there for a reason.

The specification is very strange here.  If you read it carefully, you 
will be confused.

In the 3rd paragraph of section 12.3, it says:

    The combination of a Start transaction followed by an End
    transaction can transfer up
    to 63 bytes of IPMI message.  The Middle transaction is available
    when there is a need
    to transfer an IPMI message of greater than 63 bytes.

If you read on, in a multi-part message, the first part must carry 32 
bytes.  All
intermediate ("middle") parts must carry 32 bytes.  It says the end part
can carry 1-32 bytes, but not zero bytes.  This is a typo, though. You 
will discover
that the end part can be 1-31 bytes, not 32 bytes, because the end
part is marked by the length being less than 32 bytes (even though it
doesn't explicitly say this any place).  If you look at Table 12-3 "BMC 
Multi-part Write
Middle" and Table 12-4 "BMC Multi-part Write End" they are exactly the same
except that the length in 12-4 is not =20h.  Thus the 63 bytes maximum for
two parts.

That means that you *cannot* send a 64-byte message.  If you have to 
have a middle
transaction to carry >63 bytes of data, and the first and middle must be 
32 bytes, and
the end cannot be zero bytes or 32 bytes, you are stuck.

Then at the end it mockingly says:

    Software that uses the Middle transaction should take care to
    correctly handle the
    cases where the number of IPMI message bytes is an exact multiple of 32.

It then gives no clue about what to do in this case.

So I cannot take this change unless I know what should really be done 
here.  I'm
pretty sure the middle part handling code is broken if you allow >63 
byte messages,
too, so it will take more than just this change.  I believe the current 
code will send
a zero-byte end message in that case.

-corey

>
> Signee-off-by: Kamlakant Patel <kamlakant.patel@cavium.com>
> ---
>   drivers/char/ipmi/ipmi_ssif.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> index 18e4650..2bf6f07 100644
> --- a/drivers/char/ipmi/ipmi_ssif.c
> +++ b/drivers/char/ipmi/ipmi_ssif.c
> @@ -88,6 +88,8 @@
>   #define SSIF_MSG_JIFFIES	((SSIF_MSG_USEC * 1000) / TICK_NSEC)
>   #define SSIF_MSG_PART_JIFFIES	((SSIF_MSG_PART_USEC * 1000) / TICK_NSEC)
>   
> +#define SSIF_MAX_MSG_LENGTH	255
> +
>   enum ssif_intf_state {
>   	SSIF_NORMAL,
>   	SSIF_GETTING_FLAGS,
> @@ -1500,8 +1502,8 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id)
>   			 * to be 1-31 bytes in length.  Not ideal, but
>   			 * it should work.
>   			 */
> -			if (ssif_info->max_xmit_msg_size > 63)
> -				ssif_info->max_xmit_msg_size = 63;
> +			if (ssif_info->max_xmit_msg_size > SSIF_MAX_MSG_LENGTH)
> +				ssif_info->max_xmit_msg_size = SSIF_MAX_MSG_LENGTH;
>   			break;
>   
>   		default: