Re: [PATCH v5 4/7] accel/ivpu: Add IPC driver and JSM messages

[Jeffrey Hugo <quic_jhugo@quicinc.com>]

On 1/9/2023 5:23 AM, Jacek Lawrynowicz wrote:
> The IPC driver is used to send and receive messages to/from firmware
> running on the VPU.
> 
> The only supported IPC message format is Job Submission Model (JSM)
> defined in vpu_jsm_api.h header.
> 
> Co-developed-by: Andrzej Kacprowski <andrzej.kacprowski@linux.intel.com>
> Signed-off-by: Andrzej Kacprowski <andrzej.kacprowski@linux.intel.com>
> Co-developed-by: Krystian Pradzynski <krystian.pradzynski@linux.intel.com>
> Signed-off-by: Krystian Pradzynski <krystian.pradzynski@linux.intel.com>
> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>

Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>

> +int ivpu_ipc_irq_handler(struct ivpu_device *vdev)
> +{
> +	struct ivpu_ipc_info *ipc = vdev->ipc;
> +	struct ivpu_ipc_consumer *cons;
> +	struct ivpu_ipc_hdr *ipc_hdr;
> +	struct vpu_jsm_msg *jsm_msg;
> +	unsigned long flags;
> +	bool dispatched;
> +	u32 vpu_addr;
> +
> +	/* Driver needs to purge all messages from IPC FIFO to clear IPC interrupt.
> +	 * Without purge IPC FIFO to 0 next IPC interrupts won't be generated.
> +	 */
> +	while (ivpu_hw_reg_ipc_rx_count_get(vdev)) {

Ick.  Please no in the long term?

This is an infinite loop.  In hard IRQ context.  Controlled by the 
device, which you probably shouldn't trust.

However the real fix for this is to move to threaded_irqs.  Which is 
going to be a huge refactor for you.  Rate limiting doesn't appear viable.

If I understand things correctly, the chances that the device will 
generate a large count, or update the count as fast or faster than the 
driver are low, but it should still be fixed.

How about a high priority todo to convert to threaded irqs?  At the same 
time you can update the return value for this function which seems to 
not be checked anywhere, and also the comment here which is not proper 
multi-line style.