linux-kernel - Re: [PATCH 1/2] [PCI] Check that MWI bit really did get set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <200610151716.36337.david-b@pacbell.net>
Date:	Sun, 15 Oct 2006 17:16:35 -0700
From:	David Brownell <david-b@...bell.net>
To:	Andrew Morton <akpm@...l.org>
Cc:	alan@...rguk.ukuu.org.uk, matthew@....cx,
	val_henson@...ux.intel.com, netdev@...r.kernel.org,
	linux-pci@...ey.karlin.mff.cuni.cz, linux-kernel@...r.kernel.org,
	gregkh@...e.de
Subject: Re: [PATCH 1/2] [PCI] Check that MWI bit really did get set

> > I agree that set_mwo() should set MWI if possible, and fail cleanly
> > if it couldn't (for whatever reason).  Thing is, choosing to treat
> > that as an error must be the _driver's_ choice ... it'd be wrong to force
> > that policy into the _interface_ by forcing must_check etc.
> 
> No.  If pci_set_mwi() detects an unexpected error then the driver should
> take some action: report it, recover from it, fail to load, etc.  If the
> driver fails to do any of this then it's a buggy driver.

But what is an "unexpected" "error"?  Not every fault that's unexpected
is an error; consider a page fault.

Consider also kfree(NULL).  The same motivation for drivers not needing
to validate that parameter is behind arguing that device drivers should
not need to poke around in PCI config space to verify that the device
supports MWI; and should have the flexibility to ignore the return code,
just like kfree() returns no value.

> You, the driver author _do not know_ what pci_set_mwi() does at present, on
> all platforms, nor do you know what it does in the future. 

I know that it enables MWI accesses ... or fails.  Beyond that, there
should be no reason to care.  If the hardware can use a lower-overhead
type of PCI bus cycle, I want it to do so.  If not, no sweat.

> This is not a terribly important issue, and it is far from the worst case
> of missed error-checking which we have in there. 

The reason I think it's important enough to continue this discussion is
that as it currently stands, it's a good example of a **BAD** interface
design ... since it's pointlessly marked as must_check.  (See appended
patch to fix that issue.)

If you wouldn't want all callers of kfree() to say "if (ptr) kfree(ptr);";
why then would anyone want to demand

	... read the pci config space byte (and cleanly handle errors) ...
	... check for the MWI bit ...
	... if it's set (so we "expect" this next call to succeed) then:
		... call pci_set_mwi() ...
		... test set_mwi() value ...
		... ignore that value ...

where the first three lines duplicate code _inside_ pci_set_mwi(), and the
last two lines are obviously a pure waste of code and effort.  I'd want to
know the criteria by which "if(ptr)" is judged a waste of effort in all
callers, but that more extensive PCI configspace logic was not...

- Dave

-------------------- CUT HERE

Remove bogus annotation of pci_set_mwi() as __must_check.  It's completely
reasonable for drivers to not care about the return code, when all they're
doing is enabling an optional performance-improving mechanism that's often
not even available.

Signed-off-by: David Brownell <dbrownell@...rs.sourceforge.net>

--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -499,7 +499,7 @@ int __must_check pci_enable_device_bars(
 void pci_disable_device(struct pci_dev *dev);
 void pci_set_master(struct pci_dev *dev);
 #define HAVE_PCI_SET_MWI
-int __must_check pci_set_mwi(struct pci_dev *dev);
+int pci_set_mwi(struct pci_dev *dev);
 void pci_clear_mwi(struct pci_dev *dev);
 void pci_intx(struct pci_dev *dev, int enable);
 int pci_set_dma_mask(struct pci_dev *dev, u64 mask);

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