linux-kernel - Re: [PATCH 3/6] PCI: dwc: fsd: Add FSD PCIe Controller driver support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20221121231847.GA141229@bhelgaas>
Date:   Mon, 21 Nov 2022 17:18:47 -0600
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Shradha Todi <shradha.t@...sung.com>
Cc:     bhelgaas@...gle.com, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, kishon@...com, vkoul@...nel.org,
        lpieralisi@...nel.org, kw@...ux.com, mani@...nel.org,
        arnd@...db.de, gregkh@...uxfoundation.org, alim.akhtar@...sung.com,
        ajaykumar.rs@...sung.com, rcsekar@...sung.com,
        sriranjani.p@...sung.com, bharat.uppal@...sung.com,
        s.prashar@...sung.com, aswani.reddy@...sung.com,
        pankaj.dubey@...sung.com, p.rajanbabu@...sung.com,
        niyas.ahmed@...sung.com, chanho61.park@...sung.com,
        linux-pci@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-phy@...ts.infradead.org
Subject: Re: [PATCH 3/6] PCI: dwc: fsd: Add FSD PCIe Controller driver support

On Mon, Nov 21, 2022 at 04:22:07PM +0530, Shradha Todi wrote:
> Add PCIe controller driver file for PCIe controller
> found in fsd SoC family. This driver adds support for both RC
> and EP mode.
> 
> Signed-off-by: Niyas Ahmed S T <niyas.ahmed@...sung.com>
> Signed-off-by: Pankaj Dubey <pankaj.dubey@...sung.com>
> Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@...sung.com>
> Signed-off-by: Shradha Todi <shradha.t@...sung.com>

Would be much, much better if drivers can be combined as Krzysztof
suggests.  Superficial comments below if not.

> +config PCIE_FSD
> +	bool "Samsung FSD PCIe Controller"
> +	default n

Unnecessary since "default n" is the default.  Please read and learn
from the existing Kconfig for other drivers.

> +config PCIE_FSD_HOST
> +	bool "PCIe FSD Host Mode"
> +	depends on PCI

Shouldn't be necessary because the entire menu depends on PCI:

  menu "DesignWare PCI Core Support"
	  depends on PCI

> +	depends on PCI_MSI_IRQ_DOMAIN || PCI_DOMAIN
> +	select PCIE_DW_HOST
> +	select PCIE_FSD
> +	default n

Unnecessary.

> +config PCIE_FSD_EP
> +	bool "PCIe FSD Endpoint Mode"
> +	depends on PCI_ENDPOINT
> +	select PCIE_DW_EP
> +	select PCIE_FSD
> +	default n

Unnecessary.

> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PCIe host controller driver for Tesla fsd SoC
> + *
> + * Copyright (C) 2017-2022 Samsung Electronics Co., Ltd. http://www.samsung.com
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.

Unnecessary copyright text since you have SPDX at the top.

> + * struct fsd_pcie - representation of the pci controller

s/pci/PCI/ in English text (you do it inconsistently)
s/pcie/PCIe/ same

> + * @pci: representation of dwc pcie device structure
> + * @pdata: private data to determine the oprations supported by device

s/oprations/operations/

> + * @appl_base: represent the appl base
> + * @sysreg: represent the system register base
> + * @sysreg_base: represents the offset of the system register required

"representation", "represent", "represents"?  Pick one.

> + * @phy: represents the phy device associated for the controller

s/phy/PHY/ in English text

> +	/* External Local Bus interface(ELBI) Register */

Space before "(" in English text.

> +		dev_err(dev, "failed to map dbi_base\n");
> +		dev_err(dev, "Sysreg regmap lookup failed.\n");
> +		dev_err(dev, "Couldn't get the register offset for syscon!\n");
> +		dev_err(dev, "couldn't get aux clock\n");

Random capitalization of driver messages.  Pick one.

> +	/* assert LTSSM enable */
> +		/* Return success as link might come up later */

Random capitalization of comments.  Pick one.

> +			case RADM_VENDOR_MSG:
> +				pr_info("Interrupt received for\n");

Must include device identification.  This one also looks incomplete
("for ..."?)

> +			/* To indicate that controller has accepted to send
> +			 * Latency Tolerance reporting message

Use standard multi-line comment format.

> +			dev_err(dev, "failed to get msi irq\n");

s/msi/MSI/ in English text.
s/irq/IRQ/ also

> +	.start_link	= fsd_pcie_establish_link,

fsd_pcie_start_link() to match function pointer name.

> +	pci->dbi_base2 = NULL;
> +	pci->dbi_base = NULL;
> +	pci->atu_base = NULL;

Unnecessary since you used kzalloc().

> +	dev_info(dev, "FSD PCIe probe completed successfully\n");

Questionable utility.  Most drivers don't emit this, or if they do,
they include some useful information like address, IRQ, etc.

> +	dev_err(dev, "PCIe Failed to set 36 bit dma mask\n");

s/dma/DMA/ in English text.