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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <C79B248886DD134989C8FF6B096A91AB91B616BECE@BGMAIL01.nvidia.com>
Date:	Wed, 5 Jun 2013 20:27:44 +0530
From:	Jay Agarwal <jagarwal@...dia.com>
To:	'Stephen Warren' <swarren@...dotorg.org>
CC:	"linux@....linux.org.uk" <linux@....linux.org.uk>,
	"thierry.reding@...onic-design.de" <thierry.reding@...onic-design.de>,
	"bhelgaas@...gle.com" <bhelgaas@...gle.com>,
	Laxman Dewangan <ldewangan@...dia.com>,
	"olof@...om.net" <olof@...om.net>, Hiroshi Doyu <hdoyu@...dia.com>,
	Prashant Gaikwad <pgaikwad@...dia.com>,
	"mturquette@...aro.org" <mturquette@...aro.org>,
	Peter De Schrijver <pdeschrijver@...dia.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	Juha Tukkinen <jtukkinen@...dia.com>,
	Krishna Thota <kthota@...dia.com>
Subject: RE: [PATCH V3 2/4] ARM: tegra: pcie: Add tegra3 support

> > 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().
Yes you are right, it is not required. I will revert it along with taking care of other comments.
--
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/