Received-SPF: pass (google.com: domain of linux-kernel+bounces-214228-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <9559478e-5382-4de6-a755-29b115d5f5c9@os.amperecomputing.com>
Date: Fri, 14 Jun 2024 09:29:11 +0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 2/2] ipmi: ssif_bmc: support skipping ARM SBMR
 bootprogress response
To: Potin Lai <potin.lai.pt@gmail.com>, Corey Minyard <minyard@acm.org>,
 Rob Herring <robh+dt@kernel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
 Conor Dooley <conor+dt@kernel.org>
Cc: openipmi-developer@lists.sourceforge.net, devicetree@vger.kernel.org,
 linux-kernel@vger.kernel.org, Patrick Williams <patrick@stwcx.xyz>,
 Cosmo Chou <cosmo.chou@quantatw.com>, Potin Lai <potin.lai@quantatw.com>
References: <20240612043255.1849007-1-potin.lai.pt@gmail.com>
 <20240612043255.1849007-3-potin.lai.pt@gmail.com>
Content-Language: en-CA
From: Quan Nguyen <quan@os.amperecomputing.com>
In-Reply-To: <20240612043255.1849007-3-potin.lai.pt@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?TGQxSGFqUjg3Zm9KTGExbHV3cTE4Ty9xM0Z5TFN5RnlhaWxybkhtZFRZOG9H?=
 =?utf-8?B?dm9qV1padGJqSWttbzQxQkVYU2YvMzlTcHdKdWc5ZGtzZFc4Vjd4ZWdMWGx3?=
 =?utf-8?B?RHROSWg5WTBEQlhtNkJCVWxSZFZWaG1EeDgvdXBhdDdFZk8wK2lmSXNOckFH?=
 =?utf-8?B?RUxSR0VwcW5hWStnSlh5akhiQWF1aEZLci9hR3kwQVVNV1I0WkR3R2srQU01?=
 =?utf-8?B?VnIzamYrbFlaYlRHZjZncGVKWHBrYjJuUVgyUXBWdTA5YjJFM25SeXN2UG42?=
 =?utf-8?B?K2V4elpBZUFjSmM3SlcxME8rUXhRMnYrcFBQaGxKS2Q1cUttUzNRcEVZRHBs?=
 =?utf-8?B?bWtLSERVMk9qOCs1ZTNaREFrYTJTMGVGRFpoNzVkUWcyT1NCWjJsWkNRTUZZ?=
 =?utf-8?B?cmdUb0huZEV3eEwyR1Urc3dBTmVjaGhrOUhGSk1yQTJCQVhWbjdBRHh6Z3Vt?=
 =?utf-8?B?MkpLZ2tVcUw0Z2ZnUzIvTlc1ZjVVTHhHK1JrOTIremt3cEZWTEFGVE9rc1lQ?=
 =?utf-8?B?dU9DOG1TSUVjNFdXMXR2UFhlSVJLMHpFeFM5ampRQnFHUWw0dkduUmhZcS9Z?=
 =?utf-8?B?TGFFT3RvYVBBdStzVFhtQU51RS9ETHNOeDZsS1NvQnVTS2dyancrWStyK0FU?=
 =?utf-8?B?Qk1OczNlUGplSTF2MXk5NXhGOStMU0Z5WU1UdUFBZllNNUVSME0wVitHdXBl?=
 =?utf-8?B?c25SdFVPeURKUlBMT0JISGxnQmgxVHgyYmNvNHdkelFaSEhsWC9jeUNzdFhs?=
 =?utf-8?B?OXJ2b0RTT2lVWCtVaXFHTXhCWU9BYUZnbUFKRjdrekFGT3ZzTzJOMzI5cFRV?=
 =?utf-8?B?SC9QeGp3NTVhMnZidGFRVU9YZjNNdm1yZHZWREZnTTFISkYzeUMwTWE1WFJz?=
 =?utf-8?B?c2FOSHp4T3kzUm5jVzI4NjVoUVVnUUJTam9tWWNjRWlDOEZMMVA3Y09MYTE3?=
 =?utf-8?B?bHBCeWhXK3EyaTQ4Z0ErdnlZVTJZWlJ5ZWVVSlgwdVY4L25aekxMZ2duK0Zv?=
 =?utf-8?B?dzZ2bTBiTFBMNnBuZzhnOUpIbk1FNTd3MDI3aW5xN3lJTHVzZHBPYy83TFQ3?=
 =?utf-8?B?aUJzQWFST2ZHVlpFNlg2UlJEeWJLRUQ2WjRCNnQzOVhRRnd4NldJNmxOa1lT?=
 =?utf-8?B?S2hpM0FHdUFDRTU2ZlZDYmsrTWQvL3JZMnArdXNWU3RrTWZtYnpaR0R1bzNh?=
 =?utf-8?B?UWdhajdrN3hiZ3diemRzeFNxM3BpdkZMMjcwTys0MUd1QzBIUHBZWDZaMlUw?=
 =?utf-8?B?UTY3bXdBaXp0ckVtNk5SbzVXTVpVTjVCLzMvOTZvc0RiQ3ZORS9ZcEk3eUMr?=
 =?utf-8?B?KzJscDdSVmdvNmJJTFBwRXR1ZFh3ckxMZ1AwSjFIWmMzMk9TWU5XYnBBTFMr?=
 =?utf-8?B?TnVFaHFEYzZ3REtsU2dqb0hMTFhKTG8vN2lWWHVKM2w5azRobDB3RzlEcnVD?=
 =?utf-8?B?ODdmYjhFZHMySWJkcVcxWkNMSWRRLytVMDg1QWVpZjBQU044cXFpa0NicnBP?=
 =?utf-8?B?c09uUFVZaWJJMG9hSVNvMW1iV0VROTFkdVpzYmdRenl1ZVZSTXRhbkRCVk9Y?=
 =?utf-8?B?RGF4VXdRVW5rQlYxVUxUZDV1NHNuYUhqTERKamtEVVZPWVl3dGRtc3dCZ3Jr?=
 =?utf-8?B?N1RiVmdiNjFLSUdIaUk2ZGtOaFRNRkxrZEMvWEtQWDdiblVvTTdwdlljSWVU?=
 =?utf-8?B?UFBZTUtqa21kaUdmbDNSSzdUMmNhbmI3VUVpVGZMU2VCaVVlV044aUlDWDlt?=
 =?utf-8?B?VW5UQnEyL0NuaStHdU1RUVowYTN0c1Q3SXIrOWE4SDVrK25qZkErVTNnNnFW?=
 =?utf-8?B?dHc3U2grcmFzd1hpWnN2eS9XM1JPZ05BM2I1YmdTWEhpUG9Fd3llbWtmRy90?=
 =?utf-8?B?WmN4eGVlWUNwMDV5VDZ6UmM5em56Z01QV29PUGZCOStCN0pwd3dmMkJQUlB0?=
 =?utf-8?B?Z3JOQzA0SEVCeUJKMlRteHVGSTRBb1RGTndxMVJsZGFrcGkzWi90cWxFbjdp?=
 =?utf-8?B?OWNMYy9MOVhwY3BJVzZwZXRBRVEza2o5MmNRSFgyMGxSbDd3OWNCcnJKM3li?=
 =?utf-8?B?L1d1azd5TWg4UTZ0a3lPKzVrTVJoYlB2bE01ZkhHLzY4QXU1OUtVOHRGQ01m?=
 =?utf-8?B?b1R2cHV0eUoyWGJ4MllZWEhSVmZzdzc4NEdJamxQbVdQNFdmeEdlR1NhVXM2?=
 =?utf-8?Q?2cp9K02pJntp/sbFcyJnC50=3D?=
X-OriginatorOrg: os.amperecomputing.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2513b14a-104e-4083-7659-08dc8c19ccbe
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR01MB8193.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Jun 2024 02:29:21.7761
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3bc2b170-fd94-476d-b0ce-4229bdc904a7
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ZJMMYe8Gx7xhsknb4vK6hSmHIoj1x9KysjS1J+7jSjDnlhlU+nHkHWJ28/PZLhxUXfWo41Yh47/1Nlusha9Dow89XQzTNid1Oulqn6ILbsM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR01MB7892


On 12/06/2024 11:32, Potin Lai wrote:
> In ARM SBMR document, the host can chosse to not read back the response of
> “Send Boot Progress Code” command.
> 

Thanks for proposing a solution for the case.

As per my understanding from the ARM SBMR document, the "host can choose 
not to read back the response of the "Send Boot Progress Code" command" 
is from the implementation note (U) in Section F.1.1, which is to 
provide guidance on the implementation, not a rule. This item also 
clarifies some consequences if the host decides not to read back the 
response.

There is also an information statement (I) in this section that 
recommends reading back the response after sending "Send Boot Progress 
Code." I found this statement suitable for SSIF, which was designed as 
single-threaded from the beginning.

I'm not totally sure about this understanding yet so please let me know 
if any.

Thank you,
- Quan

> To avoid SSIF being in a wrong state due to host not read back the
> response, add the implementation of "arm-sbmr,skip-bootprogress-response"
> property for skipping the response of "Send Boot Progress Code" command
> from userspace.
> 
> Signed-off-by: Potin Lai <potin.lai.pt@gmail.com>
> ---
>   drivers/char/ipmi/ssif_bmc.c | 25 +++++++++++++++++++++++++
>   1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/char/ipmi/ssif_bmc.c b/drivers/char/ipmi/ssif_bmc.c
> index 56346fb328727..3386a8bd18afd 100644
> --- a/drivers/char/ipmi/ssif_bmc.c
> +++ b/drivers/char/ipmi/ssif_bmc.c
> @@ -39,6 +39,11 @@
>   #define SSIF_IPMI_MULTIPART_READ_START          0x3
>   #define SSIF_IPMI_MULTIPART_READ_MIDDLE         0x9
>   
> +#define GET_NETFN(netfn_lun)                    ((netfn_lun >> 2) & 0xfe)
> +#define IPMI_GROUP_EXT_NETFN                    0x2C
> +#define IPMI_SBMR_GROUP                         0xAE
> +#define IPMI_SBMR_BOOTPROGRESS_CMD              0x02
> +
>   /*
>    * IPMI 2.0 Spec, section 12.7 SSIF Timing,
>    * Request-to-Response Time is T6max(250ms) - T1max(20ms) - 3ms = 227ms
> @@ -102,6 +107,8 @@ struct ssif_bmc_ctx {
>   	struct ssif_part_buffer part_buf;
>   	struct ipmi_ssif_msg    response;
>   	struct ipmi_ssif_msg    request;
> +	/* Flag to skip response of Send Boot Progress Code */
> +	bool                    skip_bootprogress_resp;
>   };
>   
>   static inline struct ssif_bmc_ctx *to_ssif_bmc(struct file *file)
> @@ -187,6 +194,20 @@ static ssize_t ssif_bmc_write(struct file *file, const char __user *buf, size_t
>   		return -EINVAL;
>   
>   	spin_lock_irqsave(&ssif_bmc->lock, flags);
> +	if (ssif_bmc->skip_bootprogress_resp &&
> +	    GET_NETFN(msg.payload[0]) == IPMI_GROUP_EXT_NETFN &&
> +	    msg.payload[1] == IPMI_SBMR_BOOTPROGRESS_CMD &&
> +	    msg.payload[3] == IPMI_SBMR_GROUP) {
> +		if (ssif_bmc->response_timer_inited) {
> +			del_timer(&ssif_bmc->response_timer);
> +			ssif_bmc->response_timer_inited = false;
> +		}
> +		ssif_bmc->busy = false;
> +		memset(&ssif_bmc->request, 0, sizeof(struct ipmi_ssif_msg));
> +		spin_unlock_irqrestore(&ssif_bmc->lock, flags);
> +		return count;
> +	}
> +
>   	while (ssif_bmc->response_in_progress) {
>   		spin_unlock_irqrestore(&ssif_bmc->lock, flags);
>   		if (file->f_flags & O_NONBLOCK)
> @@ -806,6 +827,10 @@ static int ssif_bmc_probe(struct i2c_client *client)
>   	if (!ssif_bmc)
>   		return -ENOMEM;
>   
> +	if (of_property_read_bool(client->dev.of_node,
> +				  "arm-sbmr,skip-bootprogress-response"))
> +		ssif_bmc->skip_bootprogress_resp = true;
> +
>   	spin_lock_init(&ssif_bmc->lock);
>   
>   	init_waitqueue_head(&ssif_bmc->wait_queue);