linux-kernel - Re: [PATCH 3/4] mmc: sdhci-of-esdhc: fix host version for T4240-R1.0-R2.0

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: <1464829874.16584.163.camel@buserror.net>
Date:	Wed, 01 Jun 2016 20:11:14 -0500
From:	Scott Wood <oss@...error.net>
To:	Arnd Bergmann <arnd@...db.de>, linuxppc-dev@...ts.ozlabs.org
Cc:	Ulf Hansson <ulf.hansson@...aro.org>,
	Yangbo Lu <yangbo.lu@....com>,
	Mark Rutland <mark.rutland@....com>,
	Xiaobo Xie <xiaobo.xie@....com>,
	"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
	"linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
	Qiang Zhao <qiang.zhao@....com>,
	Russell King <linux@....linux.org.uk>,
	Bhupesh Sharma <bhupesh.sharma@...escale.com>,
	Joerg Roedel <joro@...tes.org>,
	Claudiu Manoil <claudiu.manoil@...escale.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Rob Herring <robh+dt@...nel.org>,
	Santosh Shilimkar <ssantosh@...nel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Yang-Leo Li <leoyang.li@....com>,
	"iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
	Kumar Gala <galak@...eaurora.org>
Subject: Re: [PATCH 3/4] mmc: sdhci-of-esdhc: fix host version for
 T4240-R1.0-R2.0

On Mon, 2016-05-30 at 15:16 +0200, Arnd Bergmann wrote:
> This is a rewrite of an earlier patch from Yangbo Lu, adding a quirk
> for the NXP QorIQ T4240 in the detection of the host device version.
> 
> Unfortunately, this device cannot be detected using the compatible
> string, as we have to support existing DTS files that use the generic
> "fsl,t4240-esdhc" identifier but that have other host versions that
> are correctly detected.
> 
> Signed-off-by: Arnd Bergmann <arnd@...db.de>
> 
> diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of
> -esdhc.c
> index 3f34d354f1fc..1d4814fe4cb2 100644
> --- a/drivers/mmc/host/sdhci-of-esdhc.c
> +++ b/drivers/mmc/host/sdhci-of-esdhc.c
> @@ -73,14 +73,16 @@ static u32 esdhc_readl_fixup(struct sdhci_host *host,
>  static u16 esdhc_readw_fixup(struct sdhci_host *host,
>  				     int spec_reg, u32 value)
>  {
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_esdhc *esdhc = sdhci_pltfm_priv(pltfm_host);
>  	u16 ret;
>  	int shift = (spec_reg & 0x2) * 8;
>  
>  	if (spec_reg == SDHCI_HOST_VERSION)
> -		ret = value & 0xffff;
> -	else
> -		ret = (value >> shift) & 0xffff;
> -	return ret;
> +		return esdhc->vendor_ver << SDHCI_VENDOR_VER_SHIFT |
> +		       esdhc->spec_ver;
> +
> +	return (value >> shift) & 0xffff;
>  }
>  
>  static u8 esdhc_readb_fixup(struct sdhci_host *host,
> @@ -562,16 +564,32 @@ static const struct sdhci_pltfm_data
> sdhci_esdhc_le_pdata = {
>  	.ops = &sdhci_esdhc_le_ops,
>  };
>  
> +#define T4240_HOST_VER ((VENDOR_V_23 << SDHCI_VENDOR_VER_SHIFT) |
> SDHCI_SPEC_200)
> +static const struct soc_device_attribute esdhc_t4240_quirk = {
> +	/* T4240 revision < 0x20 uses vendor version 23, SDHCI version 200
> */
> +	{ .soc_id = "T4*(0x824000)", .revision = "0x[01]?",
> +	  .data = (void *)(uintptr_t)(T4240_HOST_VER) },

Why should this code need to care that the string begins with "T4"?  This
creates dual maintenance if that were to change.  It's also broken because
T4240 has compatible = "fsl,t4240-device-config", "fsl,qoriq-device-config
-2.0" and thus with these patches it would incorrectly show up as "P series
(0x824000)".  The compatible string of this node was never meant to be a key
for choosing a string to describe the system to userspace.

0x824000 is a magic number which should be represented symbolically.

If T4240 is affected, then so are the reduced-core variants T4160 and T4080,
but 0x824000 doesn't match them (Yangbo's patch had the same problem).  And
please don't respond with "0x824*"

You also didn't strip out the E bit of SVR which indicates encryption
capability and nothing else (Yangbo's patch did not have this problem because
it used SVR_SOC_VER).

What happens if the revision condition is more complicated, such as <= 0x20
with 0x21 being fine?  Multiple quirk entries where before we had as simple
comparison?

I fail to see how this approach is an improvement (much less one that needs to
hold up a patchset that is fixing a problem and is not touching any generic
code).  Why does this need to be a string?

-Scott