[PATCH 1/1] clocksource: sh_mtu2: Fix irq leaks when sh_mtu2_setup() fails

[PATCH 1/1] clocksource: sh_mtu2: Fix irq leaks when sh_mtu2_setup() fails

[Takeshi Yoshimura]

My static checker detected irq leaks in sh_mtu2_setup(). The problem
happens when the first request_irq() succeeds but the later ones fail.
This situation also results that the clockevent for the first channel
is registered although the driver fails. So I introduce
sh_mtu2_setup_channels() to ensure the consistency of all the clockevent
and irqs even if the error occurs.

Signed-off-by: Takeshi Yoshimura <yos@sslab.ics.keio.ac.jp>
---
 drivers/clocksource/sh_mtu2.c | 65 ++++++++++++++++++++++++++-----------------
 1 file changed, 39 insertions(+), 26 deletions(-)

diff --git a/drivers/clocksource/sh_mtu2.c b/drivers/clocksource/sh_mtu2.c
index 3d88698..5ddc433 100644
--- a/drivers/clocksource/sh_mtu2.c
+++ b/drivers/clocksource/sh_mtu2.c
@@ -344,38 +344,54 @@ static int sh_mtu2_register(struct sh_mtu2_channel *ch, const char *name)
 	return 0;
 }
 
-static int sh_mtu2_setup_channel(struct sh_mtu2_channel *ch, unsigned int index,
-				 struct sh_mtu2_device *mtu)
+static int sh_mtu2_setup_channels(struct sh_mtu2_device *mtu)
 {
 	static const unsigned int channel_offsets[] = {
 		0x300, 0x380, 0x000,
 	};
 	char name[6];
-	int irq;
+	int irq[3];
 	int ret;
+	int i;
 
-	ch->mtu = mtu;
-
-	sprintf(name, "tgi%ua", index);
-	irq = platform_get_irq_byname(mtu->pdev, name);
-	if (irq < 0) {
-		/* Skip channels with no declared interrupt. */
-		return 0;
+	for (i = 0; i < mtu->num_channels; ++i) {
+		struct sh_mtu2_channel *ch = &mtu->channels[i];
+
+		ch->mtu = mtu;
+		sprintf(name, "tgi%ua", i);
+		irq[i] = platform_get_irq_byname(mtu->pdev, name);
+		if (irq[i] < 0) {
+			/* Skip channels with no declared interrupt. */
+			continue;
+		}
+
+		ret = request_irq(irq[i], sh_mtu2_interrupt,
+				  IRQF_TIMER | IRQF_IRQPOLL | IRQF_NOBALANCING,
+				  dev_name(&ch->mtu->pdev->dev), ch);
+		if (ret) {
+			dev_err(&ch->mtu->pdev->dev, "ch%u: failed to request irq %d\n",
+				i, irq[i]);
+			goto err;
+		}
 	}
 
-	ret = request_irq(irq, sh_mtu2_interrupt,
-			  IRQF_TIMER | IRQF_IRQPOLL | IRQF_NOBALANCING,
-			  dev_name(&ch->mtu->pdev->dev), ch);
-	if (ret) {
-		dev_err(&ch->mtu->pdev->dev, "ch%u: failed to request irq %d\n",
-			index, irq);
-		return ret;
+	for (i = 0; i < mtu->num_channels; ++i) {
+		struct sh_mtu2_channel *ch = &mtu->channels[i];
+
+		ch->base = mtu->mapbase + channel_offsets[i];
+		ch->index = i;
+		sh_mtu2_register(ch, dev_name(&mtu->pdev->dev));
 	}
 
-	ch->base = mtu->mapbase + channel_offsets[index];
-	ch->index = index;
+	return 0;
 
-	return sh_mtu2_register(ch, dev_name(&mtu->pdev->dev));
+err:
+	--i;
+	for (; i >= 0; --i) {
+		if (irq[i] >= 0)
+			free_irq(irq[i], &mtu->channels[i]);
+	}
+	return ret;
 }
 
 static int sh_mtu2_map_memory(struct sh_mtu2_device *mtu)
@@ -398,7 +414,6 @@ static int sh_mtu2_map_memory(struct sh_mtu2_device *mtu)
 static int sh_mtu2_setup(struct sh_mtu2_device *mtu,
 			 struct platform_device *pdev)
 {
-	unsigned int i;
 	int ret;
 
 	mtu->pdev = pdev;
@@ -433,11 +448,9 @@ static int sh_mtu2_setup(struct sh_mtu2_device *mtu,
 		goto err_unmap;
 	}
 
-	for (i = 0; i < mtu->num_channels; ++i) {
-		ret = sh_mtu2_setup_channel(&mtu->channels[i], i, mtu);
-		if (ret < 0)
-			goto err_unmap;
-	}
+	ret = sh_mtu2_setup_channels(mtu);
+	if (ret < 0)
+		goto err_unmap;
 
 	platform_set_drvdata(pdev, mtu);
 
-- 
1.9.1

Re: [PATCH 1/1] clocksource: sh_mtu2: Fix irq leaks when sh_mtu2_setup() fails

[Daniel Lezcano]

  On 06/14/2015 12:57 PM, Takeshi Yoshimura wrote:
> My static checker detected irq leaks in sh_mtu2_setup(). The problem
> happens when the first request_irq() succeeds but the later ones fail.
> This situation also results that the clockevent for the first channel
> is registered although the driver fails. So I introduce
> sh_mtu2_setup_channels() to ensure the consistency of all the clockevent
> and irqs even if the error occurs.
>
> Signed-off-by: Takeshi Yoshimura <yos@sslab.ics.keio.ac.jp>
> ---
>   drivers/clocksource/sh_mtu2.c | 65 ++++++++++++++++++++++++++-----------------
>   1 file changed, 39 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/clocksource/sh_mtu2.c b/drivers/clocksource/sh_mtu2.c
> index 3d88698..5ddc433 100644
> --- a/drivers/clocksource/sh_mtu2.c
> +++ b/drivers/clocksource/sh_mtu2.c
> @@ -344,38 +344,54 @@ static int sh_mtu2_register(struct sh_mtu2_channel *ch, const char *name)
>   	return 0;
>   }
>
> -static int sh_mtu2_setup_channel(struct sh_mtu2_channel *ch, unsigned int index,
> -				 struct sh_mtu2_device *mtu)
> +static int sh_mtu2_setup_channels(struct sh_mtu2_device *mtu)
>   {
>   	static const unsigned int channel_offsets[] = {
>   		0x300, 0x380, 0x000,
>   	};
>   	char name[6];
> -	int irq;
> +	int irq[3];

int irq[mtu->channels] ?

>   	int ret;
> +	int i;
>
> -	ch->mtu = mtu;
> -
> -	sprintf(name, "tgi%ua", index);
> -	irq = platform_get_irq_byname(mtu->pdev, name);
> -	if (irq < 0) {
> -		/* Skip channels with no declared interrupt. */
> -		return 0;
> +	for (i = 0; i < mtu->num_channels; ++i) {

For the sake of consistency, perhaps take the opportunity to replace 
'++i' by 'i++'.

> +		struct sh_mtu2_channel *ch = &mtu->channels[i];
> +
> +		ch->mtu = mtu;
> +		sprintf(name, "tgi%ua", i);
> +		irq[i] = platform_get_irq_byname(mtu->pdev, name);
> +		if (irq[i] < 0) {
> +			/* Skip channels with no declared interrupt. */
> +			continue;
> +		}
> +
> +		ret = request_irq(irq[i], sh_mtu2_interrupt,
> +				  IRQF_TIMER | IRQF_IRQPOLL | IRQF_NOBALANCING,
> +				  dev_name(&ch->mtu->pdev->dev), ch);
> +		if (ret) {
> +			dev_err(&ch->mtu->pdev->dev, "ch%u: failed to request irq %d\n",
> +				i, irq[i]);i

As 'i' is a type of 'int', 'ch%u' must be 'ch%d'

> +			goto err;
> +		}
>   	}
>
> -	ret = request_irq(irq, sh_mtu2_interrupt,
> -			  IRQF_TIMER | IRQF_IRQPOLL | IRQF_NOBALANCING,
> -			  dev_name(&ch->mtu->pdev->dev), ch);
> -	if (ret) {
> -		dev_err(&ch->mtu->pdev->dev, "ch%u: failed to request irq %d\n",
> -			index, irq);
> -		return ret;
> +	for (i = 0; i < mtu->num_channels; ++i) {
> +		struct sh_mtu2_channel *ch = &mtu->channels[i];
> +
> +		ch->base = mtu->mapbase + channel_offsets[i];
> +		ch->index = i;

'ch->index' is 'unsigned int' but 'i' is 'int'

> +		sh_mtu2_register(ch, dev_name(&mtu->pdev->dev));

sh_mtu2_register return value was checked before this patch.

>   	}
>
> -	ch->base = mtu->mapbase + channel_offsets[index];
> -	ch->index = index;
> +	return 0;
>
> -	return sh_mtu2_register(ch, dev_name(&mtu->pdev->dev));
> +err:
> +	--i;
> +	for (; i >= 0; --i) {

	for (i--; i >= 0; i--)

> +		if (irq[i] >= 0)
> +			free_irq(irq[i], &mtu->channels[i]);
> +	}
> +	return ret;
>   }

-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog