Re: [PATCH V2 01/10] accel/amdxdna: Add a new driver for AMD AI Engine

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 8/14/24 14:53, Jeffrey Hugo wrote:
On 8/14/2024 2:24 PM, Lizhi Hou wrote:

On 8/14/24 11:46, Jeffrey Hugo wrote:
On 8/14/2024 12:16 PM, Lizhi Hou wrote:

On 8/9/24 09:11, Jeffrey Hugo wrote:
On 8/5/2024 11:39 AM, Lizhi Hou wrote:
diff --git a/drivers/accel/amdxdna/aie2_pci.c b/drivers/accel/amdxdna/aie2_pci.c
new file mode 100644
index 000000000000..3660967c00e6
--- /dev/null
+++ b/drivers/accel/amdxdna/aie2_pci.c
@@ -0,0 +1,182 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2023-2024, Advanced Micro Devices, Inc.
+ */
+
+#include <linux/amd-iommu.h>
+#include <linux/errno.h>
+#include <linux/firmware.h>
+#include <linux/iommu.h>

You are clearly missing linux/pci.h and I suspect many more.
Other headers are indirectly included by "aie2_pci.h" underneath.

aie2_pci.h also does not directly include linux/pci.h

it is aie2_pci.h --> amdxdna_pci_drv.h --> linux/pci.h.

It compiles without any issue.

I did not doubt that it compiled.  To be clear, I'm pointing out what I believe is poor style - relying on implicit includes.

There is no reason that amdxdna_pci_drv.h needs to include linux/pci.h (at-least in this patch).  That header file doesn't use any of the content from pci.h.  If this were merged, I suspect it would be valid for someone to post a cleanup patch that removes pci.h from amdxdna_pci_drv.h.

The problem comes when someone does a tree wide refactor of some header - perhaps moving a function out of pci.h into something else.  If they grep the source tree, they'll find amdxdna_pci_drv.h includes pci.h but really doesn't use it.  They likely won't see aie2_pci.c which may break because of the refactor.  Even more problematic is if pci.h is including something that you need, and you are not including it anywhere. The code will still compile, but maybe in the next kernel cycle pci.h no longer includes that thing.  Your code will break.

The 4 includes you have here seems entirely too little, and I'm not clearly seeing the logic of what gets explicitly included vs what is implicitly included.  firmware.h is explicitly included, but pci.h is not, yet it seems like you use a lot more from pci.h.

There is the include what you use project that attempts to automate this, although I don't know how well it works with kernel code - https://github.com/include-what-you-use/include-what-you-use

Ok. got your point and I will cleanup include.


Lizhi




+
+    ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
+    if (ret) {
+        XDNA_ERR(xdna, "Failed to set DMA mask: %d", ret);
+        goto release_fw;
+    }
+
+    nvec = pci_msix_vec_count(pdev);

This feels weird.  Can your device advertise variable number of MSI-X vectors?  It only works if all of the vectors are used?
That is possible. the driver supports different hardware. And the fw assigns vector for hardware context dynamically. So the driver needs to allocate all vectors ahead.

So, if the device requests N MSIs, but the host is only able to satisfy 1 (or some number less than N), the fw is completely unable to function?
The fw may return interrupt 2 is assigned to hardware context. Then the driver may not deal with it in this case. I think it is ok to fail if the system has very limited resource.

Ok.  I suspect you'll want to change that behavior in the future with a fw update, but if this is how things work today, then this is how the driver must be.



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux