linux-scsi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Dan Williams <dan.j.williams@intel.com>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-scsi <linux-scsi@vger.kernel.org>,
	"Jiang, Dave" <dave.jiang@intel.com>,
	David Milburn <dmilburn@redhat.com>,
	"Ciechanowski, Ed" <ed.ciechanowski@intel.com>,
	"Nadolski, Edmund" <edmund.nadolski@intel.com>,
	"Danecki, Jacek" <jacek.danecki@intel.com>
Subject: Re: [GIT] isci: remote_device state_handlers and base_object	removal
Date: Wed, 04 May 2011 07:59:57 -0700	[thread overview]
Message-ID: <4DC169ED.5050101@intel.com> (raw)
In-Reply-To: <20110504095507.GA30387@lst.de>

On 5/4/2011 2:55 AM, Christoph Hellwig wrote:
> In general this looks like the way to go.  The only think I'm not overly
> happy wih is how the sci_base_object removal is handled.  In general it
> shouldn't be replaced by untyped void pointers, by proper container_of
> usage.  I guess for most instances it doesn't matter too much as the
> different layers of structures for the same object are in the process of
> beeing merged anyway, but it's fairly significant for the state machine.

As it turns out I gave this same feedback internally when reviewing the 
series.  I grabbed this early version for a couple reasons:

1/ wanted to get wider testing of the removal of structure member 
position assumptions.

2/ this arrived before I had the patches to start killing off the 
substate machines so it was not quite ready for this conversion.

Will circle back and re-add the type-safety after the substate machines 
are gone and the unification is completed.

> All the enter/exit handler should be passed a struct sci_base_state_machine *,
> and then use container_of to get at the containing structure, thus removing
> the need for the state_machine_owner field in struct sci_base_state_machine.

Ah yes, forgot about that field.

> And while you're at it _please_ remove all the utterly pointless kerneldoc
> comments for the state enter/exit handlers.

Looks like I managed to kill them all for remote_device, will double 
check that this gets done for the rest.

      reply	other threads:[~2011-05-04 14:59 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-05-03 16:45 [GIT] isci: remote_device state_handlers and base_object removal Dan Williams
2011-05-04  9:55 ` Christoph Hellwig
2011-05-04 14:59   ` Dan Williams [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4DC169ED.5050101@intel.com \
    --to=dan.j.williams@intel.com \
    --cc=dave.jiang@intel.com \
    --cc=dmilburn@redhat.com \
    --cc=ed.ciechanowski@intel.com \
    --cc=edmund.nadolski@intel.com \
    --cc=hch@lst.de \
    --cc=jacek.danecki@intel.com \
    --cc=linux-scsi@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).