Re: [PATCH 01/15] platform: goldfish: pipe: Remove the goldfish_interrupt_tasklet global variable
On Tue, Oct 02, 2018 at 03:33:11PM -0700, Roman Kiryanov wrote: > > I don't see a "global" variable here, what are you referring to. > > > > This one: > > > > > -static DECLARE_TASKLET(goldfish_interrupt_tasklet, > > > goldfish_interrupt_task, 0); > > > > ? > > Yes. > > > That looks static to this file to me. > > what am I missing here? > > It has a global lifetime but it is visible only in this file. How do I > say better, just drop the word "global"? Yes. It's not a global variable at all. It's file-scope only. What you are doing is moving the variable to be attached to the device itself, allowing you to have multiple devices handled by the same code, which is great. But please document it as such. Same goes for the other patches in this series. thanks, greg k-h
Re: [PATCH 01/15] platform: goldfish: pipe: Remove the goldfish_interrupt_tasklet global variable
On Tue, Oct 02, 2018 at 03:33:11PM -0700, Roman Kiryanov wrote: > > I don't see a "global" variable here, what are you referring to. > > > > This one: > > > > > -static DECLARE_TASKLET(goldfish_interrupt_tasklet, > > > goldfish_interrupt_task, 0); > > > > ? > > Yes. > > > That looks static to this file to me. > > what am I missing here? > > It has a global lifetime but it is visible only in this file. How do I > say better, just drop the word "global"? Yes. It's not a global variable at all. It's file-scope only. What you are doing is moving the variable to be attached to the device itself, allowing you to have multiple devices handled by the same code, which is great. But please document it as such. Same goes for the other patches in this series. thanks, greg k-h
Re: [PATCH 01/15] platform: goldfish: pipe: Remove the goldfish_interrupt_tasklet global variable
> I don't see a "global" variable here, what are you referring to. > > This one: > > > -static DECLARE_TASKLET(goldfish_interrupt_tasklet, > > goldfish_interrupt_task, 0); > > ? Yes. > That looks static to this file to me. > what am I missing here? It has a global lifetime but it is visible only in this file. How do I say better, just drop the word "global"?
Re: [PATCH 01/15] platform: goldfish: pipe: Remove the goldfish_interrupt_tasklet global variable
> I don't see a "global" variable here, what are you referring to. > > This one: > > > -static DECLARE_TASKLET(goldfish_interrupt_tasklet, > > goldfish_interrupt_task, 0); > > ? Yes. > That looks static to this file to me. > what am I missing here? It has a global lifetime but it is visible only in this file. How do I say better, just drop the word "global"?
Re: [PATCH 01/15] platform: goldfish: pipe: Remove the goldfish_interrupt_tasklet global variable
On Tue, Oct 02, 2018 at 03:18:49PM -0700, r...@google.com wrote: > From: Roman Kiryanov > > This is a series of patches to remove mutable global variables > to introduce another version of the pipe driver for the older > host interface. I don't want to have two driver states where only > one is used. I don't see a "global" variable here, what are you referring to. This one: > -static DECLARE_TASKLET(goldfish_interrupt_tasklet, goldfish_interrupt_task, > 0); ? That looks static to this file to me. what am I missing here? greg k-h
Re: [PATCH 01/15] platform: goldfish: pipe: Remove the goldfish_interrupt_tasklet global variable
On Tue, Oct 02, 2018 at 03:18:49PM -0700, r...@google.com wrote: > From: Roman Kiryanov > > This is a series of patches to remove mutable global variables > to introduce another version of the pipe driver for the older > host interface. I don't want to have two driver states where only > one is used. I don't see a "global" variable here, what are you referring to. This one: > -static DECLARE_TASKLET(goldfish_interrupt_tasklet, goldfish_interrupt_task, > 0); ? That looks static to this file to me. what am I missing here? greg k-h
[PATCH 01/15] platform: goldfish: pipe: Remove the goldfish_interrupt_tasklet global variable
From: Roman Kiryanov
This is a series of patches to remove mutable global variables
to introduce another version of the pipe driver for the older
host interface. I don't want to have two driver states where only
one is used.
Signed-off-by: Roman Kiryanov
---
drivers/platform/goldfish/goldfish_pipe.c | 24 +++
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/drivers/platform/goldfish/goldfish_pipe.c
b/drivers/platform/goldfish/goldfish_pipe.c
index 56665e879e5a..ba9aede17d57 100644
--- a/drivers/platform/goldfish/goldfish_pipe.c
+++ b/drivers/platform/goldfish/goldfish_pipe.c
@@ -208,6 +208,9 @@ struct goldfish_pipe_dev {
int irq;
int version;
unsigned char __iomem *base;
+
+ /* an irq tasklet to run goldfish_interrupt_task */
+ struct tasklet_struct irq_tasklet;
};
static struct goldfish_pipe_dev goldfish_pipe_dev;
@@ -582,14 +585,14 @@ static struct goldfish_pipe *signalled_pipes_pop_front(
return pipe;
}
-static void goldfish_interrupt_task(unsigned long unused)
+static void goldfish_interrupt_task(unsigned long dev_addr)
{
/* Iterate over the signalled pipes and wake them one by one */
+ struct goldfish_pipe_dev *dev = (struct goldfish_pipe_dev *)dev_addr;
struct goldfish_pipe *pipe;
int wakes;
- while ((pipe = signalled_pipes_pop_front(_pipe_dev, )) !=
- NULL) {
+ while ((pipe = signalled_pipes_pop_front(dev, )) != NULL) {
if (wakes & PIPE_WAKE_CLOSED) {
pipe->flags = 1 << BIT_CLOSED_ON_HOST;
} else {
@@ -605,7 +608,6 @@ static void goldfish_interrupt_task(unsigned long unused)
wake_up_interruptible(>wake_queue);
}
}
-static DECLARE_TASKLET(goldfish_interrupt_tasklet, goldfish_interrupt_task, 0);
/*
* The general idea of the interrupt handling:
@@ -648,7 +650,7 @@ static irqreturn_t goldfish_pipe_interrupt(int irq, void
*dev_id)
spin_unlock_irqrestore(>lock, flags);
- tasklet_schedule(_interrupt_tasklet);
+ tasklet_schedule(>irq_tasklet);
return IRQ_HANDLED;
}
@@ -800,9 +802,14 @@ static void write_pa_addr(void *addr, void __iomem *portl,
void __iomem *porth)
static int goldfish_pipe_device_init(struct platform_device *pdev)
{
struct goldfish_pipe_dev *dev = _pipe_dev;
- int err = devm_request_irq(>dev, dev->irq,
- goldfish_pipe_interrupt,
- IRQF_SHARED, "goldfish_pipe", dev);
+ int err;
+
+ tasklet_init(>irq_tasklet, _interrupt_task,
+(unsigned long)dev);
+
+ err = devm_request_irq(>dev, dev->irq,
+ goldfish_pipe_interrupt,
+ IRQF_SHARED, "goldfish_pipe", dev);
if (err) {
dev_err(>dev, "unable to allocate IRQ for v2\n");
return err;
@@ -854,6 +861,7 @@ static int goldfish_pipe_device_init(struct platform_device
*pdev)
static void goldfish_pipe_device_deinit(struct platform_device *pdev)
{
misc_deregister(_pipe_miscdev);
+ tasklet_kill(_pipe_dev.irq_tasklet);
kfree(goldfish_pipe_dev.pipes);
free_page((unsigned long)goldfish_pipe_dev.buffers);
}
--
2.19.0.605.g01d371f741-goog
[PATCH 01/15] platform: goldfish: pipe: Remove the goldfish_interrupt_tasklet global variable
From: Roman Kiryanov
This is a series of patches to remove mutable global variables
to introduce another version of the pipe driver for the older
host interface. I don't want to have two driver states where only
one is used.
Signed-off-by: Roman Kiryanov
---
drivers/platform/goldfish/goldfish_pipe.c | 24 +++
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/drivers/platform/goldfish/goldfish_pipe.c
b/drivers/platform/goldfish/goldfish_pipe.c
index 56665e879e5a..ba9aede17d57 100644
--- a/drivers/platform/goldfish/goldfish_pipe.c
+++ b/drivers/platform/goldfish/goldfish_pipe.c
@@ -208,6 +208,9 @@ struct goldfish_pipe_dev {
int irq;
int version;
unsigned char __iomem *base;
+
+ /* an irq tasklet to run goldfish_interrupt_task */
+ struct tasklet_struct irq_tasklet;
};
static struct goldfish_pipe_dev goldfish_pipe_dev;
@@ -582,14 +585,14 @@ static struct goldfish_pipe *signalled_pipes_pop_front(
return pipe;
}
-static void goldfish_interrupt_task(unsigned long unused)
+static void goldfish_interrupt_task(unsigned long dev_addr)
{
/* Iterate over the signalled pipes and wake them one by one */
+ struct goldfish_pipe_dev *dev = (struct goldfish_pipe_dev *)dev_addr;
struct goldfish_pipe *pipe;
int wakes;
- while ((pipe = signalled_pipes_pop_front(_pipe_dev, )) !=
- NULL) {
+ while ((pipe = signalled_pipes_pop_front(dev, )) != NULL) {
if (wakes & PIPE_WAKE_CLOSED) {
pipe->flags = 1 << BIT_CLOSED_ON_HOST;
} else {
@@ -605,7 +608,6 @@ static void goldfish_interrupt_task(unsigned long unused)
wake_up_interruptible(>wake_queue);
}
}
-static DECLARE_TASKLET(goldfish_interrupt_tasklet, goldfish_interrupt_task, 0);
/*
* The general idea of the interrupt handling:
@@ -648,7 +650,7 @@ static irqreturn_t goldfish_pipe_interrupt(int irq, void
*dev_id)
spin_unlock_irqrestore(>lock, flags);
- tasklet_schedule(_interrupt_tasklet);
+ tasklet_schedule(>irq_tasklet);
return IRQ_HANDLED;
}
@@ -800,9 +802,14 @@ static void write_pa_addr(void *addr, void __iomem *portl,
void __iomem *porth)
static int goldfish_pipe_device_init(struct platform_device *pdev)
{
struct goldfish_pipe_dev *dev = _pipe_dev;
- int err = devm_request_irq(>dev, dev->irq,
- goldfish_pipe_interrupt,
- IRQF_SHARED, "goldfish_pipe", dev);
+ int err;
+
+ tasklet_init(>irq_tasklet, _interrupt_task,
+(unsigned long)dev);
+
+ err = devm_request_irq(>dev, dev->irq,
+ goldfish_pipe_interrupt,
+ IRQF_SHARED, "goldfish_pipe", dev);
if (err) {
dev_err(>dev, "unable to allocate IRQ for v2\n");
return err;
@@ -854,6 +861,7 @@ static int goldfish_pipe_device_init(struct platform_device
*pdev)
static void goldfish_pipe_device_deinit(struct platform_device *pdev)
{
misc_deregister(_pipe_miscdev);
+ tasklet_kill(_pipe_dev.irq_tasklet);
kfree(goldfish_pipe_dev.pipes);
free_page((unsigned long)goldfish_pipe_dev.buffers);
}
--
2.19.0.605.g01d371f741-goog
