linux-kernel - Re: [PATCH 1/8] mmc: sdhci-st: Intial support for ST SDHCI controller

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]

Date:	Mon, 2 Jun 2014 12:06:39 +0100
From:	Peter Griffin <peter.griffin@...aro.org>
To:	Lee Jones <lee.jones@...aro.org>
Cc:	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	maxime.coquelin@...com, patrice.chotard@...com,
	srinivas.kandagatla@...il.com, chris@...ntf.net,
	ulf.hansson@...aro.org, kernel@...inux.com,
	linux-mmc@...r.kernel.org, devicetree@...r.kernel.org,
	Giuseppe Cavallaro <peppe.cavallaro@...com>
Subject: Re: [PATCH 1/8] mmc: sdhci-st: Intial support for ST SDHCI controller

Hi Lee,

Thanks for your feedback, all your other comments will be fixed in v2. 
However see comments below for this patch

> > +	clk_prepare_enable(clk);
> 
> Move this down as far as it will go.  When do you _need_ the clock
> running by?
> 
> > +	host->mmc->caps |= MMC_CAP_8_BIT_DATA | MMC_CAP_BUS_WIDTH_TEST
> > +			| MMC_CAP_1_8V_DDR;
> > +
> > +	if (of_property_read_bool(np, "non-removable"))
> > +		host->mmc->caps |= MMC_CAP_NONREMOVABLE;
> > +
> > +	pltfm_host = sdhci_priv(host);
> > +	pltfm_host->clk = clk;
> > +
> > +	ret = sdhci_add_host(host);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed sdhci_add_host\n");
> > +		goto err_out;
> 
> If it's possible to move the clk_prepare enable down past here, then
> you only need to do sdhci_pltfm_free() and you can remove all of the
> err_out error path.

No its not possible. sdhci_add_host() reads registers on
the IP, if the clock isn't enabled the system can hang.

> 
> > +	}
> > +
> > +	pltfm_host->priv = pdata;
> > +
> > +	platform_set_drvdata(pdev, host);
> > +
> > +	host_version = readw_relaxed((host->ioaddr + SDHCI_HOST_VERSION));
> 
> Do we want to be doing any error checking for unsupported devices
> here?

Not that I'm aware of. This is just a debug print I added as it is useful
for debugging. Its not unheard for software folks not to be told that the
IP version has changed in a new SoC, so comparing dmesg traces of working
kernels with non working ones which include IP versions etc can often
shed some light on whats happening.

Arguably if the maintainers think its helpful then it could be added in 
sdhci_add_host(), and every platform would benefit. Or if its deemed 
not helpful then it can be removed!

Cheers,

Peter.
--
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/