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

[Aditya Garg <gargaditya08@live.com>]

Hi

> On 10 Mar 2025, at 1:45 PM, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> 
> 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.
> 

For all changes requested above, I'll get them fixed. Thank you!