From: Mark Maule <maule@sgi.com>
To: Matthew Wilcox <matthew@wil.cx>
Cc: linux-ia64@vger.kernel.org, linux-kernel@vger.kernel.org,
Tony Luck <tony.luck@intel.com>
Subject: Re: [PATCH 2/4] msi vector targeting abstractions
Date: Wed, 21 Dec 2005 13:33:00 -0600 [thread overview]
Message-ID: <20051221193300.GK9920@sgi.com> (raw)
In-Reply-To: <20051221190558.GD2361@parisc-linux.org>
On Wed, Dec 21, 2005 at 12:05:58PM -0700, Matthew Wilcox wrote:
> On Wed, Dec 21, 2005 at 12:42:41PM -0600, Mark Maule wrote:
>
> > @@ -108,28 +125,38 @@
> > if (!(pos = pci_find_capability(entry->dev, PCI_CAP_ID_MSI)))
> > return;
> >
> > + pci_read_config_dword(entry->dev, msi_upper_address_reg(pos),
> > + &address_hi);
> > pci_read_config_dword(entry->dev, msi_lower_address_reg(pos),
> > - &address.lo_address.value);
> > - address.lo_address.value &= MSI_ADDRESS_DEST_ID_MASK;
> > - address.lo_address.value |= (cpu_physical_id(dest_cpu) <<
> > - MSI_TARGET_CPU_SHIFT);
> > - entry->msi_attrib.current_cpu = cpu_physical_id(dest_cpu);
> > + &address_lo);
> > +
> > + msi_callouts.msi_target(vector, dest_cpu,
> > + &address_hi, &address_lo);
> > +
> > + pci_write_config_dword(entry->dev, msi_upper_address_reg(pos),
> > + address_hi);
> > pci_write_config_dword(entry->dev, msi_lower_address_reg(pos),
> > - address.lo_address.value);
> > + address_lo);
>
> But actually, I don't understand why you don't just pass a msg_address
> pointer to msi_target instead.
Mainly I did it this way 'cause msg_address seems to be geared toward specific
hw (apic?). In the case of altix interrupt hw, we don't know about
dest_mode et. al., but only care about the raw address.
I think this style makes it clearer that the core code should only be
using opaque data when interacting with the platform hooks and the MSI
registers.
>
> (last two points apply throughtout this patch)
>
> >
> > + (*msi_callouts.msi_teardown)(vector);
> > +
>
> Yuck. There's a reason C allows you to call through function pointers as if
> they were functions.
My bad ... I used the alternate style elsewhere, just botched this one up.
>
> > +int
> > +msi_register_callouts(struct msi_callouts *co)
> > +{
> > + msi_callouts = *co; /* structure copy */
> > + return 0;
>
> Why do it this way instead of having a pointer to a struct?
Are you suggesting just have:
struct msi_callouts *msi_callouts = (some default value or NULL)
and then having each platform just assign msi_callouts in their msi_arch_init?
Doesn't matter to me either way ... I thought having an interface to set
the callouts was cleaner.
>
> > -struct msg_data {
> > +union msg_data {
> > + struct {
>
> How about leaving struct msg_data alone and adding
>
> union good_name {
> struct msg_data;
> u32 value;
> }
>
> Or possibly struct msg_data should just be deleted and we should use
> shift/mask to access the contents of it. ISTR GCC handled that much
> better.
Christoph had similiar comments. Will put some thought into it.
Mark
next prev parent reply other threads:[~2005-12-21 19:33 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2005-12-21 18:42 [PATCH 0/4] msi abstractions and support for altix Mark Maule
2005-12-21 18:42 ` [PATCH 1/4] msi archetecture init hook Mark Maule
2005-12-21 18:53 ` Matthew Wilcox
2005-12-21 19:03 ` Mark Maule
2005-12-21 19:40 ` Matthew Wilcox
2005-12-21 18:42 ` [PATCH 2/4] msi vector targeting abstractions Mark Maule
2005-12-21 18:56 ` Christoph Hellwig
2005-12-21 19:17 ` Mark Maule
2005-12-21 19:05 ` Matthew Wilcox
2005-12-21 19:17 ` linux-os (Dick Johnson)
2005-12-21 19:33 ` Mark Maule [this message]
2005-12-22 10:36 ` Christoph Hellwig
2005-12-22 13:59 ` Mark Maule
2005-12-21 18:42 ` [PATCH 3/4] per-platform IA64_{FIRST,LAST}_DEVICE_VECTOR definitions Mark Maule
2005-12-21 19:09 ` Matthew Wilcox
2005-12-21 19:18 ` Mark Maule
2005-12-21 19:32 ` Matthew Wilcox
2005-12-22 6:26 ` Keith Owens
2005-12-22 13:14 ` Matthew Wilcox
2005-12-21 18:42 ` [PATCH 4/4] altix: msi support Mark Maule
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=20051221193300.GK9920@sgi.com \
--to=maule@sgi.com \
--cc=linux-ia64@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=matthew@wil.cx \
--cc=tony.luck@intel.com \
/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).