On 04/04/14 14:04, Dan Carpenter wrote: > On Fri, Apr 04, 2014 at 12:26:27PM +0300, Kristina Martšenko wrote: >> On 03/04/14 13:13, Dan Carpenter wrote: >>> On Thu, Apr 03, 2014 at 01:00:53PM +0300, Kristina Martšenko wrote: >>>> On 03/04/14 11:32, Dan Carpenter wrote: >>>>> On Tue, Mar 25, 2014 at 01:45:09AM +0200, Kristina Martšenko wrote: >>>>>> Use a mutex instead of a spinlock in goldfish_nand.c, as suggested by >>>>>> the TODO list. >>>>>> >>>>>> Signed-off-by: Kristina Martšenko <kristina.martsenko@xxxxxxxxx> >>>>> >>>>> Have you tested this change? >>>> >>>> Nope, just compile-tested. After a day of trying to get the emulator to >>>> work I finally gave up and decided that it looked okay enough... I >>>> should have mentioned under the patch description that it wasn't tested, >>>> sorry. >>> >>> It's not a wrong thing to submit patches that you can't test, but in >>> this case the irq save/restores make me nervous. I can't see that they >>> served any purpose and it's certainly not unheard of for staging code to >>> do pointless things for unexplainable reasons. But on the other hand, I >>> would feel a lot more comfortable if this change were tested or if there >>> were more comments about how the change is safe. >> >> I'm not sure I understand. A mutex doesn't disable interrupts, so the >> cpu irq flags should be the same after the mutex-protected code as they >> were before. I.e. it would have the same effect as the save/restore. Or >> am I missing something? > > A mutex doesn't disable IRQs but the original code used > spin_lock_irqsave() so the original code did disable IRQs. > > Looking at it now, I'm not sure there was a reason to disable IRQs... > This driver doesn't have an IRQ handler. Your change is probably fine. Yes, I didn't find any interrupt handlers either, which is partially why I thought it was (probably) safe. Kristina _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
