Re: Bug in i2c-core?
Adding Rob to CC as he wrote of_irq_get and put it into platform_get_irq. Rob, we use of_irq_get() in the I2C core and the question is now if we need to dispose the mapping and if so what would be a good place for it so managed devices will not have their mappings removed before the managed irq is removed. Ping. Just so you know: Without further information, I will revert the patch in question around rc4/rc5. I'd still like to know if the non-disposing of the mapping in platform_get_irq() is intentional. I'll defer that to Rob. I'm fine with the revert at the moment. Added the revert to for-current now with your and Dmitry's ack. signature.asc Description: Digital signature
Re: Bug in i2c-core?
Hi Wolfram, On Sunday 08 March 2015 09:26:17 Wolfram Sang wrote: On Wed, Mar 04, 2015 at 09:22:37AM +0100, Wolfram Sang wrote: I am writing an I2C touchscreen driver for an i.MX6 based board. I compiled it as a module and when I unload it, I get the following warning: # modprobe sx8654 [ 46.261494] input: SX8654 I2C Touchscreen as /devices/soc0/soc/210.aips-bus/21a.i2c/i2c-0/0-0048/input/in put1 # rmmod sx8654 [ 76.435223] [ cut here ] [ 76.439909] WARNING: CPU: 0 PID: 134 at fs/proc/generic.c:552 remove_proc_entry+0x148/0x164() [ 76.448582] remove_proc_entry: removing non-empty directory 'irq/208', leaking at least 'sx8654' ... When I revert commit e4df3a0 (i2c: core: Dispose OF IRQ mapping at client removal time) I don't get the warning. Is this a bug in the i2c-core or am I doing something wrong in my driver? Yes, this commit breaks all drivers using devm* for IRQ management on OF-based systemsi because devm* cleanup happens in device code, after bus's remove() method returns. I'd recommend reverting and finding a better way (making cleanup a custom devm action as well?). Ouch, my bad. Wolfram, any opinion ? The original patch fixes a real bug, so we shouldn't just revert it. Looking at it some more: What bug does it fix? Anything you experienced? Good question, and I have to confess that I don't really remember :-/ I wonder if we really need e4df3a0 because I can't see where platform_get_irq, the major user of of_irq_get, disposes the mapping. irq_create_of_mapping() will return an already assigned mapping if called twice. I've reached the same conclusion after reading the code. I was concerned about resource leakage, but that doesn't seem to be an issue. I don't know yet, though, if mappings are static or if a mapping can be routed to another irq controller over some time because theoretically they can be dynamically added/removed. Adding Rob to CC as he wrote of_irq_get and put it into platform_get_irq. Rob, we use of_irq_get() in the I2C core and the question is now if we need to dispose the mapping and if so what would be a good place for it so managed devices will not have their mappings removed before the managed irq is removed. Ping. Just so you know: Without further information, I will revert the patch in question around rc4/rc5. I'd still like to know if the non-disposing of the mapping in platform_get_irq() is intentional. I'll defer that to Rob. I'm fine with the revert at the moment. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bug in i2c-core?
On Wed, Mar 04, 2015 at 09:22:37AM +0100, Wolfram Sang wrote: I am writing an I2C touchscreen driver for an i.MX6 based board. I compiled it as a module and when I unload it, I get the following warning: # modprobe sx8654 [ 46.261494] input: SX8654 I2C Touchscreen as /devices/soc0/soc/210.aips-bus/21a.i2c/i2c-0/0-0048/input/input1 # rmmod sx8654 [ 76.435223] [ cut here ] [ 76.439909] WARNING: CPU: 0 PID: 134 at fs/proc/generic.c:552 remove_proc_entry+0x148/0x164() [ 76.448582] remove_proc_entry: removing non-empty directory 'irq/208', leaking at least 'sx8654' ... When I revert commit e4df3a0 (i2c: core: Dispose OF IRQ mapping at client removal time) I don't get the warning. Is this a bug in the i2c-core or am I doing something wrong in my driver? Yes, this commit breaks all drivers using devm* for IRQ management on OF-based systemsi because devm* cleanup happens in device code, after bus's remove() method returns. I'd recommend reverting and finding a better way (making cleanup a custom devm action as well?). Ouch, my bad. Wolfram, any opinion ? The original patch fixes a real bug, so we shouldn't just revert it. Looking at it some more: What bug does it fix? Anything you experienced? I wonder if we really need e4df3a0 because I can't see where platform_get_irq, the major user of of_irq_get, disposes the mapping. irq_create_of_mapping() will return an already assigned mapping if called twice. I don't know yet, though, if mappings are static or if a mapping can be routed to another irq controller over some time because theoretically they can be dynamically added/removed. Adding Rob to CC as he wrote of_irq_get and put it into platform_get_irq. Rob, we use of_irq_get() in the I2C core and the question is now if we need to dispose the mapping and if so what would be a good place for it so managed devices will not have their mappings removed before the managed irq is removed. Ping. Just so you know: Without further information, I will revert the patch in question around rc4/rc5. I'd still like to know if the non-disposing of the mapping in platform_get_irq() is intentional. Thanks, Wolfram signature.asc Description: Digital signature
Re: Bug in i2c-core?
I am writing an I2C touchscreen driver for an i.MX6 based board. I compiled it as a module and when I unload it, I get the following warning: # modprobe sx8654 [ 46.261494] input: SX8654 I2C Touchscreen as /devices/soc0/soc/210.aips-bus/21a.i2c/i2c-0/0-0048/input/input1 # rmmod sx8654 [ 76.435223] [ cut here ] [ 76.439909] WARNING: CPU: 0 PID: 134 at fs/proc/generic.c:552 remove_proc_entry+0x148/0x164() [ 76.448582] remove_proc_entry: removing non-empty directory 'irq/208', leaking at least 'sx8654' ... When I revert commit e4df3a0 (i2c: core: Dispose OF IRQ mapping at client removal time) I don't get the warning. Is this a bug in the i2c-core or am I doing something wrong in my driver? Yes, this commit breaks all drivers using devm* for IRQ management on OF-based systemsi because devm* cleanup happens in device code, after bus's remove() method returns. I'd recommend reverting and finding a better way (making cleanup a custom devm action as well?). Ouch, my bad. Wolfram, any opinion ? The original patch fixes a real bug, so we shouldn't just revert it. Looking at it some more: What bug does it fix? Anything you experienced? I wonder if we really need e4df3a0 because I can't see where platform_get_irq, the major user of of_irq_get, disposes the mapping. irq_create_of_mapping() will return an already assigned mapping if called twice. I don't know yet, though, if mappings are static or if a mapping can be routed to another irq controller over some time because theoretically they can be dynamically added/removed. Adding Rob to CC as he wrote of_irq_get and put it into platform_get_irq. Rob, we use of_irq_get() in the I2C core and the question is now if we need to dispose the mapping and if so what would be a good place for it so managed devices will not have their mappings removed before the managed irq is removed. Thanks, Wolfram signature.asc Description: Digital signature
Re: Bug in i2c-core?
Hi Dmitry, On Friday 27 February 2015 08:59:44 Dmitry Torokhov wrote: On Fri, Feb 27, 2015 at 12:09:51PM +0100, Sébastien SZYMANSKI wrote: Hi, I am writing an I2C touchscreen driver for an i.MX6 based board. I compiled it as a module and when I unload it, I get the following warning: # modprobe sx8654 [ 46.261494] input: SX8654 I2C Touchscreen as /devices/soc0/soc/210.aips-bus/21a.i2c/i2c-0/0-0048/input/input1 # rmmod sx8654 [ 76.435223] [ cut here ] [ 76.439909] WARNING: CPU: 0 PID: 134 at fs/proc/generic.c:552 remove_proc_entry+0x148/0x164() [ 76.448582] remove_proc_entry: removing non-empty directory 'irq/208', leaking at least 'sx8654' ... When I revert commit e4df3a0 (i2c: core: Dispose OF IRQ mapping at client removal time) I don't get the warning. Is this a bug in the i2c-core or am I doing something wrong in my driver? Yes, this commit breaks all drivers using devm* for IRQ management on OF-based systemsi because devm* cleanup happens in device code, after bus's remove() method returns. I'd recommend reverting and finding a better way (making cleanup a custom devm action as well?). Ouch, my bad. Wolfram, any opinion ? The original patch fixes a real bug, so we shouldn't just revert it. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bug in i2c-core?
Yes, this commit breaks all drivers using devm* for IRQ management on OF-based systemsi because devm* cleanup happens in device code, after bus's remove() method returns. I'd recommend reverting and finding a better way (making cleanup a custom devm action as well?). Ouch, my bad. Wolfram, any opinion ? The original patch fixes a real bug, so we shouldn't just revert it. I guess the usual rules apply. Let's try to fix it for v4.0, otherwise I have to revert it. signature.asc Description: Digital signature
Re: Bug in i2c-core?
Dmitry, Uwe, On Fri, 27 Feb 2015 19:39:49 +0100, Uwe Kleine-König wrote: Input is allocated with devm_* and input_register_device knows how to deal with that: ah, something new each day. Thanks for teaching me. Thanks as well, I also discovered that thanks to this discussion. Best regards, Thomas -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bug in i2c-core?
Dear Sébastien SZYMANSKI, On Fri, 27 Feb 2015 12:09:51 +0100, Sébastien SZYMANSKI wrote: error = input_register_device(sx8654-input); if (error) return error; Where is your -remove() function that unregisters the input device? Best regards, Thomas -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bug in i2c-core?
On Fri, Feb 27, 2015 at 08:59:44AM -0800, Dmitry Torokhov wrote: On Fri, Feb 27, 2015 at 12:09:51PM +0100, Sébastien SZYMANSKI wrote: Hi, I am writing an I2C touchscreen driver for an i.MX6 based board. I compiled it as a module and when I unload it, I get the following warning: # modprobe sx8654 [ 46.261494] input: SX8654 I2C Touchscreen as /devices/soc0/soc/210.aips-bus/21a.i2c/i2c-0/0-0048/input/input1 # rmmod sx8654 [ 76.435223] [ cut here ] [ 76.439909] WARNING: CPU: 0 PID: 134 at fs/proc/generic.c:552 remove_proc_entry+0x148/0x164() [ 76.448582] remove_proc_entry: removing non-empty directory 'irq/208', leaking at least 'sx8654' ... When I revert commit e4df3a0 (i2c: core: Dispose OF IRQ mapping at client removal time) I don't get the warning. Is this a bug in the i2c-core or am I doing something wrong in my driver? Yes, this commit breaks all drivers using devm* for IRQ management on OF-based systemsi because devm* cleanup happens in device code, after bus's remove() method returns. I'd recommend reverting and finding a better way (making cleanup a custom devm action as well?). Also it seems that the original patch is incomplete as I do not see us disposing the mapping when probe fails... -- Dmitry -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bug in i2c-core?
On Fri, Feb 27, 2015 at 07:29:30AM -0800, Dmitry Torokhov wrote: On February 27, 2015 6:37:25 AM PST, Thomas Petazzoni thomas.petazz...@free-electrons.com wrote: Dear Sébastien SZYMANSKI, On Fri, 27 Feb 2015 12:09:51 +0100, Sébastien SZYMANSKI wrote: error = input_register_device(sx8654-input); if (error) return error; Where is your -remove() function that unregisters the input device? Everything seems to be allocated with devm so remove is not needed. Everything is devm_* but input_register_device, right? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | http://www.pengutronix.de/ | -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bug in i2c-core?
On Fri, Feb 27, 2015 at 12:09:51PM +0100, Sébastien SZYMANSKI wrote: Hi, I am writing an I2C touchscreen driver for an i.MX6 based board. I compiled it as a module and when I unload it, I get the following warning: # modprobe sx8654 [ 46.261494] input: SX8654 I2C Touchscreen as /devices/soc0/soc/210.aips-bus/21a.i2c/i2c-0/0-0048/input/input1 # rmmod sx8654 [ 76.435223] [ cut here ] [ 76.439909] WARNING: CPU: 0 PID: 134 at fs/proc/generic.c:552 remove_proc_entry+0x148/0x164() [ 76.448582] remove_proc_entry: removing non-empty directory 'irq/208', leaking at least 'sx8654' ... When I revert commit e4df3a0 (i2c: core: Dispose OF IRQ mapping at client removal time) I don't get the warning. Is this a bug in the i2c-core or am I doing something wrong in my driver? Yes, this commit breaks all drivers using devm* for IRQ management on OF-based systemsi because devm* cleanup happens in device code, after bus's remove() method returns. I'd recommend reverting and finding a better way (making cleanup a custom devm action as well?). Thanks. -- Dmitry -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Bug in i2c-core?
);
return -EIO;
}
error = i2c_smbus_write_byte_data(client, I2C_REG_CHANMASK,
(CONV_X | CONV_Y));
if (error 0) {
dev_err(client-dev, writing to I2C_REG_CHANMASK failed);
return -EIO;
}
error = i2c_smbus_write_byte_data(client, I2C_REG_IRQMASK,
(IRQ_PENTOUCH_TOUCHCONVDONE |
IRQ_PENRELEASE));
if (error 0) {
dev_err(client-dev, writing to I2C_REG_IRQMASK failed);
return -EIO;
}
error = i2c_smbus_write_byte_data(client, I2C_REG_TOUCH1,
(CONDIRQ | FILT_7SA));
if (error 0) {
dev_err(client-dev, writing to I2C_REG_TOUCH1 failed);
return -EIO;
}
error = devm_request_threaded_irq(client-dev, client-irq,
NULL, sx8654_irq,
IRQF_TRIGGER_FALLING |
IRQF_ONESHOT,
client-name, sx8654);
if (error) {
dev_err(client-dev,
Failed to enable IRQ %d, error: %d\n,
client-irq, error);
return error;
}
/* Disable the IRQ, we'll enable it in sx8654_open() */
disable_irq(client-irq);
error = input_register_device(sx8654-input);
if (error)
return error;
i2c_set_clientdata(client, sx8654);
return 0;
}
static const struct of_device_id sx8654_of_match[] = {
{ .compatible = semtech,sx8654, },
{ },
};
MODULE_DEVICE_TABLE(of, sx8654_of_match);
static const struct i2c_device_id sx8654_id_table[] = {
{ semtech_sx8654, 0 },
{ },
};
MODULE_DEVICE_TABLE(i2c, sx8654_id_table);
static struct i2c_driver sx8654_driver = {
.driver = {
.name = sx8654,
.owner = THIS_MODULE,
.of_match_table = sx8654_of_match,
},
.id_table = sx8654_id_table,
.probe = sx8654_probe,
};
module_i2c_driver(sx8654_driver);
MODULE_AUTHOR(Sébastien Szymanski sebastien.szyman...@armadeus.com);
MODULE_DESCRIPTION(Semtech SX8654 I2C Touchscreen Driver);
MODULE_LICENSE(GPL);
When I revert commit e4df3a0 (i2c: core: Dispose OF IRQ mapping at
client removal time) I don't get the warning.
Is this a bug in the i2c-core or am I doing something wrong in my driver?
--
Sébastien SZYMANSKI
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bug in i2c-core?
Hello, On Fri, Feb 27, 2015 at 09:46:11AM -0800, Dmitry Torokhov wrote: On Fri, Feb 27, 2015 at 06:05:45PM +0100, Uwe Kleine-König wrote: On Fri, Feb 27, 2015 at 07:29:30AM -0800, Dmitry Torokhov wrote: On February 27, 2015 6:37:25 AM PST, Thomas Petazzoni thomas.petazz...@free-electrons.com wrote: Dear Sébastien SZYMANSKI, On Fri, 27 Feb 2015 12:09:51 +0100, Sébastien SZYMANSKI wrote: error = input_register_device(sx8654-input); if (error) return error; Where is your -remove() function that unregisters the input device? Everything seems to be allocated with devm so remove is not needed. Everything is devm_* but input_register_device, right? Input is allocated with devm_* and input_register_device knows how to deal with that: ah, something new each day. Thanks for teaching me. Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | http://www.pengutronix.de/ | -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bug in i2c-core?
On February 27, 2015 6:37:25 AM PST, Thomas Petazzoni thomas.petazz...@free-electrons.com wrote: Dear Sébastien SZYMANSKI, On Fri, 27 Feb 2015 12:09:51 +0100, Sébastien SZYMANSKI wrote: error = input_register_device(sx8654-input); if (error) return error; Where is your -remove() function that unregisters the input device? Everything seems to be allocated with devm so remove is not needed. Thanks. -- Dmitry -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
