On Fri, Oct 09 2020 at 11:46, David Woodhouse wrote:
@@ -45,12 +45,11 @@ enum irq_alloc_type {
};
> +static void mp_swizzle_msi_dest_bits(struct irq_data *irq_data, void *_entry)
> +{
> + struct msi_msg msg;
> + u32 *entry = _entry;
Why is this a void * argument and then converting it to a u32 *? Just to
make that function completely unreadable?
> +
> + irq_chip_compose_msi_msg(irq_data, &msg);
Lacks a comment. Also mp_swizzle... is a misnomer as this invokes the
msi compose function which is not what the function name suggests.
> + /*
> + * They're in a bit of a random order for historical reasons, but
> + * the IO/APIC is just a device for turning interrupt lines into
> + * MSIs, and various bits of the MSI addr/data are just swizzled
> + * into/from the bits of Redirection Table Entry.
> + */
> + entry[0] &= 0xfffff000;
> + entry[0] |= (msg.data & (MSI_DATA_DELIVERY_MODE_MASK |
> + MSI_DATA_VECTOR_MASK));
> + entry[0] |= (msg.address_lo & MSI_ADDR_DEST_MODE_MASK) << 9;
> +
> + entry[1] &= 0xffff;
> + entry[1] |= (msg.address_lo & MSI_ADDR_DEST_ID_MASK) << 12;
Sorry, but this is unreviewable gunk. The whole msi_msg setup sucks with
this unholy macro maze. I have a half finished series which allows
architectures to provide shadow members for data, address_* so this can
be done proper with bitfields.
Aside of that it works magically because polarity,trigger and mask bit
have been set up before. But of course a comment about this is
completely overrated.
Thanks,
tglx
