On Wed, Jul 28, 2021 at 03:34:18PM +0100, Sudeep Holla wrote:

On Mon, Jul 12, 2021 at 03:18:26PM +0100, Cristian Marussi wrote:

quoted

Add a check for the presence of .poll_done transport operation so that
transports that do not need to support polling mode have no need to provide
a dummy .poll_done callback either and polling mode can be disabled in the
SCMI core for that tranport.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---

Hi,

quoted

 drivers/firmware/arm_scmi/driver.c | 43 ++++++++++++++++++------------
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index a952b6527b8a..4183d25c9289 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c

@@ -777,25 +777,34 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
 	}
 
 	if (xfer->hdr.poll_completion) {
-		ktime_t stop = ktime_add_ns(ktime_get(), SCMI_MAX_POLL_TO_NS);
-
-		spin_until_cond(scmi_xfer_done_no_timeout(cinfo, xfer, stop));
-
-		if (ktime_before(ktime_get(), stop)) {
-			unsigned long flags;
-
-			/*
-			 * Do not fetch_response if an out-of-order delayed
-			 * response is being processed.
-			 */
-			spin_lock_irqsave(&xfer->lock, flags);
-			if (xfer->state == SCMI_XFER_SENT_OK) {
-				info->desc->ops->fetch_response(cinfo, xfer);
-				xfer->state = SCMI_XFER_RESP_OK;
+		if (info->desc->ops->poll_done) {
+			ktime_t stop = ktime_add_ns(ktime_get(),
+						    SCMI_MAX_POLL_TO_NS);
+
+			spin_until_cond(scmi_xfer_done_no_timeout(cinfo, xfer,
+								  stop));
+
+			if (ktime_before(ktime_get(), stop)) {
+				unsigned long flags;
+
+				/*
+				 * Do not fetch_response if an out-of-order delayed
+				 * response is being processed.
+				 */
+				spin_lock_irqsave(&xfer->lock, flags);
+				if (xfer->state == SCMI_XFER_SENT_OK) {
+					info->desc->ops->fetch_response(cinfo,
+									xfer);
+					xfer->state = SCMI_XFER_RESP_OK;
+				}
+				spin_unlock_irqrestore(&xfer->lock, flags);
+			} else {
+				ret = -ETIMEDOUT;
 			}
-			spin_unlock_irqrestore(&xfer->lock, flags);
 		} else {
-			ret = -ETIMEDOUT;
+			dev_warn_once(dev,
+				      "Polling mode is not supported by transport.\n");
+			ret = EINVAL;

Can't we just return this error as early as possible if the user isn't
expected to use polling with this transport ? That would simplify the patch
(as most of it is due to indentation which can go away) as you need not
check it later ?

Yes, indeed at first it was something like

if (xfer->hdr.poll_completion && !info->desc->ops->poll_done)
	return -EINVAL;

at the very beginning of do_xfer(), even before attempting to send
anything...and I liked much more but then I thought I would have run such
if-test for each and every command do_xfer() attempted...but maybe it's
just irrelevant and instead much more tidy if done as above.

I'll fix it as above.

Thanks,
Cristian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel