linux-kernel - Re: sun50i-a64-pinctrl WARN

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: <20170429212857.tvdsxi4tlft3mdpj@angband.pl>
Date:   Sat, 29 Apr 2017 23:28:57 +0200
From:   Adam Borowski <kilobyte@...band.pl>
To:     Tejun Heo <tj@...nel.org>
Cc:     Andre Przywara <andre.przywara@....com>,
        Icenowy Zheng <icenowy@...c.xyz>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: sun50i-a64-pinctrl WARN_ON drivers/base/dd.c:349

On Fri, Apr 28, 2017 at 06:03:14PM -0400, Tejun Heo wrote:
> On Tue, Apr 18, 2017 at 10:12:16AM +0100, Andre Przywara wrote:
> > Yeah, so I stack-dumped on the zero allocations and indeed they are
> > called from cleanup functions:
> > drivers/pinctrl/pinmux.c:pinmux_generic_free_functions():
> > 	devm_kzalloc(sizeof(*indices) * pctldev->num_functions, ...)
> > (and another one I don't know from the top of the my head, logs at home)
> >
> > So my hunch was that once EPROBE_DEFER triggers the devres cleanup, it
> > uses some reverse list traversal to release all allocated resources
> > (backwards!), so missing those which get (appended) during the process.
> > But I don't think that would not work with the locking.
> > So I have to dig deeper tonight in my logs.
> 
> If this is a valid use case, we can change devm to repeat till empty
> but it's a weird thing to do to allocate from a release function.
> 
> So, something like this.  Only compile tested.

I've tested this, and the results are mixed.  It _does_ make the WARN go
away, but on the other hand, much later during the boot, another driver
(sun8i-emac) gets deferred and never undefers.

Without the patch, there's:
[ ok ] Synthesizing the initial hotplug events...done.
[    6.666984] bus: 'platform': driver_probe_device: matched device 1c30000.ethernet with driver sun8i-emac
[    6.676520] bus: 'platform': really_probe: probing driver sun8i-emac with device 1c30000.ethernet
[    6.686479] driver: 'sun8i-emac': driver_bound: bound to device '1c30000.ethernet'
[....] [    6.694132] bus: 'platform': really_probe: bound device 1c30000.ethernet to driver sun8i-emac
[ ok ng for /dev to be fully populated...done.
<snip>
[....] Configuring network interfaces...[   11.414884] libphy: 1c30000.ethernet: probed
[   11.449742] bus: 'mdio_bus': driver_probe_device: matched device 1c30000.ethernet-0:00 with driver RTL8211E Gigabit Ethernet
[   11.461013] bus: 'mdio_bus': really_probe: probing driver RTL8211E Gigabit Ethernet with device 1c30000.ethernet-0:00
[   11.471666] driver: 'RTL8211E Gigabit Ethernet': driver_bound: bound to device '1c30000.ethernet-0:00'
[   11.481007] bus: 'mdio_bus': really_probe: bound device 1c30000.ethernet-0:00 to driver RTL8211E Gigabit Ethernet
[   11.491527] RTL8211E Gigabit Ethernet 1c30000.ethernet-0:00: attached PHY driver [RTL8211E Gigabit Ethernet] (mii_bus:phy_addr=1c30000.ethernet-0:00, irq=-1)
[   11.505608] sun8i-emac 1c30000.ethernet: device MAC address slot 0 02:ba:8f:9e:44:8f

With the patch:
[ ok ] Synthesizing the initial hotplug events...done.
[....] Waiting for /dev to be fully populated...[    6.325314] bus: 'platform': driver_probe_device: matched device 1c30000.ethernet with driver sun8i-emac
[    6.334842] bus: 'platform': really_probe: probing driver sun8i-emac with device 1c30000.ethernet
[    6.344253] platform 1c30000.ethernet: Driver sun8i-emac requests probe deferral
[    6.351662] platform 1c30000.ethernet: Added to deferred list
done.
<snip>
[....] Configuring network interfaces...Cannot find device "eth0"
ifup: failed to bring up eth0
failed.


There's a concern, though, that this second driver is not in mainline (and
won't ever be -- it's being replaced by dwmac-sun8i, but the latter is not
yet up to scratch), thus I'm not sure how relevant this is.

The second concern is that I'm using the same tree
(icenowy/sunxi64-4.11-rc7) and config (plus filesystems) as Icenowy, on the
same hardware, yet I did see this warning in versions much earlier than her,
and it doesn't go away in -next for me either.  All that differs are
toolchain versions (I'm on Debian unstable) and perhaps some u-boot bits.

Because of the amount of churn in sunxi land and the driver's mainlining
status, I think it'd be good to wait after the merge window that starts
tomorrow, and re-test then.

Full console output with the patch applied attached.


Meow!


> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> index 71d5770..d2a9f34 100644
> --- a/drivers/base/devres.c
> +++ b/drivers/base/devres.c
> @@ -509,13 +509,21 @@ static int release_nodes(struct device *dev, struct list_head *first,
>  int devres_release_all(struct device *dev)
>  {
>  	unsigned long flags;
> +	int cnt = 0, ret;
>  
>  	/* Looks like an uninitialized device structure */
>  	if (WARN_ON(dev->devres_head.next == NULL))
>  		return -ENODEV;
> -	spin_lock_irqsave(&dev->devres_lock, flags);
> -	return release_nodes(dev, dev->devres_head.next, &dev->devres_head,
> -			     flags);
> +
> +	/* Release callbacks may create new nodes, repeat till empty */
> +	do {
> +		spin_lock_irqsave(&dev->devres_lock, flags);
> +		ret = release_nodes(dev, dev->devres_head.next,
> +				    &dev->devres_head, flags);
> +		cnt += ret;
> +	} while (ret);
> +
> +	return cnt;
>  }
>  
>  /**
> 

-- 
Don't be racist.  White, amber or black, all beers should be judged based
solely on their merits.  Heck, even if occasionally a cider applies for a
beer's job, why not?
On the other hand, corpo lager is not a race.

View attachment "with-patch" of type "text/plain" (35699 bytes)