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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
