| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200127173232.000045ac@Huawei.com>
Date: Mon, 27 Jan 2020 17:32:32 +0000
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Cristian Marussi <cristian.marussi@....com>
CC: <linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>,
<james.quinlan@...adcom.com>, <lukasz.luba@....com>,
<sudeep.holla@....com>
Subject: Re: [RFC PATCH 03/11] firmware: arm_scmi: Add support for
notifications message processing
On Mon, 20 Jan 2020 12:23:25 +0000
Cristian Marussi <cristian.marussi@....com> wrote:
> From: Sudeep Holla <sudeep.holla@....com>
>
> 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@....com>
> Signed-off-by: Cristian Marussi <cristian.marussi@....com>
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.
> +{
> + 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.
> +
> }
>
> /**
Powered by blists - more mailing lists