Re: [RFC PATCH v2] fpga: Introduce new fpga subsystem

[Jason Gunthorpe]

On Fri, Oct 04, 2013 at 06:28:23PM +0200, Michal Simek wrote:
> > I strongly encourage you to use text strings to indicate the state of
> > the configuration FSM, and I *really* think you should rework things
> > to have an explicit configuration FSM rather than trying to bodge one
> > together with a bunch of bit flags.
> 
> Any favourite names for states?
> Or ready, write_init, write_complete is enough for now?

Doesnt really matter to me, don't forget error states. Transisionts to
ready, write_init and write_complete can all fail.

> > I wonder if this is right, it seems like a strange way to make a class
> > subsystem, usually the struct fpga_manager would contain the 'struct
> > device' (not a pointer, so you can use container_of) and drvdata would
> > be reserved for something else.
> 
> I am not following you here. mrg structure is connected with the driver
> it means when driver is removed then this structure is freed.

You've got your mgr structure and then later you allocate a struct
device as well, the mgr can be free'd before the struct device is
released, due to the way ref counting works. You are not doing
anything to compensate for that.

> > This seems to create lifetime issues since the devm above will be
> > free'd when the platform driver is released, but the class device will
> > live on with the stray pointer. Better to allocate everything against
> > the class device below.
> 
> device in unregistered before this structure is freed.
> fpga_mgr_unregister() is called in the platform driver in remove function.

Which is the problem, device_unregister dosen't delete 'mgr->dev', and it
doesn't mean the sysfs call backs are uncallable, so you have a free'd
dangling pointer in mgr->dev, and an object lifetime issue.

> > What happens when userspace is holding one of the sysfs files open and
> > you unload the module? Looks like bad things?
> 
> I didn't test this but feel free to check it.

You should fix these problems before your driver reaches Linus.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/