Re: [PATCH v3 04/17] ASoC: Intel: avs: Inter process communication

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 2022-03-04 5:09 PM, Ranjani Sridharan wrote:
> On Fri, 2022-03-04 at 15:57 +0100, Cezary Rojewski wrote:

...

>> +static inline void avs_ipc_err(struct avs_dev *adev, struct
>> avs_ipc_msg *tx,
>> +			       const char *name, int error)
>> +{
>> +	/*
>> +	 * If IPC channel is blocked e.g.: due to ongoing recovery,
> Do you mean firmware recovery? In which cases do you perform a
> recovery?

Thanks for feedback! Consider dropping the unnecessary bits so it is 
easier to navigate through your responses.

Please note: kernel mailing list is not for explaining SW <-> FW 
communication details. Feel free to contact my colleagues from firmware 
team if in need of any FW-iface details.

That goes for most of the comments found below too.

-

Yes, I mean firmware recovery here. Code found in this patchset shows 
that message timeout is one of the reasons.

>> +	 * -EPERM error code is expected and thus it's not an actual
>> error.
> And what happens in this case? do you retry the IPC after recovery?


Not at all. Why would you want retry IPC after recovery in the first place?

>> +	 */
>> +	if (error == -EPERM)
>> +		dev_dbg(adev->dev, "%s 0x%08x 0x%08x failed: %d\n",
>> name,
>> +			tx->glb.primary, tx->glb.ext.val, error);
>> +	else
>> +		dev_err(adev->dev, "%s 0x%08x 0x%08x failed: %d\n",
>> name,
>> +			tx->glb.primary, tx->glb.ext.val, error);
>> +}
>> +
>> +irqreturn_t avs_dsp_irq_handler(int irq, void *dev_id);
>> +irqreturn_t avs_dsp_irq_thread(int irq, void *dev_id);
>> +void avs_dsp_process_response(struct avs_dev *adev, u64 header);
>> +int avs_dsp_send_msg_timeout(struct avs_dev *adev,
>> +			     struct avs_ipc_msg *request,
>> +			     struct avs_ipc_msg *reply, int timeout);
>> +int avs_dsp_send_msg(struct avs_dev *adev,
>> +		     struct avs_ipc_msg *request, struct avs_ipc_msg
>> *reply);
>> +int avs_dsp_send_rom_msg_timeout(struct avs_dev *adev,
>> +				 struct avs_ipc_msg *request, int
>> timeout);
>> +int avs_dsp_send_rom_msg(struct avs_dev *adev, struct avs_ipc_msg
>> *request);
>> +void avs_dsp_interrupt_control(struct avs_dev *adev, bool enable);
>> +int avs_ipc_init(struct avs_ipc *ipc, struct device *dev);
>> +void avs_ipc_block(struct avs_ipc *ipc);
>> +
>>   #endif /* __SOUND_SOC_INTEL_AVS_H */
>> diff --git a/sound/soc/intel/avs/ipc.c b/sound/soc/intel/avs/ipc.c
>> new file mode 100644
>> index 000000000000..c0722f8b195f
>> --- /dev/null
>> +++ b/sound/soc/intel/avs/ipc.c
>> @@ -0,0 +1,387 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +//
>> +// Copyright(c) 2021 Intel Corporation. All rights reserved.
>> +//
>> +// Authors: Cezary Rojewski <cezary.rojewski@intel.com>
>> +//          Amadeusz Slawinski <amadeuszx.slawinski@linux.intel.com>
>> +//
>> +
>> +#include <linux/slab.h>
>> +#include <sound/hdaudio_ext.h>
>> +#include "avs.h"
>> +#include "messages.h"
>> +#include "registers.h"
>> +
>> +#define AVS_IPC_TIMEOUT_MS	300
>> +
>> +static void avs_dsp_receive_rx(struct avs_dev *adev, u64 header)
>> +{
>> +	struct avs_ipc *ipc = adev->ipc;
>> +	union avs_reply_msg msg = AVS_MSG(header);
>> +
>> +	ipc->rx.header = header;
>> +	/* Abort copying payload if request processing was
>> unsuccessful. */
> This seems misplaced? Why would you called this function is the status
> showed an error?


This comment is part of Pierre's feedback, not misplaced at all.

Header can be message-specific and contain valid info to perform even on 
failure.

>> +	if (!msg.status)
>> +		memcpy_fromio(ipc->rx.data, avs_uplink_addr(adev),
>> +			      ipc->rx.size);
>> +}
>> +
>> +static void avs_dsp_process_notification(struct avs_dev *adev, u64
>> header)
>> +{
>> +	struct avs_notify_mod_data mod_data;
>> +	union avs_notify_msg msg = AVS_MSG(header);
>> +	size_t data_size = 0;
>> +	void *data = NULL;
>> +
>> +	/* Ignore spurious notifications until handshake is
>> established. */
>> +	if (!adev->ipc->ready && msg.notify_msg_type !=
>> AVS_NOTIFY_FW_READY) {
>> +		dev_dbg(adev->dev, "FW not ready, skip notification:
>> 0x%08x\n",
>> +			msg.primary);
>> +		return;
>> +	}
>> +
>> +	/* Calculate notification payload size. */
>> +	switch (msg.notify_msg_type) {
>> +	case AVS_NOTIFY_FW_READY:
>> +		break;
>> +
>> +	case AVS_NOTIFY_PHRASE_DETECTED:
>> +		data_size = sizeof(struct avs_notify_voice_data);
>> +		break;
>> +
>> +	case AVS_NOTIFY_RESOURCE_EVENT:
>> +		data_size = sizeof(struct avs_notify_res_data);
>> +		break;
>> +
>> +	case AVS_NOTIFY_MODULE_EVENT:
>> +		/* To know the total payload size, header needs to be
>> read first. */
>> +		memcpy_fromio(&mod_data, avs_uplink_addr(adev),
>> sizeof(mod_data));
>> +		data_size = sizeof(mod_data) + mod_data.data_size;
>> +		break;
>> +
>> +	default:
>> +		dev_info(adev->dev, "unknown notification: 0x%08x\n",
>> +			 msg.primary);
> info? should it be a warning?


Pierre's feedback was exactly the opposite.

>> +		break;
>> +	}
>> +
>> +	if (data_size) {
>> +		data = kmalloc(data_size, GFP_KERNEL);
>> +		if (!data)
>> +			return;
> Should this function be modified to return the error? If it failed
> here, all subsequent IPC's rec'd will also fail isnt it?


Hmm.. we are servicing an IPC here, can't return an error code, really. 
Also, some memory could have been freed between IRQs.

>> +
>> +		memcpy_fromio(data, avs_uplink_addr(adev), data_size);
>> +	}
>> +
>> +	/* Perform notification-specific operations. */
>> +	switch (msg.notify_msg_type) {
>> +	case AVS_NOTIFY_FW_READY:
>> +		dev_dbg(adev->dev, "FW READY 0x%08x\n", msg.primary);
>> +		adev->ipc->ready = true;
>> +		complete(&adev->fw_ready);
>> +		break;
>> +
>> +	default:
>> +		break;
>> +	}
>> +
>> +	kfree(data);
> You alloc memory for "data", copy the data and free it? Where is it
> used?


This patch implement the backbone for the entire IPC protocol. Specific 
handlers are not found here.

>> +static void avs_ipc_msg_init(struct avs_ipc *ipc, struct avs_ipc_msg
>> *reply)
>> +{
>> +	lockdep_assert_held(&ipc->rx_lock);
>> +
>> +	ipc->rx.header = 0;
>> +	ipc->rx.size = reply ? reply->size : 0;
>> +	ipc->rx_completed = false;
>> +
>> +	reinit_completion(&ipc->done_completion);
>> +	reinit_completion(&ipc->busy_completion);
>> +}
>> +
>> +static void avs_dsp_send_tx(struct avs_dev *adev, struct avs_ipc_msg
>> *tx)
> send_tx? send and tx both imply the same isnt it? maybe just use one or
> the other?


Matter of taste I believe. Also, aligned with the catpt-driver.

>> +static int avs_dsp_do_send_msg(struct avs_dev *adev, struct
>> avs_ipc_msg *request,
>> +			       struct avs_ipc_msg *reply, int timeout)
>> +{
>> +	struct avs_ipc *ipc = adev->ipc;
>> +	int ret;
>> +
>> +	if (!ipc->ready)
>> +		return -EPERM;
>> +
>> +	mutex_lock(&ipc->msg_mutex);
>> +
>> +	spin_lock(&ipc->rx_lock);
>> +	avs_ipc_msg_init(ipc, reply);
>> +	avs_dsp_send_tx(adev, request);
>> +	spin_unlock(&ipc->rx_lock);
>> +
>> +	ret = avs_ipc_wait_busy_completion(ipc, timeout);
>> +	if (ret) {
>> +		if (ret == -ETIMEDOUT) {
>> +			dev_crit(adev->dev, "communication severed: %d,
>> rebooting dsp..\n",
> Where does this reboot happen?


This patch implement the backbone for the entire IPC protocol. Specific 
handlers are not found here. Message remained.


Regards,
Czarek