netdev - Re: [PATCH -next] smsc911x: Fix crash seen if neither ACPI nor OF is configured or used

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: <20150826201628.GU4215@atomide.com>
Date:	Wed, 26 Aug 2015 13:16:29 -0700
From:	Tony Lindgren <tony@...mide.com>
To:	Guenter Roeck <linux@...ck-us.net>
Cc:	Steve Glendinning <steve.glendinning@...well.net>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	Jeremy Linton <jeremy.linton@....com>,
	linux-omap@...r.kernel.org
Subject: Re: [PATCH -next] smsc911x: Fix crash seen if neither ACPI nor OF is
 configured or used

* Guenter Roeck <linux@...ck-us.net> [150826 11:37]:
> On 08/26/2015 10:04 AM, Tony Lindgren wrote:
> >Hi,
> >
> >* Guenter Roeck <linux@...ck-us.net> [150817 13:48]:
> >>Commit 0b50dc4fc971 ("Convert smsc911x to use ACPI as well as DT") makes
> >>the call to smsc911x_probe_config() unconditional, and no longer fails if
> >>there is no device node. device_get_phy_mode() is called unconditionally,
> >>and if there is no phy node configured returns an error code. This error
> >>code is assigned to phy_interface, and interpreted elsewhere in the code
> >>as valid phy mode. This in turn causes qemu to crash when running a
> >>variant of realview_pb_defconfig.
> >>
> >>	qemu: hardware error: lan9118_read: Bad reg 0x86
> >>
> >>Fixes: 0b50dc4fc971 ("Convert smsc911x to use ACPI as well as DT")
> >>Cc: Jeremy Linton <jeremy.linton@....com>
> >>Cc Graeme Gregory <graeme.gregory@...aro.org>
> >>Signed-off-by: Guenter Roeck <linux@...ck-us.net>
> >>---
> >>  drivers/net/ethernet/smsc/smsc911x.c | 7 ++++++-
> >>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
> >>index 0f21aa3bb537..34f97684506b 100644
> >>--- a/drivers/net/ethernet/smsc/smsc911x.c
> >>+++ b/drivers/net/ethernet/smsc/smsc911x.c
> >>@@ -2367,12 +2367,17 @@ static const struct smsc911x_ops shifted_smsc911x_ops = {
> >>  static int smsc911x_probe_config(struct smsc911x_platform_config *config,
> >>  				 struct device *dev)
> >>  {
> >>+	int phy_interface;
> >>  	u32 width = 0;
> >>
> >>  	if (!dev)
> >>  		return -ENODEV;
> >>
> >>-	config->phy_interface = device_get_phy_mode(dev);
> >>+	phy_interface = device_get_phy_mode(dev);
> >>+	if (phy_interface < 0)
> >>+		return phy_interface;
> >>+
> >>+	config->phy_interface = phy_interface;
> >>
> >>  	device_get_mac_address(dev, config->mac, ETH_ALEN);
> >
> >Looks like this change makes at least omap boards using smsc911x
> >fail with -22 for me in Linux next.
> >
> >Do any of the the device tree configured smsc911x devices actually
> >have a phy configured?
> >
> 
> Ok, this is more subtle than I thought.
> 
> Previously, the code would not attempt any devicetree configuration
> if devicetree was not configured.
> 
> Now it does.
> 
> The error return from device_get_phy_mode() isn't the actual problem.
> Apparently it doesn't really matter if a nonsensical value is assigned
> to phy_interface.
> 
> The problem is that the reg-io-width property is obviously not present
> in the non-dt and non-acpi case. This overwrites the existing platform data
> configuration and selects 16 bit mode, to which the (simulated) hardware
> obviously reacts less than enthusiastic.
> 
> Fixing this properly won't be easy. If the "reg-io-width" property
> is not present or wrong, the default register width is 16 bit. Obviously,
> if neither DT nor ACPI is available, it won't be present. This causes
> the crash I had observed.

Heh OK :)
 
> Bad part is that there does not seem to be a reliable means to detect
> that platform data should be used in that situation. Other device_get_XXX
> functions return -ENXIO if that happens, but not device_property_read_u32().
> It is _supposed_ to return it per its API, but it doesn't (it returns
> -ENODATA).
> 
> We may need two separate patches, one to fix up device_property_read_u32()
> to return -ENXIO, and one to fix smsc911x_probe_config() to ignore the error
> from device_get_phy_mode(), and to bail out if device_property_read_u32()
> returns -ENXIO.

I guess the device_property_read_u32() change needs to be discussed
separately.. So probably best to fix up the regression to smsc911x
first.
 
> The simpler alternative would be to check the return value from
> device_property_read_u32() for both -ENXIO and -ENODATA.
> This would make the code independent of the necessary core changes
> (which may take a while). I tested this variant, and it works, at least
> for the non-DT case.
> 
> Does this make sense ?

Yeh I think that would allow fixing up the smsc911x regression while
discussing the device_property_read_u32() change. Got a test patch
for me to try?

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html