Re: [PATCH] scsi: Update Aic94xx SAS/SATA Linux open source device

[Jeff Garzik <jeff@garzik.org>]

Gilbert Wu wrote:
> Subject:  [PATCH] scsi: Update Aic94xx SAS/SATA Linux open source device
> driver to fix smartctl utility problem.
> 
> Contribution:
>    Gilbert Wu <gilbert_wu@adaptec.com>
> 
> Change Log:
> 
> 
> 1. Fixed the problem that "smartctl -a /dev/some_sata_disk -d ata"
>    does not work on aic94xx device drive with SATA devices.
>    The smartctl utility send down ATA smart command to aic94xx
>    driver and fail to get the result which is stored on ATA output
>    register. 
>    In normal case, Aic94xx’s firmware does not return the value of
>    ATA output register to device driver due to performance reason.
>    To solve this issue then the driver need to turn on internal
>    flag (CSMI_TASK) for particular ATA command to enable firmware
>    to return the value of ATA output register.
> 
> 
> 
> 
> Patch: apply to aic94xx-sas-2.6.git development tree
> 
> Signed-off-by: Gilbert Wu <gilbert_wu@adaptec.com>

NAK

1) re-invents ATA command constants (a third time!).

Fix: use the constants in include/linux/ata.h


2) ignores the large number of ATA commands that also need register output.

Fix: invert the test.  the only time when we do NOT want the ATA 
register output is upon successful completion of "hot path" READ/WRITE 
commands.  For ALL other cases, we need the D2H FIS and/or SDB FIS data 
reflecting register state.


3) Patches are normally imported by automated tools, and your email body 
is copied directly into the permanent kernel changelog, archived for all 
eternity.  As such, you should format your patch description in a 
similar fashion to other patches.

Fixes:
a) Subject line way too long and redundant.  Use:

	[PATCH] aic94xx: fix smartctl utility problem

Execute 'git log' in the kernel repository for more examples.  The email 
subject is always copied into the kernel changelog as the FIRST LINE of 
the change.  This is a one-line summary of your patch.

b) Do not write "Subject: [...]" in email body.  Make this the subject 
of your email, per guidelines at http://linux.yyz.us/patch-format.html 
and Documentation/SubmittingPatches

c) Remove the redundant line
	Contribution:
	   Gilbert Wu <gilbert_wu@adaptec.com>
(this is redundant to your email address in the email, and the 
Signed-off-by line)

d) Remove the redundant line
	Change Log:
(we already know its a changelog)

e) Move temporal and meta-comments after the "---" separator, so that 
this text is not copied into the permanent kernel changelog.  See 
Documentation/SubmittingPatches for more info.

Regards,

	Jeff


-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html