On Thu, Jan 06, 2011 at 09:20:44AM +0000, Russell King - ARM Linux wrote:
> On Wed, Jan 05, 2011 at 03:08:34PM -0800, Greg KH wrote:
> > On Wed, Jan 05, 2011 at 11:03:42PM +0000, Russell King - ARM Linux wrote:
> > > On Wed, Jan 05, 2011 at 05:51:00PM +0100, Sebastian Andrzej Siewior wrote:
> > > > +static void plat_dev_release(struct device *dev)
> > > > +{
> > > > + struct ce4100_i2c_device *sd = container_of(dev,
> > > > + struct ce4100_i2c_device, pdev.dev);
> > > > +
> > > > + of_device_node_put(&sd->pdev.dev);
> > > > +}
> > > > +static int add_i2c_device(struct pci_dev *dev, int bar,
> > > > + struct ce4100_i2c_device *sd)
> > > > +{
> > > > + struct platform_device *pdev = &sd->pdev;
> > > > + struct i2c_pxa_platform_data *pdata = &sd->pdata;
> > > ...
> > > > + pdev->name = "ce4100-i2c";
> > > > + pdev->dev.release = plat_dev_release;
> > > > + pdev->dev.parent = &dev->dev;
> > > > +
> > > > + pdev->dev.platform_data = pdata;
> > > > + pdev->resource = sd->res;
> > > ...
> > > > +static int __devinit ce4100_i2c_probe(struct pci_dev *dev,
> > > > + const struct pci_device_id *ent)
> > > > +{
> > > > + sds = kzalloc(sizeof(*sds), GFP_KERNEL);
> > > > + if (!sds)
> > > > + goto err_mem;
> > > > +
> > > > + pci_set_drvdata(dev, sds);
> > > > +
> > > > + for (i = 0; i < ARRAY_SIZE(sds->sd); i++) {
> > > > + ret = add_i2c_device(dev, i, &sds->sd[i]);
> > > > + if (ret) {
> > > > + while (--i >= 0)
> > > > + platform_device_unregister(&sds->sd[i].pdev);
> > > ...
> > > > +static void __devexit ce4100_i2c_remove(struct pci_dev *dev)
> > > ...
> > > > + for (i = 0; i < ARRAY_SIZE(sds->sd); i++)
> > > > + platform_device_unregister(&sds->sd[i].pdev);
> > > > +
> > > > + pci_disable_device(dev);
> > > > + kfree(sds);
> > > > +}
> > >
> > > I see we're still repeating the same mistakes with lifetime rules of
> > > sysfs objects.
> > >
> > > Any struct device which has been registered into the device model can
> > > remain indefinitely live after it's been unregistered (by, eg, if
> > > userspace holds a reference to it via sysfs.)
> >
> > Actually this race is almost not possible these days with the rework
> > that Tejun did on sysfs, so it's not easy to test for this anymore.
>
> Is it wise to make such a problematical bug harder to trigger without
> completely preventing it triggering?
>
> A different approach was taken with IRQ handling where people were
> registering handlers before the driver was ready for it to be called -
> request_irq() would explicitly call the handler as soon as it was
> registered to provoke bugs.
>
> Surely for these lifetime violations a similar approach should be taken.
> Make the kernel more likely to oops should a violation occur before the
> developer can get the code out the door. One way I can think of doing
> that is when devices are unregistered but not yet released, place them
> on a list which is periodically scanned, and not only is the device
> dereferenced by also the release function.
>
> When the use count drops to zero, don't immediately release, but wait
> a number of polls.
>
> If either goes away before the device has been released, then we
> predictably oops, and the developer gets to know about his violation
> of the rules immediately.
Yes, that would be a good way to resolve this, patches are glady welcome
:)
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
