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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Anantha Subramanyam wrote:
This patch adds ATAPI DMA support for HT1000 (aka BCM5785) & HT1100
(aka BCM11000) SATA Controller.

Signed-off-by: Anantha Subramanyam <ananth@xxxxxxxxxxxx>

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.

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

[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux