[PATCH] geode-mfgpt: restore previous behavior for selecting IRQ

[PATCH] geode-mfgpt: restore previous behavior for selecting IRQ

[Jens Rottmann]

geode-mfgpt: restore previous behavior for selecting IRQ

The MFGPT IRQ used to be, in order of decreasing priority,
* IRQ supplied by the user as a boot-time parameter,
* IRQ previously set by the BIOS or another driver,
* default IRQ given at compile time.

Return to this behavior, which got broken when splitting the
MFGPT/clocksource driver for 2.6.33-rc1.

Signed-off-by: Jens Rottmann <JRottmann@LiPPERTEmbedded.de>
---

Hi Andres,

a bit more detail. Up to 2.6.32 the MFGPT IRQ was chosen like this:

static int irq;
static int __init mfgpt_setup(char *str)
{
	get_option(&str, &irq);
	return 1;
}
__setup("mfgpt_irq=", mfgpt_setup);
[...]
/* Choose IRQ: if none supplied, keep IRQ already set or use default */
if (!*irq)
	*irq = (zsel >> shift) & 0xF;	// may have been set by the BIOS
if (!*irq)
	*irq = MFGPT_DEFAULT_IRQ;	// #defined as 7

The code in 2.6.33-rc3 after the MFGPT/clocksource split looks like this:

static int timer_irq = CONFIG_CS5535_MFGPT_DEFAULT_IRQ;	// defaults to 7
module_param_named(irq, timer_irq, int, 0644);
[...]
if (!*irq)
	*irq = (zsel >> shift) & 0xF;		// gets ignored now
if (!*irq)
	*irq = CONFIG_CS5535_MFGPT_DEFAULT_IRQ;	// again!?

The patch removes the first of the two ocurrances of
CONFIG_CS5535_MFGPT_DEFAULT_IRQ, reverting the IRQ selection to the previous
(working) behavior.


BTW, while looking into this I stumbled upon the fact that cs5535_mfgpt_init()
doesn't free up the timer in the error path if cs5535_mfgpt_setup_irq() or
setup_irq() fail. However, some comments in cs5535-mfgpt.c (stating that it's
not possible to release the individual timer hardware) make me think this might
have been skipped on purpose? Could you please have a look?

Thanks,
Jens

--- linux-2.6.33-rc3-git4/drivers/clocksource/cs5535-clockevt.c
+++ previous_behavior/drivers/clocksource/cs5535-clockevt.c
@@ -21,7 +21,7 @@
 
 #define DRV_NAME "cs5535-clockevt"
 
-static int timer_irq = CONFIG_CS5535_MFGPT_DEFAULT_IRQ;
+static int timer_irq;
 module_param_named(irq, timer_irq, int, 0644);
 MODULE_PARM_DESC(irq, "Which IRQ to use for the clock source MFGPT ticks.");
 
_

Re: [PATCH] geode-mfgpt: restore previous behavior for selecting IRQ

[Andres Salomon]

On Tue, 12 Jan 2010 11:14:30 +0100
Jens Rottmann <JRottmann@LiPPERTEmbedded.de> wrote:

> geode-mfgpt: restore previous behavior for selecting IRQ
> 
> The MFGPT IRQ used to be, in order of decreasing priority,
> * IRQ supplied by the user as a boot-time parameter,
> * IRQ previously set by the BIOS or another driver,
> * default IRQ given at compile time.
> 
> Return to this behavior, which got broken when splitting the
> MFGPT/clocksource driver for 2.6.33-rc1.
> 
> Signed-off-by: Jens Rottmann <JRottmann@LiPPERTEmbedded.de>
> ---
> 
> Hi Andres,
> 
> a bit more detail. Up to 2.6.32 the MFGPT IRQ was chosen like this:
> 
> static int irq;
> static int __init mfgpt_setup(char *str)
> {
> 	get_option(&str, &irq);
> 	return 1;
> }
> __setup("mfgpt_irq=", mfgpt_setup);
> [...]
> /* Choose IRQ: if none supplied, keep IRQ already set or use default
> */ if (!*irq)
> 	*irq = (zsel >> shift) & 0xF;	// may have been set by
> the BIOS if (!*irq)
> 	*irq = MFGPT_DEFAULT_IRQ;	// #defined as 7
> 
> The code in 2.6.33-rc3 after the MFGPT/clocksource split looks like
> this:
> 
> static int timer_irq = CONFIG_CS5535_MFGPT_DEFAULT_IRQ;	//
> defaults to 7 module_param_named(irq, timer_irq, int, 0644);
> [...]
> if (!*irq)
> 	*irq = (zsel >> shift) & 0xF;		// gets ignored
> now if (!*irq)
> 	*irq = CONFIG_CS5535_MFGPT_DEFAULT_IRQ;	// again!?
> 
> The patch removes the first of the two ocurrances of
> CONFIG_CS5535_MFGPT_DEFAULT_IRQ, reverting the IRQ selection to the
> previous (working) behavior.

You're right, thanks.  For the patch below:

Acked-by: Andres Salomon <dilinger@collabora.co.uk>


> 
> 
> BTW, while looking into this I stumbled upon the fact that
> cs5535_mfgpt_init() doesn't free up the timer in the error path if
> cs5535_mfgpt_setup_irq() or setup_irq() fail. However, some comments
> in cs5535-mfgpt.c (stating that it's not possible to release the
> individual timer hardware) make me think this might have been skipped
> on purpose? Could you please have a look?

Oh, you mean in the clockevt driver?  Yeah, we can't really free the
timer, unfortunately.

It actually might not be a bad idea to reverse the code so that the IRQ
allocation happens first, since we can clean that up if mfgpt
allocation fails.   Feel like sending a patch? :)



> 
> Thanks,
> Jens
> 
> --- linux-2.6.33-rc3-git4/drivers/clocksource/cs5535-clockevt.c
> +++ previous_behavior/drivers/clocksource/cs5535-clockevt.c
> @@ -21,7 +21,7 @@
>  
>  #define DRV_NAME "cs5535-clockevt"
>  
> -static int timer_irq = CONFIG_CS5535_MFGPT_DEFAULT_IRQ;
> +static int timer_irq;
>  module_param_named(irq, timer_irq, int, 0644);
>  MODULE_PARM_DESC(irq, "Which IRQ to use for the clock source MFGPT
> ticks."); 
> _
> 

[PATCH] cs5535-clockevt: free timer in IRQ setup error path

[Jens Rottmann]

cs5535-clockevt: free timer in IRQ setup error path

Due to a hardware limitation cs5535_mfgpt_free_timer() cannot actually release
the timer hardware, but it will at least free the now unreferenced struct
associated with it so calling it is the cleaner thing to do.

Signed-off-by: Jens Rottmann <JRottmann@LiPPERTEmbedded.de>
---

Hi Andres,

> > cs5535_mfgpt_init() doesn't free up the timer in the error path

> Yeah, we can't really free the timer, unfortunately.
> It actually might not be a bad idea to reverse the code so that the
> IRQ allocation happens first, since we can clean that up if mfgpt
> allocation fails.

The problem is that cs5535_mfgpt_setup_irq() depends on the return value of cs5535_mfgpt_alloc_timer(), because it needs to know the timer nr both to read out the previous IRQ (if any) and to setup the IRQ. (And setup_irq() in turn depends on cs5535_mfgpt_setup_irq() for getting the actual IRQ number.) I guess for reversing the order we'd have to split both cs5535_mfgpt_alloc_timer() and cs5535_mfgpt_setup_irq() into two parts, one to do all checking and detecting and a second run to actually touch the hardware. But I don't fancy the mess this is likely to result in ...

For now all I'm able to provide is this small patch that inserts the missing cs5535_mfgpt_free_timer() calls. This at least plugs the (really minor) mem leak - better than nothing.

Maybe cs5535_mfgpt_free_timer() can be made more intelligent to set mfgpt->avail again if the hardware isn't actually in a non-reversible state yet, but I don't know what this depends on and I lack the time to explore this further.

Fortunately not freeing the timers only affects users who manually want to try several IRQs by repeatedly doing "modprobe cs5535-clockevt irq=..." and who after 3-6 failed attempts will have to reboot to get another batch of free timers. Once the module was loaded successfully it cannot be unloaded anyway.

Best regards,
Jens

--- linux-2.6.33-rc5/drivers/clocksource/cs5535-clockevt.c
+++ timer_freed_on_error/drivers/clocksource/cs5535-clockevt.c
@@ -154,14 +154,14 @@
 	if (cs5535_mfgpt_setup_irq(timer, MFGPT_CMP2, &timer_irq)) {
 		printk(KERN_ERR DRV_NAME ": Could not set up IRQ %d\n",
 				timer_irq);
-		return -EIO;
+		goto err_timer;
 	}
 
 	/* And register it with the kernel */
 	ret = setup_irq(timer_irq, &mfgptirq);
 	if (ret) {
 		printk(KERN_ERR DRV_NAME ": Unable to set up the interrupt.\n");
-		goto err;
+		goto err_irq;
 	}
 
 	/* Set the clock scale and enable the event mode for CMP2 */
@@ -184,8 +184,10 @@
 
 	return 0;
 
-err:
+err_irq:
 	cs5535_mfgpt_release_irq(cs5535_event_clock, MFGPT_CMP2, &timer_irq);
+err_timer:
+	cs5535_mfgpt_free_timer(cs5535_event_clock);
 	printk(KERN_ERR DRV_NAME ": Unable to set up the MFGPT clock source\n");
 	return -EIO;
 }
_

[PATCH] cs5535-mfgpt: reuse timers that have never been set up

[Jens Rottmann]

cs5535-mfgpt: reuse timers that have never been set up

The MFGPT hardware may be set up only once, therefore cs5535_mfgpt_free_timer()
didn't re-set the timer's "avail" bit.  However if a timer is freed before it
has actually been in use then it may be made available again.

Signed-off-by: Jens Rottmann <JRottmann@LiPPERTEmbedded.de>
---

Hi Andres,

Jens wrote:
> Andres wrote:
>> Jens wrote:
>>> cs5535_mfgpt_init() doesn't free up the timer in the error path
>>
>> Yeah, we can't really free the timer, unfortunately.
>> It actually might not be a bad idea to reverse the code so that the
>> IRQ allocation happens first, since we can clean that up if mfgpt
>> allocation fails.
>
> [...] I guess for reversing the order we'd have to split both
> cs5535_mfgpt_alloc_timer() and cs5535_mfgpt_setup_irq() into two parts [...]
> Maybe cs5535_mfgpt_free_timer() can be made more intelligent to set
> mfgpt->avail again if the hardware isn't actually in a non-reversible state
> yet [...]

As you can see I did just that. What do you think?

Cheers,
Jens

--- linux-2.6.35-rc4/drivers/misc/cs5535-mfgpt.c
+++ mfgpt_reuse_timers/drivers/misc/cs5535-mfgpt.c
@@ -211,6 +211,17 @@
  */
 void cs5535_mfgpt_free_timer(struct cs5535_mfgpt_timer *timer)
 {
+	unsigned long flags;
+	uint16_t val;
+
+	/* timer can be made available again only if never set up */
+	val = cs5535_mfgpt_read(timer, MFGPT_REG_SETUP);
+	if (!(val & MFGPT_SETUP_SETUP)) {
+		spin_lock_irqsave(&timer->chip->lock, flags);
+		__set_bit(timer->nr, timer->chip->avail);
+		spin_unlock_irqrestore(&timer->chip->lock, flags);
+	}
+
 	kfree(timer);
 }
 EXPORT_SYMBOL_GPL(cs5535_mfgpt_free_timer);
_