Re: [PATCH 0/17]: New SCSI target framework (SCST) with dev handlersand 2 target drivers

[Dmitry Torokhov]

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-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/