Hi Vladislav, On Tue, Sep 14, 2010 at 06:36:30PM +0400, Vladislav Bolkhovitin wrote: > Hi All, > > Please review this second iteration of the patch set of the new > (although, perhaps, the oldest) SCSI target framework for Linux SCST > with a set of dev handlers and 2 target drivers: for local access > (scst_local) and for Infiniband SRP (srpt). > I am afraid that the way the code was split into the patches will hinder the review process. Normally every patch consists of a usable on its own piece of code (or a logically compete change). This way the unit of work (coding, testing or reviewing) is clearly defined and easier to comprehend. Your patch series, unfortunately, splits Makefile and Kconfig changes separately from the drivers (which will cause build breakages should it be applied as is and someone needs to bisect), introduces header files in their entirety even when some of the data there is not needed till much later, uses facilities (for example sysfs bindings) that have not been introduced yet... I am afraid you'd have to rework the splitting process. Also patch descriptions should be improved: "This patch contains SYSFS interface implementation" - what are you trying to solve? - main points? - why offloading of actions to a separate thread? This should be in the patch description. BTW, why are you using an exclusive thread instead of exclusive workqueue for this? Also, kobject_del() + kobject_put() == kobject_unregister(). And too many "naked returns", I believe common style is to only have return for non-void functions. Thanks. -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
