Hi,

On 12.01.2023 19:18, Jeffrey Hugo wrote:
> 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.kacprow...@linux.intel.com>
>> Signed-off-by: Andrzej Kacprowski <andrzej.kacprow...@linux.intel.com>
>> Co-developed-by: Krystian Pradzynski <krystian.pradzyn...@linux.intel.com>
>> Signed-off-by: Krystian Pradzynski <krystian.pradzyn...@linux.intel.com>
>> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynow...@linux.intel.com>
> 
> Reviewed-by: Jeffrey Hugo <quic_jh...@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.

OK, I've added this at the top of the TODO and fixed the comment.
Regarding the ivpu_ipc_irq_handler() return code it is checked in the next 
patch in ivpu_wait_for_ready().

Regards,
Jacek



Reply via email to