linux-kernel - Re: [PATCH V3 2/4] ARM: tegra: pcie: Add tegra3 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 for Android: free password hash cracker in your pocket

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

Message-ID: <51AE3D3B.6080102@wwwdotorg.org>
Date:	Tue, 04 Jun 2013 13:17:15 -0600
From:	Stephen Warren <swarren@...dotorg.org>
To:	Jay Agarwal <jagarwal@...dia.com>
CC:	linux@....linux.org.uk, thierry.reding@...onic-design.de,
	bhelgaas@...gle.com, ldewangan@...dia.com, olof@...om.net,
	hdoyu@...dia.com, pgaikwad@...dia.com, mturquette@...aro.org,
	pdeschrijver@...dia.com, linux-arm-kernel@...ts.infradead.org,
	linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org, jtukkinen@...dia.com, kthota@...dia.com
Subject: Re: [PATCH V3 2/4] ARM: tegra: pcie: Add tegra3 support

On 06/04/2013 12:57 PM, Jay Agarwal wrote:
> - Enable PCIe root port 2 for Cardhu
> - Make private data structure for each SoC
> - Add required Tegra30 clocks and regulators
> - Add Tegra30 specific code in enable controller
> - Corrected logic in read/write config space to
>   display right device number on bus 0
> - Fixed unnecessary freeing of tegra_pcie_bus structure in tegra_pcie_remove
> - Added Tegra30 specific properties in pci binding document

> diff --git a/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt b/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt

> +- avdd-supply: Power supply for controller (1.05V)

> +  "cml": The Tegra clock of that name

Both those changes need to mention that those additions are only
required for Tegra30, not Tegra20. In other words,

+- avdd-supply: Power supply for controller (1.05V) (not required for
Tegra20)

+  "cml": The Tegra clock of that name (not required for Tegra20)

You probably also want to explicitly mention nvidia,tegra30-pcie as a
legal compatible value.

> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c

> +/* used to differentiate tegra chips code */
> +struct tegra_pcie_soc_data {
> +	unsigned int num_max_ports;

nit: "num max" seems redundant. max_ports or num_ports would be better.

>  struct tegra_pcie_port {
> @@ -384,7 +408,7 @@ static int tegra_pcie_read_conf(struct pci_bus *bus, unsigned int devfn,
>  		struct tegra_pcie_port *port;
>  
>  		list_for_each_entry(port, &pcie->ports, list) {
> -			if (port->index + 1 == slot) {
> +			if (port->index == slot) {

This and the equivalent change in tegra_pcie_write_conf() seem like a
bug-fix unrelated to the addition of Tegra30 support. Hence, they should
be a separate patch.


> @@ -398,7 +422,6 @@ static int tegra_pcie_read_conf(struct pci_bus *bus, unsigned int devfn,
>  			*value = 0xffffffff;
>  			return PCIBIOS_DEVICE_NOT_FOUND;
>  		}
> -
>  		addr += tegra_pcie_conf_offset(devfn, where);

Unnecessary white-space change.

> @@ -1549,10 +1660,9 @@ static int tegra_pcie_remove(struct platform_device *pdev)
>  	struct tegra_pcie_bus *bus;
>  	int err;
>  
> -	list_for_each_entry(bus, &pcie->busses, list) {
> +	list_for_each_entry(bus, &pcie->busses, list)
>  		vunmap(bus->area->addr);
> -		kfree(bus);
> -	}
> +	kfree(bus);

This doesn't look right. Can you please explain it further? This is
looping over every bus in a dynamic list, so surely each entry needs to
be freed? Did you do this to avoid a crash? If so, the issue is likely
that the loop should use list_for_each_entry_safe() rather than
list_for_each_entry().
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/