RE: [PATCH 1/1] mwifiex: queue main work from main process when bailing on races

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Daniel,

> >>  	/* Check if already processing */
> >>  	if (adapter->mwifiex_processing) {
> >>  		spin_unlock_irqrestore(&adapter->main_proc_lock, flags);
> >> +		queue_work(adapter->workqueue, &adapter->main_work);
> >
> > This is specific to SDIO interface,
> 
> Is it really? By checking adapter->mwifiex_processing, the driver seems
> to expect mwifiex_main_process() to be called from multiple execution
> paths, and in that case, we will always loose one execution cycle in

You are right. I overlooked it.

> case we bail early. I actually wonder why this didn't hit us earlier,
> but I might miss a detail.

I guess, in your case, the interrupt comes in at line 363 where you have passed the int_status or RX_RCVD checking but the mwifiex_processing flag is still true.

 361         if ((adapter->int_status) || IS_CARD_RX_RCVD(adapter))
 362                 goto process_start;
 363
 364         spin_lock_irqsave(&adapter->main_proc_lock, flags);
 365         adapter->mwifiex_processing = false;
 366         spin_unlock_irqrestore(&adapter->main_proc_lock, flags);

The interrupt thread exits because mwifiex_processing is true.
Therefore the mwifiex_main_process misses this interrupt.

> 
> OTOH, the worst thing that can happen if the function is executed too
> often is that it exits early and does nothing.
> 
> > +               if (adapter->iface_type == MWIFIEX_SDIO)
> > +                       queue_work(adapter->workqueue, &adapter->main_work);
> 
> I can of course add this, but I don't fully understand why the driver
> takes care of concurrently running executing paths and then just bails
> silently in case a race is detected.

No. Your original patch is fine.
Could you resend it as [PATCH 3.12]? I will ACK in that thread.

Thanks,
Bing



--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]