[AUTOSEL,for,4.15,01/78] ipmi_si: Fix error handling of platform device

Message ID 20180308045525.7662-1-alexander.levin@microsoft.com
State New
Headers show
Series
  • [AUTOSEL,for,4.15,01/78] ipmi_si: Fix error handling of platform device
Related show

Commit Message

Sasha Levin March 8, 2018, 4:56 a.m.
From: Corey Minyard <cminyard@mvista.com>


[ Upstream commit 174134ac760275457bb0d1560a0dbe6cf8a12ad6 ]

Cleanup of platform devices created by the IPMI driver was not
being done correctly and could result in a memory leak.  So
create a local boolean to know how to clean up those platform
devices.

Reported-by: David Binderman <dcb314@hotmail.com>
Signed-off-by: Corey Minyard <cminyard@mvista.com>

Signed-off-by: Sasha Levin <alexander.levin@microsoft.com>

---
 drivers/char/ipmi/ipmi_si_intf.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

-- 
2.14.1

Comments

Jerome Brunet March 8, 2018, 10:18 a.m. | #1
On Thu, 2018-03-08 at 04:56 +0000, Sasha Levin wrote:
> From: Jerome Brunet <jbrunet@baylibre.com>

> 

> [ Upstream commit 9042b46eda33ef5db3cdfc9e12b3c8cabb196141 ]

> 

> Always check phy_write return values. Better to be safe than sorry

> 

> Reviewed-by: Andrew Lunn <andrew@lunn.ch>

> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>

> Signed-off-by: David S. Miller <davem@davemloft.net>

> Signed-off-by: Sasha Levin <alexander.levin@microsoft.com>


Hi Sasha,

I have recently received a few mails from you with [PATCH AUTOSEL], such as this
one. I'm wondering what are the criteria to select these patches? This one does
not have a "Fixes" tag and, I believe, stable was not CCed of the original
patch. Do you have another selection method ?

I really sorry if you already got this question a thousand times.

I have nothing against this patch making its way to a stable release BTW, it
won't do any harm.

> ---

>  drivers/net/phy/meson-gxl.c | 50 ++++++++++++++++++++++++++++++++++-----------

>  1 file changed, 38 insertions(+), 12 deletions(-)

> 

> diff --git a/drivers/net/phy/meson-gxl.c b/drivers/net/phy/meson-gxl.c

> index 842eb871a6e3..f431c83ba0b5 100644

> --- a/drivers/net/phy/meson-gxl.c

> +++ b/drivers/net/phy/meson-gxl.c

> @@ -26,27 +26,53 @@

>  

>  static int meson_gxl_config_init(struct phy_device *phydev)

>  {

> +	int ret;

> +

>  	/* Enable Analog and DSP register Bank access by */

> -	phy_write(phydev, 0x14, 0x0000);

> -	phy_write(phydev, 0x14, 0x0400);

> -	phy_write(phydev, 0x14, 0x0000);

> -	phy_write(phydev, 0x14, 0x0400);

> +	ret = phy_write(phydev, 0x14, 0x0000);

> +	if (ret)

> +		return ret;

> +	ret = phy_write(phydev, 0x14, 0x0400);

> +	if (ret)

> +		return ret;

> +	ret = phy_write(phydev, 0x14, 0x0000);

> +	if (ret)

> +		return ret;

> +	ret = phy_write(phydev, 0x14, 0x0400);

> +	if (ret)

> +		return ret;

>  

>  	/* Write Analog register 23 */

> -	phy_write(phydev, 0x17, 0x8E0D);

> -	phy_write(phydev, 0x14, 0x4417);

> +	ret = phy_write(phydev, 0x17, 0x8E0D);

> +	if (ret)

> +		return ret;

> +	ret = phy_write(phydev, 0x14, 0x4417);

> +	if (ret)

> +		return ret;

>  

>  	/* Enable fractional PLL */

> -	phy_write(phydev, 0x17, 0x0005);

> -	phy_write(phydev, 0x14, 0x5C1B);

> +	ret = phy_write(phydev, 0x17, 0x0005);

> +	if (ret)

> +		return ret;

> +	ret = phy_write(phydev, 0x14, 0x5C1B);

> +	if (ret)

> +		return ret;

>  

>  	/* Program fraction FR_PLL_DIV1 */

> -	phy_write(phydev, 0x17, 0x029A);

> -	phy_write(phydev, 0x14, 0x5C1D);

> +	ret = phy_write(phydev, 0x17, 0x029A);

> +	if (ret)

> +		return ret;

> +	ret = phy_write(phydev, 0x14, 0x5C1D);

> +	if (ret)

> +		return ret;

>  

>  	/* Program fraction FR_PLL_DIV1 */

> -	phy_write(phydev, 0x17, 0xAAAA);

> -	phy_write(phydev, 0x14, 0x5C1C);

> +	ret = phy_write(phydev, 0x17, 0xAAAA);

> +	if (ret)

> +		return ret;

> +	ret = phy_write(phydev, 0x14, 0x5C1C);

> +	if (ret)

> +		return ret;

>  

>  	return 0;

>  }
Greg KH March 8, 2018, 12:34 p.m. | #2
On Thu, Mar 08, 2018 at 11:18:57AM +0100, Jerome Brunet wrote:
> On Thu, 2018-03-08 at 04:56 +0000, Sasha Levin wrote:

> > From: Jerome Brunet <jbrunet@baylibre.com>

> > 

> > [ Upstream commit 9042b46eda33ef5db3cdfc9e12b3c8cabb196141 ]

> > 

> > Always check phy_write return values. Better to be safe than sorry

> > 

> > Reviewed-by: Andrew Lunn <andrew@lunn.ch>

> > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>

> > Signed-off-by: David S. Miller <davem@davemloft.net>

> > Signed-off-by: Sasha Levin <alexander.levin@microsoft.com>

> 

> Hi Sasha,

> 

> I have recently received a few mails from you with [PATCH AUTOSEL], such as this

> one. I'm wondering what are the criteria to select these patches? This one does

> not have a "Fixes" tag and, I believe, stable was not CCed of the original

> patch. Do you have another selection method ?

> 

> I really sorry if you already got this question a thousand times.

> 

> I have nothing against this patch making its way to a stable release BTW, it

> won't do any harm.


Sasha, I think by now we need a "FAQ" link somewhere to point people at
that describes how these patches are being selected as it comes up every
other week or so, and I imagine you are getting tired of repeating
yourself :)

If you want me to find some place on kernel.org to put the text, I'll be
glad to do so.

thanks,

greg k-h
Sasha Levin March 19, 2018, 3:28 p.m. | #3
On Thu, Mar 08, 2018 at 04:34:23AM -0800, Greg KH wrote:
>On Thu, Mar 08, 2018 at 11:18:57AM +0100, Jerome Brunet wrote:

>> On Thu, 2018-03-08 at 04:56 +0000, Sasha Levin wrote:

>> > From: Jerome Brunet <jbrunet@baylibre.com>

>> >

>> > [ Upstream commit 9042b46eda33ef5db3cdfc9e12b3c8cabb196141 ]

>> >

>> > Always check phy_write return values. Better to be safe than sorry

>> >

>> > Reviewed-by: Andrew Lunn <andrew@lunn.ch>

>> > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>

>> > Signed-off-by: David S. Miller <davem@davemloft.net>

>> > Signed-off-by: Sasha Levin <alexander.levin@microsoft.com>

>>

>> Hi Sasha,

>>

>> I have recently received a few mails from you with [PATCH AUTOSEL], such as this

>> one. I'm wondering what are the criteria to select these patches? This one does

>> not have a "Fixes" tag and, I believe, stable was not CCed of the original

>> patch. Do you have another selection method ?

>>

>> I really sorry if you already got this question a thousand times.

>>

>> I have nothing against this patch making its way to a stable release BTW, it

>> won't do any harm.

>

>Sasha, I think by now we need a "FAQ" link somewhere to point people at

>that describes how these patches are being selected as it comes up every

>other week or so, and I imagine you are getting tired of repeating

>yourself :)

>

>If you want me to find some place on kernel.org to put the text, I'll be

>glad to do so.


Let me write something up and run it by Julia as well.

-- 

Thanks,
Sasha

Patch

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 71fad747c0c7..7499b0cd8326 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -2045,6 +2045,7 @@  static int try_smi_init(struct smi_info *new_smi)
 	int rv = 0;
 	int i;
 	char *init_name = NULL;
+	bool platform_device_registered = false;
 
 	pr_info(PFX "Trying %s-specified %s state machine at %s address 0x%lx, slave address 0x%x, irq %d\n",
 		ipmi_addr_src_to_str(new_smi->io.addr_source),
@@ -2173,6 +2174,7 @@  static int try_smi_init(struct smi_info *new_smi)
 				rv);
 			goto out_err;
 		}
+		platform_device_registered = true;
 	}
 
 	dev_set_drvdata(new_smi->io.dev, new_smi);
@@ -2279,10 +2281,11 @@  static int try_smi_init(struct smi_info *new_smi)
 	}
 
 	if (new_smi->pdev) {
-		platform_device_unregister(new_smi->pdev);
+		if (platform_device_registered)
+			platform_device_unregister(new_smi->pdev);
+		else
+			platform_device_put(new_smi->pdev);
 		new_smi->pdev = NULL;
-	} else if (new_smi->pdev) {
-		platform_device_put(new_smi->pdev);
 	}
 
 	kfree(init_name);