On Wed, Sep 07, 2022 at 04:55:18AM -0700, Christoph Hellwig wrote: > On Wed, Sep 07, 2022 at 12:43:30AM +0000, Tian, Kevin wrote: > > > From: Christoph Hellwig > > > Sent: Tuesday, September 6, 2022 5:42 PM > > > > > > What is the point? This adds indirect calls, and actually creates > > > more boilerplate code in the drivers. i.g. when using this code there > > > is more, and harder to read code. > > > > The point is to align with struct device life cycle when it's introduced > > to vfio_device. The object is released via put_device() then what would > > be the alternative if the driver doesn't provide a @release callback? > > > > and with @release then naturally @init is also expected. > > No, with a release no @init is expected. The init method is one > of the major obsfucations here, only topped by the weird > vfio_alloc_device macro. Yes, that saves about 4 lines of code > in every driver, but places a burden on the struct layout and > very much obsfucated things. Without vfio_alloc_device and > the init method I think much of this would make a lot more sense. > > See the patch below that goes on top of this series to show how > undoing these two would look on mbochs. It it a slight reduction > lines of code, but more readable and much less churn compared > to the status before this series. I've seen alot of error handling bugs caused by open-coding patterns like this. People get confused about what the lifecycle is and botch the error unwinds, almost 100% of the time :\ They call kfree when they should call put_device, they call put_device before initing enough stuff that the release callback doesn't crash, double free stuff by calling put_device at the wrong point, and so on. The advantage of init/release is the strict pairing and the core code helping get the error unwind right, by not calling release until init succeeds. The advantage of the vfio_alloc_device() is not saving 4 lines, it is giving the drivers a simple/sane error handling strategy. Goto unwind inside init, release undoes everything init does and the probe path only calls put_device(). It is simple and logical to implement and hard to make subtle bugs. Specifically it eliminates the open coded transition of kfree to put_device that seems so difficult for people to get right. netdev has done a version of this, so has rdma, and it works well. Jason
