[rt2x00-users] [PATCH 05/12] rt2x00: Introduce tasklets for interrupt handling

[Gertjan van Wingerde]

On 01/14/11 23:53, Helmut Schaa wrote:
> Hi Gertjan,
> 
> Am Freitag, 14. Januar 2011 schrieb Gertjan van Wingerde:
>> On 01/14/11 10:41, Helmut Schaa wrote:
>>> No functional changes, just preparation for moving interrupt handling to
>>> tasklets. The tasklets are disabled by default. Drivers making use of
>>> them need to enable the tasklets when the device state is set to IRQ_ON.
>>>
>>> Signed-off-by: Helmut Schaa <helmut.schaa at googlemail.com>
>>> ---
>>>  drivers/net/wireless/rt2x00/rt2x00.h    |   13 +++++++++++++
>>>  drivers/net/wireless/rt2x00/rt2x00dev.c |   21 +++++++++++++++++++++
>>>  2 files changed, 34 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/rt2x00/rt2x00.h b/drivers/net/wireless/rt2x00/rt2x00.h
>>> index 01a4788..2375722 100644
>>> --- a/drivers/net/wireless/rt2x00/rt2x00.h
>>> +++ b/drivers/net/wireless/rt2x00/rt2x00.h
>>> @@ -520,6 +520,10 @@ struct rt2x00lib_ops {
>>>  	 * TX status tasklet handler.
>>>  	 */
>>>  	void (*txstatus_tasklet) (unsigned long data);
>>> +	void (*pretbtt_tasklet) (unsigned long data);
>>> +	void (*tbtt_tasklet) (unsigned long data);
>>> +	void (*rxdone_tasklet) (unsigned long data);
>>> +	void (*autowake_tasklet) (unsigned long data);
>>>  
>>>  	/*
>>>  	 * Device init handlers.
>>> @@ -906,6 +910,15 @@ struct rt2x00_dev {
>>>  	 * Tasklet for processing tx status reports (rt2800pci).
>>>  	 */
>>>  	struct tasklet_struct txstatus_tasklet;
>>> +	struct tasklet_struct pretbtt_tasklet;
>>> +	struct tasklet_struct tbtt_tasklet;
>>> +	struct tasklet_struct rxdone_tasklet;
>>> +	struct tasklet_struct autowake_tasklet;
>>> +
>>> +	/*
>>> +	 * Protect the interrupt mask register.
>>> +	 */
>>> +	spinlock_t irqmask_lock;
>>>  };
>>>  
>>>  /*
>>
>> Is there any resource overhead involved with tasklets (e.g. additional kernel threads, memory, etc)?
>> It seems that we could do with less tasklets, as for instance pretbtt and tbtt can share a tasklet, as
>> they never will happen at the same time. The same would be true for autowake and rxdone, and possibly
>> txstatus.
>>
>> So, if there is no additional resource overhead with having all these tasklets, then I'm fine, otherwise
>> I think we should cut down a little bit on the number of tasklets that we use.
> 
> Hmm, good point. Since tasklets are executed as softirq there are no new kernel
> threads involved. Of course we need some more memory for the tasklet_structs.
> From a runtime POV I don't think it makes much difference in memory consumption.
> 
> The problem with using one tasklet for tbtt and pretbtt is that we would have
> to somehow store which interrupt to process. Having one tasklet per interrupt
> doesn't have to store anything between the irq_handler and the tasklets.
> 
> But you're right that tbtt and pretbtt should never run at the same time, it
> just cannot happen. So basically both could be processed by the same tasklet.

OK. I see your point on the difficulties in communicating to the tasklet which interrupt has fired.
Since there isn't any real resource impact for tasklets, I guess it is fine to have all these tasklets.

So,

Acked-by: Gertjan van Wingerde <gwingerde at gmail.com>

>  
>>> diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c b/drivers/net/wireless/rt2x00/rt2x00dev.c
>>> index 04bf9af..9856652 100644
>>> --- a/drivers/net/wireless/rt2x00/rt2x00dev.c
>>> +++ b/drivers/net/wireless/rt2x00/rt2x00dev.c
>>> @@ -828,6 +828,26 @@ static int rt2x00lib_probe_hw(struct rt2x00_dev *rt2x00dev)
>>>  	}
>>>  
>>>  	/*
>>> +	 * Initialize tasklets if used by the driver. Tasklets are
>>> +	 * disabled until the interrupts are turned on. The driver
>>> +	 * has to handle that.
>>> +	 */
>>> +#define RT2X00_TASKLET_INIT(taskletname) \
>>> +	if (rt2x00dev->ops->lib->taskletname) { \
>>> +		tasklet_init(&rt2x00dev->taskletname, \
>>> +			     rt2x00dev->ops->lib->taskletname, \
>>> +			     (unsigned long)rt2x00dev); \
>>> +		tasklet_disable(&rt2x00dev->taskletname); \
>>> +	}
>>> +
>>> +	RT2X00_TASKLET_INIT(pretbtt_tasklet);
>>> +	RT2X00_TASKLET_INIT(tbtt_tasklet);
>>> +	RT2X00_TASKLET_INIT(rxdone_tasklet);
>>> +	RT2X00_TASKLET_INIT(autowake_tasklet);
>>> +
>>> +#undef RT2X00_TASKLET_INIT
>>> +
>>> +	/*
>>>  	 * Register HW.
>>>  	 */
>>>  	status = ieee80211_register_hw(rt2x00dev->hw);
>>
>> Ouch, that's one ugly macro, with the assumed naming for taskletname and the struct members.
>> However, I don't see how this can be done in an easier manner with less uglyness, so I guess
>> it is fine for now.
> 
> Hehe, I had it open coded before, the macro was Ivos suggestion ;)
> 
> Thanks for the quick review,
> Helmut
>