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
