lkml.org 
[lkml]   [2005]   [Dec]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/4] msi vector targeting abstractions
On Wed, Dec 21, 2005 at 12:42:41PM -0600, Mark Maule wrote:
> Abstract portions of the MSI core for platforms that do not use standard
> APIC interrupt controllers. This is implemented through a set of callouts
> which default to current behavior, but which can be overridden by calling
> msi_register_callouts() in the platform msi init code.

we tend to calls these _ops or _operations instead of _callouts.
Also I'd suggest to not keep the generic ones where they are but
in a separate file and let the existing plattforms calls msi_register()
with the ops table for those. This keeps the interface symmetric instead
of favouring the first implementation.

> @@ -89,10 +91,25 @@
> }
>
> #ifdef CONFIG_SMP
> +static void msi_target_generic(unsigned int vector,
> + unsigned int dest_cpu,
> + uint32_t *address_hi, /* in/out */
> + uint32_t *address_lo) /* in/out */

Please try to use u32 instead of uint32_t everywhere. Dito for other
sizes and signed types.

> +{
> + struct msg_address address;
> +
> + address.lo_address.value = *address_lo;
> + address.lo_address.value &= MSI_ADDRESS_DEST_ID_MASK;
> + address.lo_address.value |=
> + (cpu_physical_id(dest_cpu) << MSI_TARGET_CPU_SHIFT);
> +
> + *address_lo = address.lo_address.value;
> +}

Why do we need the full struct msg_address here? What about just:

static void msi_target_apic(unsigned int vector, unsigned int dest_cpu,
u32 *address_hi, u32 *address_lo)
{
u32 addr = *address_lo;

addr &= MSI_ADDRESS_DEST_ID_MASK;
addr |= (cpu_physical_id(dest_cpu) << MSI_TARGET_CPU_SHIFT);

*address_lo = addr;
}

> + (*msi_callouts.msi_teardown)(vector);
> +

just
msi_ops.teardown(vector);

> +union msg_data {
> + struct {
> #if defined(__LITTLE_ENDIAN_BITFIELD)
> - __u32 vector : 8;
> - __u32 delivery_mode : 3; /* 000b: FIXED | 001b: lowest prior */
> - __u32 reserved_1 : 3;
> - __u32 level : 1; /* 0: deassert | 1: assert */
> - __u32 trigger : 1; /* 0: edge | 1: level */
> - __u32 reserved_2 : 16;
> + __u32 vector : 8;
> + __u32 delivery_mode : 3; /* 000b: FIXED */
> + /* 001b: lowest prior */
> + __u32 reserved_1 : 3;
> + __u32 level : 1; /* 0: deassert | 1: assert */
> + __u32 trigger : 1; /* 0: edge | 1: level */
> + __u32 reserved_2 : 16;
> #elif defined(__BIG_ENDIAN_BITFIELD)
> - __u32 reserved_2 : 16;
> - __u32 trigger : 1; /* 0: edge | 1: level */
> - __u32 level : 1; /* 0: deassert | 1: assert */
> - __u32 reserved_1 : 3;
> - __u32 delivery_mode : 3; /* 000b: FIXED | 001b: lowest prior */
> - __u32 vector : 8;
> + __u32 reserved_2 : 16;
> + __u32 trigger : 1; /* 0: edge | 1: level */
> + __u32 level : 1; /* 0: deassert | 1: assert */
> + __u32 reserved_1 : 3;
> + __u32 delivery_mode : 3; /* 000b: FIXED */
> + /* 001b: lowest prior */
> + __u32 vector : 8;
> #else
> #error "Bitfield endianness not defined! Check your byteorder.h"
> #endif
> + }u;
> + __u32 value;
> } __attribute__ ((packed));


While you're cleaning things up, could you please kill the horrible abuse
of bitfields for H/W structures and do proper masking instead.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-12-21 19:59    [W:0.526 / U:0.096 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site