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

[Daniel Lezcano <daniel.lezcano@linaro.org>]

  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