Re: [PATCH] staging: goldfish: switch from spinlock to mutex
On Sat, Apr 05, 2014 at 05:38:05PM +0300, Kristina Martšenko wrote: On 04/04/14 16:30, Dan Carpenter wrote: On Fri, Apr 04, 2014 at 02:46:14PM +0300, Kristina Martšenko wrote: Yes, I didn't find any interrupt handlers either, which is partially why I thought it was (probably) safe. What's the other part of why it was safe? Put that stuff in the changelog. When we're reviewing patches we're always interested to know if it's safe. :) The other part was whether the code was allowed to sleep. I figured it probably was since it's only accessed from MTD callback functions and other drivers in drivers/mtd/devices/ also sleep in those functions (either by using mutexes or by calling schedule() directly). I didn't mention why I thought it was safe in the changelog because I thought maybe the reasons would seem too obvious to more experienced kernel developers. Yeah. In the end, I looked up those functions and saw that they sleep as well. On the staging list, we're not going to be subsystem experts. Also I prefer just emailing people my questions instead of looking things up in the code since hopefully they know the answers already if they wrote the patch. Should I test and resend the patch with a new changelog? Whatever... The patch is fine. Reviewed-by: Dan Carpenter dan.carpen...@oracle.com regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: goldfish: switch from spinlock to mutex
On 04/04/14 16:30, Dan Carpenter wrote: On Fri, Apr 04, 2014 at 02:46:14PM +0300, Kristina Martšenko wrote: Yes, I didn't find any interrupt handlers either, which is partially why I thought it was (probably) safe. What's the other part of why it was safe? Put that stuff in the changelog. When we're reviewing patches we're always interested to know if it's safe. :) The other part was whether the code was allowed to sleep. I figured it probably was since it's only accessed from MTD callback functions and other drivers in drivers/mtd/devices/ also sleep in those functions (either by using mutexes or by calling schedule() directly). I didn't mention why I thought it was safe in the changelog because I thought maybe the reasons would seem too obvious to more experienced kernel developers. Should I test and resend the patch with a new changelog? Kristina ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: goldfish: switch from spinlock to mutex
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.martse...@gmail.com 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? In any case I can take another shot at getting the Android emulator to work so that I can test this. (Probably not anytime soon though.) Kristina ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: goldfish: switch from spinlock to mutex
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.martse...@gmail.com 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. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: goldfish: switch from spinlock to mutex
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.martse...@gmail.com 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 de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: goldfish: switch from spinlock to mutex
On Fri, Apr 04, 2014 at 02:46:14PM +0300, Kristina Martšenko wrote: Yes, I didn't find any interrupt handlers either, which is partially why I thought it was (probably) safe. What's the other part of why it was safe? Put that stuff in the changelog. When we're reviewing patches we're always interested to know if it's safe. :) regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: goldfish: switch from spinlock to mutex
Use a mutex instead of a spinlock in goldfish_nand.c, as suggested by
the TODO list.
Signed-off-by: Kristina Martšenko kristina.martse...@gmail.com
---
drivers/staging/goldfish/README | 1 -
drivers/staging/goldfish/goldfish_nand.c | 15 +++
2 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/goldfish/README b/drivers/staging/goldfish/README
index 93d65b0..183af00 100644
--- a/drivers/staging/goldfish/README
+++ b/drivers/staging/goldfish/README
@@ -5,7 +5,6 @@ Audio
NAND
-- Switch from spinlock to mutex
- Remove excess checking of parameters in calls
- Use dma coherent memory not kmalloc/__pa for the memory (this is just
a cleanliness issue not a correctness one)
diff --git a/drivers/staging/goldfish/goldfish_nand.c
b/drivers/staging/goldfish/goldfish_nand.c
index eca0873..7f606f7 100644
--- a/drivers/staging/goldfish/goldfish_nand.c
+++ b/drivers/staging/goldfish/goldfish_nand.c
@@ -24,13 +24,14 @@
#include linux/vmalloc.h
#include linux/mtd/mtd.h
#include linux/platform_device.h
+#include linux/mutex.h
#include asm/div64.h
#include goldfish_nand_reg.h
struct goldfish_nand {
- spinlock_t lock;
+ struct mutexlock;
unsigned char __iomem *base;
struct cmd_params *cmd_params;
size_t mtd_count;
@@ -77,10 +78,9 @@ static u32 goldfish_nand_cmd(struct mtd_info *mtd, enum
nand_cmd cmd,
{
struct goldfish_nand *nand = mtd-priv;
u32 rv;
- unsigned long irq_flags;
unsigned char __iomem *base = nand-base;
- spin_lock_irqsave(nand-lock, irq_flags);
+ mutex_lock(nand-lock);
if (goldfish_nand_cmd_with_params(mtd, cmd, addr, len, ptr, rv)) {
writel(mtd - nand-mtd, base + NAND_DEV);
writel((u32)(addr 32), base + NAND_ADDR_HIGH);
@@ -90,7 +90,7 @@ static u32 goldfish_nand_cmd(struct mtd_info *mtd, enum
nand_cmd cmd,
writel(cmd, base + NAND_COMMAND);
rv = readl(base + NAND_RESULT);
}
- spin_unlock_irqrestore(nand-lock, irq_flags);
+ mutex_unlock(nand-lock);
return rv;
}
@@ -307,12 +307,11 @@ static int goldfish_nand_init_device(struct
platform_device *pdev,
u32 name_len;
u32 result;
u32 flags;
- unsigned long irq_flags;
unsigned char __iomem *base = nand-base;
struct mtd_info *mtd = nand-mtd[id];
char *name;
- spin_lock_irqsave(nand-lock, irq_flags);
+ mutex_lock(nand-lock);
writel(id, base + NAND_DEV);
flags = readl(base + NAND_DEV_FLAGS);
name_len = readl(base + NAND_DEV_NAME_LEN);
@@ -329,7 +328,7 @@ static int goldfish_nand_init_device(struct platform_device
*pdev,
goldfish nand dev%d: size %llx, page %d, extra %d, erase %d\n,
id, mtd-size, mtd-writesize,
mtd-oobsize, mtd-erasesize);
- spin_unlock_irqrestore(nand-lock, irq_flags);
+ mutex_unlock(nand-lock);
mtd-priv = nand;
@@ -405,7 +404,7 @@ static int goldfish_nand_probe(struct platform_device *pdev)
if (nand == NULL)
return -ENOMEM;
- spin_lock_init(nand-lock);
+ mutex_init(nand-lock);
nand-base = base;
nand-mtd_count = num_dev;
platform_set_drvdata(pdev, nand);
--
1.8.5.5
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
