Re: [PATCH RFC] staging: Add driver to communicate with the T2 Security Chip

[Dan Carpenter]

On Sun, Mar 09, 2025 at 08:40:31AM +0000, Aditya Garg wrote:
> diff --git a/drivers/staging/apple-bce/apple_bce.c b/drivers/staging/apple-bce/apple_bce.c
> new file mode 100644
> index 000000000..c66e0c8d5
> --- /dev/null
> +++ b/drivers/staging/apple-bce/apple_bce.c
> @@ -0,0 +1,448 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include "apple_bce.h"
> +#include <linux/module.h>
> +#include <linux/crc32.h>
> +#include "audio/audio.h"
> +#include <linux/version.h>
> +
> +static dev_t bce_chrdev;
> +static struct class *bce_class;
> +
> +struct apple_bce_device *global_bce;
> +
> +static int bce_create_command_queues(struct apple_bce_device *bce);
> +static void bce_free_command_queues(struct apple_bce_device *bce);
> +static irqreturn_t bce_handle_mb_irq(int irq, void *dev);
> +static irqreturn_t bce_handle_dma_irq(int irq, void *dev);
> +static int bce_fw_version_handshake(struct apple_bce_device *bce);
> +static int bce_register_command_queue(struct apple_bce_device *bce, struct bce_queue_memcfg *cfg, int is_sq);
> +
> +static int apple_bce_probe(struct pci_dev *dev, const struct pci_device_id *id)
> +{
> +	struct apple_bce_device *bce = NULL;
> +	int status = 0;
> +	int nvec;
> +
> +	pr_info("apple-bce: capturing our device\n");
> +
> +	if (pci_enable_device(dev))
> +		return -ENODEV;

	ret = pci_enable_device(dev);
	if (ret)
		return ret;

> +	if (pci_request_regions(dev, "apple-bce")) {

Same everywhere.  Propagate the error code.

> +		status = -ENODEV;
> +		goto fail;

Instead of "goto fail;" it's better to use a full unwind ladder
with better label names.
https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/

> +	}
> +	pci_set_master(dev);
> +	nvec = pci_alloc_irq_vectors(dev, 1, 8, PCI_IRQ_MSI);
> +	if (nvec < 5) {
> +		status = -EINVAL;
> +		goto fail;
> +	}
> +
> +	bce = kzalloc(sizeof(struct apple_bce_device), GFP_KERNEL);

	bce = kzalloc(sizeof(*bce), GFP_KERNEL);

> +	if (!bce) {
> +		status = -ENOMEM;
> +		goto fail;
> +	}
> +
> +	bce->pci = dev;
> +	pci_set_drvdata(dev, bce);
> +
> +	bce->devt = bce_chrdev;
> +	bce->dev = device_create(bce_class, &dev->dev, bce->devt, NULL, "apple-bce");
> +	if (IS_ERR_OR_NULL(bce->dev)) {
> +		status = PTR_ERR(bce_class);


device_create() can't return NULL.
https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/

> +		goto fail;
> +	}
> +
> +	bce->reg_mem_mb = pci_iomap(dev, 4, 0);
> +	bce->reg_mem_dma = pci_iomap(dev, 2, 0);
> +
> +	if (IS_ERR_OR_NULL(bce->reg_mem_mb) || IS_ERR_OR_NULL(bce->reg_mem_dma)) {
> +		dev_warn(&dev->dev, "apple-bce: Failed to pci_iomap required regions\n");
> +		goto fail;
> +	}
> +
> +	bce_mailbox_init(&bce->mbox, bce->reg_mem_mb);
> +	bce_timestamp_init(&bce->timestamp, bce->reg_mem_mb);
> +
> +	spin_lock_init(&bce->queues_lock);
> +	ida_init(&bce->queue_ida);
> +
> +	if ((status = pci_request_irq(dev, 0, bce_handle_mb_irq, NULL, dev, "bce_mbox")))

I think checkpatch will complain about this.  Do it as.

	status = pci_request_irq(dev, 0, bce_handle_mb_irq, NULL, dev, "bce_mbox");
	if (status)

> +		goto fail;
> +	if ((status = pci_request_irq(dev, 4, NULL, bce_handle_dma_irq, dev, "bce_dma")))
> +		goto fail_interrupt_0;
> +
> +	if ((status = dma_set_mask_and_coherent(&dev->dev, DMA_BIT_MASK(37)))) {
> +		dev_warn(&dev->dev, "dma: Setting mask failed\n");
> +		goto fail_interrupt;
> +	}
> +
> +	/* Gets the function 0's interface. This is needed because Apple only accepts DMA on our function if function 0
> +	   is a bus master, so we need to work around this. */
> +	bce->pci0 = pci_get_slot(dev->bus, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
> +#ifndef WITHOUT_NVME_PATCH

Delete dead code?

> +	if ((status = pci_enable_device_mem(bce->pci0))) {
> +		dev_warn(&dev->dev, "apple-bce: failed to enable function 0\n");
> +		goto fail_dev0;
> +	}
> +#endif
> +	pci_set_master(bce->pci0);
> +
> +	bce_timestamp_start(&bce->timestamp, true);
> +
> +	if ((status = bce_fw_version_handshake(bce)))
> +		goto fail_ts;
> +	pr_info("apple-bce: handshake done\n");
> +
> +	if ((status = bce_create_command_queues(bce))) {
> +		pr_info("apple-bce: Creating command queues failed\n");
> +		goto fail_ts;
> +	}
> +
> +	global_bce = bce;

Do this right before the "return 0;"?

> +
> +	bce_vhci_create(bce, &bce->vhci);

Check for errors.

> +
> +	return 0;
> +
> +fail_ts:
> +	bce_timestamp_stop(&bce->timestamp);
> +#ifndef WITHOUT_NVME_PATCH
> +	pci_disable_device(bce->pci0);
> +fail_dev0:
> +#endif
> +	pci_dev_put(bce->pci0);
> +fail_interrupt:
> +	pci_free_irq(dev, 4, dev);
> +fail_interrupt_0:
> +	pci_free_irq(dev, 0, dev);
> +fail:
> +	if (bce && bce->dev) {
> +		device_destroy(bce_class, bce->devt);
> +
> +		if (!IS_ERR_OR_NULL(bce->reg_mem_mb))
> +			pci_iounmap(dev, bce->reg_mem_mb);
> +		if (!IS_ERR_OR_NULL(bce->reg_mem_dma))
> +			pci_iounmap(dev, bce->reg_mem_dma);
> +
> +		kfree(bce);
> +	}
> +
> +	pci_free_irq_vectors(dev);
> +	pci_release_regions(dev);
> +	pci_disable_device(dev);
> +
> +	if (!status)
> +		status = -EINVAL;

This is like saying "if the code is buggy then fix it" but it's better to
just not introduce bugs.  Which sounds difficult and unreliable, but it's
even more difficult and unreliable to predict which bugs people will add
and fix them correctly.

> +	return status;
> +}

regards,
dan carpenter