Re: [PATCH] arm: Fix me in bios32.c

[Russell King - ARM Linux]

On Sun, Jul 20, 2014 at 12:06:19AM -0400, Nicholas Krause wrote:
> This fixs a fix me in bios32.c for pci_fixup_it8152 as this if
> statement is incorrect needs to be checked against the class bits
> not the whole address for the two or conditions and since they don't
> have define statements outside of their numeratical value.

Unfortunately, this does not address the FIXME at all.  The FIXME does
not state that the if statement is incorrect at all.

> -	/* fixup for ITE 8152 devices */
> -	/* FIXME: add defines for class 0x68000 and 0x80103 */
>  	if ((dev->class >> 8) == PCI_CLASS_BRIDGE_HOST ||
> -	    dev->class == 0x68000 ||
> -	    dev->class == 0x80103) {
> +	   (dev->class >> 8) == 0x680 ||
> +	   (dev->class >> 8) == 0x801) {

The FIXME states that there should be defines for these values (0x68000
and 0x80103).

The FIXME statement itself is slightly wrong in that these values are
*not* class IDs.  0x680 and 0x801 are the class ID values.

We have definitions for class IDs 0x680 and 0x801 in
include/linux/pci_ids.h, which are:

#define PCI_CLASS_BRIDGE_OTHER          0x0680
#define PCI_CLASS_SYSTEM_DMA            0x0801

So, to fix the stated issue, without changing the functionality of the
code, this is what I expected you to produce:

	/* fixup for ITE 8152 devices */
	if ((dev->class >> 8) == PCI_CLASS_BRIDGE_HOST ||
	    dev->class == PCI_CLASS_BRIDGE_OTHER << 8 ||
	    def->class == PCI_CLASS_SYSTEM_DMA << 8 | 0x03)

The reason is that "PCI_CLASS_BRIDGE_OTHER << 8" evaluates to 0x68000
and "PCI_CLASS_SYSTEM_DMA << 8 | 0x03" evaluates to 0x80103.  Therefore,
the compiled code is the same as the original, because the actual
numerical check is the same as it always was.

As your solution results in a functional change to the code, that would
introduce a new bug - it results in this test matching any PCI device
with any programming interface for class "Bridge other" and "System DMA",
whereas before the code was looking for a specific value there.

Whether this results in the code incorrectly matching devices on the
system(s) which use this code is difficult to know, so my only choice
here is to reject your change.

This is why my fellow kernel developers are asking you to stop trying to
fix this stuff - unless you can produce provably correct patches (or at
least patches that appear to have no functional change) then you are
burdening people with your education, and you will get yourself an
undesirable reputation.

Even with patches which appear to have no functional change, some
maintainers won't take them unless they have actually been tested on
real hardware, or that other people agree that the change is a correct
one.  That is because there is a long history of apparantly correct
changes being made which result in subtle bugs.

Bear in mind that a "FIXME" comment indicates that something is not
fully correct in some way, and even though it may not be fully correct,
the resulting code _works_ on the devices that it has been tested with.
Fixing the "FIXME" may result in the code stopping working on those
devices - especially if it changes the functionality of the code like
your patch above.

So, it's often best to leave FIXMEs alone if you don't know what the
right solution should be.

I hope you will pause, and think about the issues I've raised before
continuing with your current project of trying to fix FIXMEs.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/