On Fri, Mar 18, 2011 at 8:56 AM, Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
[..]
> You absolutely have to fix it before it can be merged. Gems like
> duplicating all wire level constants and structures for SCSI, SAS
> and ATA in your own headers is simply not acceptable.
Hence the comment "Many of these want to be deleted in favor of the
in-kernel equivalent." in "[RFC PATCH 10/10] isci/core: common
definitions and utility functions' [1].
> I'm not sure where you got the idea this could be remotely acceptable.
Never did. We're reworking the driver that was handed to us.
To date fixing bugs in the current code (things like removing
phys_to_virt and virt_to_phys in favor of proper dma-api and kmap
usage) has delayed this rework... still plan to fix it.
> Also the source layout is a real mess, almost comparably to the broacade
> FC driver - there really is no point in having a header for barely more
> than a structure or inline.
Agreed. We consolidated the c files, now need to go back and do the
same for the headers. There is also a namespacecheck driven cleanup
that needs to happen as well.
> Also care to explain the design rationale for all these specific
> semi-statemachines? To my untrained eye they really looks like
> pointless obsfucation, but maybe there's a good reason that should be
> explained in long comments.
They model asynchronous hardware / protocol states and are loosely
analogous to the ata_sff_hsm implementation in libata in terms of
needing to handle transport layer details of sata, smp, and ssp. The
hardware is focused on accelerating the fast paths and let's software
handle many slow path conditions, for example ata pio requests require
driver interaction to progress the request through several states at a
per frame level. We'll put together some better documentation but
there are a lot of conditions and states that software is required to
handle and the state machines keep these organized.
There have been some patches to make them more readable (8e131f53,
ba647271, 03355704), but this work has not been completed across all
the state machines. I will say I think the controller state machine
is one that could be eventually factored out / absorbed into the lldd
because there are no asynchronous hardware-event driven controller
state transitions.
> And please don't make the code like ACPICA, macro names like
> SCU_CONTEXT_COMMAND_REQUEST_POST_TC_ABORT or
> SCIC_SDS_STP_PACKET_REQUEST_STARTED_PACKET_PHASE_AWAIT_TC_COMPLETION_SUBSTATE
> just need to go.
Ok.
> Also please get rid of ALL UPPERCASE SCREAMING NAMES for structures.
Unfortunately, the automated scripts we developed at the beginning of
the cleanup effort only caught definitions of the form:
typedef struct UPPERCASE_STRUCT { } UPPERCASE_STRUCT_T;
...and missed ones of the form:
struct UPPERCASE_STRUCT { };
...so we need to manually handle these stragglers.
> FYI:
>
> u32 connection_inactivity_timeout:16;
>
> is a really dumb idea. Please take the crackpipe away from whatever
> idiot came up with junk like that and use the proper right sized integer
> types. Also for sub-byte field please absolutely avoid bitfields.
> Their layout is unportable, and properly locking them is a nightmare.
> Just define a flags field and use bit operations on them.
Yeah. it does not make sense to define bitfields for things that are
correctly aligned base types.
--
Dan
[1]: http://marc.info/?l=linux-scsi&m=129975444506569&w=2
--
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
