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