netdev - Re: [PATCH 3/3] net: allwinner: sun4i-emac: fix emac SRAM mapping

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: <20150127181350.GK7200@lukather>
Date:	Tue, 27 Jan 2015 19:13:50 +0100
From:	Maxime Ripard <maxime.ripard@...e-electrons.com>
To:	Jens Kuske <jenskuske@...il.com>
Cc:	Arnd Bergmann <arnd@...db.de>, Lee Jones <lee.jones@...aro.org>,
	netdev@...r.kernel.org, devicetree@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, Chen-Yu Tsai <wens@...e.org>,
	linux-sunxi@...glegroups.com
Subject: Re: [PATCH 3/3] net: allwinner: sun4i-emac: fix emac SRAM mapping

Hi,

On Mon, Jan 26, 2015 at 03:34:49PM +0100, Jens Kuske wrote:
> Hi,
> 
> On 25/01/15 17:25, Maxime Ripard wrote:
> >Hi Jens,
> >
> >On Sun, Jan 25, 2015 at 04:49:19PM +0100, Jens Kuske wrote:
> >>The EMAC needs SRAM block A3_A4 being mapped to EMAC peripheral to
> >>work. This is done by the bootloader most of the time, but U-Boot
> >>Falcon Mode, for example, skips emac initialization and SRAM would
> >>stay mapped to the CPU.
> >
> >Thanks for reviving this.
> >
> >>Signed-off-by: Jens Kuske <jenskuske@...il.com>
> >>---
> >>  drivers/net/ethernet/allwinner/Kconfig      |  1 +
> >>  drivers/net/ethernet/allwinner/sun4i-emac.c | 18 ++++++++++++++++++
> >>  2 files changed, 19 insertions(+)
> >>
> >>diff --git a/drivers/net/ethernet/allwinner/Kconfig b/drivers/net/ethernet/allwinner/Kconfig
> >>index d8d95d4..508a288 100644
> >>--- a/drivers/net/ethernet/allwinner/Kconfig
> >>+++ b/drivers/net/ethernet/allwinner/Kconfig
> >>@@ -28,6 +28,7 @@ config SUN4I_EMAC
> >>  	select MII
> >>  	select PHYLIB
> >>  	select MDIO_SUN4I
> >>+	select MFD_SYSCON
> >>          ---help---
> >>            Support for Allwinner A10 EMAC ethernet driver.
> >>
> >>diff --git a/drivers/net/ethernet/allwinner/sun4i-emac.c b/drivers/net/ethernet/allwinner/sun4i-emac.c
> >>index 1fcd556..86c891d 100644
> >>--- a/drivers/net/ethernet/allwinner/sun4i-emac.c
> >>+++ b/drivers/net/ethernet/allwinner/sun4i-emac.c
> >>@@ -18,6 +18,8 @@
> >>  #include <linux/gpio.h>
> >>  #include <linux/interrupt.h>
> >>  #include <linux/irq.h>
> >>+#include <linux/mfd/syscon.h>
> >>+#include <linux/mfd/syscon/sun4i-sc.h>
> >>  #include <linux/mii.h>
> >>  #include <linux/module.h>
> >>  #include <linux/netdevice.h>
> >>@@ -28,6 +30,7 @@
> >>  #include <linux/of_platform.h>
> >>  #include <linux/platform_device.h>
> >>  #include <linux/phy.h>
> >>+#include <linux/regmap.h>
> >>
> >>  #include "sun4i-emac.h"
> >>
> >>@@ -78,6 +81,7 @@ struct emac_board_info {
> >>
> >>  	struct phy_device	*phy_dev;
> >>  	struct device_node	*phy_node;
> >>+	struct regmap		*sc;
> >>  	unsigned int		link;
> >>  	unsigned int		speed;
> >>  	unsigned int		duplex;
> >>@@ -862,6 +866,18 @@ static int emac_probe(struct platform_device *pdev)
> >>  		goto out;
> >>  	}
> >>
> >>+	/* Map SRAM_A3_A4 to EMAC */
> >>+	db->sc = syscon_regmap_lookup_by_compatible(
> >>+						"allwinner,sun4i-a10-syscon");
> >>+	if (IS_ERR(db->sc)) {
> >>+		dev_err(&pdev->dev, "failed to find syscon regmap\n");
> >>+		ret = PTR_ERR(db->sc);
> >>+		goto out;
> >>+	}
> >>+
> >>+	regmap_update_bits(db->sc, SUN4I_SC1, SUN4I_SC1_SRAM_A3_A4_MAP_MASK,
> >>+						SUN4I_SC1_SRAM_A3_A4_MAP_EMAC);
> >>+
> >
> >I don't think that using a syscon is the right solution here.
> >
> >All this SRAM mapping thing is mutually exclusive, and will possibly
> >impact other drivers as well.
> 
> Each single SRAM area can only be mapped to a single peripheral, so as long
> as the driver only changes bits related to his own area nothing can go wrong
> I believe.

Which is exactly my point. This kind of assumption is very
fragile. Such mistakes might be made, will slip in under any review
and might get un-noticed for a very long time.

While adding a simple driver at least would make this obvious when two
drivers will contend for the same SRAM mapping.

> SRAM_C2 looks like it can be mapped do different devices (AE, CE, ACE), but
> as far as I understand this, they are all related to the ACE device, sharing
> a common register space, and would have to be handled by a single driver
> anyway (if that will ever happen without docs)
> https://linux-sunxi.org/ACE_Register_guide
> 
> >I think this is a more a case for a small driver in drivers/soc that
> >would take care of this, and make sure that client drivers don't step
> >on each other's toe.
> 
> I'm not convinced this is necessary, but what would this driver do different
> than a basic regmap? Check if the area is already mapped by any driver and
> deny mapping it again by a different driver? Which different driver, each
> area is only interesting for a single device/driver? Except maybe mapping it
> to CPU as general purpose sram, but that would need some direct agreement
> with the driver to steal its memory anyway.

Off the top of my head:
  - Make sure no one step on each other's toe, including the CPU that
    might require a SRAM for several things (suspend, cpuidle, PSCI,
    etc.)
  - Provide a comprehensive status of the various SRAM and what they
    are mapped to in debugfs
  - Provide an abstraction to the SRAM mapping IP. You make the
    assumption that the client drivers will always use on an A10 or an
    SoC that has the same SRAMs, and the same IP to control which
    device they are mapped to. This might not be true for other
    drivers, and other SoCs.
  - Make any adjustment to these registers that wouldn't fit in any
    client drivers.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)