Re: Bug in i2c-core?

[Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>]

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/2100000.aips-bus/21a0000.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