On 11/20/19 5:22 PM, Thomas Gleixner wrote:
On Wed, 20 Nov 2019, Luck, Tony wrote:
On Wed, Nov 20, 2019 at 10:53:39PM +0100, Borislav Petkov wrote:
On Wed, Nov 20, 2019 at 02:23:49PM -0700, Dave Jiang wrote:
+static inline void iosubmit_cmds512(void __iomem *dst, const void *src,
+ size_t count)
An iosubmit function which returns void and doesn't tell its callers
whether it succeeded or not? That looks non-optimal to say the least.
That's the underlying functionality of the MOVDIR64B instruction. A
posted write so no way to know if it succeeded. When using dedicated
queues the caller must keep count of how many operations are in flight
and not send more than the depth of the queue.
Why isn't there a fallback function which to call when the CPU doesn't
support movdir64b?
This particular driver has no option for fallback. Descriptors can
only be submitted with MOVDIR64B (to dedicated queues ... in later
patch series support for shared queues will be added, but those require
ENQCMD or ENQCMDS to submit).
The driver bails out at the beginning of the probe routine if the
necessary instructions are not supported:
+ /*
+ * If the CPU does not support write512, there's no point in
+ * enumerating the device. We can not utilize it.
+ */
+ if (!cpu_has_write512())
+ return -ENXIO;
Though we should always get past that as this PCI device ID shouldn't
every appear on a system that doesn't have the support. Device is on
the die, not a plug-in card.
Then the condition in the iosubmit function is just prone to silently paper
over any bug in a driver:
+ if (!cpu_has_write512())
+ return;
This should at least issue a WARN_ON_ONCE()
Thanks! I'll be adding the WARN_ON_ONCE() for the cap check. Also with
the alignment check Borislav mentioned, I'll add a WARN_ON() for failures.
Thanks,
tglx
