Re: [RFC PATCH 03/11] firmware: arm_scmi: Add support for notifications message processing

[Cristian Marussi]

Hi

On 27/01/2020 17:32, Jonathan Cameron wrote:
> On Mon, 20 Jan 2020 12:23:25 +0000
> Cristian Marussi <cristian.marussi@xxxxxxx> wrote:
> 
>> From: Sudeep Holla <sudeep.holla@xxxxxxx>
>>
>> Add the mechanisms to distinguish notifications from delayed responses and
>> to properly fetch notification messages upon reception: notifications
>> processing does not continue further after the fetch phase.
>>
>> Signed-off-by: Sudeep Holla <sudeep.holla@xxxxxxx>
>> Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>
> 
> Couple of bits that seem more interesting than expected inline...
> 
>> ---
>>  drivers/firmware/arm_scmi/driver.c | 92 +++++++++++++++++++++---------
>>  1 file changed, 65 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
>> index 9611e8037d77..28ed1f0cb417 100644
>> --- a/drivers/firmware/arm_scmi/driver.c
>> +++ b/drivers/firmware/arm_scmi/driver.c
>> @@ -212,6 +212,15 @@ static void scmi_fetch_response(struct scmi_xfer *xfer,
>>  	memcpy_fromio(xfer->rx.buf, mem->msg_payload + 4, xfer->rx.len);
>>  }
>>  
>> +static void scmi_fetch_notification(struct scmi_xfer *xfer, size_t max_len,
>> +				    struct scmi_shared_mem __iomem *mem)
>> +{
>> +	/* Skip only length of header in payload area i.e 4 bytes */
>> +	xfer->rx.len = min_t(size_t, max_len, ioread32(&mem->length) - 4);
>> +
>> +	memcpy_fromio(xfer->rx.buf, mem->msg_payload, xfer->rx.len);
>> +}
>> +
>>  /**
>>   * pack_scmi_header() - packs and returns 32-bit header
>>   *
>> @@ -339,6 +348,58 @@ __scmi_xfer_put(struct scmi_xfers_info *minfo, struct scmi_xfer *xfer)
>>  	spin_unlock_irqrestore(&minfo->xfer_lock, flags);
>>  }
>>  
>> +static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr)
>> +{
>> +	struct scmi_xfer *xfer;
>> +	struct device *dev = cinfo->dev;
>> +	struct scmi_info *info = handle_to_scmi_info(cinfo->handle);
>> +	struct scmi_xfers_info *minfo = &info->rx_minfo;
>> +	struct scmi_shared_mem __iomem *mem = cinfo->payload;
>> +
>> +	xfer = scmi_xfer_get(cinfo->handle, minfo);
>> +	if (IS_ERR(xfer)) {
>> +		dev_err(dev, "failed to get free message slot (%ld)\n",
>> +			PTR_ERR(xfer));
>> +		iowrite32(SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE,
>> +			  &mem->channel_status);
>> +		return;
>> +	}
>> +
>> +	unpack_scmi_header(msg_hdr, &xfer->hdr);
>> +	scmi_dump_header_dbg(dev, &xfer->hdr);
>> +	scmi_fetch_notification(xfer, info->desc->max_msg_size, mem);
>> +	__scmi_xfer_put(minfo, xfer);
>> +
>> +	iowrite32(SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE, &mem->channel_status);
>> +}
>> +
>> +static void scmi_handle_xfer_delayed_resp(struct scmi_chan_info *cinfo,
>> +					  u16 xfer_id, bool delayed_resp)
> 
> Hmm. A function called *_delayed_resp that takes a boolean to say if
> it is a delayed_resp is in the category of non obvious....  Needs a rename
> at the very least.
> 

Changed logic and naming in v2, also because now v2 is on top of Viresh series
which makes transport layer independent. (in fact an additional dedicated patch
is dedicated to extend the transport layer for notifications too)


>> +{
>> +	struct scmi_xfer *xfer;
>> +	struct device *dev = cinfo->dev;
>> +	struct scmi_info *info = handle_to_scmi_info(cinfo->handle);
>> +	struct scmi_xfers_info *minfo = &info->tx_minfo;
>> +	struct scmi_shared_mem __iomem *mem = cinfo->payload;
>> +
>> +	/* Are we even expecting this? */
>> +	if (!test_bit(xfer_id, minfo->xfer_alloc_table)) {
>> +		dev_err(dev, "message for %d is not expected!\n", xfer_id);
>> +		return;
>> +	}
>> +
>> +	xfer = &minfo->xfer_block[xfer_id];
>> +
>> +	scmi_dump_header_dbg(dev, &xfer->hdr);
>> +
>> +	scmi_fetch_response(xfer, mem);
>> +
>> +	if (delayed_resp)
>> +		complete(xfer->async_done);
>> +	else
>> +		complete(&xfer->done);
>> +}
>> +
>>  /**
>>   * scmi_rx_callback() - mailbox client callback for receive messages
>>   *
>> @@ -355,41 +416,18 @@ static void scmi_rx_callback(struct mbox_client *cl, void *m)
>>  {
>>  	u8 msg_type;
>>  	u32 msg_hdr;
>> -	u16 xfer_id;
>> -	struct scmi_xfer *xfer;
>>  	struct scmi_chan_info *cinfo = client_to_scmi_chan_info(cl);
>> -	struct device *dev = cinfo->dev;
>> -	struct scmi_info *info = handle_to_scmi_info(cinfo->handle);
>> -	struct scmi_xfers_info *minfo = &info->tx_minfo;
>>  	struct scmi_shared_mem __iomem *mem = cinfo->payload;
>>  
>>  	msg_hdr = ioread32(&mem->msg_header);
>>  	msg_type = MSG_XTRACT_TYPE(msg_hdr);
>> -	xfer_id = MSG_XTRACT_TOKEN(msg_hdr);
>>  
>>  	if (msg_type == MSG_TYPE_NOTIFICATION)
>> -		return; /* Notifications not yet supported */
>> -
>> -	/* Are we even expecting this? */
>> -	if (!test_bit(xfer_id, minfo->xfer_alloc_table)) {
>> -		dev_err(dev, "message for %d is not expected!\n", xfer_id);
>> -		return;
>> -	}
>> -
>> -	xfer = &minfo->xfer_block[xfer_id];
>> -
>> -	scmi_dump_header_dbg(dev, &xfer->hdr);
>> -
>> -	scmi_fetch_response(xfer, mem);
>> -
>> -	trace_scmi_rx_done(xfer->transfer_id, xfer->hdr.id,
>> -			   xfer->hdr.protocol_id, xfer->hdr.seq,
>> -			   msg_type);
>> -
>> -	if (msg_type == MSG_TYPE_DELAYED_RESP)
>> -		complete(xfer->async_done);
>> +		scmi_handle_notification(cinfo, msg_hdr);
>>  	else
>> -		complete(&xfer->done);
>> +		scmi_handle_xfer_delayed_resp(cinfo, MSG_XTRACT_TOKEN(msg_hdr),
>> +					      msg_type);
> First I wondered why this wasn't a switch which would make a clear distinction
> between notification path and delayed response... 
> 
> However, it seems delayed_resp path also handles other values of msg_type,
> though only 0 which is a command I think...
> 
> Passing a enum that I think can take 4 values, only 3 of which are defined
> into a function as a boolean is 'interesting'. Don't do that.
> 

Reworked the logic in v2

Thanks

Regards

Cristian
> 
>> +
>>  }
>>  
>>  /**
> 
>