linux-kernel - Re: [PATCH v3 1/6] phy: add a driver for the Berlin SATA PHY

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: <16480004.aaDhzlOZgi@wuerfel>
Date:	Wed, 14 May 2014 15:02:34 +0200
From:	Arnd Bergmann <arnd@...db.de>
To:	linux-arm-kernel@...ts.infradead.org
Cc:	Antoine Ténart 
	<antoine.tenart@...e-electrons.com>,
	sebastian.hesselbarth@...il.com, tj@...nel.org, kishon@...com,
	thomas.petazzoni@...e-electrons.com, zmxu@...vell.com,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-ide@...r.kernel.org, alexandre.belloni@...e-electrons.com,
	jszhang@...vell.com
Subject: Re: [PATCH v3 1/6] phy: add a driver for the Berlin SATA PHY

On Wednesday 14 May 2014 11:48:57 Antoine Ténart wrote:
> +static int phy_berlin_sata_power_on(struct phy *phy)
> +{
> +       struct phy_berlin_desc *desc = phy_get_drvdata(phy);
> +       struct phy_berlin_priv *priv = to_berlin_sata_phy_priv(desc);
> +       u32 regval;
> +
> +       spin_lock(&priv->lock);
> +
> +       /* Power up PHY */
> +       writel(CONTROL_REGISTER, priv->base + HOST_VSA_ADDR);
> +       regval = readl(priv->base + HOST_VSA_DATA);
> +       regval &= ~(desc->val);
> +       writel(regval, priv->base + HOST_VSA_DATA);
> +
> +       /* Configure MBus */
> +       writel(MBUS_SIZE_CONTROL, priv->base + HOST_VSA_ADDR);
> +       regval = readl(priv->base + HOST_VSA_DATA);
> +       regval |= MBUS_WRITE_REQUEST_SIZE_128 | MBUS_READ_REQUEST_SIZE_128;
> +       writel(regval, priv->base + HOST_VSA_DATA);
> +
> +       spin_unlock(&priv->lock);
> +
> +       return 0;
> +}
> +
> +static int phy_berlin_sata_power_off(struct phy *phy)
> +{
> +       struct phy_berlin_desc *desc = phy_get_drvdata(phy);
> +       struct phy_berlin_priv *priv = to_berlin_sata_phy_priv(desc);
> +       u32 regval;
> +
> +       spin_lock(&priv->lock);
> +
> +       /* Power down PHY */
> +       writel(CONTROL_REGISTER, priv->base + HOST_VSA_ADDR);
> +       regval = readl(priv->base + HOST_VSA_DATA);
> +       regval |= desc->val;
> +       writel(regval, priv->base + HOST_VSA_DATA);
> +
> +       spin_unlock(&priv->lock);
> +
> +       return 0;

I don't get this part: you have a reference to the phy here,
but then you go poking the phy registers from the SATA driver
rather than calling a PHY API function.


> +                * By default the PHY node is used to request and match a PHY.
> +                * We describe one PHY per sub-node here. Use the right node.
> +                */
> +               phy->dev.of_node = child;
> +
> +               priv->phys[phy_id].phy = phy;
> +               priv->phys[phy_id].val = desc[phy_id].val;
> +               priv->phys[phy_id].index = phy_id;
> +               phy_set_drvdata(phy, &priv->phys[phy_id]);

And here, you set a driver specific value into a structure used by the
PHY.

Both of these are layering violations. You should either use the PHY
interfaces correctly so the SATA driver doesn't have to know about the
specific, or not use a PHY device node at all and do everything in
the SATA front-end.

	Arnd
--
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/