| Message ID | 1311007508-8096-2-git-send-email-shawn.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Monday, July 18, 2011 09:45:07 AM Shawn Guo wrote: > The patch makes a copy of platform data into driver data, so that any > reference to platform_data after .probe exits can be avoided. And why is this beneficial? I am of the opinion that platform data should stay on (and be accessed through a const pointer to ensure that the driver will not alter it). Thanks.
On Mon, Jul 18, 2011 at 11:02 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Monday, July 18, 2011 09:45:07 AM Shawn Guo wrote: >> The patch makes a copy of platform data into driver data, so that any >> reference to platform_data after .probe exits can be avoided. > > And why is this beneficial? I am of the opinion that platform data should > stay on (and be accessed through a const pointer to ensure that the driver > will not alter it). Because when using the device tree, there is no platform_data. g.
On Monday, July 18, 2011 10:15:27 AM Grant Likely wrote: > On Mon, Jul 18, 2011 at 11:02 AM, Dmitry Torokhov > > <dmitry.torokhov@gmail.com> wrote: > > On Monday, July 18, 2011 09:45:07 AM Shawn Guo wrote: > >> The patch makes a copy of platform data into driver data, so that any > >> reference to platform_data after .probe exits can be avoided. > > > > And why is this beneficial? I am of the opinion that platform data > > should stay on (and be accessed through a const pointer to ensure > > that the driver will not alter it). > > Because when using the device tree, there is no platform_data. > So allocate it... That's what Davids patch does. BTW, you never gave ACK for the final version and I'd prefer to have it for the DT bindings. Thanks.
On Mon, Jul 18, 2011 at 10:02:44AM -0700, Dmitry Torokhov wrote: > On Monday, July 18, 2011 09:45:07 AM Shawn Guo wrote: > > The patch makes a copy of platform data into driver data, so that any > > reference to platform_data after .probe exits can be avoided. > > And why is this beneficial? I am of the opinion that platform data should > stay on (and be accessed through a const pointer to ensure that the driver > will not alter it). > To me, it's a common sense that platform data should not be referenced after .probe exits, so that any platform code providing the data can claim the data as __initconst. When we build multiple platforms which all have their own platform data for gpio_keys in the kernel, there is only one of them will be used by gpio_keys driver, others should be freed after init, no?
On Tue, Jul 19, 2011 at 09:17:26AM +0800, Shawn Guo wrote: > On Mon, Jul 18, 2011 at 10:02:44AM -0700, Dmitry Torokhov wrote: > > On Monday, July 18, 2011 09:45:07 AM Shawn Guo wrote: > > > The patch makes a copy of platform data into driver data, so that any > > > reference to platform_data after .probe exits can be avoided. > > > > And why is this beneficial? I am of the opinion that platform data should > > stay on (and be accessed through a const pointer to ensure that the driver > > will not alter it). > > > To me, it's a common sense that platform data should not be referenced > after .probe exits, so that any platform code providing the data can > claim the data as __initconst. That's totally buggered, and that's putting it kindly. Consider a driver built as a module, vs built-in. If you build it as a module, your driver data is stale by the time you insert the module. It's not much better with it built-in - if you have hotplug enabled, you can unbind and rebind the driver, which means that the .probe function can be called long after the .init sections have been discarded. So no, this is no justification for the patch. Don't *ever* make any platform devices or any data pointed to by a platform device discardable after init time. It's an oops waiting to happen.
On Tue, Jul 19, 2011 at 08:48:41AM +0100, Russell King - ARM Linux wrote: > On Tue, Jul 19, 2011 at 09:17:26AM +0800, Shawn Guo wrote: > > On Mon, Jul 18, 2011 at 10:02:44AM -0700, Dmitry Torokhov wrote: > > > On Monday, July 18, 2011 09:45:07 AM Shawn Guo wrote: > > > > The patch makes a copy of platform data into driver data, so that any > > > > reference to platform_data after .probe exits can be avoided. > > > > > > And why is this beneficial? I am of the opinion that platform data should > > > stay on (and be accessed through a const pointer to ensure that the driver > > > will not alter it). > > > > > To me, it's a common sense that platform data should not be referenced > > after .probe exits, so that any platform code providing the data can > > claim the data as __initconst. > > That's totally buggered, and that's putting it kindly. > Well, you can tell I'm an idiot, but I was not trying to do what you say here. > Consider a driver built as a module, vs built-in. If you build it as a > module, your driver data is stale by the time you insert the module. > It's not much better with it built-in - if you have hotplug enabled, you > can unbind and rebind the driver, which means that the .probe function > can be called long after the .init sections have been discarded. > > So no, this is no justification for the patch. > > Don't *ever* make any platform devices or any data pointed to by a > platform device discardable after init time. It's an oops waiting to > happen. > Something learnt. Thanks, Russell.
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c index 97bada4..85b5685 100644 --- a/drivers/input/keyboard/gpio_keys.c +++ b/drivers/input/keyboard/gpio_keys.c @@ -27,7 +27,7 @@ #include <linux/gpio.h> struct gpio_button_data { - struct gpio_keys_button *button; + struct gpio_keys_button button; struct input_dev *input; struct timer_list timer; struct work_struct work; @@ -111,7 +111,7 @@ static void gpio_keys_disable_button(struct gpio_button_data *bdata) /* * Disable IRQ and possible debouncing timer. */ - disable_irq(gpio_to_irq(bdata->button->gpio)); + disable_irq(gpio_to_irq(bdata->button.gpio)); if (bdata->timer_debounce) del_timer_sync(&bdata->timer); @@ -132,7 +132,7 @@ static void gpio_keys_disable_button(struct gpio_button_data *bdata) static void gpio_keys_enable_button(struct gpio_button_data *bdata) { if (bdata->disabled) { - enable_irq(gpio_to_irq(bdata->button->gpio)); + enable_irq(gpio_to_irq(bdata->button.gpio)); bdata->disabled = false; } } @@ -167,13 +167,13 @@ static ssize_t gpio_keys_attr_show_helper(struct gpio_keys_drvdata *ddata, for (i = 0; i < ddata->n_buttons; i++) { struct gpio_button_data *bdata = &ddata->data[i]; - if (bdata->button->type != type) + if (bdata->button.type != type) continue; if (only_disabled && !bdata->disabled) continue; - __set_bit(bdata->button->code, bits); + __set_bit(bdata->button.code, bits); } ret = bitmap_scnlistprintf(buf, PAGE_SIZE - 2, bits, n_events); @@ -215,11 +215,11 @@ static ssize_t gpio_keys_attr_store_helper(struct gpio_keys_drvdata *ddata, for (i = 0; i < ddata->n_buttons; i++) { struct gpio_button_data *bdata = &ddata->data[i]; - if (bdata->button->type != type) + if (bdata->button.type != type) continue; - if (test_bit(bdata->button->code, bits) && - !bdata->button->can_disable) { + if (test_bit(bdata->button.code, bits) && + !bdata->button.can_disable) { error = -EINVAL; goto out; } @@ -230,10 +230,10 @@ static ssize_t gpio_keys_attr_store_helper(struct gpio_keys_drvdata *ddata, for (i = 0; i < ddata->n_buttons; i++) { struct gpio_button_data *bdata = &ddata->data[i]; - if (bdata->button->type != type) + if (bdata->button.type != type) continue; - if (test_bit(bdata->button->code, bits)) + if (test_bit(bdata->button.code, bits)) gpio_keys_disable_button(bdata); else gpio_keys_enable_button(bdata); @@ -319,7 +319,7 @@ static struct attribute_group gpio_keys_attr_group = { static void gpio_keys_report_event(struct gpio_button_data *bdata) { - struct gpio_keys_button *button = bdata->button; + struct gpio_keys_button *button = &bdata->button; struct input_dev *input = bdata->input; unsigned int type = button->type ?: EV_KEY; int state = (gpio_get_value_cansleep(button->gpio) ? 1 : 0) ^ button->active_low; @@ -351,7 +351,7 @@ static void gpio_keys_timer(unsigned long _data) static irqreturn_t gpio_keys_isr(int irq, void *dev_id) { struct gpio_button_data *bdata = dev_id; - struct gpio_keys_button *button = bdata->button; + struct gpio_keys_button *button = &bdata->button; BUG_ON(irq != gpio_to_irq(button->gpio)); @@ -494,7 +494,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev) unsigned int type = button->type ?: EV_KEY; bdata->input = input; - bdata->button = button; + bdata->button = *button; error = gpio_keys_setup_key(pdev, bdata, button); if (error) @@ -550,7 +550,6 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev) static int __devexit gpio_keys_remove(struct platform_device *pdev) { - struct gpio_keys_platform_data *pdata = pdev->dev.platform_data; struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev); struct input_dev *input = ddata->input; int i; @@ -559,13 +558,13 @@ static int __devexit gpio_keys_remove(struct platform_device *pdev) device_init_wakeup(&pdev->dev, 0); - for (i = 0; i < pdata->nbuttons; i++) { - int irq = gpio_to_irq(pdata->buttons[i].gpio); + for (i = 0; i < ddata->n_buttons; i++) { + int irq = gpio_to_irq(ddata->data[i].button.gpio); free_irq(irq, &ddata->data[i]); if (ddata->data[i].timer_debounce) del_timer_sync(&ddata->data[i].timer); cancel_work_sync(&ddata->data[i].work); - gpio_free(pdata->buttons[i].gpio); + gpio_free(ddata->data[i].button.gpio); } input_unregister_device(input); @@ -579,12 +578,13 @@ static int __devexit gpio_keys_remove(struct platform_device *pdev) static int gpio_keys_suspend(struct device *dev) { struct platform_device *pdev = to_platform_device(dev); - struct gpio_keys_platform_data *pdata = pdev->dev.platform_data; + struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev); int i; if (device_may_wakeup(&pdev->dev)) { - for (i = 0; i < pdata->nbuttons; i++) { - struct gpio_keys_button *button = &pdata->buttons[i]; + for (i = 0; i < ddata->n_buttons; i++) { + struct gpio_keys_button *button = + &ddata->data[i].button; if (button->wakeup) { int irq = gpio_to_irq(button->gpio); enable_irq_wake(irq); @@ -599,12 +599,11 @@ static int gpio_keys_resume(struct device *dev) { struct platform_device *pdev = to_platform_device(dev); struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev); - struct gpio_keys_platform_data *pdata = pdev->dev.platform_data; int i; - for (i = 0; i < pdata->nbuttons; i++) { + for (i = 0; i < ddata->n_buttons; i++) { - struct gpio_keys_button *button = &pdata->buttons[i]; + struct gpio_keys_button *button = &ddata->data[i].button; if (button->wakeup && device_may_wakeup(&pdev->dev)) { int irq = gpio_to_irq(button->gpio); disable_irq_wake(irq);
The patch makes a copy of platform data into driver data, so that any reference to platform_data after .probe exits can be avoided. Signed-off-by: Shawn Guo <shawn.guo@linaro.org> Cc: Phil Blundell <pb@handhelds.org> Cc: Dmitry Torokhov <dtor@mail.ru> --- drivers/input/keyboard/gpio_keys.c | 45 +++++++++++++++++------------------ 1 files changed, 22 insertions(+), 23 deletions(-)