Re: [PATCH v3 03/17] powerpc: Add PFO support to the VIO bus
* Benjamin Herrenschmidt (b...@kernel.crashing.org) wrote:
Hrm... I don't like that much:
+ if (op-timeout)
+ deadline = jiffies + msecs_to_jiffies(op-timeout);
+
+ while (true) {
+ hret = plpar_hcall_norets(H_COP, op-flags,
+ vdev-resource_id,
+ op-in, op-inlen, op-out,
+ op-outlen, op-csbcpb);
+
+ if (hret == H_SUCCESS ||
+ (hret != H_NOT_ENOUGH_RESOURCES
+hret != H_BUSY hret != H_RESOURCE) ||
+ (op-timeout time_after(deadline, jiffies)))
+ break;
+
+ dev_dbg(dev, %s: hcall ret(%ld), retrying.\n, __func__, hret);
+ }
+
Is this meant to be called in atomic context ? If not, maybe it should
at the very least do a cond_resched() ?
Else, what about ceding the processor ? Or at the very least reducing
the thread priority for a bit ?
Shouldn't we also enforce to always have a timeout ? IE. Something like
30s or so if nothing specified to avoid having the kernel just hard
lock...
In general I don't like that sort of synchronous code, I'd rather return
the busy status up the chain which gives a chance to the caller to take
more appropriate measures depending on what it's doing, but that really
depends what you use that synchronous call for. I suppose if it's for
configuration type operations, it's ok...
This function is called in atomic context, it is used by PFO-type device
drivers to perform operations with the nest accelerator unit (like
crypto acceleration).
Having the timeout and retries in this function is the wrong thing to do.
We'll resubmit this without the loop and the caller will be responsible for
retrying the operations.
I would rather have the caller cede the processor or alter thread
priority where appropriate than doing that in this function. I don't
think this should be done in this crypto driver.
--Rob Jennings
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 03/17] powerpc: Add PFO support to the VIO bus
Hrm... I don't like that much:
+ if (op-timeout)
+ deadline = jiffies + msecs_to_jiffies(op-timeout);
+
+ while (true) {
+ hret = plpar_hcall_norets(H_COP, op-flags,
+ vdev-resource_id,
+ op-in, op-inlen, op-out,
+ op-outlen, op-csbcpb);
+
+ if (hret == H_SUCCESS ||
+ (hret != H_NOT_ENOUGH_RESOURCES
+ hret != H_BUSY hret != H_RESOURCE) ||
+ (op-timeout time_after(deadline, jiffies)))
+ break;
+
+ dev_dbg(dev, %s: hcall ret(%ld), retrying.\n, __func__, hret);
+ }
+
Is this meant to be called in atomic context ? If not, maybe it should
at the very least do a cond_resched() ?
Else, what about ceding the processor ? Or at the very least reducing
the thread priority for a bit ?
Shouldn't we also enforce to always have a timeout ? IE. Something like
30s or so if nothing specified to avoid having the kernel just hard
lock...
In general I don't like that sort of synchronous code, I'd rather return
the busy status up the chain which gives a chance to the caller to take
more appropriate measures depending on what it's doing, but that really
depends what you use that synchronous call for. I suppose if it's for
configuration type operations, it's ok...
Cheers,
Ben.
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 03/17] powerpc: Add PFO support to the VIO bus
Else, what about ceding the processor ? Or at the very least reducing the thread priority for a bit ? Shouldn't we also enforce to always have a timeout ? IE. Something like 30s or so if nothing specified to avoid having the kernel just hard lock... In general I don't like that sort of synchronous code, I'd rather return the busy status up the chain which gives a chance to the caller to take more appropriate measures depending on what it's doing, but that really depends what you use that synchronous call for. I suppose if it's for configuration type operations, it's ok... In any case, don't resend the whole series, just that one patch. Cheers, Ben. -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 03/17] powerpc: Add PFO support to the VIO bus
Add support for the Platform Facilities Option (PFO) to the VIO bus.
These devices have a separate root node in OpenFirmware which
requires additional parsing to map into the existing VIO device
structure fields. This adds the interface for PFO device drivers to
make synchronous hypervisor calls.
Signed-off-by: Robert Jennings r...@linux.vnet.ibm.com
Signed-off-by: Kent Yoder k...@linux.vnet.ibm.com
---
arch/powerpc/include/asm/vio.h | 46 +++
arch/powerpc/kernel/vio.c | 273 ++--
2 files changed, 280 insertions(+), 39 deletions(-)
diff --git a/arch/powerpc/include/asm/vio.h b/arch/powerpc/include/asm/vio.h
index 6bfd5ff..b19adf7 100644
--- a/arch/powerpc/include/asm/vio.h
+++ b/arch/powerpc/include/asm/vio.h
@@ -46,6 +46,48 @@
struct iommu_table;
+/*
+ * Platform Facilities Option (PFO)-specific data
+ */
+
+/* Starting unit address for PFO devices on the VIO BUS */
+#define VIO_BASE_PFO_UA0x5000
+
+/**
+ * vio_pfo_op - PFO operation parameters
+ *
+ * @flags: h_call subfunctions and modifiers
+ * @in: Input data block logical real address
+ * @inlen: If non-negative, the length of the input data block. If negative,
+ * the length of the input data descriptor list in bytes.
+ * @out: Output data block logical real address
+ * @outlen: If non-negative, the length of the input data block. If negative,
+ * the length of the input data descriptor list in bytes.
+ * @csbcpb: Logical real address of the 4k naturally-aligned storage block
+ * containing the CSB optional FC field specific CPB
+ * @timeout: # of milliseconds to retry h_call, 0 for no timeout.
+ * @hcall_err: pointer to return the h_call return value, else NULL
+ */
+struct vio_pfo_op {
+ u64 flags;
+ s64 in;
+ s64 inlen;
+ s64 out;
+ s64 outlen;
+ u64 csbcpb;
+ void *done;
+ unsigned long handle;
+ unsigned int timeout;
+ long hcall_err;
+};
+
+/* End PFO specific data */
+
+enum vio_dev_family {
+ VDEVICE,/* The OF node is a child of /vdevice */
+ PFO,/* The OF node is a child of /ibm,platform-facilities */
+};
+
/**
* vio_dev - This structure is used to describe virtual I/O devices.
*
@@ -58,6 +100,7 @@ struct vio_dev {
const char *name;
const char *type;
uint32_t unit_address;
+ uint32_t resource_id;
unsigned int irq;
struct {
size_t desired;
@@ -65,6 +108,7 @@ struct vio_dev {
size_t allocated;
atomic_t allocs_failed;
} cmo;
+ enum vio_dev_family family;
struct device dev;
};
@@ -95,6 +139,8 @@ extern void vio_cmo_set_dev_desired(struct vio_dev *viodev,
size_t desired);
extern void __devinit vio_unregister_device(struct vio_dev *dev);
+extern int vio_h_cop_sync(struct vio_dev *vdev, struct vio_pfo_op *op);
+
struct device_node;
extern struct vio_dev *vio_register_device_node(
diff --git a/arch/powerpc/kernel/vio.c b/arch/powerpc/kernel/vio.c
index a3a9990..cb87301 100644
--- a/arch/powerpc/kernel/vio.c
+++ b/arch/powerpc/kernel/vio.c
@@ -14,7 +14,9 @@
* 2 of the License, or (at your option) any later version.
*/
+#include linux/cpu.h
#include linux/types.h
+#include linux/delay.h
#include linux/stat.h
#include linux/device.h
#include linux/init.h
@@ -709,13 +711,26 @@ static int vio_cmo_bus_probe(struct vio_dev *viodev)
struct vio_driver *viodrv = to_vio_driver(dev-driver);
unsigned long flags;
size_t size;
+ bool dma_capable = false;
+
+ /* A device requires entitlement if it has a DMA window property */
+ switch (viodev-family) {
+ case VDEVICE:
+ if (of_get_property(viodev-dev.of_node,
+ ibm,my-dma-window, NULL))
+ dma_capable = true;
+ break;
+ case PFO:
+ dma_capable = false;
+ break;
+ default:
+ dev_warn(dev, unknown device family: %d\n, viodev-family);
+ BUG();
+ break;
+ }
- /*
-* Check to see that device has a DMA window and configure
-* entitlement for the device.
-*/
- if (of_get_property(viodev-dev.of_node,
- ibm,my-dma-window, NULL)) {
+ /* Configure entitlement for the device. */
+ if (dma_capable) {
/* Check that the driver is CMO enabled and get desired DMA */
if (!viodrv-get_desired_dma) {
dev_err(dev, %s: device driver does not support CMO\n,
@@ -1050,6 +1065,94 @@ static void vio_cmo_sysfs_init(void) { }
EXPORT_SYMBOL(vio_cmo_entitlement_update);
EXPORT_SYMBOL(vio_cmo_set_dev_desired);
+
+/*
+ * Platform Facilities Option (PFO) support
+ */
+
+/**
+ * vio_h_cop_sync - Perform a synchronous PFO co-processor operation
+ *
+ * @vdev - Pointer to a struct
