Re: [PATCH 2.6.23.11]: [sata_svw]: Add ATAPI DMA support for HT1x00 SATA controller

[Jeff Garzik <jeff@garzik.org>]

Anantha Subramanyam wrote:
> This patch adds ATAPI DMA support for HT1000 (aka BCM5785) & HT1100
> (aka BCM11000) SATA Controller.
> 
> Signed-off-by: Anantha Subramanyam <ananth@broadcom.com>

Thanks much for your patience.  Feedback:

Engineering process issues:

1) patch is word-wrapped, and cannot be applied by automatic tools.  In
Linux, email is second only to the C compiler as a tool critical to
development work.  It might take some time to get things set up
correctly -- but that's a one-time cost.


2) several occurences of "whitespace mangling": tabs were converted to
spaces, again making our automated tools unable to apply the patch.


Consistency (with other Linux drivers) issues:

1) avoid C++ comments.  The rest of the driver exclusively uses /* style
(and 95% of the rest of the kernel does too).  You should follow the
existing coding style whenever modifying existing code.


2) don't define a struct twice, once for each endian
(_k2_sata_cmd_desc_s).  We use a very specific style, which the "sparse"
source code checking tool knows and checks:

	a) define a single version of the struct

	b) use C types that indicate endianness:  __le32, __be16, etc.

	c) in the source code, add endian conversions:

		foo->prd_tbl_base_lo = cpu_to_le32(address);

	   These endian conversions are automatically optimized out
	   on like-endian platforms, and are heavily optimized on
	   unlike-endian platforms.


3) use POSIX-style struct definitions:

	OK:

		struct foo {
			...
		};

	Not OK:

		typedef struct _foo {
			...
		} foo_t;


4) avoid what we call "StudlyCaps" :)  The preferred Linux style used in
the vast majority of code is lower case, with underscores separating words:

	OK:
		prd_tbl_hi
		prd_cnt
		prd_tbl_lo

	Not OK:

		sl_PrdTblBaseLow
		sw_PrdCount
		sw_PrdTblBaseHigh


5) similarly, avoid Hungarian notation, where the type is indicated in
the variable/member name.

	OK:	prd_tbl_lo
	Not OK:	sl_prd_tbl_lo


6) No need to add an extra indentation level after a 'return' statement:

> static int k2_sata_check_atapi_dma(struct ata_queued_cmd *qc) { +	u8
> command = qc->scsicmd->cmnd[0]; + if (qc->ap->flags &
> K2_FLAG_NO_ATAPI_DMA) return -1;	/* ATAPI DMA not supported */ +	else
>  +	{


7) static inline functions are preferred over cpp macros, because they
are far more type-safe.

> +#define K2_IS_SATA_STS_GOOD(sata_sts)      \ +    (((sata_sts &
> K2_SATA_DET_MASK) == K2_DEV_DET_PHY_RDY) && (((sata_sts &
> K2_SATA_INTF_MASK)>>8) == K2_SATA_INFT_ACTIVE))

The C compiler is smart enough to ensure there is no penalty for using a
function rather than a macro.


8) in k2_ht1x_port_start(), use the standard Linux 'goto unwind' method
of error handling:

	pspi = kmalloc(sizeof(k2_sata_port_info), GFP_KERNEL);
	if (!pspi)
		goto err_out;

	pspi->pcmd_desc = dma_alloc_coherent(dev, ...);
	if (!pspi->pcmd_desc)
		goto err_out_pspi;

	pspi->pcdb_cpybuf = dma_alloc_coherent(dev, ...);
	if (!pspi->pcdb_cpybuf)
		goto err_out_pcmd;

	/* ... etc ... */

	return 0;

     err_out_pcmd:
	dma_free_coherent(pspi->pcmd_desc);
     err_out_pspi:
	kfree(pspi);
     err_out:
	return rc;


9) don't add unnecessary casts from a void pointer:

> k2_sata_port_info* pspi = (k2_sata_port_info*)ap->private_data;


10) remove redundant comment...  by definition this statement is always 
true, since your patch will be merged into a kernel later than 2.6.18.

> +/*
> +later version of libata (kernel 2.6.18 & later) have a elaborate
> +error handling mech that includes multilevel of resets (soft, hard,
> post...)  
> +so we plug into that
> +
> +*/


11) K2_SATA_WAIT_MDIO_DONE() should be a function, not a macro


12) overall, make sure your patch passes 'sparse' (see 
Documentation/sparse.txt) and 'checkpatch' (see scripts/checkpatch.pl) 
checks before submission.  There are other minor issues I did not bother 
to outline here.





Operational issues:

1) Use of 'volatile' is almost always the wrong thing to do:

> int k2_ht1x_qdma_pause(struct ata_port *ap, int pause)
> +{
> +	k2_sata_port_info* pspi = (k2_sata_port_info*)ap->private_data;
> +	u32 qsr;
> +	volatile int i = 0;

In Linux, if you feel you need 'volatile', then really you most likely 
need (a) to remove it, (b) to use a barrier(), or (c) use a spinlock. 
'volatile' across SMP machines is rather useless, and ill-defined in the 
C specifications.


2) remove redundant initialization of static var.  Static variables are, 
by definition, initialized to zero.  Explicit initialization simply 
wastes space in the initialized variable area.


> -	static int printed_version;
> +	static int printed_version = 0;


3) we try _very_ hard to avoid all dependencies on the state produced by 
BIOS.  The three main reasons:

	a) the driver may be used on non-x86 platforms, where
	   x86 BIOS code simply cannot be executed

	b) during suspend/resume, the driver must be able to
	   initialize the controller fully after transition
	   from PCI D3 to PCI D0

	c) PCI controller hotplug

Thus, I would like to avoid the need for the 'skip_init' module 
parameter.  Users and automated Linux install tools will be completely 
unaware of skip_init, and will not have enough knowledge to use it.  It 
is best to simply Do The Right Thing in all cases.


4) don't use udelay/mdelay when you can sleep:

> +inline void k2_lnx_sleep( u32 usec_delay)
> +{
> +    (usec_delay > 1000) ? mdelay(usec_delay/1000 + 1):
> udelay(usec_delay);
> +}


5) is it reasonable to use QDMA for ATA also?


6) use spin_lock() rather than spin_lock_irqsave() in 
k2_ht1x_interrupt().  You're not competing with other interrupts AFAICS.


7) in k2_ht1x_qdma_intr(), ap->hsm_task_state generally refers to PIO 
state.  ditto for the use of ata_hsm_move() in k2_ht1x_handle_qdma_error().

Can you explain the need, there?

It is more likely that you should override ->qc_issue and handle things 
at a top level, rather than overriding specific pieces of the ATAPI host 
state machine.  The HSM was not really designed to be overridden piecemeal.