| Message ID | 1440614948-27428-1-git-send-email-linux@roeck-us.net |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On 08/26/2015 01:49 PM, Guenter Roeck wrote: > Check the return value from device_property_read_u32() to see if there > is a suitable firmware interface to read the data, and abort if not. > The function should return -ENXIO in that case; however, it returns > -ENODATA. Check for both. > > Fixes: 62ee783bf1f8 ("smsc911x: Fix crash seen if neither ACPI nor OF is configured or used") > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > Needs testing. RFC because I am not sure if the -ENODATA check is acceptable. I'm not really sure about it myself. I can think of cases where it might cause problems. That said it does work in an ACPI environment with or without the _DSD block. If the DSD/property isn't set, obviously the device doesn't configure (but it doesn't crash either) so that is good and an overall improvement for ACPI. Also, I personally might have hoisted the reg-io-width ahead of the device_get_phy_mode() and removed the phy checks, but I don't imagine there is much functional difference at this point. Tested-by: Jeremy Linton <jeremy.linton@arm.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/26/2015 12:44 PM, Jeremy Linton wrote: > On 08/26/2015 01:49 PM, Guenter Roeck wrote: >> Check the return value from device_property_read_u32() to see if there >> is a suitable firmware interface to read the data, and abort if not. >> The function should return -ENXIO in that case; however, it returns >> -ENODATA. Check for both. >> >> Fixes: 62ee783bf1f8 ("smsc911x: Fix crash seen if neither ACPI nor OF is configured or used") >> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >> --- >> Needs testing. RFC because I am not sure if the -ENODATA check is acceptable. > > I'm not really sure about it myself. I can think of cases where it might cause problems. That said it does work in an ACPI environment with or without the _DSD block. If the DSD/property isn't set, obviously the device doesn't configure (but it doesn't crash either) so that is good and an overall improvement for ACPI. > > Also, I personally might have hoisted the reg-io-width ahead of the device_get_phy_mode() and removed the phy checks, but I don't imagine there is much functional difference at this point. > > Tested-by: Jeremy Linton <jeremy.linton@arm.com> > I'll post a separate two-patch series which introduces -ENXIO as return value. Guenter -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c index 6eef3251d833..81e29e420fd0 100644 --- a/drivers/net/ethernet/smsc/smsc911x.c +++ b/drivers/net/ethernet/smsc/smsc911x.c @@ -2369,23 +2369,29 @@ static int smsc911x_probe_config(struct smsc911x_platform_config *config, { int phy_interface; u32 width = 0; + int err; phy_interface = device_get_phy_mode(dev); if (phy_interface < 0) - return phy_interface; - + phy_interface = PHY_INTERFACE_MODE_NA; config->phy_interface = phy_interface; device_get_mac_address(dev, config->mac, ETH_ALEN); - device_property_read_u32(dev, "reg-shift", &config->shift); - - device_property_read_u32(dev, "reg-io-width", &width); - if (width == 4) + err = device_property_read_u32(dev, "reg-io-width", &width); + /* device_property_read_u32() should return -ENXIO if there is no + * suitable firmware interface. In reality it returns -ENODATA. + * Check for both and use platform data if that is the case. + */ + if (err == -ENXIO || err == -ENODATA) + return err; + if (!err && width == 4) config->flags |= SMSC911X_USE_32BIT; else config->flags |= SMSC911X_USE_16BIT; + device_property_read_u32(dev, "reg-shift", &config->shift); + if (device_property_present(dev, "smsc,irq-active-high")) config->irq_polarity = SMSC911X_IRQ_POLARITY_ACTIVE_HIGH;
Commit 62ee783bf1f8 ("smsc911x: Fix crash seen if neither ACPI nor OF is configured or used") introduces an error check for the return value from device_get_phy_mode() and bails out if there is an error. Unfortunately, there are configurations where no phy is configured. Those configurations now fail. To fix the problem, assign a phy interface mode of PHY_INTERFACE_MODE_NA if device_get_phy_mode() returns an error. Check the return value from device_property_read_u32() to see if there is a suitable firmware interface to read the data, and abort if not. The function should return -ENXIO in that case; however, it returns -ENODATA. Check for both. Fixes: 62ee783bf1f8 ("smsc911x: Fix crash seen if neither ACPI nor OF is configured or used") Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- Needs testing. RFC because I am not sure if the -ENODATA check is acceptable. drivers/net/ethernet/smsc/smsc911x.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)