Re: [PATCH 01/15] media: intel/ipu6: add Intel IPU6 PCI device driver

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, Jul 27, 2023 at 03:15:44PM +0800, bingbu.cao@intel.com wrote:
> From: Bingbu Cao <bingbu.cao@intel.com>
> 
> Intel Image Processing Unit 6th Gen includes input and processing systems
> but the hardware presents itself as a single PCI device in system.
> 
> IPU6 PCI device driver basically does PCI configurations and load
> the firmware binary, initialises IPU virtual bus, and sets up platform
> specific variants to support multiple IPU6 devices in single device
> driver.

> +#ifndef IPU6_PLATFORM_REGS_H
> +#define IPU6_PLATFORM_REGS_H

Missing

bits.h

...

> +#define IPU6_ISYS_CSI_PORT_IRQ(irq_num)		(1 << (irq_num))

BIT() ?

...

> +#define S2M_PIXEL_SOC_PIXEL_REMAPPING_FLAG_NO_REMAPPING 0xE4o

Why capital?

...

> +/*
> + * csi_be_soc_pixel_remapping is for the enabling of the pixel remapping.

Is it reference to a function? Please use func() format everywhere.

> + * This remapping is exactly like the stream2mmio remapping.
> + */
> +#define CSI_BE_SOC_PIXEL_REMAPPING_FLAG_NO_REMAPPING    0xE4

Why capital?

> +enum nci_ab_access_mode {
> +	NCI_AB_ACCESS_MODE_RW,	/* read & write */
> +	NCI_AB_ACCESS_MODE_RO,	/* read only */
> +	NCI_AB_ACCESS_MODE_WO,	/* write only */
> +	NCI_AB_ACCESS_MODE_NA	/* No access at all */

Leave trailing comma, since it doesn't look like a terminating entry.

> +};

...

> +#define IPU6_PSYS_GPDEV_IRQ_FWIRQ(n)		(1 << (n))

BIT() ?

> +#endif /* IPU6_PLATFORM_REGS_H */

...

> +#ifndef IPU6_PLATFORM_H
> +#define IPU6_PLATFORM_H
> +
> +#include "ipu6-fw-isys.h"
> +
> +#define IPU6_NAME			"intel-ipu6"
> +
> +#define IPU6SE_FIRMWARE_NAME		"intel/ipu6se_fw.bin"
> +#define IPU6EP_FIRMWARE_NAME		"intel/ipu6ep_fw.bin"
> +#define IPU6_FIRMWARE_NAME		"intel/ipu6_fw.bin"
> +#define IPU6EPMTL_FIRMWARE_NAME		"intel/ipu6epmtl_fw.bin"

Maybe


#define IPU6_FIRMWARE_FOLDER		"intel"

#define IPU6SE_FIRMWARE_NAME		"ipu6se_fw.bin"
#define IPU6EP_FIRMWARE_NAME		"ipu6ep_fw.bin"
#define IPU6_FIRMWARE_NAME		"ipu6_fw.bin"
#define IPU6EPMTL_FIRMWARE_NAME		"ipu6epmtl_fw.bin"

And then concatenate in the code?

Or

#define IPU6SE_FIRMWARE_NAME		IPU6_FIRMWARE_FOLDER "/ipu6se_fw.bin"
#define IPU6EP_FIRMWARE_NAME		IPU6_FIRMWARE_FOLDER "/ipu6ep_fw.bin"
#define IPU6_FIRMWARE_NAME		IPU6_FIRMWARE_FOLDER "/ipu6_fw.bin"
#define IPU6EPMTL_FIRMWARE_NAME		IPU6_FIRMWARE_FOLDER "/ipu6epmtl_fw.bin"

?

> +#endif

...

> +#include <linux/acpi.h>

Not used.

> +#include <linux/bitfield.h>
> +#include <linux/debugfs.h>
> +#include <linux/device.h>
> +#include <linux/interrupt.h>
> +#include <linux/firmware.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/pci.h>
> +#include <linux/pm_qos.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/timer.h>

Check this block if all of them are being used / needed and otherwise,
if something is missing (property.h and slab.h, for example).

> +#include "ipu6.h"
> +#include "ipu6-bus.h"
> +#include "ipu6-buttress.h"
> +#include "ipu6-cpd.h"
> +#include "ipu6-isys.h"
> +#include "ipu6-mmu.h"
> +#include "ipu6-platform.h"
> +#include "ipu6-platform-buttress-regs.h"
> +#include "ipu6-platform-isys-csi2-reg.h"
> +#include "ipu6-platform-regs.h"
> +#include "../ipu-bridge.h"

Ditto.

...

> +struct ipu6_cell_program_t {

No _t.
_t is used for typedef:s.

> +	u32 magic_number;
> +
> +	u32 blob_offset;
> +	u32 blob_size;
> +
> +	u32 start[3];
> +
> +	u32 icache_source;
> +	u32 icache_target;
> +	u32 icache_size;
> +
> +	u32 pmem_source;
> +	u32 pmem_target;
> +	u32 pmem_size;
> +
> +	u32 data_source;
> +	u32 data_target;
> +	u32 data_size;
> +
> +	u32 bss_target;
> +	u32 bss_size;
> +
> +	u32 cell_id;
> +	u32 regs_addr;
> +
> +	u32 cell_pmem_data_bus_address;
> +	u32 cell_dmem_data_bus_address;
> +	u32 cell_pmem_control_bus_address;
> +	u32 cell_dmem_control_bus_address;
> +
> +	u32 next;
> +	u32 dummy[2];

> +} __packed;

Why?! Please justify this.

...

> +	prog = (struct ipu6_cell_program_t *)((u64)isp->cpd_fw->data +
> +					      (server_fw_addr -
> +					       dma_addr));

This looks awful. Can you utilise something like offsetof()?

...

> +static int ipu6_isys_check_fwnode_graph(struct fwnode_handle *fwnode)
> +{
> +	struct fwnode_handle *endpoint;

> +	if (IS_ERR_OR_NULL(fwnode))
> +		return -EINVAL;

Isn't this already embedded in the below call?

But okay, you probably know better what's going on here.

> +	endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
> +	if (endpoint) {
> +		fwnode_handle_put(endpoint);
> +		return 0;
> +	}
> +
> +	return ipu6_isys_check_fwnode_graph(fwnode->secondary);
> +}

...

> +	ret = ipu6_isys_check_fwnode_graph(fwnode);
> +	if (ret) {

> +		if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) {

Why is this check not part of the above call?

> +			dev_err(&pdev->dev,
> +				"fwnode graph has no endpoints connection\n");
> +			return ERR_PTR(-EINVAL);

> +		}
> +
> +		ret = ipu_bridge_init(pdev);
> +		if (ret) {
> +			dev_err_probe(&pdev->dev, ret,
> +				      "IPU6 bridge init failed\n");
> +			return ERR_PTR(ret);
> +		}
> +	}

...

> +	isys_adev = ipu6_bus_initialize_device(pdev, parent, pdata, ctrl,
> +					       IPU6_ISYS_NAME);
> +	if (IS_ERR(isys_adev)) {
> +		dev_err_probe(&pdev->dev, PTR_ERR(isys_adev),
> +			      "ipu6_bus_add_device(isys_adev) failed\n");

I read another call name above... What is this message about?

> +		kfree(pdata);
> +		return ERR_CAST(isys_adev);
> +	}

...

> +	ret = ipu6_bus_add_device(isys_adev);
> +
> +	return ret ? ERR_PTR(ret) : isys_adev;

No memory cleanups?

...

> +	ret = ipu6_bus_add_device(psys_adev);
> +
> +	return ret ? ERR_PTR(ret) : psys_adev;

Ditto.

...

> +static int ipu6_pci_config_setup(struct pci_dev *dev, u8 hw_ver)
> +{
> +	u16 pci_command;
> +	int ret;
> +
> +	pci_read_config_word(dev, PCI_COMMAND, &pci_command);
> +	pci_command |= PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER;
> +	pci_write_config_word(dev, PCI_COMMAND, pci_command);

Huh?! Why is it needed and why PCI core is not enough for these?

> +	/* No PCI msi capability for IPU6EP */
> +	if (hw_ver == IPU6_VER_6EP || hw_ver == IPU6_VER_6EP_MTL) {
> +		/* likely do nothing as msi not enabled by default */
> +		pci_disable_msi(dev);
> +		return 0;
> +	}

> +	ret = pci_enable_msi(dev);
> +	if (ret)
> +		dev_err(&dev->dev, "Failed to enable msi (%d)\n", ret);
> +	return ret;

No, use pci_alloc_irq_vectors() and respective clean up.

> +}

...

> +	ret = pcim_enable_device(pdev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to enable PCI device (%d)\n", ret);
> +		return ret;

		return dev_err_probe(...);

> +	}

> +	dev_info(&pdev->dev, "Device 0x%x (rev: 0x%x)\n",
> +		 pdev->device, pdev->revision);

Useless noise. We have this in the kernel dmesg anyway.
Why do you need a dup?

...

> +	ret = pcim_iomap_regions(pdev, 1 << IPU6_PCI_BAR, pci_name(pdev));

BIT()

> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to I/O mem remapping (%d)\n", ret);
> +		return ret;

dev_err_probe()

> +	}

> +	dev_dbg(&pdev->dev, "physical base address 0x%llx\n", phys);

Useless, see above.

...

> +	iomap = pcim_iomap_table(pdev);

> +	if (!iomap) {
> +		dev_err(&pdev->dev, "Failed to iomap table (%d)\n", ret);
> +		return -ENODEV;
> +	}

Dead code.

> +	isp->base = iomap[IPU6_PCI_BAR];

...

> +	default:
> +		dev_err(&pdev->dev, "Unsupported IPU6 device %x\n", id->device);
> +		return -ENODEV;

dev_err_probe()

> +	}

...

> +	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(39));
> +	if (ret) {

> +		dev_err(&pdev->dev, "Failed to set DMA mask (%d)\n", ret);
> +		return ret;

Ditto.

> +	}

...

> +	ret = devm_request_threaded_irq(&pdev->dev, pdev->irq,
> +					ipu6_buttress_isr,
> +					ipu6_buttress_isr_threaded,
> +					IRQF_SHARED, IPU6_NAME, isp);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Requesting irq failed(%d)\n", ret);
> +		return ret;

Ditto.

> +	}

...

> +	ret = request_cpd_fw(&isp->cpd_fw, isp->cpd_fw_name, &pdev->dev);
> +	if (ret) {
> +		dev_err(&isp->pdev->dev, "Requesting signed firmware failed\n");

Ditto.
		ret = dev_err_probe(...);

...and so on below.

> +		goto buttress_exit;
> +	}

...

> +	isys_ctrl = devm_kzalloc(&pdev->dev, sizeof(*isys_ctrl), GFP_KERNEL);
> +	if (!isys_ctrl) {
> +		ret = -ENOMEM;
> +		goto out_ipu6_bus_del_devices;
> +	}
> +
> +	memcpy(isys_ctrl, &isys_buttress_ctrl, sizeof(*isys_ctrl));

devm_kmemdup()?

...

> +	psys_ctrl = devm_kzalloc(&pdev->dev, sizeof(*psys_ctrl), GFP_KERNEL);
> +	if (!psys_ctrl) {
> +		ret = -ENOMEM;
> +		goto out_ipu6_bus_del_devices;
> +	}
> +
> +	memcpy(psys_ctrl, &psys_buttress_ctrl, sizeof(*psys_ctrl));

Ditto?

...

> +	ret = pm_runtime_get_sync(&isp->psys->auxdev.dev);
> +	if (ret < 0) {

Leaking reference count.

> +		dev_err(&isp->psys->auxdev.dev, "Failed to get runtime PM\n");
> +		goto out_ipu6_bus_del_devices;
> +	}

...

> +	if (!IS_ERR_OR_NULL(isp->psys) && !IS_ERR_OR_NULL(isp->psys->mmu))
> +		ipu6_mmu_cleanup(isp->psys->mmu);
> +	if (!IS_ERR_OR_NULL(isp->isys) && !IS_ERR_OR_NULL(isp->isys->mmu))
> +		ipu6_mmu_cleanup(isp->isys->mmu);

All these conditionals should be hidden in the callee.

> +	if (!IS_ERR_OR_NULL(isp->psys))

Why it doesn't cover all above?

> +		pm_runtime_put(&isp->psys->auxdev.dev);

...

> +static void ipu6_pci_reset_prepare(struct pci_dev *pdev)
> +{
> +	struct ipu6_device *isp = pci_get_drvdata(pdev);
> +
> +	dev_warn(&pdev->dev, "FLR prepare\n");

???

> +	pm_runtime_forbid(&isp->pdev->dev);
> +}

> +static void ipu6_pci_reset_done(struct pci_dev *pdev)
> +{
> +	struct ipu6_device *isp = pci_get_drvdata(pdev);
> +
> +	ipu6_buttress_restore(isp);
> +	if (isp->secure_mode)
> +		ipu6_buttress_reset_authentication(isp);
> +
> +	isp->need_ipc_reset = true;
> +	pm_runtime_allow(&isp->pdev->dev);
> +
> +	dev_info(&pdev->dev, "IPU6 PCI FLR completed\n");

???

Are these debug leftovers?

> +}

...

> +static const struct dev_pm_ops ipu6_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(&ipu6_suspend, &ipu6_resume)
> +	SET_RUNTIME_PM_OPS(&ipu6_suspend, &ipu6_runtime_resume, NULL)

Use new macros instead of obsolete ones.

> +};

...

> +static const struct pci_device_id ipu6_pci_tbl[] = {
> +	{ PCI_VDEVICE(INTEL, IPU6_PCI_ID) },
> +	{ PCI_VDEVICE(INTEL, IPU6SE_PCI_ID) },
> +	{ PCI_VDEVICE(INTEL, IPU6EP_ADL_P_PCI_ID) },
> +	{ PCI_VDEVICE(INTEL, IPU6EP_ADL_N_PCI_ID) },
> +	{ PCI_VDEVICE(INTEL, IPU6EP_RPL_P_PCI_ID) },
> +	{ PCI_VDEVICE(INTEL, IPU6EP_MTL_PCI_ID) },
> +	{ }
> +};

Can you make sure the device IDs are in the standard format?
PCI_DEVICE_ID_INTEL_... (if I remember correctly the template)


> +static struct pci_driver ipu6_pci_driver = {
> +	.name = IPU6_NAME,
> +	.id_table = ipu6_pci_tbl,
> +	.probe = ipu6_pci_probe,
> +	.remove = ipu6_pci_remove,
> +	.driver = {
> +		.pm = &ipu6_pm_ops,

pm_ptr()

> +	},
> +	.err_handler = &pci_err_handlers,
> +};

...

> +static int __init ipu6_init(void)
> +{
> +	int ret;
> +
> +	ret = pci_register_driver(&ipu6_pci_driver);
> +	if (ret)
> +		pr_warn("can't register PCI driver (%d)\n", ret);
> +
> +	return ret;
> +}
> +
> +static void __exit ipu6_exit(void)
> +{
> +	pci_unregister_driver(&ipu6_pci_driver);
> +}
> +
> +module_init(ipu6_init);
> +module_exit(ipu6_exit);

Use module_pci_driver().

...

> +MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>");
> +MODULE_AUTHOR("Tianshu Qiu <tian.shu.qiu@intel.com>");
> +MODULE_AUTHOR("Bingbu Cao <bingbu.cao@intel.com>");
> +MODULE_AUTHOR("Qingwu Zhang <qingwu.zhang@intel.com>");
> +MODULE_AUTHOR("Yunliang Ding <yunliang.ding@intel.com>");
> +MODULE_AUTHOR("Hongju Wang <hongju.wang@intel.com>");

So many authors and so many issues with the code.
Did you pass an internal review?

...

> +#ifndef IPU6_H
> +#define IPU6_H
> +
> +#include <linux/firmware.h>
> +#include <linux/ioport.h>
> +#include <linux/list.h>
> +#include <linux/pci.h>
> +
> +#include "ipu6-buttress.h"

As usual, missing headers and not used one. Internally we have a wiki page that
describes hints and tricks for driver programming, please find it and read and
fix your series accordingly. Maybe you need to go for a new round(s) of
internal review.

I'm stopping here for now.

> +#endif /* IPU6_H */

-- 
With Best Regards,
Andy Shevchenko