From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f51.google.com (mail-wm1-f51.google.com [209.85.128.51]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 8D8BA221F08 for ; Mon, 10 Mar 2025 08:14:54 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.51 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741594497; cv=none; b=XPOHUUoRfyPG2uCdm9rl1nmhxmFLDRovu8OL9UqMdWE3L76qOQH9vZcSoCfzdH3PkIn7h3zZ8GWWuMquAT7sIF9nuLkbSYEf4jmVEdD1YHp1Vb/cpsvhC369s9z48/acz/nLveaf5tQLMvpd9QvfnCgqLBxeG78noA4S1tAmYC8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741594497; c=relaxed/simple; bh=ieOnLpKZdMXwTRsppKzD3lKC3vFircXxUr3yv6CciMo=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=awv9BcHg7s0/KK50061FQKCQ0oteZV9Wc0E5A7BjV6/Z7aG0sXNhdQebK3wg8QBEIbE2Rv66yZiZ2yp3W3gwo9dJsq/DmdAOb3QVBYZ976TNJODeIxOJ04mJcYoqpFhRpWsfmo/SUnL7Ju5rajBhtnSeemg7OfgOG8/JiAZFynU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=Gp78hMvK; arc=none smtp.client-ip=209.85.128.51 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="Gp78hMvK" Received: by mail-wm1-f51.google.com with SMTP id 5b1f17b1804b1-43bc4b16135so22269725e9.1 for ; Mon, 10 Mar 2025 01:14:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1741594493; x=1742199293; darn=lists.linux.dev; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=8KkC1l654ivSQJHfpAMIQ+ZSAHbGtBz6q5j4dS4aJJk=; b=Gp78hMvKKVOi7AuX33ZpNST/RRjJ+4AxjeZwgXR0SOHNf0HXzDBubz83u38OS1bAUR kJNwrTWfuera28hxrHNxOfS1LutEpWILchA5yLhdBqszJUI8MY/Pf3uJoTFy9Y6U8EOx RQBjzPCvon64iioekZWSYWXatBoBiNKMaJVO8KpeGsDKJ9B5MlkAITd1ngD7C7HRCA9d x41EUHuxWg4/R/x2tE8K7PSW11E52t5jCO1iDzpIjgnqXIqEwarpVLuOrtrWwuRjsx9v mLBZXr1MtxKq/ux6XWaopmM2KVVlmu8J2gEbI6S27r9xKiAgDnEOfnXSPtzW0Ztz1JPk KR1g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1741594493; x=1742199293; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=8KkC1l654ivSQJHfpAMIQ+ZSAHbGtBz6q5j4dS4aJJk=; b=AgLxT5uSLKmKVWw8Y/aN9OdM5fvXqA3e5pBVauGgxyxb4Jcelee84hURkmL0vPIDuA q5ZPZcbCb/8kfAMJtP5O7VOj9LVsEns+S0tJuBT2cY/sgPG9D72rg/o1K8YSgfUkIxQw id7LSc8ezg60D+w/1OGIKPm21ddDG3nyE3wT7INAEA2jcwcXguW309z+E6O8aenKBJQ7 bVyQmhRCtBqfaRQk2LvWRUdPqxHpP2YNis8NETNDYGg42Eug0jcLEYJqYcyCznom9eqc oxqPcbdaa8SyM2HRwhRtaRHvRI/z01lh7ZA6vUOWzr7SM4OndsAqOETXrFCLcGN2UNpq rGSw== X-Forwarded-Encrypted: i=1; AJvYcCW5Z8sV1ryejxbaHm0hQeNCn8TgT/dHcV16lWoGdmjLpBESb9ZQC8tGtGkO01eZF7H6IwI5GX82wJe/4gDz@lists.linux.dev X-Gm-Message-State: AOJu0YxhCTSS9XGFpP/O9QCtbHIheLlDXxJH5RFv+GCj69TjF4a1NQUV qXks+hzno2Ij79IfFgDhNspxz+1odOVpe3jQdgtlR0JS62f0IU4LFK3EiQ+fYf0= X-Gm-Gg: ASbGncudwoiEyJcMK0fwVQ3H4KYRuWnAC9nK9hwK5OkTH1uktp/6LgKJvqTlwa1fulS 7HMJmAaxSYvwa5/wA1VWP+LQxWMSAu+SMe6fRbUzXf4HCc0MbHMggSIzDwQMDCOSX+SKSZEvAyo hDfmVsBvzwHzTpkJjlrpQFsv1z+rF+Z1H4ovTrN1Fi66YwDX9C6O4Dk9OdcnZXlM+K6YZ/+jHiY pKS77d/tGJw+rf/LncS6rv5UWF3x3rHgDduT+GW5faXEBlYHnhZ2XueW5/NTkoxjPs5Ly8rNgnk Ov/Hq6FdisV5XL0DxBe1zhdP/Np1ixKjzWIpTO0SkhZGdvFx1Q== X-Google-Smtp-Source: AGHT+IGAuAAY07Q8AHFN3C6TD8HtaPVPa6Tfe7wsUJ+gFJZ+qrYDD1atk9hsCbpXzVbCKHdNzeFEDw== X-Received: by 2002:a05:600c:3b1f:b0:43c:f6c6:578c with SMTP id 5b1f17b1804b1-43cf6c6588bmr17772455e9.15.1741594492829; Mon, 10 Mar 2025 01:14:52 -0700 (PDT) Received: from localhost ([196.207.164.177]) by smtp.gmail.com with UTF8SMTPSA id 5b1f17b1804b1-43bd91338cesm156803745e9.7.2025.03.10.01.14.51 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 10 Mar 2025 01:14:52 -0700 (PDT) Date: Mon, 10 Mar 2025 11:14:48 +0300 From: Dan Carpenter To: Aditya Garg Cc: "gregkh@linuxfoundation.org" , "bhelgaas@google.com" , "joro@8bytes.org" , "will@kernel.org" , "robin.murphy@arm.com" , "andriy.shevchenko@linux.intel.com" , "linux-staging@lists.linux.dev" , Linux Kernel Mailing List , "linux-pci@vger.kernel.org" , "iommu@lists.linux.dev" , Aun-Ali Zaidi , "paul@mrarm.io" , Orlando Chamberlain Subject: Re: [PATCH RFC] staging: Add driver to communicate with the T2 Security Chip Message-ID: References: <1A12CB39-B4FD-4859-9CD7-115314D97C75@live.com> Precedence: bulk X-Mailing-List: linux-staging@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1A12CB39-B4FD-4859-9CD7-115314D97C75@live.com> 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 > +#include > +#include "audio/audio.h" > +#include > + > +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